Re: [PATCH 6/6] iio: adc: ti-adc128s052: Support ROHM BD79104

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 31 Mar 2025 11:03:58 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The ROHM BD79104 ADC has identical SPI communication logic as the
> ti-adc128s052. Eg, SPI transfer should be 16 clk cycles, conversion is
> started when the CS is pulled low, and channel selection is done by
> writing the channel ID after two zero bits. Data is contained in
> big-endian format in the last 12 bits.

Nicely found match.  Sometimes these are tricky to spot.

> 
> The BD79104 has two input voltage pins. Data sheet uses terms "vdd" and
> "iovdd". The "vdd" is used also as an analog reference voltage. Hence
> the driver expects finding these from the device-tree, instead of having
> the "vref" only as TI's driver.
> 
> NOTE: The TI's data sheet[1] does show that the TI's IC does actually
> have two voltage inputs as well. Pins are called Va (analog reference)
> and Vd (digital supply pin) - but I keep the existing driver behaviour
> for the TI's IC "as is", because I have no HW to test changes, and
> because I have no real need to touch it.
> 
> NOTE II: The BD79104 requires SPI MODE 3.
> 
> NOTE III: I used evaluation board "BD79104FV-EVK-001" made by ROHM. With
> this board I had to drop the SPI speed below the 20M which is mentioned
> in the data-sheet [2]. This, however, may be a limitation of the EVK
> board, not the component itself.
> 
> [1]: https://www.ti.com/lit/ds/symlink/adc128s052.pdf
> 
> [2]:
> https://fscdn.rohm.com/en/products/databook/datasheet/ic/data_converter/dac/bd79104fv-la-e.pdf
> 
Prefer Datasheet tags with # [1] 
after them for the cross references.  

Those belong here in the tag block (no blank lines)
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

One request for an additional cleanup precursor patch given you are
touching the relevant code anyway.   It's a small one that you can
test so hope you don't mind doing that whilst here.

I'm relying on the incredibly small chance anyone has a variable
regulator wired up to the reference that they are modifying at runtime.
I have seen that done (once long ago on a crazy dev board for a really
noisy humidity sensor) when the reference was VDD but not on a separate
reference pin.  That means we almost certainly won't break the existing
parts and can't have a regression on your new one so we should be fine
to make the change.

Thanks,

Jonathan

> 
> ---
> ---
>  drivers/iio/adc/Kconfig         |  2 +-
>  drivers/iio/adc/ti-adc128s052.c | 40 +++++++++++++++++++++++++++++----
>  2 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 849c90203071..148a52b3db94 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1425,7 +1425,7 @@ config TI_ADC128S052
>  	depends on SPI
>  	help
>  	  If you say yes here you get support for Texas Instruments ADC128S052,
> -	  ADC122S021 and ADC124S021 chips.
> +	  ADC122S021, ADC124S021 and ROHM Semiconductor BD79104 chips.
>  
>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-adc128s052.
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index c68ee4e17a03..c7283d606d2e 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -21,6 +21,9 @@
>  struct adc128_configuration {
>  	const struct iio_chan_spec	*channels;
>  	u8				num_channels;
> +	const char			*refname;
> +	int				num_other_regulators;
> +	const char * const		(*other_regulators)[];
>  };
>  
>  struct adc128 {
> @@ -119,10 +122,28 @@ static const struct iio_chan_spec adc124s021_channels[] = {
>  	ADC128_VOLTAGE_CHANNEL(3),
>  };
>  
> +static const char * const bd79104_regulators[] = { "iovdd" };
> +
>  static const struct adc128_configuration adc128_config[] = {
> -	{ adc128s052_channels, ARRAY_SIZE(adc128s052_channels) },
> -	{ adc122s021_channels, ARRAY_SIZE(adc122s021_channels) },
> -	{ adc124s021_channels, ARRAY_SIZE(adc124s021_channels) },
> +	{
> +		.channels = adc128s052_channels,
> +		.num_channels = ARRAY_SIZE(adc128s052_channels),
> +		.refname = "vref",
> +	}, {
> +		.channels = adc122s021_channels,
> +		.num_channels = ARRAY_SIZE(adc122s021_channels),
> +		.refname = "vref",
> +	}, {
> +		.channels = adc124s021_channels,
> +		.num_channels = ARRAY_SIZE(adc124s021_channels),
> +		.refname = "vref",
> +	}, {
> +		.channels = adc128s052_channels,
> +		.num_channels = ARRAY_SIZE(adc128s052_channels),
> +		.refname = "vdd",
> +		.other_regulators = &bd79104_regulators,
> +		.num_other_regulators = 1,
> +	},
>  };
>  
>  static const struct iio_info adc128_info = {
> @@ -157,7 +178,7 @@ static int adc128_probe(struct spi_device *spi)
>  	indio_dev->channels = config->channels;
>  	indio_dev->num_channels = config->num_channels;
>  
> -	adc->reg = devm_regulator_get(&spi->dev, "vref");
> +	adc->reg = devm_regulator_get(&spi->dev, config->refname);

Hmm. It is very unusual for a reference regulator to be variable
after power up.  As such, maybe refactor the driver to use
devm_regulator_get_enable_read_voltage() which will save a few lines of
code and generally be easier to read.

That would need to be a separate precursor patch though.


>  	if (IS_ERR(adc->reg))
>  		return PTR_ERR(adc->reg);
>  
> @@ -169,6 +190,15 @@ static int adc128_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> +	if (config->num_other_regulators) {
> +		ret = devm_regulator_bulk_get_enable(&spi->dev,
> +						config->num_other_regulators,
> +						*config->other_regulators);
> +		if (ret)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to enable regulators\n");
> +	}
> +
>  	devm_mutex_init(&spi->dev, &adc->lock);
>  
>  	return devm_iio_device_register(&spi->dev, indio_dev);
> @@ -182,6 +212,7 @@ static const struct of_device_id adc128_of_match[] = {
>  	{ .compatible = "ti,adc124s021", .data = &adc128_config[2] },
>  	{ .compatible = "ti,adc124s051", .data = &adc128_config[2] },
>  	{ .compatible = "ti,adc124s101", .data = &adc128_config[2] },
> +	{ .compatible = "rohm,bd79104", .data = &adc128_config[3] },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, adc128_of_match);
> @@ -194,6 +225,7 @@ static const struct spi_device_id adc128_id[] = {
>  	{ "adc124s021", (kernel_ulong_t)&adc128_config[2] },
>  	{ "adc124s051", (kernel_ulong_t)&adc128_config[2] },
>  	{ "adc124s101", (kernel_ulong_t)&adc128_config[2] },
> +	{ "bd79104", (kernel_ulong_t)&adc128_config[3] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, adc128_id);