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: <aSTJML3fxp0sSeCq@lipo.home.arpa>
Date: Mon, 24 Nov 2025 23:08:00 +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!

On Mon, Nov 24, 2025 at 08:41:32PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 07:29:06PM +0200, Petre Rodan wrote:
> 
> > > Why explicit assignments? Is it related to some HW register?
> > 
> > no registers, I just need to ensure the two arrays
> > 
> > static const char * const abp2_triplet_variants[ABP2_VARIANTS_MAX] = {
> > 	[ABP2001BA] = "001BA", [ABP21_6BA] = "1.6BA", [ABP22_5BA] = "2.5BA", ..
> > 
> > static const struct abp2_range_config abp2_range_config[ABP2_VARIANTS_MAX] = {
> > 	[ABP2001BA] = { .pmin =       0, .pmax =  100000 },
> >    	[ABP21_6BA] = { .pmin =       0, .pmax =  160000 }, ..
> > 
> > keep being consistent and are resistant to later editing.
> 
> So, if it's pure software numbering, just drop assignments in the enum.

so you want the consistency check to be dropped? we have data in two different arrays and they must be kept in perfect sync. if I were to remove the assignments someone comes a few years in the future, inserts a new device in the abp2_triplet_variants array at position 84 out of 113, also inserts a new {pmin, pmax} into the abp2_range_config array accidentally at position 83 and the compiler will be none the wiser.
just the day before I had to remove a variant because of a typo in the datasheet. I cheat and use a script to generate the structs [1], but if I had to modify them by hand, the assignments would make sure I delete the proper line.

am I proud of this? no, and I told you my preference. this is just a compromise that uses the non-specific match function and still provides a guardrail for future editing.

[1] https://codeberg.org/subDIMENSION/lkm_sandbox/src/branch/main/honeywell_abp2030pa/scripts/parse_variants_table.sh

[..]

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

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

I also have a bughunt in the spi-omap2-mcspi driver related to improper CS delays in queued transfers, regmap will probably just be an extra layer of abstraction I will have to go thru :/

[..]

oh, and

struct abp2_spi_buf {
	u8 tx[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
};

static int abp2_spi_init(struct device *dev)
{
	struct spi_device *spi = to_spi_device(dev);
	struct abp2_spi_buf *buf;

	buf = devm_kzalloc(dev, sizeof(*buf), GFP_KERNEL);

> Using devm_*() here is most likely a mistake. What is the object lifetime in
> comparison to the used device?

I did think that placing this into the abp2_data struct would be a better idea, but I was not sure how to handle the alignment issue since there is already the read buffer there:

#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);
};

how do I make sure both 7byte buffers are aligned? can I __align twice in a struct as above? or should I align only the first buffer and make it 8bytes long?
I had a close look and even if the SoC's SPI driver supports both DMA and PIO, I've seen it pick PIO every single time while talking to my pressure sensor.

best regards,
peter

-- 
petre rodan