Re: [PATCH v2 1/3] iio: magnetometer: ak8975: Fix reading for ak099xx sensors

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 06 Aug 2024 08:10:18 +0200
Barnabás Czémán <barnabas.czeman@mainlining.org> wrote:

Hi Barnabás,

Welcome to IIO.

> ST2 register read should be placed after read measurment data,
> because it will get correct values after it.

What is the user visible result of this? Do we detect errors when none
are there?  Do we have a datasheet reference for the status being
update on the read command, not after the trigger?
>
Needs a Fixes tag to let us know how far to backport the fix.

A few comments inline.
 
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> ---
>  drivers/iio/magnetometer/ak8975.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index dd466c5fa621..925d76062b3e 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -692,22 +692,7 @@ static int ak8975_start_read_axis(struct ak8975_data *data,
>  	if (ret < 0)
>  		return ret;
>  
> -	/* This will be executed only for non-interrupt based waiting case */
> -	if (ret & data->def->ctrl_masks[ST1_DRDY]) {
> -		ret = i2c_smbus_read_byte_data(client,
> -					       data->def->ctrl_regs[ST2]);
> -		if (ret < 0) {
> -			dev_err(&client->dev, "Error in reading ST2\n");
> -			return ret;
> -		}
> -		if (ret & (data->def->ctrl_masks[ST2_DERR] |
> -			   data->def->ctrl_masks[ST2_HOFL])) {
> -			dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
> -			return -EINVAL;
> -		}
> -	}
> -
This completely removes the check from the _fill_buffer() path

> -	return 0;
> +	return !(ret & data->def->ctrl_masks[ST1_DRDY]);
returning a positive value here is unusual enough you should add a comment for
the function + use that return value.

>  }
>  
>  /* Retrieve raw flux value for one of the x, y, or z axis.  */
> @@ -731,6 +716,20 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>  	ret = i2c_smbus_read_i2c_block_data_or_emulated(
>  			client, def->data_regs[index],
>  			sizeof(rval), (u8*)&rval);
No longer gated on ret & data->def->ctrl_masks[ST1_DRDY] which seems unintentional.

Still need a check on ret here.

> +	ret = i2c_smbus_read_byte_data(client,
> +				       data->def->ctrl_regs[ST2]);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Error in reading ST2\n");
> +		goto exit;
> +	}
> +
> +	if (ret & (data->def->ctrl_masks[ST2_DERR] |
> +		   data->def->ctrl_masks[ST2_HOFL])) {
> +		dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
>  	if (ret < 0)
>  		goto exit;

And this one ends up redundant I think which suggests to me the
code is inserted a few lines early.

>  
>