| Message ID | 20240809-ak09918-v3-2-6b036db4d5ec@mainlining.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AK09918 | expand |
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. 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; > } > >
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; >> } >> >>
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); return ret; }
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> --- drivers/iio/magnetometer/ak8975.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)