On Wed, Nov 23, 2011 at 09:47:36PM +0100, Andreas Ruprecht wrote:
> As was pointed out by Dan, some data types could be changed from
> long (as we only had strict_strtol here before) to a smaller type
> by using the corresponding kstrto* functions.
>
> I'm not sure about the sca_3000_set_free_fall_mode() function, as
> to what could be in buf. From my interpretation it's only a matter
> of a 0/1 decision, but I wanted to have someone else give their
> opinion on that before changing it.
>
> The same probably applies to sca_3000_set_ring_int() in the file
> sca3000_ring.
Leaving things alone is always safe. No one can argue with it.
>
> Regards,
> Andreas Ruprecht
^^^^^^^^^^^^^^^^
Don't put that into the changelog.
>
> Signed-off-by: Andreas Ruprecht <rupran@xxxxxxxxxxxx>
> ---
> drivers/staging/iio/accel/adis16220_core.c | 4 ++--
> drivers/staging/iio/accel/lis3l02dq_core.c | 4 ++--
> drivers/staging/iio/accel/sca3000_core.c | 9 ++++-----
> 3 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/iio/accel/adis16220_core.c b/drivers/staging/iio/accel/adis16220_core.c
> index 9966b64..bb72eb3 100644
> --- a/drivers/staging/iio/accel/adis16220_core.c
> +++ b/drivers/staging/iio/accel/adis16220_core.c
> @@ -167,9 +167,9 @@ static ssize_t adis16220_write_16bit(struct device *dev,
> struct iio_dev *indio_dev = dev_get_drvdata(dev);
> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> int ret;
> - long val;
> + u16 val;
>
> - ret = kstrtol(buf, 10, &val);
> + ret = kstrtou16(buf, 10, &val);
> if (ret)
> goto error_ret;
> ret = adis16220_spi_write_reg_16(iindio_dev, this_attr->address, val);
I think I should have explained this better. This is the change I
suggested and so obviously I approve of what you did here. The
original code silently truncated high numbers to u16 and returned
success. The new code returns -ERANGE when the numbers are too high.
> diff --git a/drivers/staging/iio/accel/lis3l02dq_core.c b/drivers/staging/iio/accel/lis3l02dq_core.c
> index c466057..40c940f 100644
> --- a/drivers/staging/iio/accel/lis3l02dq_core.c
> +++ b/drivers/staging/iio/accel/lis3l02dq_core.c
> @@ -331,11 +331,11 @@ static ssize_t lis3l02dq_write_frequency(struct device *dev,
> size_t len)
> {
> struct iio_dev *indio_dev = dev_get_drvdata(dev);
> - long val;
> + u16 val;
> int ret;
> u8 t;
>
> - ret = kstrtol(buf, 10, &val);
> + ret = kstrtou16(buf, 10, &val);
kstrtoXX functions return -ERANGE if you pass something that is too
large for the data type. I should have explained that earlier...
I don't think it's necessarily what we want to do here. The current
code returns -EINVAL and that seems good.
It might make more sense to make val an unsigned long instead. You
can't have a negative frequency right?
> if (ret)
> return ret;
>
> diff --git a/drivers/staging/iio/accel/sca3000_core.c b/drivers/staging/iio/accel/sca3000_core.c
> index 6a2721b..1455990 100644
> --- a/drivers/staging/iio/accel/sca3000_core.c
> +++ b/drivers/staging/iio/accel/sca3000_core.c
> @@ -382,10 +382,10 @@ sca3000_store_measurement_mode(struct device *dev,
> struct sca3000_state *st = iio_priv(indio_dev);
> int ret;
> int mask = 0x03;
> - long val;
> + int val;
>
> mutex_lock(&st->lock);
> - ret = kstrtol(buf, 10, &val);
> + ret = kstrtoint(buf, 10, &val);
We're doing bit ops on val so it shouldn't be signed. The original
code had it as long which is also signed. We only care about the
lowest 8 bits. It's hard to say what it should be without knowing
how this is used or more context. My guess is unsigned long, but
leaving it alone is also safe.
If you are going to change the behavior, that's something very
important and you should break each behavior change out into its
own patch and write a description justifying why the behavior
change is appropriate.
Part of the problem is that before we only had 4 functions but now
we have many so it's not a simple matter of running sed.
regards,
dan carpenter
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
