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

[arnd@arndb.de]

> 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