Message-ID: <f8ec547a-5924-4563-aa1d-dde8227844fa@gmail.com>
Date: Mon, 26 May 2025 09:17:54 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 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,
 Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: ad7476: Support ROHM BU79100G

On 15/05/2025 20:06, Jonathan Cameron wrote:
> On Wed, 14 May 2025 12:21:30 +0300
> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> 
>> On 14/05/2025 10:38, Matti Vaittinen wrote:
>>> On 13/05/2025 11:26, Matti Vaittinen wrote:
>>>> ROHM BU79100G is a 12-bit, single channel ADC. Support reading ADC
>>>> measurements using the ad7476.c
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
>>>> ---
>>>>    drivers/iio/adc/ad7476.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>
>>> For anyone who might hit this mail thread later:
>>>
>>> Conor made me realize that, for now, the BU79100G looks identical to the
>>> ads7866. Thus, these code-changes aren't needed at the moment, and this
>>> patch can be dropped. For those who wish to use BU79100G, please
>>> introduce it as
>>>
>>> compatible = "rohm,bu79100g", "ti,ads7866";
>>>    
>>
>> I was too hasty.
>>
>> It seems to me that the fallback won't work with the current driver
>> because the driver is not populating the of_match_table, but is relying
>> solely on the spi_device_id table.
>>
>> Judging a quick code reading, the spi_driver_id table entries are
>> matched to the modalias:
>> https://elixir.bootlin.com/linux/v6.15-rc6/source/drivers/spi/spi.c#L393
>>
>> Which is (as far as I understand), generated from the first compatible:
>> https://elixir.bootlin.com/linux/v6.15-rc6/source/drivers/of/base.c#L1170
>>
>> and not from the fallback one.
>>
>> I suppose this means that we would need to add the of_match_table entry
>> for the ti,ads7866 to make the fallback entry to match the driver.
>>
>> But...
>>
>> The __spi_register_driver() has following comment:
>> 	/*
>> 	 * For Really Good Reasons we use spi: modaliases not of:
>> 	 * modaliases for DT so module autoloading won't work if we
>> 	 * don't have a spi_device_id as well as a compatible string.
>> 	 */
>> https://elixir.bootlin.com/linux/v6.15-rc6/source/drivers/spi/spi.c#L487
>>
>> So, having the of_match_table for would not be sufficient for the
>> autoloading, which would still require the bu79100g to be in the
>> spi_device_id table.
>>
>> Am I missing something? I don't see how the Linux SPI drivers benefit
>> from the fallback entries in the dt? (Not saying fallbacks wouldn't be
>> The Right Thing To Do. Ideally DTs aren't for Linux only, maybe some
>> other systems can utilize them). To me it seems I still need to add the
>> spi_device_id entry for the BU79100G, and of_match_table has no
>> additional benefit? If this is right, then this patch is still relevant,
>> even though the binding should be done as in v2.
> +CC Mark Brown and linux-spi.

A quick test with device-tree having SPI device adc0:

adc: adc@0 {
	compatible = "rohm,bu79100g", "ti,ads7866";
};

yields:
root@arm:/home/debian# cat 
/sys/devices/platform/ocp/48000000.interconnect/48000000.interconnect:segment@...8030000.target-module/48030000.spi/spi_master/spi0/spi0.0/modalias
spi:bu79100g

AFAICS, this means the module must have an ID for the 'bu79100g' - 
relying on the 'ti,ads7866' do not work.

Just for a comparison, (platform device) serial node:

uart0: serial@...09000 {
	compatible = "ti,am3352-uart", "ti,omap3-uart";
};

yields:
root@arm:/home/debian# cat 
/sys/devices/platform/ocp/44c00000.interconnect/44c00000.interconnect:segment@...000/44e09050.target-module/44e09000.serial/modalias
of:NserialT(null)Cti,am3352-uartCti,omap3-uart

- which I suppose means the userland can match also modules which only 
have the 'ti,omap3-uart' fallback without the 'ti,am3352-uart'

I spent some quality time reading:

https://lore.kernel.org/all/564B2DD5.2060502@osg.samsung.com/
https://lkml.org/lkml/2014/9/19/179
https://lkml.org/lkml/2015/8/20/109

What I picked from it is that we have ... intersting history.

If I understood it right (please, correct me if I didn't):

- For historical reasons, the SPI core always sends spi:<foo> modalias, 
instead of device-tree variant of:vendor,<foo>
- The of-variant (from other subsystems) seems to be containing modalias 
matching all the compatibles, including potential fallback compatibles.
- The spi-variant seems to only contain a single modalias, matching the 
first compatible and not the fallback(s).
- The user-space uses the SPI one, or the OF one, not both. Eg, sending 
both wouldn't help unless user-space was changed.
- The SPI variant, for historical reasons, does not include vendor prefix.

=> Changing from SPI one to OF one has not been the way to go.

What I don't know is if the spi:<foo> modalias list could be expanded to 
also include the fallbacks from the device-tree, or if the user-space 
loading implementation(s) would work with multiple spi-modaliases.

Anyhow, it seems to me we don't currently have a de-facto way to utilize 
the device-tree fallback information in the SPI device drivers(?), but 
we need to populate all the IDs in the driver's ID list(s).

So, in order to (reliably) support the BU79100G (with currently existing 
user-space module loading), we need to add the ID for it in the driver 
(as was done in the v1)?

Any better opinions? :)

Yours,
	-- Matti

Powered by blists - more mailing lists