Re: [PATCH v3 1/3] iio: mpl3115: use get_unaligned_be24 to retrieve pressure data

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 10 Nov 2025 16:59:32 +0100
Antoni Pokusinski <apokusinski01@gmail.com> wrote:

> On Sun, Nov 09, 2025 at 04:38:40PM +0000, Jonathan Cameron wrote:
> > On Thu, 6 Nov 2025 22:33:49 -0300
> > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> >   
> > > On 11/05, Antoni Pokusinski wrote:  
> > > > The pressure measurement result is arranged as 20-bit unsigned value
> > > > residing in three 8-bit registers. Hence, it can be retrieved using
> > > > get_unaligned_be24 and by applying 4-bit shift.
> > > > 
> > > > Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
> > > > ---
> > > >  drivers/iio/pressure/mpl3115.c | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c    
> > > ...  
> > > >  
> > > > -		*val = be32_to_cpu(tmp) >> chan->scan_type.shift;
> > > > +		*val = get_unaligned_be24(tmp) >> 4;    
> > > hmm, now the number of bits shifted is dissociated from the channel characteristics.
> > > We can do
> > > 		*val = get_unaligned_be24(tmp) >> (24 - chan->scan_type.realbits);  
> > This encodes that the field is always aligned to the maximum bit. Whilst it might
> > be true, there is nothing inherent that says it must be.
> > 
> > I'm not sure why we aren't using chan->scan_type.shift though.  
> The chan->scan_type.shift is 12 for the pressure channel, because
> .realbits is 32. In order to better reflect the actual data format,
> the pressure .shift and .realbits should be changed to 4 and 24 respectively
> and the we could use the chan->scan_type.shift in here indeed.
> 
> But then the `iio_generic_buffer` tool should also be updated so that it
> can manage the scan_data with realbits not being in the form 2^n.
> Currently it supports only scan sizes of 1,2,4,8 bytes [1].

> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/tools/iio/iio_generic_buffer.c#n189

I think this is confusing storagebits and realbits.

storagebits is always power of 2 * 8 because we want them naturally aligned for
efficient accesses.  realbits is however many bits of actual data we have, so once we
shift off the bottom "shift" bits, how many to mask.
  
This confusion isn't helped by inconsistent names between that
tool and the kernel. 

Anyhow, I indeed now see why it is shifted by 4 here - thanks for talking me through it!

You could do the much messier
*val = get_unaligned_be24(tmp) >> (chan->scan_type.shift - (chan->scan_type_storage_bits - 24));
Which is hideous.  Perhaps a comment will do the job.

/*
 * Note that chan->scan_type.shift accounts for 24 bit big endian data being
 * read into the lower addresses of a 32 bit buffer - hence shift here is 4 rather
 * than 12.
 */

Or as another option. Could do in _fill_trig_buffer() do

ret = i2c_smbus_read_i2c_block_data(data->client,
			MPL3115_OUT_PRESS, 3, &buffer[pos + 1]);
Then set the shift for the pressure channel to 4.  That is, read the 3 bytes
after leave the most significant byte as 0. 

Whilst technically an ABI change, and correctly written software shouldn't notice.

Jonathan

 
> >   
> > > or maybe
> > > 		*val = get_unaligned_be24(tmp) >> (sizeof(tmp) - chan->scan_type.realbits);  
> > 
> > That one needs a BYTES_TO_BITS factor too.
> >   
> > > but it starts becoming too long IMO. Even longer if `tmp` gets a more meaningful
> > > name. Ah well, any of the three forms should work the same at the end of day so
> > > no strong opinion.
> > > 
> > > Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
> > >   
> > > >  		return IIO_VAL_INT;
> > > >  	}
> > > >  	case IIO_TEMP: { /* in 0.0625 celsius / LSB */
> > > > -- 
> > > > 2.25.1
> > > >     
> >  
> Kind regards,
> Antoni Pokusinski
>