diff mbox series

[v5,03/10] iio: accel: adxl345: measure right-justified

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

Commit Message

Lothar Rubusch Dec. 5, 2024, 5:13 p.m. UTC
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(-)

Comments

Jonathan Cameron Dec. 8, 2024, 1:34 p.m. UTC | #1
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;
Lothar Rubusch Dec. 9, 2024, 10:18 p.m. UTC | #2
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;
>
Jonathan Cameron Dec. 11, 2024, 6:55 p.m. UTC | #3
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 mbox series

Patch

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;