Re: [PATCH 1/5] iio: Use .realbits to extend a small signed integer

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 30 Oct 2021 13:35:19 +0200
Lars-Peter Clausen <lars@metafoo.de> 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:  
>