On Thu, Apr 16, 2020 at 04:09:16PM +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
> bis switch() statement inside ak8974_read_raw().
>
> Use an intermediary s16* variable since we read s16 but
> the external API required an int* so this way we get
> explicit casting.
>
> Cc: Nick Reitemeyer <nick.reitemeyer@xxxxxx>
> Cc: Stephan Gerhold <stephan@xxxxxxxxxxx>
> Cc: Michał Mirosław <mirq-linux@xxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v1->v2:
> - Break out as a separate patch.
> ---
> drivers/iio/magnetometer/ak8974.c | 51 +++++++++++++++++++------------
> 1 file changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
> index ade4ed8f67d2..5361647b9054 100644
> --- a/drivers/iio/magnetometer/ak8974.c
> +++ b/drivers/iio/magnetometer/ak8974.c
> @@ -554,46 +554,57 @@ static int ak8974_detect(struct ak8974 *ak8974)
> return 0;
> }
>
> +static int ak8974_measure(struct ak8974 *ak8974, unsigned long address, s16 *val)
> +{
> + __le16 hw_values[3];
> + int ret;
> +
> + pm_runtime_get_sync(&ak8974->i2c->dev);
> + mutex_lock(&ak8974->lock);
> +
> + ret = ak8974_trigmeas(ak8974);
> + if (ret)
> + goto out_unlock;
> + ret = ak8974_getresult(ak8974, hw_values);
> + if (ret)
> + goto out_unlock;
> + *val = (s16)le16_to_cpu(hw_values[address]);
This will still work if the 'val' argument was 'int *'. And it would make
'outval' in the caller redundant.
> +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);
> + s16 outval;
>
> 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;
> + goto out_err_read;
You could get rid of the gotos to shorten and clear up the code.
> }
> - ret = ak8974_trigmeas(ak8974);
> - 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 = ak8974_measure(ak8974, chan->address, &outval);
> + if (ret)
> + goto out_err_read;
> + *val = outval;
> ret = IIO_VAL_INT;
> + break;
> }
> -
> - out_unlock:
> - mutex_unlock(&ak8974->lock);
> - pm_runtime_mark_last_busy(&ak8974->i2c->dev);
> - pm_runtime_put_autosuspend(&ak8974->i2c->dev);
> -
> +out_err_read:
> return ret;
> }
[...]
Best Regards,
Michał Mirosław
