[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a7da0c7296188542c14a54985995a8fa@mainlining.org>
Date: Sat, 17 Aug 2024 14:58:42 +0200
From: barnabas.czeman@...nlining.org
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Jonathan Albrieux <jonathan.albrieux@...il.com>,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux@...nlining.org
Subject: Re: [PATCH v3 2/4] iio: magnetometer: ak8975: Fix reading for ak099xx
sensors
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@...nlining.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@...nlining.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;
>> }
>>
>>
Powered by blists - more mailing lists