Re: [PATCH 1/2 v2] iio: magnetometer: ak8974: Break out measurement

["Michał Mirosław" <mirq-linux@rere.qmqm.pl>]

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