Re: [PATCH v3 2/4] iio: magnetometer: ak8975: Fix reading for ak099xx sensors

[Jonathan Cameron]

On Fri, 09 Aug 2024 22:25:40 +0200
Barnabás Czémán <barnabas.czeman@xxxxxxxxxxxxxx> wrote:

> Move ST2 reading with overflow handling after measurement data
> reading.
> ST2 register read have to be read after read measurment data,
> because it means end of the reading and realease the lock on the data.
> Remove ST2 read skip on interrupt based waiting because ST2 required to
> be read out at and of the axis read.
> 
> Signed-off-by: Barnabás Czémán <barnabas.czeman@xxxxxxxxxxxxxx>
This still needs a fixes tag to tell us when the bug was first
introduced into the driver so that we know how far to back port it.

One other comment inline.

Thanks,

Jonathan

> ---
>  drivers/iio/magnetometer/ak8975.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 6357666edd34..f8355170f529 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;
> -		}
> -	}
> -
> -	return 0;
> +	return !data->def->ctrl_regs[ST1_DRDY];
>  }
>  
>  /* Retrieve raw flux value for one of the x, y, or z axis.  */
> @@ -734,6 +719,20 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>  	if (ret < 0)
>  		goto exit;
>  
> +	/* Read out ST2 for release lock */
> +	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;
> +	}
> +
>  	mutex_unlock(&data->lock);
>  
>  	pm_runtime_mark_last_busy(&data->client->dev);
> @@ -746,7 +745,7 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>  
>  exit:
>  	mutex_unlock(&data->lock);
> -	dev_err(&client->dev, "Error in reading axis\n");
> +	dev_err(&client->dev, "Error in reading axis %d\n", ret);

In most of the paths above there is already a detailed print. I'd not bother
adding the return value to this one.  It just adds noise to the patch.

>  	return ret;
>  }
>  
>