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

[barnabas . czeman]

On 2024-08-06 18:19, Jonathan Cameron wrote:
> On Tue, 06 Aug 2024 08:10:18 +0200
> Barnabás Czémán <barnabas.czeman@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> >  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.
It is checked exactly before the measurement data read, it is the return 
value of ak8975_start_read_axis.
The read section should be ST1 -> measurement -> ST2, exactly the same 
can be found in the datasheets.
> 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.
>
> >