[v3] iio: mpl3115: support for events

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

Posted by Antoni Pokusinski 1 month, 2 weeks ago

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
index c212dfdf59ff..3f1fa9fe3c76 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -16,6 +16,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/property.h>
+#include <linux/unaligned.h>
 
 #include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
@@ -125,7 +126,7 @@ static int mpl3115_read_info_raw(struct mpl3115_data *data,
 
 	switch (chan->type) {
 	case IIO_PRESSURE: { /* in 0.25 pascal / LSB */
-		__be32 tmp = 0;
+		u8 tmp[3];
 
 		guard(mutex)(&data->lock);
 		ret = mpl3115_request(data);
@@ -134,11 +135,11 @@ static int mpl3115_read_info_raw(struct mpl3115_data *data,
 
 		ret = i2c_smbus_read_i2c_block_data(data->client,
 						    MPL3115_OUT_PRESS,
-						    3, (u8 *) &tmp);
+						    sizeof(tmp), tmp);
 		if (ret < 0)
 			return ret;
 
-		*val = be32_to_cpu(tmp) >> chan->scan_type.shift;
+		*val = get_unaligned_be24(tmp) >> 4;
 		return IIO_VAL_INT;
 	}
 	case IIO_TEMP: { /* in 0.0625 celsius / LSB */
-- 
2.25.1

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

Posted by Marcelo Schmitt 1 month, 1 week ago

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);
or maybe
		*val = get_unaligned_be24(tmp) >> (sizeof(tmp) - chan->scan_type.realbits);
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
>

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

Posted by Jonathan Cameron 1 month, 1 week ago

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.

> 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
> >

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

Posted by Antoni Pokusinski 1 month, 1 week ago

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
> 
> > 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

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

Posted by Jonathan Cameron 1 month, 1 week ago

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
>

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

Posted by Andy Shevchenko 1 month, 1 week ago

On Wed, Nov 05, 2025 at 10:56:13AM +0100, 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.

get_unaligned_be24()

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
independently on the below.

...

> +		u8 tmp[3];

While at it, you also may rename it to something better

${foo}_be24;

where ${foo} should be replaced to the meaningful name.

-- 
With Best Regards,
Andy Shevchenko