From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <54F9704B.3040501@metafoo.de> References: <1425630960-7142-1-git-send-email-somyaanand214@gmail.com> <54F9704B.3040501@metafoo.de> Date: Fri, 6 Mar 2015 15:25:25 +0530 Message-ID: Subject: Re: [Outreachy kernel] [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16 From: Somya Anand To: Lars-Peter Clausen Cc: Daniel Baluta , outreachy-kernel@googlegroups.com, Jonathan Cameron , "linux-iio@vger.kernel.org" Content-Type: multipart/alternative; boundary=001a113655b600cc1105109baf7e List-ID: --001a113655b600cc1105109baf7e Content-Type: text/plain; charset=UTF-8 On Fri, Mar 6, 2015 at 2:45 PM, Lars-Peter Clausen wrote: > On 03/06/2015 09:58 AM, Daniel Baluta wrote: > >> Hi Somya, >> >> On Fri, Mar 6, 2015 at 10:36 AM, Somya Anand >> wrote: >> >>> In the adis16220_read16bit() function we earlier used a s16 value 'val' >>> which is used by the adis_read_reg_16 function to read data and takes a >>> u16 value as a parameter. >>> >>> So, this patch changes the data type of 'val' from s16 to u16 for further >>> simplification of code and thereby avoiding unnecessary typecast. >>> >>> Signed-off-by: Somya Anand >>> >> > Patch looks ok. But it changes the semantics of the function, the reason > why this is a s16 instead of a u16 is because at some point the function > was used to read register that contained signed 16 bit values. The code is > essentially the short version of: > > u16 uval; > adis_read_reg_16(&st->adis, this_attr->address, &uval); > sprintf("%d\n", sign_extend32(uval, 15)); > I have asked Daniel regarding this earlier because while reading the code, I could not get the exact purpose of using s16. Thanks for clearing my doubt. > > This patch removes the extra sign extension. Which is ok, since the only > user of the function uses it to read a 10 unsigned value which will lead to > the same result in both cases. But the commit message should mention this, > since it is not just the removal of a unnecessary type cast. > > Sure, I will write a better commit message so that it would be more descriptive. > - Lars > Somya --001a113655b600cc1105109baf7e Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Fri, Mar 6, 2015 at 2:45 PM, Lars-Peter Clausen &l= t;lars@metafoo.de&= gt; wrote:
On 03/= 06/2015 09:58 AM, Daniel Baluta wrote:
Hi Somya,

On Fri, Mar 6, 2015 at 10:36 AM, Somya Anand <somyaanand214@gmail.com> wrote:
In the adis16220_read16bit() function we earlier used a s16 value 'val&= #39;
which is used by the adis_read_reg_16 function to read data and takes a
u16 value as a parameter.

So, this patch changes the data type of 'val' from s16 to u16 for f= urther
simplification of code and thereby avoiding unnecessary typecast.

Signed-off-by: Somya Anand <somyaanand214@gmail.com>

 Patch looks ok. But it changes the semantics of the function, the reason wh= y this is a s16 instead of a u16 is because at some point the function was = used to read register that contained signed 16 bit values. The code is esse= ntially the short version of:

=C2=A0 =C2=A0 =C2=A0 =C2=A0 u16 uval;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 adis_read_reg_16(&st->adis, this_attr-&g= t;address, &uval);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 sprintf("%d\n", sign_extend32(uval, 1= 5));

=C2=A0 =C2=A0 I have asked Daniel = regarding this earlier because while reading the code, I could not get the = exact purpose of using s16. Thanks for clearing my doubt.=C2=A0

This patch removes the extra sign extension. Which is ok, since the only us= er of the function uses it to read a 10 unsigned value which will lead to t= he same result in both cases. But the commit message should mention this, s= ince it is not just the removal of a unnecessary type cast.

=C2=A0 =C2=A0Sure, I will write a bette= r commit message so that it would be more descriptive. =C2=A0
- Lars

=C2=A0 Somya=C2=A0=

--001a113655b600cc1105109baf7e-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <54F9704B.3040501@metafoo.de> Date: Fri, 06 Mar 2015 10:15:55 +0100 From: Lars-Peter Clausen MIME-Version: 1.0 To: Daniel Baluta , Somya Anand CC: outreachy-kernel@googlegroups.com, Jonathan Cameron , "linux-iio@vger.kernel.org" Subject: Re: [Outreachy kernel] [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16 References: <1425630960-7142-1-git-send-email-somyaanand214@gmail.com> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed List-ID: On 03/06/2015 09:58 AM, Daniel Baluta wrote: > Hi Somya, > > On Fri, Mar 6, 2015 at 10:36 AM, Somya Anand wrote: >> In the adis16220_read16bit() function we earlier used a s16 value 'val' >> which is used by the adis_read_reg_16 function to read data and takes a >> u16 value as a parameter. >> >> So, this patch changes the data type of 'val' from s16 to u16 for further >> simplification of code and thereby avoiding unnecessary typecast. >> >> Signed-off-by: Somya Anand Patch looks ok. But it changes the semantics of the function, the reason why this is a s16 instead of a u16 is because at some point the function was used to read register that contained signed 16 bit values. The code is essentially the short version of: u16 uval; adis_read_reg_16(&st->adis, this_attr->address, &uval); sprintf("%d\n", sign_extend32(uval, 15)); This patch removes the extra sign extension. Which is ok, since the only user of the function uses it to read a 10 unsigned value which will lead to the same result in both cases. But the commit message should mention this, since it is not just the removal of a unnecessary type cast. - Lars From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <1425630960-7142-1-git-send-email-somyaanand214@gmail.com> References: <1425630960-7142-1-git-send-email-somyaanand214@gmail.com> Date: Fri, 6 Mar 2015 10:58:54 +0200 Message-ID: Subject: Re: [Outreachy kernel] [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16 From: Daniel Baluta To: Somya Anand Cc: outreachy-kernel@googlegroups.com, Jonathan Cameron , Lars-Peter Clausen , "linux-iio@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 List-ID: Hi Somya, On Fri, Mar 6, 2015 at 10:36 AM, Somya Anand wrote: > In the adis16220_read16bit() function we earlier used a s16 value 'val' > which is used by the adis_read_reg_16 function to read data and takes a > u16 value as a parameter. > > So, this patch changes the data type of 'val' from s16 to u16 for further > simplification of code and thereby avoiding unnecessary typecast. > > Signed-off-by: Somya Anand Ideally, subject should tell us "Why?" the patch is needed, instead of "What?" the patch does. The latter should be easy to understand reading the code. e.g: staging: iio: adis16220: Avoid unnecessary typecast > --- > drivers/staging/iio/accel/adis16220_core.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/iio/accel/adis16220_core.c b/drivers/staging/iio/accel/adis16220_core.c > index d478f51..7a4a0fd 100644 > --- a/drivers/staging/iio/accel/adis16220_core.c > +++ b/drivers/staging/iio/accel/adis16220_core.c > @@ -28,16 +28,15 @@ static ssize_t adis16220_read_16bit(struct device *dev, > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > struct adis16220_state *st = iio_priv(indio_dev); > ssize_t ret; > - s16 val = 0; > + u16 val = 0; > > /* Take the iio_dev status lock */ > mutex_lock(&indio_dev->mlock); > - ret = adis_read_reg_16(&st->adis, this_attr->address, > - (u16 *)&val); > + ret = adis_read_reg_16(&st->adis, this_attr->address, &val); > mutex_unlock(&indio_dev->mlock); > if (ret) > return ret; > - return sprintf(buf, "%d\n", val); > + return sprintf(buf, "%u\n", val); > } > > static ssize_t adis16220_write_16bit(struct device *dev, Other than that it looks good to me. We will need an ACK from Jonathan / Lars on this. thanks, Daniel.