Re: [PATCH v1 2/3] dt-bindings: iio: adc: add AD7191

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 22 Dec 2024 14:48:08 +0000
Conor Dooley <conor@kernel.org> wrote:

> On Sat, Dec 21, 2024 at 05:56:01PM +0200, Alisa-Dariana Roman wrote:
> > AD7191 is a pin-programmable, ultralow noise 24-bit sigma-delta ADC
> > designed for precision bridge sensor measurements. It features two
> > differential analog input channels, selectable output rates,
> > programmable gain, internal temperature sensor and simultaneous
> > 50Hz/60Hz rejection.
> > 
> > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>

Maybe I'm over thinking things, but comments inline about possibility of
pinstrapping holding this device in a particular configuration, without
the GPIOS connected.

> > ---
> >  .../bindings/iio/adc/adi,ad7191.yaml          | 128 ++++++++++++++++++
> >  MAINTAINERS                                   |   7 +
> >  2 files changed, 135 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> > new file mode 100644
> > index 000000000000..f3e596918c22
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> > @@ -0,0 +1,128 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2025 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/adi,ad7191.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices AD7191 ADC device driver
> > +
> > +maintainers:
> > +  - Alisa-Dariana Roman <alisa.roman@analog.com>
> > +
> > +description: |
> > +  Bindings for the Analog Devices AD7191 ADC device. Datasheet can be
> > +  found here:
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7191.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,ad7191
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-cpol: true
> > +
> > +  spi-cpha: true
> > +
> > +  clocks:
> > +    maxItems: 1
> > +    description:
> > +      Optionally, either a crystal can be attached externally between MCLK1 and
> > +      MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
> > +      pin. If absent, internal 4.92MHz clock is used.  
> 
> Without clock-names, how do you tell the difference between driven ctal and
> the cmos-compatible clock? That CLKSEL's job?

Seems it's an unusual part and there is no config associated with whether we
have a clock or an xtal, so we probably don't need the name.

Related to that, in many cases I'd expect clksel to be tied to always
use the external clock or not (depending on whether one is connected)
not to be a gpio.  So you probably need to take that configuration into
account as well.

Similar may apply for the odr, and pga pins.  Sometimes people
hardwire those things.  There are examples in tree (I can't point at one
right now though) that deal with this. Fairly sure at least 1 ADI part
has a binding to handle that.  (the ad7606 does a bit of this as it needs
a particular pinstrap for sw-mode).

You should be fine with chan and temp always being GPIOs as it would be weird
to buy a part with the extra channels + temperature sensor and not wire it
to be useable.

> 
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  avdd-supply:
> > +    description: AVdd voltage supply
> > +
> > +  dvdd-supply:
> > +    description: DVdd voltage supply
> > +
> > +  vref-supply:
> > +    description: Vref voltage supply
> > +
> > +  odr1-gpios:
> > +    description: GPIO connected to ODR1 pin for output data rate selection
> > +    maxItems: 1
> > +
> > +  odr2-gpios:
> > +    description: GPIO connected to ODR2 pin for output data rate selection
> > +    maxItems: 1
> > +
> > +  pga1-gpios:
> > +    description: GPIO connected to PGA1 pin for gain selection
> > +    maxItems: 1
> > +
> > +  pga2-gpios:
> > +    description: GPIO connected to PGA2 pin for gain selection
> > +    maxItems: 1
> > +
> > +  temp-gpios:
> > +    description: GPIO connected to TEMP pin for temperature sensor enable
> > +    maxItems: 1
> > +
> > +  chan-gpios:
> > +    description: GPIO connected to CHAN pin for input channel selection
> > +    maxItems: 1
> > +
> > +  clksel-gpios:
> > +    description: GPIO connected to CLKSEL pin for clock source selection
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - avdd-supply
> > +  - dvdd-supply
> > +  - vref-supply
> > +  - spi-cpol
> > +  - spi-cpha  
> 
> > +  - odr1-gpios
> > +  - odr2-gpios
> > +  - pga1-gpios
> > +  - pga2-gpios  
> 
> For these 4, since all are required, seems like grouping as odr-pgios
> and pga-gpios would be a good idea?
Agreed except for the annoying option of pin strapping.  Maybe we ignore that
for now.  If it becomes a problem, we can add it safely as a driver predating
that will try to grab the gpios and fail if it sees a DT not providing them.
So will fail safe before we add pinstrapping.  Maybe we will never need to.

Jonathan