Message-ID: <ryiifvt7vedgyjgwx3bydty5fvlkffnsjpptj2sc462h3ji4hc@nrna47ekhmni>
Date: Mon, 3 Nov 2025 13:22:16 +0100
From: Jorge Marques <gastmaier@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
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

On Mon, Nov 03, 2025 at 11:19:50AM +0100, Jorge Marques wrote:
> On Sun, Nov 02, 2025 at 12:37:27PM +0000, Jonathan Cameron wrote:
> > On Tue, 28 Oct 2025 16:31:46 +0100
> > Jorge Marques <gastmaier@...il.com> wrote:
> > 
> > > On Sat, Oct 18, 2025 at 04:21:13PM +0100, Jonathan Cameron wrote:
> > > > On Mon, 13 Oct 2025 09:28:00 +0200
> > > > Jorge Marques <jorge.marques@...log.com> wrote:
> > > >   
> > > > > This adds a new page to document how to use the ad4062 ADC driver.
> > > > > 
> > > > > Signed-off-by: Jorge Marques <jorge.marques@...log.com>  
> > > > Hi Jorge,
> > > > 
> > > > Various comments inline.
> > > > 
> > > > Thanks,
> > > > 
> > > > Jonathan
> > > >   
> > > 
> > > Hi Jonathan,
> > > 
> > > > > ---
> > > > >  Documentation/iio/ad4062.rst | 89 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >  MAINTAINERS                  |  1 +
> > > > >  2 files changed, 90 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/iio/ad4062.rst b/Documentation/iio/ad4062.rst
> > > > > new file mode 100644
> > > > > index 0000000000000000000000000000000000000000..b486d7fe1916d2963c94581be3696cf58d51ca48
> > > > > --- /dev/null
> > > > > +++ b/Documentation/iio/ad4062.rst
> > > > > @@ -0,0 +1,89 @@
> > > > > +.. SPDX-License-Identifier: GPL-2.0-only
> > > > > +
> > > > > +=============
> > > > > +AD4062 driver
> > > > > +=============
> > > > > +
> > > > > +ADC driver for Analog Devices Inc. AD4060/AD4062 devices. The module name is
> > > > > +``ad4062``.
> > > > > +
> > > > > +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?
> * dev_en use case is to shutdown/turn-on an AFE, amplifiers,
>   but I wouldn't support in this 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.
> 
> > > 
> > > >   
> > > > > +
> > > > > +The ADC convert-start happens on the SDA rising edge of the I3C stop (P) bit
> > > > > +at the end of the read command.
> > > > > +
> > > > > +The two programmable GPIOS are optional and have a role assigned if present in
> > > > > +the devicetree:
> > > > > +
> > > > > +- GP1: Is assigned the role of Data Ready signal.  
> > > > 
> > > > I assume that's only the case if GP1 is provided?  If GP0 is the only one
> > > > we should allow use that for data ready.  As long as the DT allows that it is
> > > > permissible for the driver to not do so for now.
> > > >   
> > > 
> > > Suggested `adi,gp*-mode` should solve.
> > > 
> > > > > +
> > > > > +Device attributes
> > > > > +=================
> > > > > +
> > > > > +The ADC contains only one channel with following attributes:
> > > > > +
> > > > > +.. list-table:: Channel attributes
> > > > > +   :header-rows: 1
> > > > > +
> > > > > +   * - Attribute
> > > > > +     - Description
> > > > > +   * - ``in_voltage_calibscale``
> > > > > +     - Sets the scale factor to multiply the raw value.  
> > > > That's confusing.  This should be hardware 'tweak' to compensate for
> > > > calibration or similar.  The text doesn't make it clear where that multiply
> > > > is happening. Sounds too much like _scale.  
> > > 
> > > The user does raw * _scale * _calibscale to get the value in volts.
> > 
> > That's not ABI compliant so no general purpose user space is going to do that.
> > So a hard no for this being what userspace needs to apply.
> > 
> > I'm not particularly keen on calibscale for things other than tweaking so
> > that raw * _scale is in milivolts.
> > 
> > Normally if we have other forms of controllable scaling it's a question
> > of wrapping up any such scan factors into a writeable _scale.
> > 
> 
> Ack, will update to raw * _scale, embedding the _calibscale into the
> _scale value.
> 

