Message ID | 20241205171343.308963-4-l.rubusch@gmail.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events | expand |
On Thu, 5 Dec 2024 17:13:36 +0000 Lothar Rubusch <l.rubusch@gmail.com> wrote: > Make measurements right-justified, since it is the default for the > driver and sensor. By not setting the ADXL345_DATA_FORMAT_JUSTIFY bit, > the data becomes right-judstified. This was the original setting, there > is no reason to change it to left-justified, where right-justified > simplifies working on the registers. > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> I'm still confused by this one. Does this change affect the data output to userspace? If seems like it definitely should. If it does we have an ABI regression somewhere. Is it currently broken and wasn't at some earlier stage, or is this the patch breaking things? If it worked and currently doesn't send a fix. If this changes a previously working ABI then drop this patch. Alternative being to fix up the scale handling to incorporate this justification change. Jonathan > --- > drivers/iio/accel/adxl345_core.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c > index 88df9547bd6..98ff37271f1 100644 > --- a/drivers/iio/accel/adxl345_core.c > +++ b/drivers/iio/accel/adxl345_core.c > @@ -184,7 +184,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > struct iio_dev *indio_dev; > u32 regval; > unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE | > - ADXL345_DATA_FORMAT_JUSTIFY | > ADXL345_DATA_FORMAT_FULL_RES | > ADXL345_DATA_FORMAT_SELF_TEST); > int ret;
Dear IIO-ML, Hi Jonathan! On Sun, Dec 8, 2024 at 2:35 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 5 Dec 2024 17:13:36 +0000 > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > Make measurements right-justified, since it is the default for the > > driver and sensor. By not setting the ADXL345_DATA_FORMAT_JUSTIFY bit, > > the data becomes right-judstified. This was the original setting, there > > is no reason to change it to left-justified, where right-justified > > simplifies working on the registers. > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > > I'm still confused by this one. Does this change affect the data output > to userspace? If seems like it definitely should. If it does we have > an ABI regression somewhere. Is it currently broken and wasn't at some > earlier stage, or is this the patch breaking things? No, it should not affect the userspace. This setting opens the mask for regmap/update bits to allow for changing the data format. My point is rather, does it actually makes sense to allow to change the data format, since the driver will use just one format. The bit was never applied, it's just the mask here. May I ask you, if you could also could give me a brief feedback to the three questions in the cover letter to this series? I would really appreciate, since I'm still unsure if I actually verified everything correctly. From what I did about this bit, I removed and set the justified bit in STREAM and in BYPASSED mode (current mode), without any difference in the results in iio_info or iio_readdev. The numbers look generally odd to me, though. And, I'd rather like to ask to still wait with applying the patches, if this is ok for you? But, perhaps with the answers of the cover letter items, it could become clearer to me. I'm still about to measure and verify against the old and the input driver results as comparison. Best, L > If it worked and currently doesn't send a fix. If this changes a previously > working ABI then drop this patch. Alternative being to fix up the scale > handling to incorporate this justification change. > > Jonathan > > > --- > > drivers/iio/accel/adxl345_core.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c > > index 88df9547bd6..98ff37271f1 100644 > > --- a/drivers/iio/accel/adxl345_core.c > > +++ b/drivers/iio/accel/adxl345_core.c > > @@ -184,7 +184,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > > struct iio_dev *indio_dev; > > u32 regval; > > unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE | > > - ADXL345_DATA_FORMAT_JUSTIFY | > > ADXL345_DATA_FORMAT_FULL_RES | > > ADXL345_DATA_FORMAT_SELF_TEST); > > int ret; >
On Mon, 9 Dec 2024 23:18:45 +0100 Lothar Rubusch <l.rubusch@gmail.com> wrote: > Dear IIO-ML, Hi Jonathan! > > On Sun, Dec 8, 2024 at 2:35 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Thu, 5 Dec 2024 17:13:36 +0000 > > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > > > Make measurements right-justified, since it is the default for the > > > driver and sensor. By not setting the ADXL345_DATA_FORMAT_JUSTIFY bit, > > > the data becomes right-judstified. This was the original setting, there > > > is no reason to change it to left-justified, where right-justified > > > simplifies working on the registers. > > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > > > > I'm still confused by this one. Does this change affect the data output > > to userspace? If seems like it definitely should. If it does we have > > an ABI regression somewhere. Is it currently broken and wasn't at some > > earlier stage, or is this the patch breaking things? > > No, it should not affect the userspace. > > This setting opens the mask for regmap/update bits to allow for > changing the data format. > My point is rather, does it actually makes sense to allow to change > the data format, since > the driver will use just one format. The bit was never applied, it's > just the mask here. Ah. Got it. > > May I ask you, if you could also could give me a brief feedback to the > three questions in > the cover letter to this series? Reading cover letters? Never! :) Thanks for the heads up. I tend to skip straight past them unless I am looking for something specific... oops. > > I would really appreciate, since I'm still unsure if I actually > verified everything correctly. > From what I did about this bit, I removed and set the justified bit in > STREAM and in > BYPASSED mode (current mode), without any difference in the results in > iio_info or > iio_readdev. The numbers look generally odd to me, though. And, I'd > rather like to ask > to still wait with applying the patches, if this is ok for you? But, > perhaps with the answers > of the cover letter items, it could become clearer to me. I'm still > about to measure and > verify against the old and the input driver results as comparison. I'd use the tools/iio tooling. It pretty prints channel data. I suspect there are tools in the set you are using that do that but I'm not the person to ask. Jonathan > > Best, > L > > > > If it worked and currently doesn't send a fix. If this changes a previously > > working ABI then drop this patch. Alternative being to fix up the scale > > handling to incorporate this justification change. > > > > Jonathan > > > > > --- > > > drivers/iio/accel/adxl345_core.c | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c > > > index 88df9547bd6..98ff37271f1 100644 > > > --- a/drivers/iio/accel/adxl345_core.c > > > +++ b/drivers/iio/accel/adxl345_core.c > > > @@ -184,7 +184,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > > > struct iio_dev *indio_dev; > > > u32 regval; > > > unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE | > > > - ADXL345_DATA_FORMAT_JUSTIFY | > > > ADXL345_DATA_FORMAT_FULL_RES | > > > ADXL345_DATA_FORMAT_SELF_TEST); > > > int ret; > >
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c index 88df9547bd6..98ff37271f1 100644 --- a/drivers/iio/accel/adxl345_core.c +++ b/drivers/iio/accel/adxl345_core.c @@ -184,7 +184,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, struct iio_dev *indio_dev; u32 regval; unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE | - ADXL345_DATA_FORMAT_JUSTIFY | ADXL345_DATA_FORMAT_FULL_RES | ADXL345_DATA_FORMAT_SELF_TEST); int ret;
Make measurements right-justified, since it is the default for the driver and sensor. By not setting the ADXL345_DATA_FORMAT_JUSTIFY bit, the data becomes right-judstified. This was the original setting, there is no reason to change it to left-justified, where right-justified simplifies working on the registers. Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> --- drivers/iio/accel/adxl345_core.c | 1 - 1 file changed, 1 deletion(-)