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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Dec 03, 2025 at 11:02:45AM +0000, Nuno Sá wrote:
> 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:

Nuno, may you please remove unrelated context when replying?

...

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

...

> > > +       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? 

My point here is to have a standard way of handling "reset" pin independently
of what's beneath in the HW — GPIO or other means to assert/deassert it.

> 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).

If this is the case, it needs to be fixed, but reset framework provides
_optional() API, that's what should be used for the cases where reset is
optional. Let reset framework to handle that.

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

Again, same here, reset framework provides _shared() API which we do not want to
use here, we want to use _exclusive() one.

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

It's unrelated, because from the user's perspective it communicates via reset
framework.

> "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.

And it has no direct relation to this driver in such a case. Of course the
generic reset controller driver should try to cover as many users as possible.
Then on the framework level they can decide how they want to use it.

It's more flexible that way, so I expect most of the reset controller drivers
to allow shared accesses. And it's orthogonal to the discussion here.

> Or maybe I'm missing the point...

I think so, see above.

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

Consider this as a helper :-)

-- 
With Best Regards,
Andy Shevchenko