Re: [PATCH v3 2/3] iio: adc: Initial support for AD4134

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2025-12-02 at 23:26 +0200, Andy Shevchenko wrote:
> On Tue, Dec 2, 2025 at 10:55 PM Marcelo Schmitt
> <marcelo.schmitt@analog.com> wrote:
> > 
> > AD4134 is a 24-bit, 4-channel, simultaneous sampling, precision
> > analog-to-digital converter (ADC). The device can be managed through SPI or
> > direct control of pin logical levels (pin control mode). The AD4134 design
> > also features a dedicated bus for ADC sample data output. Though, this
> > initial driver for AD4134 only supports usual SPI connections.
> > 
> > Add basic support for AD4134 that enables single-shot ADC sample read.
> 
> ...
> 
> > I tried using the reset-gpio driver to handle AD4134 reset GPIO. I had changed
> > the device tree to set a reset-controller node and had referenced that from the
> > ADC node. I also updated the ad4134 driver to use a reset controller to handle
> > its reset GPIO. Though, after those changes, the AD4134 driver would defer
> > device initialization forever because it missed a reset-controller. To make the
> > reset-gpio driver probe and instantiate a reset controller, it would take a
> > platform device to be set within a machine-specific, hardcoded platform data.
> > AD4134 is not bound to any specific platform, so it doesn't make much sense to
> > have a reset-gpio platform device for that. Thanks for mentioning reset-gpio. It
> > was interesting looking into the reset-gpio driver and the reset framework. It
> > looks cool. But I don't think the reset-gpio driver suits the AD4134 reset use
> > case.
> 
> Bart converted it to be an aux driver and it should work. Please, give
> a try after v6.19-rc1 is out.
> 
> ...
> 
> > +       /*
> > +        * To be able to read data from all 4 channels through a single line, we
> > +        * set DOUTx output format to 0 in the digital interface config register
> > +        * (0x12). With that, data from all four channels is serialized and
> > +        * output on DOUT0. During probe, we also set SDO_PIN_SRC_SEL in
> 
> During the probe
> 
> > +        * DEVICE_CONFIG_1 register to duplicate DOUT0 on the SDO pin. Combined,
> > +        * those configurations enable ADC data read through a conventional SPI
> > +        * interface. Now we read data from all channels but keep only the bits
> > +        * from the requested one.
> > +        */
> > +       for (i = 0; i < ARRAY_SIZE(ad4134_chan_set); i++) {
> > +               ret = spi_write_then_read(st->spi, NULL, 0, st->rx_buf,
> > +                                         BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS));
> > +               if (ret)
> > +                       return ret;
> > +
> > +               if (i != AD4134_VREG_CH(reg))
> > +                       continue;
> > +               *val = get_unaligned_be24(st->rx_buf);
> 
> Hmm...
> 
> In this case it might be better to  use
> 
>   if (i == ...)
>     *val = ...
> 
> but it's still unclear on how many times the conditional can be true
> in the loop.
> 
> > +       }
> 
> ...
> 
> > +static int ad4134_clock_select(struct ad4134_state *st)
> > +{
> > +       struct device *dev = &st->spi->dev;
> > +       struct clk *sys_clk;
> > +
> > +       /*
> > +        * AD4134 requires one external clock source and only one external clock
> > +        * source can be provided at a time. Try get a crystal provided clock.
> > +        * If that fails, try to get a CMOS clock.
> > +        */
> > +       sys_clk = devm_clk_get_optional_enabled(dev, "xtal1-xtal2");
> > +       if (IS_ERR_OR_NULL(sys_clk)) {
> > +               if (PTR_ERR(sys_clk) == -EPROBE_DEFER)
> > +                       return -EPROBE_DEFER;
> 
> But this will ignore other errors when clock _is_ available.
> 
> This should be as simple as
> 
> sys_clk = devm_clk_get_...(...);
> if (!sys_clk)
>   sys_clk = devm_clk_get_enabled(...);
> if (IS_ERR(...))
>   return dev_err_probe(..., PTR_ERR(...), ...);
> 
> See how other drivers do in the similar situation (IIRC 8250_dw does this).
> 
> > +               /* Try the CMOS clock */
> > +               sys_clk = devm_clk_get_enabled(dev, "clkin");
> > +               if (IS_ERR(sys_clk)) {
> > +                       if (PTR_ERR(sys_clk) == -EPROBE_DEFER)
> > +                               return -EPROBE_DEFER;
> > +
> > +                       return dev_err_probe(dev, PTR_ERR(sys_clk),
> > +                                            "failed to get external clock\n");
> > +               }
> > +       }
> > +
> > +       st->sys_clk_hz = clk_get_rate(sys_clk);
> > +       if (st->sys_clk_hz != AD4134_EXT_CLOCK_MHZ)
> > +               dev_warn(dev, "invalid external clock frequency %lu\n",
> > +                        st->sys_clk_hz);
> > +
> > +       return 0;
> > +}
> 
> ...
> 
> > +       reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +       if (IS_ERR(reset_gpio))
> > +               return dev_err_probe(dev, PTR_ERR(reset_gpio),
> > +                                    "failed to find reset GPIO\n");
> > +
> > +       if (reset_gpio) {
> > +               fsleep(AD4134_RESET_TIME_US);
> > +               gpiod_set_value_cansleep(reset_gpio, 0);
> > +       }
> 
> I still think that reset-gpio driver is the right way to go (after
> Bart's changes, which should be part of v6.19-rc1).

Hmm, can you share why we should have a reset controller for the above? 

Unless I'm missing something, even with the aux device, you'll need the code to
optionally add it which (I think) will already force you to check the existence for
the pin (which would be a bit odd IMO).

But more importantly, for things like the above I'm failing to see the benefit in
registering a reset controller. In fact, I think it would be dangerous to "allow" other
potential consumers to randomly reset the device. 

If you look at Krzysztof's log when adding the driver, you see:

"Add a simple driver to control GPIO-based resets using the reset
controller API for the cases when the GPIOs are shared and reset should
be coordinated.  The driver is expected to be used by reset core
framework for ad-hoc reset controllers."

Key point is *GPIOs are shared and reset should be coordinated*. That is not the case here.

Or maybe I'm missing the point...

Having said the above, I would be up for some kind of helper in gpiolib. I still see way too
often people misinterpreting the meaning of GPIOD_OUT_HIGH and that the value in
gpiod_set_value_cansleep() means assert/deassert.

- Nuno Sá
 
> 
> ...
> 
> The rest LGTM.