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 for Android: free password hash cracker in your pocket

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

Message-ID: <aSf3RUeghPcC80VG@sunspire.home.arpa>
Date: Thu, 27 Nov 2025 09:01:25 +0200
From: Petre Rodan <petre.rodan@...dimension.ro>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
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

Hello Andy,

thank you for your feedback thus far.

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.

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

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

> > #define ABP2_MEASUREMENT_RD_SIZE 7
> > 
> > struct abp2_data {
> > 	struct device *dev;
> > 	const struct abp2_ops *ops;
> > 	s32 pmin;
> > 	s32 pmax;
> [..]
> > 	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.

I tried the code above, printk-ed the pointers of those arrays (and the timestamp)
and they all get nicely 64byte aligned.
since I've been told in the past both buffers need to be aligned, I guess this
is the code you require?

best regards,
peter