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.