Fixup on my side, there is no need to "embed _calibscale into _scale".
The only change that needs to be made is re-wording the doc to clarify
that it is the hw that compensates for calibration errors:

    * - ``in_voltage_calibscale``
-     - Sets the scale factor to multiply the raw value.
+     - Sets the gain scaling factor that the hardware applies to the sample,
+       to compensate for system gain error.

(datasheet p22, Gain Scaling)
The mv reading is raw * _scale.

Best regards,
Jorge
> > > 
> > > _calibscale is 1 by default and serves two purposes:
> > > 
> > > * small hw corrections (matches ABI); or 
> > > * an arbitrary user set scale, ideally also for corrections, but not
> > >   constrained.
> > > 
> > > For the prior, the device does support computing the appropriate
> > > _calibscale value, but it is not implemented.
> > > 
> > > If it was implemented, it would occur once during driver initialization
> > > and then _calibscale default value would be approximately 1, (e.g.,
> > > 0.997).
> > > 
> > > >   
> > > > > +   * - ``in_voltage_oversampling_ratio``
> > > > > +     - Sets device's burst averaging mode to over sample using the
> > > > > +       internal sample rate. Value 1 disable the burst averaging mode.
> > > > > +   * - ``in_voltage_oversampling_ratio_available``
> > > > > +     - List of available oversampling values.
> > > > > +   * - ``in_voltage_raw``
> > > > > +     - Returns the raw ADC voltage value.
> > > > > +   * - ``in_voltage_scale``
> > > > > +     - Returs the channel scale in reference to the reference voltage  
> > > > 
> > > > Spell check needed.  Also this describes why it might take different values
> > > > but not the bit users care about which is the standard ABI thing of
> > > > Real value in mV = _raw * _scale 
> > > >   
> > > Ack, will describe
> > > 
> > >  raw * _scale * _calibscale
> > 
> > As above, no to this.  It must be just raw * _scale.
> > 
> 
> Ack.
> 
> > > 
> > > > > +       ``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.
> 
> > > e.g.,:
> > > 
> > >   Oversampling 4, sampling ratio 2Hz
> > >   Sampling trigger |        |        |        |        |
> > >   ADC conversion   ++++     ++++     ++++     ++++     ++++
> > >   ADC data ready      *        *        *        *        *
> > > 
> > >   Oversampling 4, sampling ratio 1Hz
> > >   Sampling trigger |        |        |        |        |
> > >   ADC conversion   + + + +  + + + +  + + + +  + + + +  + + + +
> > >   ADC data ready         *        *        *        *        *
> > > 
> > > For this driver, the sampling trigger is a register access (the stop bit
> > > of the i3c transfer to be exact), so in buffer mode it reads as fast as
> > > possible.
> > > 
> > > > > +   * - ``sampling_frequency_available``
> > > > > +     - Lists the available device internal sample rates.
> > > > > +
> > > > > +Interrupts
> > > > > +==========
> > > > > +
> > > > > +The interrupts are mapped through the ``interrupt-names`` and ``interrupts``
> > > > > +properties.
> > > > > +
> > > > > +The ``interrupt-names`` ``gp1`` entry sets the role of Data Ready signal.
> > > > > +If it is not present, the driver fallback to enabling the same role as an
> > > > > +I3C IBI.  
> > > > 
> > > > It feels like it should be easy to use the other GPO pin in this case if that
> > > > is present. 
> > > >   
> > > adi-gp0-mode should solve.
> > > 
> > > I wouldn't auto-set the mode by the position in the interrupt-names
> > > array, but let me know your opinion.
> > 
> > Mode when multiple are equally valid (for a given device wiring)
> > is purely down to what the driver wants to do. There is no benefit in
> > encoding that in dt.
> > Thanks,
> > 
> > Jonathan
> > 
> 
> Best Regards,
> Jorge

Powered by blists - more mailing lists