| 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
| ||
|
Message-ID: <stmpyitqnjst2l26xdlkfqdedczpnxhoydges7xumtl5e46pof@cyidmsvdtdnj>
Date: Thu, 7 Aug 2025 14:01:06 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 09/10] iio: adc: ad7476: Support ROHM BD79105
On Thu, Aug 07, 2025 at 12:35:25PM +0300, Matti Vaittinen wrote:
> The ROHM BD79105 is a simple 16-bit ADC accessible via SPI*.
>
> The BD79105 has a CONVSTART pin, which must be set high to start the ADC
> conversion. Unlike with the ad7091 and ad7091r which also have a
> CONVSTART pin, the BD79105 requires that the pin must remain high also
> for the duration of the SPI access.
>
> (*) Couple of words about the SPI. The BD79105 has pins named as
> CONVSTART, SCLK, DIN and DOUT. For the curious reader, DIN is not SPI
> ISO.
>
> DIN is a signal which can be used as a chip-select. When DIN is pulled
> low, the ADC will output the completed measurement via DOUT as SCLK is
> clocked. According to the data-sheet, the DIN can also be used for
> daisy-chaining multiple ADCs. Furthermore, DOUT can be used also for a
> 'data-ready' -IRQ. These modes aren't supported by this driver.
>
> Support reading ADC scale and data from the BD79105 using SPI, when DIN
> is used as a chip-select.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> ---
> Revision history:
> v1 => v2:
> - Fix the conversion delay for the BD79105
> - Drop unnecessary GPIO check from the convstart disable
> - Drop unintended whitespace change
> - Fix spelling
> ---
IIUC, for this chip the CONV GPIO is actually mandatory no? If so, we
should likely fail probe in case there's no GPIO. And we could also change
the dt bindings accordingly.
Some more comments inline...
> drivers/iio/adc/ad7476.c | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> index 8914861802be..aa8a522633eb 100644
> --- a/drivers/iio/adc/ad7476.c
> +++ b/drivers/iio/adc/ad7476.c
> @@ -31,6 +31,7 @@ struct ad7476_chip_info {
> struct iio_chan_spec channel[2];
> void (*reset)(struct ad7476_state *);
> void (*conversion_pre_op)(struct ad7476_state *st);
> + void (*conversion_post_op)(struct ad7476_state *st);
> bool has_vref;
> bool has_vdrive;
> };
> @@ -39,6 +40,7 @@ struct ad7476_state {
> struct spi_device *spi;
> struct gpio_desc *convst_gpio;
> void (*conversion_pre_op)(struct ad7476_state *st);
> + void (*conversion_post_op)(struct ad7476_state *st);
Pointer duplication again :)
> struct spi_transfer xfer;
> struct spi_message msg;
> struct iio_chan_spec channel[2];
> @@ -63,6 +65,21 @@ static void ad7091_convst(struct ad7476_state *st)
> udelay(1); /* Conversion time: 650 ns max */
> }
>
> +static void bd79105_convst_disable(struct ad7476_state *st)
> +{
> + gpiod_set_value(st->convst_gpio, 0);
> +}
> +
> +static void bd79105_convst_enable(struct ad7476_state *st)
> +{
> + if (!st->convst_gpio)
> + return;
I think the pattern for optional GPIOs is to just call
gpiod_set_value_*() and the lib handles NULL pointers. Also the above is
not coeherent with bd79105_convst_disable().
> +
> + gpiod_set_value(st->convst_gpio, 1);
gpiod_set_value_cansleep()? I do see the driver is calling the same API
in other places but I do not see a reason for it... So, precursor patch?
- Nuno Sá
> + /* Worst case, 2790 nS required for conversion */
> + ndelay(2790);
> +}
> +
> static irqreturn_t ad7476_trigger_handler(int irq, void *p)
> {
> struct iio_poll_func *pf = p;
> @@ -80,6 +97,8 @@ static irqreturn_t ad7476_trigger_handler(int irq, void *p)
> iio_push_to_buffers_with_ts(indio_dev, st->data, sizeof(st->data),
> iio_get_time_ns(indio_dev));
> done:
> + if (st->conversion_post_op)
> + st->conversion_post_op(st);
> iio_trigger_notify_done(indio_dev->trig);
>
> return IRQ_HANDLED;
> @@ -271,6 +290,20 @@ static const struct ad7476_chip_info ltc2314_14_chip_info = {
> .has_vref = true,
> };
>
> +static const struct ad7476_chip_info bd79105_chip_info = {
> + .channel[0] = AD7091R_CHAN(16),
> + .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> + /*
> + * The BD79105 starts ADC data conversion when the CONVSTART line is
> + * set HIGH. The CONVSTART must be kept HIGH until the data has been
> + * read from the ADC.
> + */
> + .conversion_pre_op = bd79105_convst_enable,
> + .conversion_post_op = bd79105_convst_disable,
> + .has_vref = true,
> + .has_vdrive = true,
> +};
> +
> static const struct iio_info ad7476_info = {
> .read_raw = &ad7476_read_raw,
> };
> @@ -325,6 +358,7 @@ static int ad7476_probe(struct spi_device *spi)
> }
>
> st->conversion_pre_op = chip_info->conversion_pre_op;
> + st->conversion_post_op = chip_info->conversion_post_op;
> st->convst_gpio = devm_gpiod_get_optional(&spi->dev,
> "adi,conversion-start",
> GPIOD_OUT_LOW);
> @@ -400,6 +434,7 @@ static const struct spi_device_id ad7476_id[] = {
> { "ads7866", (kernel_ulong_t)&ads7866_chip_info },
> { "ads7867", (kernel_ulong_t)&ads7867_chip_info },
> { "ads7868", (kernel_ulong_t)&ads7868_chip_info },
> + { "bd79105", (kernel_ulong_t)&bd79105_chip_info },
> /*
> * The ROHM BU79100G is identical to the TI's ADS7866 from the software
> * point of view. The binding document mandates the ADS7866 to be
> --
> 2.50.1
>
Powered by blists - more mailing lists