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@xxxxxxxxxx> 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@xxxxxxxxxx> > > 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@xxxxxxxxxx> > > > + > > > +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:
signature.asc
Description: PGP signature
