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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5700 bytes --]

On Sun, Dec 22, 2024 at 06:18:57PM +0000, Jonathan Cameron wrote:
> 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.

To me, it's a cosmetic nicety to merge them, so I'm happy with any valid
reason to not do so.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]