[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240806171925.7c512c63@jic23-huawei>
Date: Tue, 6 Aug 2024 17:19:25 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Barnabás Czémán <barnabas.czeman@...nlining.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 v2 1/3] iio: magnetometer: ak8975: Fix reading for
ak099xx sensors
On Tue, 06 Aug 2024 08:10:18 +0200
Barnabás Czémán <barnabas.czeman@...nlining.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@...nlining.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.
>
>
Powered by blists - more mailing lists