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

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

[Somya Anand]

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>
---
 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,
-- 
1.9.1

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 --]

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

[arnd]

> Somya Anand <somyaanand214@gmail.com> hat am 6. März 2015 um 09:36
> geschrieben:

> 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);

In a patch like this, it would be best to also drop the initialization to zero,
which is
not used here. Not initializing it helps the compiler provide useful warnings if
the code gets changed to return an otherwise uninitialized result.

> - 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);
>  }
>  

This looks wrong: Unless you know better, you should assume that the sprintf
format
string is correct and that the value is indeed signed for the purpose of this
file.

The reason why adis_read_reg_16() takes a u16 argument instead of s16 is that it
accesses a register that doesn't have a concept of signedness. I would just
leave
the code as it is, alternatively you could have two variables and explicitly
cast from
u16 to s16 with a comment.

     Arnd