Message-ID: <aSTiFxAolJ0JeUTj@smile.fi.intel.com>
Date: Tue, 25 Nov 2025 00:54:15 +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 Mon, Nov 24, 2025 at 11:08:00PM +0200, Petre Rodan wrote:
> 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.

That's why enum is there. Had I told you to remove it? No! enum should stay
as well as the explicit indexed assignments, assignments in _enum_ should go.
Just look around how other drivers do with enums which are not related to
the HW registers.

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

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
- the common locking schema that allows to share the same regmap among
different drivers (depends on the functionality of the parts of the HW)
- debugging facilities are available out-of-the-box

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

We have drivers in the kernel with two buffers in the same structure.

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

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.

...

Homework:
Why do we need both to be aligned? Do you get the idea what is this all about?

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists