Message-ID: <20251109123153.65cea85a@jic23-huawei>
Date: Sun, 9 Nov 2025 12:31:53 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Jorge Marques <gastmaier@...il.com>
Cc: Jorge Marques <jorge.marques@...log.com>, Lars-Peter Clausen
 <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 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>, Jonathan Corbet <corbet@....net>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 2/7] docs: iio: New docs for ad4062 driver


> > > > > +Supported devices
> > > > > +=================
> > > > > +
> > > > > +The following chips are supported by this driver:
> > > > > +
> > > > > +* `AD4060 <https://www.analog.com/AD4060>`_
> > > > > +* `AD4062 <https://www.analog.com/AD4062>`_
> > > > > +
> > > > > +Wiring modes
> > > > > +============
> > > > > +
> > > > > +The ADC is interfaced through an I3C bus, and contains two programmable GPIOs.    
> > > > This raises a question on whether it makes sense for the binding to support providing
> > > > gpios from the start (as alternative to interrupts).  Seems like the two pins
> > > > are completely interchangeable so one might well be 'left' for use by some other
> > > > device that needs a gpio pin.
> > > > 
> > > > I don't mind that much if we want to leave the binding support for that for later
> > > > but in the ideal case we'd have it from the start (even if the driver doesn't
> > > > support it until we have a user).    
> > > 
> > > Yep, they are almost interchangeable except GP0 cannot be DEV_DRY (device is
> > > ready to accept serial interface communications). The question is how to
> > > represent that in the devicetree.
> > > 
> > > I am ok with adding `gpio-controller` as an optional property. If
> > > present, and if the gp0/1 is not taken by the interrupt-names property,
> > > it fallback to expose as a gpo0 (they cannot be set as gpi).
> > > 
> > > For the other roles, based on
> > > https://elixir.bootlin.com/linux/v6.18-rc3/source/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml#L109
> > > We would have
> > > 
> > >   adi,gp0-mode = "dev-en";
> > >   adi,gp1-mode = "rdy";
> > > 
> > > Some examples:
> > > 
> > >   // gp0: threshold-either (default), gp1: data ready (default)
> > >   adc@0,2ee007c0000 {
> > >       reg = <0x0 0x2ee 0x7c0000>;
> > >       vdd-supply = <&vdd>;
> > >       vio-supply = <&vio>;
> > >       ref-supply = <&ref>;
> > > 
> > >       interrupt-parent = <&gpio>;
> > >       interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> > >                    <0 1 IRQ_TYPE_EDGE_FALLING>;
> > >       interrupt-names = "gp0", "gp1";
> > >   };
> > > 
> > >   // gp0: user GPO, gp1: data ready
> > >   adc@0,2ee007c0000 {
> > >       reg = <0x0 0x2ee 0x7c0000>;
> > >       vdd-supply = <&vdd>;
> > >       vio-supply = <&vio>;
> > >       ref-supply = <&ref>;
> > > 
> > >       gpio-controller;
> > > 
> > >       interrupt-parent = <&gpio>;
> > >       interrupts = <0 1 IRQ_TYPE_EDGE_FALLING>;
> > >       interrupt-names = "gp1";
> > >   };
> > > 
> > >   // gp0: threshold crossed high value, g1: unused
> > >   adc@0,2ee007c0000 {
> > >       reg = <0x0 0x2ee 0x7c0000>;
> > >       vdd-supply = <&vdd>;
> > >       vio-supply = <&vio>;
> > >       ref-supply = <&ref>;
> > > 
> > >       interrupt-parent = <&gpio>;
> > >       interrupts = <0 0 IRQ_TYPE_EDGE_FALLING>;
> > >       interrupt-names = "gp0";
> > > 
> > >       adi,gp0-mode = "thresh-high"
> > >   };
> > > 
> > > 
> > > And the driver constraints the valid configurations.  
> >   
> 
> Hi Jonathan,
> 
> A comment on the gpio modes and sampling frequency.
> Ack on `raw * _scale`.
> 
> > More or less looks good  We have other drivers effectively doing this.
> > Whether we actively handle the final case or not in driver probably doesn't matter.
> > Wtihout the gpio-controller specified nothing can use the gpios anyway so
> > if we support them they simply won't get used.
> > 
> > I'm not sure on the gpi0-mode though.  Why is that useful? That feels like
> > a choice the driver should make dependent on what is available to it.
> > So if we've specified it is in interrupt, only the modes that might be
> > an interrupt are available to the driver.
> > 
> > The only case I can see being useful is dev_en as that's a specifically timed
> > control signal for the analog front end.
> > 
> > So a specific flag for that probably makes sense - I'm not sure if we'd
> > ever have an AFE where it would make sense to drive it from a hardware
> > signal on the ADC like this one AND from a gpio, so I don't think we need
> > to support binding this as a gpio in that case.
> >   
> 
> So, by default gp0 is set threshold-either, and gp1 data ready.
> The `adi,gp*-mode` would be useful to invert for example, have gp0 as
> data ready and gp1 as threshold-either.
> 
> On the threshold, there are 3 different crossings:
> * either: the reading crosses either the upper or lower boundary.
> * max: crosses the upper boundary.
> * min: crosses the lower boundary.
> 
> To not make the `adi,gp*-mode` attribute necessary I can:
> * make the driver set either, max, min roles based on the
>   values written to the IIO attributes, e.g. writing -1 for
>   IIO_EV_INFO_HYSTERESIS, or a max/min value high/low enough?

