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

[Jonathan Cameron <jic23@kernel.org>]

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;  
> >