| Message ID | 8e10f2d82362ca7c207324a5a97bb1759581acea.1742474322.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support ROHM BD79104 ADC | expand |
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);
On 31/03/2025 14:22, Jonathan Cameron wrote: > 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. The change you ask for is indeed small. I have no real objections against implementing it (and I actually wrote it already) - but I am still somewhat hesitant. As you say, (it seems like) the idea of the original code is to allow changing the vref at runtime. It looks to me this might've been intentional choice. I am not terribly happy about dropping the working functionality, when the gained simplification isn't particularly massive. Because of this, I am thinking of adding the patch dropping the functionality as an RFC. Leaving that floating on the list for a while would at least have my ass partially covered ;) I'd rather not delayed the support for the BD79104 though. So - would it be okay if I didn't implement the clean-up as a precursory patch, but did it as a last patch of the series? That will make it a tad more complex to review, but it'd allow taking the BD79104 changes in while leaving the RFC to float on a list. (Also, I'm not sure if you can push an RFC in next without taking it in for the cycle?) Yours, -- Matti
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); 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);
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. 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 Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- --- drivers/iio/adc/Kconfig | 2 +- drivers/iio/adc/ti-adc128s052.c | 40 +++++++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 5 deletions(-)