ROHM BU79100G is a 12-bit, single channel ADC. Support reading ADC
measurements using the ad7476.c

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/adc/ad7476.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index 37b0515cf4fc..6c5c57de6a58 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -2,6 +2,7 @@
 /*
  * Analog Devices AD7466/7/8 AD7476/5/7/8 (A) SPI ADC driver
  * TI ADC081S/ADC101S/ADC121S 8/10/12-bit SPI ADC driver
+ * ROHM BU79100G 12-bit SPI ADC driver
  *
  * Copyright 2010 Analog Devices Inc.
  */
@@ -72,6 +73,7 @@ enum ad7476_supported_device_ids {
 	ID_ADS7866,
 	ID_ADS7867,
 	ID_ADS7868,
+	ID_BU79100G,
 	ID_LTC2314_14,
 };
 
@@ -281,6 +283,10 @@ static const struct ad7476_chip_info ad7476_chip_info_tbl[] = {
 		.channel[0] = ADS786X_CHAN(8),
 		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
 	},
+	[ID_BU79100G] = {
+		.channel[0] = ADS786X_CHAN(12),
+		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+	},
 	[ID_LTC2314_14] = {
 		.channel[0] = AD7940_CHAN(14),
 		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
@@ -311,6 +317,7 @@ static int ad7476_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
+
 	st->chip_info =
 		&ad7476_chip_info_tbl[spi_get_device_id(spi)->driver_data];
 
@@ -435,6 +442,7 @@ static const struct spi_device_id ad7476_id[] = {
 	{ "ads7866", ID_ADS7866 },
 	{ "ads7867", ID_ADS7867 },
 	{ "ads7868", ID_ADS7868 },
+	{ "bu79100g", ID_BU79100G },
 	{ "ltc2314-14", ID_LTC2314_14 },
 	{ }
 };
-- 
2.49.0

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@gmail.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";

having the ads7866 as a fallback. This should allow us to introduce 
driver side differences later, should they be needed.

V2: 
https://lore.kernel.org/all/4907a096eee1f54afae834213cf721b551382d4e.1747203712.git.mazziesaccount@gmail.com/
drops the driver changes and adds only the fallback compatible to the 
bindings.

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

On Wed, 14 May 2025 12:21:30 +0300
Matti Vaittinen <mazziesaccount@gmail.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@gmail.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.


> 
> Yours,
> 	-- Matti
> 
> 
> 

On 15/05/2025 20:06, Jonathan Cameron wrote:
> On Wed, 14 May 2025 12:21:30 +0300
> Matti Vaittinen <mazziesaccount@gmail.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@gmail.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@0/48030000.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@44e09000 {
	compatible = "ti,am3352-uart", "ti,omap3-uart";
};

yields:
root@arm:/home/debian# cat 
/sys/devices/platform/ocp/44c00000.interconnect/44c00000.interconnect:segment@200000/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