linux-kernel - Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver

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] [day] [month] [year] [list]

Message-ID: <aSgNuVyyA6AtxtKs@smile.fi.intel.com>
Date: Thu, 27 Nov 2025 10:37:13 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Petre Rodan <petre.rodan@...dimension.ro>
Cc: Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>, Nuno S? <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver

On Thu, Nov 27, 2025 at 09:01:25AM +0200, Petre Rodan wrote:
> On Tue, Nov 25, 2025 at 12:54:15AM +0200, Andy Shevchenko wrote:

...

> > > > > > So, why can't regmap SPI be used?
> > > > > 
> > > > > there are no registers, no memory map, just a 'start conversion' and the
> > > > > equivalent of a 'read conversion' command.
> > > > > any reason one would use the regmap API in this case?
> > > > 
> > > > At bare minimum the commit message should have a justification for the choice
> > > > explaining all this.
> > > 
> > > I had the justification in the cover letter instead, my bad, will include it in
> > > the commit message instead.
> > 
> > It's good to have in both.
> > 
> > > > Ideally, try to find a way how to use regmap API. We have several weeks of
> > > > time for this exercise.
> > > 
> > > you did not mention why use an API designed for devices with registers and a
> > > memory map on an IC that has neither.
> > 
> > The regmap provides several facilities that we would like to use in the drivers:
> > - the generic interface to access to the HW
> 
> in general I agree that having bus functions behind an abstraction layer can lead
> to cleaner and less duplicated code in the driver.
> 
> however, afaict in this case I still have to use the exact same low level i2c/spi
> accesses and hide them behind a regmap_bus instead of an _ops struct.
> plus there seems to be duct tape to make things seem to be what they are not.
> so the complexity would just go up a notch.
> 
> sorry but I feel that this regmap iayer would not be a clean implementation.
> I might change my mind in the future of course.

OK.

> > - the common locking schema that allows to share the same regmap among
> > different drivers (depends on the functionality of the parts of the HW)
> 
> is it a good idea for anything external expecting a real regmap to interact with
> a driver that was made to mascarade using one, lock notwithstanding?
> everything about this particular driver is standalone.
> 
> > - debugging facilities are available out-of-the-box
> 
> in these cases I just use a signal analyzer and compare with the driver's output.
> just out of curiosity, you got any pointers on a non-blocking (asynchronous) bus
> transfer debug facility?

I am not sure I got the question right. The regmap has a trace events for read
and write, so it dumps all data goes on the bus. This is done synchronously and
it blocks for a few microseconds to collect the necessary (binary) date in the
trace buffer. I don't think this affects the timings of the real transfers on
the slow busses like SPI or especially I²C.

> > We have drivers in the kernel with two buffers in the same structure.
> 
> yup. some __align twice just like in my example, some __align just once, some use
> a simple buffer placed on the stack with no apparent alignment when sending the
> data.
> 
> I've been told during an older review that both buffers need to be aligned due to
> DMA-related requirements.

Correct, and this should be done for the buffers that are subject to DMA, PIO is
fine without that. Consideration of Tx/Rx split varies, indeed. And I'm not sure
that the authors of the drivers fully understand when and what alignment is
required. Jonathan once explained to me why only a single alignment is good and
second is not needed, but I forgot that, can't reproduce right now. It's
available on lore archive somewhere.

> as I mentioned before, in my case the SoC's spi driver always uses PIO mode even
> if DMA is also implemented. so testing my code always works no matter the alignment.

[..]

> > > 	struct {
> > > 		u32 chan[2];
> > > 		aligned_s64 timestamp;
> > > 	} scan;
> > > +	u8 spi_tx_buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> > > 	u8 buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> > > };
> > You told you read books about C language...
> > 
> > Alignment is a property of a single member and a data type in general. Each
> > field of each data type may have it's own (non-default) alignment along with
> > the object alignment.
> 
> I have a hard time following this paragraph so I'm forced to use my 'sorry
> English ain't one of my first languages' excuse.

OK. I forgot the context of the above, TBH.

-- 
With Best Regards,
Andy Shevchenko