Re: [Outreachy kernel] [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16

Re: [Outreachy kernel] [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16

[Daniel Baluta]

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'
> 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 <somyaanand214@gmail.com>

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.

Re: [Outreachy kernel] [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16

[Lars-Peter Clausen]

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'
>> 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 <somyaanand214@gmail.com>

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

Re: [Outreachy kernel] [PATCH] Staging: iio: Change data type in adis16220_read_16bit to u16

[Somya Anand]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

On Fri, Mar 6, 2015 at 2:45 PM, Lars-Peter Clausen <lars@metafoo.de> 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'
>>> 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 <somyaanand214@gmail.com>
>>>
>>
> 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

[-- Attachment #2: Type: text/html, Size: 2772 bytes --]