On Sat, Oct 30, 2021 at 9:04 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Sat, 30 Oct 2021 13:35:19 +0200
> Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
>
> > On 10/30/21 1:18 PM, Gwendal Grignou wrote:
> > > When calling sign_extend32() on a channel, use its .realbit information
> > > to limit the number of bits to process, instead of a constant.
> > >
> > > Changed only simple sign_extend32() calls, when .realbits was defined in
> > > the same file. Use 'grep -r sign_extend32 $(grep -lr realbits drivers/iio/)'
> > > to locate the files.
> > >
> > > Some files were not processed:
> > > gyro/fxas21002c_core.c : function parameter changes needed.
> > > health/max30102.c: Incomplete channel definition.
> > > health/max30100.c
> >
> > I think this is good work, but it seems a bit out of place in this
> > series. I think it will be easier to get this reviewed and merged if it
> > is submitted independently. It might make sense to only have the sx9310
> > changes as part of this series and send the other ones as a separate patch.
When moving code to sx9310, I notice we were using constants instead
of realbits, and I look at how other driver deal with bit information.
> >
> > What's also missing in the commit description is the motivation for
> > this. The generated code will be a bit more complex, so there needs to
> > be a good justification. E.g. having a single source of truth for this
> > data and avoiding accidental mistakes.
That's the idea, I will update the commit accordingly.
> >
> > The patch also uses `shift` were applicable, which is not mentioned in
> > the commit dscription.
>
> Be careful. I have seen devices (with FIFOs) where the realbits doesn't
> necessarily match with a separate read path used for polled reads.
>
> It is an option for the sca3000 for example but that's carrying a hack where
> ignore that and rely on some coincidental data alignment to pretend realbits
> is 13 when it's actually 11.
I see the hack in sca3000_ring_int_process(). The driver reconciles
the FIFO and raw data access by presenting elements as carrying 13
bits of information, assuming the 2 LSB are 0.
Given user space should get the same data when reading using the _raw
attributes or the buffer, given libiio use scan_type information to
process buffer data, (see libiio iio_channel_convert())
it makes sense for the kernel to always use scan_type as well when
presenting the _raw attributes.
>
> Still in general it's a reasonable change but agree with Lars, separate series
> please.
Will do.
>
> >
> >
> > > [...]
> > > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > > index 1eb9e7b29e050..355854f0f59d2 100644
> > > --- a/drivers/iio/pressure/mpl3115.c
> > > +++ b/drivers/iio/pressure/mpl3115.c
> > > @@ -74,7 +74,7 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> > > int *val, int *val2, long mask)
> > > {
> > > struct mpl3115_data *data = iio_priv(indio_dev);
> > > - __be32 tmp = 0;
> > > + __be16 tmp = 0;
> > > int ret;
> > The be32 to be16 change might warrant its own patch. This is definitely
> > changing the behavior of the driver. And I don't think it is correct the
> > way its done. For the pressure data it is reading 3 bytes, which will
> > cause a stack overflow.
You're right, it is wrong as is. Resent in a separate patch.
> > >
> > > switch (mask) {
> > > @@ -96,7 +96,7 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> > > mutex_unlock(&data->lock);
> > > if (ret < 0)
> > > break;
> > > - *val = be32_to_cpu(tmp) >> 12;
> > > + *val = be32_to_cpu(tmp) >> chan->scan_type.shift;
> > > ret = IIO_VAL_INT;
> > > break;
> > > case IIO_TEMP: /* in 0.0625 celsius / LSB */
> > > @@ -111,7 +111,8 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> > > mutex_unlock(&data->lock);
> > > if (ret < 0)
> > > break;
> > > - *val = sign_extend32(be32_to_cpu(tmp) >> 20, 11);
> > > + *val = sign_extend32(be16_to_cpu(tmp) >> chan->scan_type.shift,
> > > + chan->scan_type.realbits - 1);
> > > ret = IIO_VAL_INT;
> > > break;
> > > default:
> >
>
Thanks for the prompt reviews and the feedback,
Gwendal.
