Re: [PATCH v2 05/22] iio: accel: adxl345: measure right-justified

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 26 Nov 2024 14:51:19 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Dear IIO Mailing-List, Hi Jonathan!
> 
> Thank you so much for the review. As you probably saw, most (all?) of
> my commits have a huge invisible question mark attached. Most of my
> questions you answered clearly. On particular topics I'd like to get
> back, though. Generally I will try to apply the requested changes to
> best of my understanding.
> 
> On Sun, Nov 24, 2024 at 7:07 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 17 Nov 2024 18:26:34 +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.  
> >
> > Surely this can't be changed independent of other changes as it will
> > change the format of the data we are processing?
> >
> > Each change must stand on it's own so that I can apply up to any
> > point in your patch set and have everything continue to work.  
> 
> This is probably not quite clear. Originally the driver was
> right-justified. One of my last commits
> (f68ebfe1501bf1110eebf5e968c4d9186cba8706) changed the driver to work
> with left-justified measurements. So, I feel changing the orginal
> behavior is wrong, and here I try to re-establish the original driver
> behavior.
> 
> When looking at the datasheet right-justified data seems to be easier
> to handle, but I don't have any personal preference.
Ok.  I'm still a little confused. Can userspace see any result from either
the earlier patch or this one?

Jonathan

> 
> Lothar
> 
> [...]
> > > ---
> > >  drivers/iio/accel/adxl345_core.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 2b62e79248..926e397678 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -184,8 +184,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > >       struct adxl34x_state *st;
> > >       struct iio_dev *indio_dev;
> > >       u32 regval;
> > > +
> > > +     /* NB: ADXL345_DATA_FORMAT_JUSTIFY or 0:  
> >         /*
> >          * NB: AD...
> >
> > is the multiline comment style all IIO drivers use (and most of the kernel
> > except for networking.
> >  
> > > +      * do right-justified: 0, then adjust resolution according to 10-bit
> > > +      * through 13-bit in channel - this is the default behavior, and can
> > > +      * be modified here by oring ADXL345_DATA_FORMAT_JUSTIFY
> > > +      */
> > >       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;  
> >