| Message ID | 20240806-ak09918-v2-1-c300da66c198@mainlining.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AK09918 | expand |
On Tue, 06 Aug 2024 08:10:18 +0200 Barnabás Czémán <barnabas.czeman@mainlining.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@mainlining.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. > >
On August 6, 2024 6:19:25 PM GMT+02:00, Jonathan Cameron <jic23@kernel.org> wrote: >On Tue, 06 Aug 2024 08:10:18 +0200 >Barnabás Czémán <barnabas.czeman@mainlining.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? Second read will fail. In the datasheet ST2 comes after measurment data read. Here is some explanation from datasheet. "When ST2 register is read, AK09918 judges that data reading is finished. Stored measurement data is protected during data reading and data is not updated. By reading ST2 register, this protection is released. It is required to read ST2 register after data reading." So if ST2 is read before measurment it will stuck at protected mode. >> >Needs a Fixes tag to let us know how far to backport the fix. I think it is broken since 09912 was added but i cannot verify i have only devices with 09918. > >A few comments inline. > >> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.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. > >> >> >
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@mainlining.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@mainlining.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. 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. > >> >>
On Tue, 06 Aug 2024 19:54:56 +0200 Barnabás Czémán <barnabas.czeman@mainlining.org> wrote: > On August 6, 2024 6:19:25 PM GMT+02:00, Jonathan Cameron <jic23@kernel.org> wrote: > >On Tue, 06 Aug 2024 08:10:18 +0200 > >Barnabás Czémán <barnabas.czeman@mainlining.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? > > Second read will fail. In the datasheet ST2 comes after measurment data read. Here is some explanation from datasheet. > > "When ST2 register is read, AK09918 judges that data reading is finished. Stored measurement data is > protected during data reading and data is not updated. By reading ST2 register, this protection is > released. It is required to read ST2 register after data reading." > Thanks. Please add more of that detail to the patch description for v3. > So if ST2 is read before measurment it will stuck at protected mode. > >> > >Needs a Fixes tag to let us know how far to backport the fix. > I think it is broken since 09912 was added but i cannot verify i have only devices with 09918. > > I wasn't meaning devices, but rather what patch broke the kernel code. It might be the original driver introduction. If we can add a Fixes tag that makes it much easier for stable + distributions to work out whether to pick the fix up or not.
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; - } - } - - return 0; + return !(ret & data->def->ctrl_masks[ST1_DRDY]); } /* 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); + 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;
ST2 register read should be placed after read measurment data, because it will get correct values after it. Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org> --- drivers/iio/magnetometer/ak8975.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-)