| Message ID | 20200417114020.31291-2-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3,v3] iio: magnetometer: ak8974: Correct realbits | expand |
On Fri, Apr 17, 2020 at 2:42 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > This breaks out the measurement code to its own function > so we can handle this without swirling it up with the > big switch() statement inside ak8974_read_raw(). > > Keep a local s16 helper variable for the signed value > coming out of the measurement before assigning it to the > integer *val. The local variable makes the code easier > to read and the compiler will optimize it if possible. > + /* > + * This explicit cast to (s16) is necessary as the measurement > + * is done in 2's complement with positive and negative values. > + * The follwing assignment to *val will then convert the signed > + * s16 value to a signed int value. > + */ > + outval = (s16)le16_to_cpu(hw_values[address]); > + *val = outval; I'm wondering if you may use sign_extend32() here.
On Fri, Apr 17, 2020 at 01:40:19PM +0200, Linus Walleij wrote: > This breaks out the measurement code to its own function > so we can handle this without swirling it up with the > big switch() statement inside ak8974_read_raw(). > > Keep a local s16 helper variable for the signed value > coming out of the measurement before assigning it to the > integer *val. The local variable makes the code easier > to read and the compiler will optimize it if possible. [...] > +static int ak8974_measure_channel(struct ak8974 *ak8974, unsigned long address, > + int *val) > +{ > + __le16 hw_values[3]; > + int ret; > + s16 outval; [...] > + /* > + * This explicit cast to (s16) is necessary as the measurement > + * is done in 2's complement with positive and negative values. > + * The follwing assignment to *val will then convert the signed > + * s16 value to a signed int value. > + */ > + outval = (s16)le16_to_cpu(hw_values[address]); > + *val = outval; The intermediate 'outval' is not needed. What you describe in the comment is a normal C integer promotion rule, so I would leave the comment out, too. IOW, this is equivalent to: *val = (s16)le16_to_cpu(...); Otherwise: Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c index f22b40ef5661..b8dbea119a67 100644 --- a/drivers/iio/magnetometer/ak8974.c +++ b/drivers/iio/magnetometer/ak8974.c @@ -554,47 +554,63 @@ static int ak8974_detect(struct ak8974 *ak8974) return 0; } +static int ak8974_measure_channel(struct ak8974 *ak8974, unsigned long address, + int *val) +{ + __le16 hw_values[3]; + int ret; + s16 outval; + + pm_runtime_get_sync(&ak8974->i2c->dev); + mutex_lock(&ak8974->lock); + + /* + * We read all axes and discard all but one, for optimized + * reading, use the triggered buffer. + */ + ret = ak8974_trigmeas(ak8974); + if (ret) + goto out_unlock; + ret = ak8974_getresult(ak8974, hw_values); + if (ret) + goto out_unlock; + /* + * This explicit cast to (s16) is necessary as the measurement + * is done in 2's complement with positive and negative values. + * The follwing assignment to *val will then convert the signed + * s16 value to a signed int value. + */ + outval = (s16)le16_to_cpu(hw_values[address]); + *val = outval; +out_unlock: + mutex_unlock(&ak8974->lock); + pm_runtime_mark_last_busy(&ak8974->i2c->dev); + pm_runtime_put_autosuspend(&ak8974->i2c->dev); + + return ret; +} + static int ak8974_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) { struct ak8974 *ak8974 = iio_priv(indio_dev); - __le16 hw_values[3]; - int ret = -EINVAL; - - pm_runtime_get_sync(&ak8974->i2c->dev); - mutex_lock(&ak8974->lock); + int ret; switch (mask) { case IIO_CHAN_INFO_RAW: if (chan->address > 2) { dev_err(&ak8974->i2c->dev, "faulty channel address\n"); - ret = -EIO; - goto out_unlock; + return -EIO; } - ret = ak8974_trigmeas(ak8974); + ret = ak8974_measure_channel(ak8974, chan->address, val); if (ret) - goto out_unlock; - ret = ak8974_getresult(ak8974, hw_values); - if (ret) - goto out_unlock; - - /* - * We read all axes and discard all but one, for optimized - * reading, use the triggered buffer. - */ - *val = (s16)le16_to_cpu(hw_values[chan->address]); - - ret = IIO_VAL_INT; + return ret; + return IIO_VAL_INT; } - out_unlock: - mutex_unlock(&ak8974->lock); - pm_runtime_mark_last_busy(&ak8974->i2c->dev); - pm_runtime_put_autosuspend(&ak8974->i2c->dev); - - return ret; + return -EINVAL; } static void ak8974_fill_buffer(struct iio_dev *indio_dev)
This breaks out the measurement code to its own function so we can handle this without swirling it up with the big switch() statement inside ak8974_read_raw(). Keep a local s16 helper variable for the signed value coming out of the measurement before assigning it to the integer *val. The local variable makes the code easier to read and the compiler will optimize it if possible. Cc: Nick Reitemeyer <nick.reitemeyer@web.de> Cc: Stephan Gerhold <stephan@gerhold.net> Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v2->v3: - Return directly from the raw read function, we need no goto:s as we got rid of the lock. - Change the measurement function to return an int * after measurement and just pass *val to the function saving a local variable. - Insert a comment explaining the explicit cast to (s16). - Rename function as ak8974_measure_channel() so the name states exactly what is going on. - Break out as a separate patch. --- drivers/iio/magnetometer/ak8974.c | 68 +++++++++++++++++++------------ 1 file changed, 42 insertions(+), 26 deletions(-)