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.
>
> 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.
>
> 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.
Still in general it's a reasonable change but agree with Lars, separate series
please.
>
>
> > [...]
> > 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.
> >
> > 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:
>
