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]

Message-ID: <20240623171946.GB202685@vamoiridPC>
Date: Sun, 23 Jun 2024 19:19:46 +0200
From: Vasileios Amoiridis <vassilisamir@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Vasileios Amoiridis <vassilisamir@...il.com>, 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 Sun, Jun 23, 2024 at 05:26:15PM +0100, Jonathan Cameron wrote:
> 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

Well, thanks for the advice :)

Cheers,
Vasilis