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

[barnabas.czeman@mainlining.org]

On 2024-08-17 14:26, Jonathan Cameron wrote:
> On Fri, 09 Aug 2024 22:25:40 +0200
> Barnabás Czémán <barnabas.czeman@mainlining.org> 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@mainlining.org>
> 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.
I have got some test results from a device with ak09916 and that was 
working
without the fix so it seems only ak09918 is strict about ST2.
In theory all ak099xx should work like this so:
Fixes: 57e73a423b1e (iio: ak8975: add ak09911 and ak09912 support)
> 
> 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;
>>  }
>> 
>>