This confuses me a little. Which direction do the thresholds apply in?
Looks from the datasheet like these are standard in that
crossing the min threshold triggers the interrupt when the value
goes below that threshold (and hysteresis means the detector doesn't
reset until the value goes above min + hysteresis)
So for that it's a falling threshold with normal hysteresis.

For the max on it will be a rising threshold with hysteresis applying below
that. The ABI docs describe this directional use of hysteresis.
Either case seems to correspond to both other interrupts enabled at the
same time.  Helpfully it's the or of the control bits so easy to support
as no obvious coupling between them.  They seem to have separate status
bits as well.


> * dev_en use case is to shutdown/turn-on an AFE, amplifiers,
>   but I wouldn't support in this series.

Often that can be tied to runtime PM, but I agree that is better
left for a future series.

> 
> So, I believe the solution for the dt-binding is add the optional
> `gpio-controller`, and if set, expose the unused (not in the
> interrupts-names) as GPOutput. And not support inversion gp0<->gp1 of
> the mode, the specific threshold crossing (max, min, either), nor the
> dev_en use case.
> 
> This then doesn't change the series much, just appends a commit adding
> the gpio-controller support.
Agreed.

...

> 
> > >   
> > > > > +       ``ref-supply``.
> > > > > +
> > > > > +Also contain the following device attributes:
> > > > > +
> > > > > +.. list-table:: Device attributes
> > > > > +   :header-rows: 1
> > > > > +
> > > > > +   * - Attribute
> > > > > +     - Description
> > > > > +   * - ``samling_frequency``    
> > > > 
> > > > Check these.. sampling_frequency.
> > > >     
> > > > > +     - Sets the sets the device internal sample rate, used in the burst
> > > > > +       averaging mode.    
> > > > 
> > > > It's not use otherwise?  That's unusual ABI.  I'd expect it to give
> > > > the right value at least when burst mode isn't used. Or is burst mode
> > > > the only way we do buffered capture?
> > > >     
> > > 
> > > It is not used otherwise, the device internal sample rate is used only
> > > to evenly distribute readings used in the burst averaging mode.
> > > There is no sampling trigger to evenly space the adc conversion reading,  
> > 
> > In event of no internal trigger, sampling_frequency is normally
> > 1/duration of a single scan (or sample if a per channel attribute).
> > 
> >   
> 
> The duration of a single scan (duration between the convert-start high
> edge until RDY falling edge), would be ((n_avg - 1) / fosc + tconv)
> (datasheet fig.56, p.31) , where fosc is the internal timer frequency
> directly exposed in this series. I can make the driver use this formula,
> just setting it will be less straight forward, since currently a clear
> enum of the values the register supports is returned by the
> sampling_frequency_available attribute. Instead, it will scale based on
> the number of samples per burst (averaging ratio/n_avg). tconv is typ
> 270ns and max 320ns.

Agreed this can get a bit fiddly :( 

There are also some slightly nasty race conditions around updating
available attributes.  We had a solution for that a while back but
I'm not sure what happened to it and the complexity only really applies
when using in kernel consumers for those values rather than sysfs where
we can apply a driver local lock.

That n_avg seems to be oversampling - hence indeed we have to deal
with the affect of that on the sampling frequency (it's documented
in the ABI as sampling frequency is for the total time for oversampling
to deliver data).

Jonathan

Powered by blists - more mailing lists