linux-kernel - Re: [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 23 Jun 2024 17:26:15 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: lars@...afoo.de, andriy.shevchenko@...ux.intel.com,
 ang.iglesiasg@...il.com, mazziesaccount@...il.com, ak@...klinger.de,
 petre.rodan@...dimension.ro, phil@...pberrypi.com, 579lpy@...il.com,
 linus.walleij@...aro.org, semen.protsenko@...aro.org,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, Adam Rizkalla
 <ajarizzo@...il.com>
Subject: Re: [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer
 support

On Sat, 22 Jun 2024 16:09:11 +0200
Vasileios Amoiridis <vassilisamir@...il.com> wrote:

> On Sat, Jun 22, 2024 at 10:40:39AM +0100, Jonathan Cameron wrote:
> > On Tue, 18 Jun 2024 01:05:40 +0200
> > Vasileios Amoiridis <vassilisamir@...il.com> wrote:
> >   
> > > BMP2xx, BME280, BMP3xx, and BMP5xx use continuous buffers for their
> > > temperature, pressure and humidity readings. This facilitates the
> > > use of burst/bulk reads in order to acquire data faster. The
> > > approach is different from the one used in oneshot captures.
> > > 
> > > BMP085 & BMP1xx devices use a completely different measurement
> > > process that is well defined and is used in their buffer_handler().
> > > 
> > > Suggested-by: Angel Iglesias <ang.iglesiasg@...il.com>
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@...il.com>
> > > Link: https://lore.kernel.org/r/20240512230524.53990-6-vassilisamir@gmail.com
> > > ---  
> > The sign extend in buffered path doesn't make much sense as we should be
> > advertising the correct bit depth for the channel and making that a userspace
> > problem.
> > 
> > I'd failed to notice you are doing endian conversions just to check
> > the skipped values. Ideally we'd leave the channels little endian
> > and include that in the channel spec.
> > 
> > Hmm. I guess this works and if we have to do the endian conversion
> > anyway isn't too bad.  It does provide slightly wrong information
> > to userspace though.
> > 
> > So even with this in place I think these channels should be real_bits 24.
> > 
> > 
> >   
> > > +static irqreturn_t bmp580_buffer_handler(int irq, void *p)
> > > +{
> > > +	struct iio_poll_func *pf = p;
> > > +	struct iio_dev *indio_dev = pf->indio_dev;
> > > +	struct bmp280_data *data = iio_priv(indio_dev);
> > > +	s32 adc_temp, adc_press;
> > > +	int ret;
> > > +
> > > +	guard(mutex)(&data->lock);
> > > +
> > > +	/* Burst read data registers */
> > > +	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB,
> > > +			       data->buf, BMP280_BURST_READ_BYTES);  
> 
> With the bulk read here, we have 24 bits of temperature and 24 bits 
> of pressure, so in total 6 bytes. The only way I can see to not do
> the endian conversion is that I memcpy the first 3 bytes to the
> data->sensor_data[1], and the last 3 bytes to the data->sensor_data[0].
> 
> If you can see any other better way please let me know, otherwise the
> other solution is to use get_unaligned_24(). Still, we won't do the
> skipping part.

If you return in cpu endian because it's convenient that is fine, but
still set the number of realbits to 24 or whatever is appropriate.

Or as you say memcpy the 3 bytes.  There is probably an arch out there
in which that is much more efficient than the endian fun, but I
can't be bothered to figure out how ;)

Jonathan