linux-kernel - Re: [PATCH 2/2] iio: adc: ad7476: Support ROHM BU79100G

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

Message-ID: <5ed56b89-8a9b-464f-9b87-f6553395a941@gmail.com>
Date: Wed, 14 May 2025 12:21:30 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Jonathan Cameron <jic23@...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: adc: ad7476: Support ROHM BU79100G

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.

Yours,
	-- Matti