| Message ID | 20250306-iio-driver-ad4052-v1-2-2badad30116c@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD4052 device family | expand |
On Thu, Mar 06, 2025 at 03:03:15PM +0100, Jorge Marques wrote: > Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058, > low-power with monitor capabilities SAR ADCs. > Contain selectable oversampling and sample rate, the latter for both > oversampling and monitor mode. > The monitor capability is exposed as an IIO threshold either direction > event. > > Signed-off-by: Jorge Marques <jorge.marques@analog.com> > --- > .../devicetree/bindings/iio/adc/adi,ad4052.yaml | 80 ++++++++++++++++++++++ > MAINTAINERS | 6 ++ > 2 files changed, 86 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..4602f1f0184d58f33883852ff6d76933758825f1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml > @@ -0,0 +1,80 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright 2025 Analog Devices Inc. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/adi,ad4052.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices AD4052 ADC family device driver > + > +maintainers: > + - Jorge Marques <jorge.marques@analog.com> > + > +description: | > + Analog Devices AD4052 Single Channel Precision SAR ADC family > + > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf > + > +properties: > + compatible: > + enum: > + - adi,ad4050 > + - adi,ad4052 > + - adi,ad4056 > + - adi,ad4058 Can you mention in your commit message what differs between these devices that makes picking one as the "base"/fallback compatible unsuitable please? > + > + reg: > + maxItems: 1 > + > + clocks: > + description: > + Reference clock > + maxItems: 1 > + > + interrupts: > + items: > + - description: threshold events. > + - description: device ready and data ready. > + > + cnv-gpios: > + maxItems: 1 > + > + spi-max-frequency: > + maximum: 62500000 > + > + vdd-supply: true > + vdd_1_8-supply: true You're allowed to use . in property names, and the _s should be -s. That said, vdd and vdd 1.8? Shouldn't both have the voltage in them in that case? > + vio-supply: true > + > +required: > + - compatible > + - reg > + - interrupts > + > +allOf: > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + compatible = "adi,ad4052"; > + reg = <0>; > + spi-max-frequency = <25000000>; > + > + interrupt-parent = <&gpio>; > + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>, > + <0 1 IRQ_TYPE_EDGE_RISING>; > + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>; > + }; > + }; > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index 06f122cb8bbd15a0076c229dfc89be0b5126f237..fef8adaee888d59e1aa3b3592dda5a8bea0b7677 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1317,6 +1317,12 @@ F: Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml > F: Documentation/iio/ad4030.rst > F: drivers/iio/adc/ad4030.c > > +ANALOG DEVICES INC AD4052 DRIVER > +M: Jorge Marques <jorge.marques@analog.com> > +S: Supported > +W: https://ez.analog.com/linux-software-drivers > +F: Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml > + > ANALOG DEVICES INC AD4130 DRIVER > M: Cosmin Tanislav <cosmin.tanislav@analog.com> > L: linux-iio@vger.kernel.org > > -- > 2.48.1 >
On Thu, Mar 6, 2025 at 3:04 PM Jorge Marques <jorge.marques@analog.com> wrote: > > Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058, > low-power with monitor capabilities SAR ADCs. > Contain selectable oversampling and sample rate, the latter for both > oversampling and monitor mode. > The monitor capability is exposed as an IIO threshold either direction > event. These sounds like they are describing the driver so aren't appropriate for this commit message. Here we should only be talking about the bindings. > > Signed-off-by: Jorge Marques <jorge.marques@analog.com> > --- > .../devicetree/bindings/iio/adc/adi,ad4052.yaml | 80 ++++++++++++++++++++++ > MAINTAINERS | 6 ++ > 2 files changed, 86 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..4602f1f0184d58f33883852ff6d76933758825f1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml > @@ -0,0 +1,80 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright 2025 Analog Devices Inc. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/adi,ad4052.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices AD4052 ADC family device driver > + > +maintainers: > + - Jorge Marques <jorge.marques@analog.com> > + > +description: | > + Analog Devices AD4052 Single Channel Precision SAR ADC family > + > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf The links above don't work for me. Instead... https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050-ad4056.pdf https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052-ad4058.pdf > + > +properties: > + compatible: > + enum: > + - adi,ad4050 > + - adi,ad4052 > + - adi,ad4056 > + - adi,ad4058 > + > + reg: > + maxItems: 1 > + > + clocks: > + description: > + Reference clock > + maxItems: 1 I don't see any pins in the datasheet about a "reference clock" input. Is this for the CNV pin? If this is for the internal clock, then we don't need a property for it. > + > + interrupts: > + items: > + - description: threshold events. > + - description: device ready and data ready. > + Since there are multiple interrupts, we should also have an interrupt-names property. Also, the interrupts should be named after the pin they are connected to, not the function. So the interrupt names should be "rdy", "gp0", and "gp1". > + cnv-gpios: > + maxItems: 1 Not necessary, but I would not mind having a description that says that the CNV pin may also be connected to the CS line of the SPI controller if it is not connected to a GPIO. > + > + spi-max-frequency: > + maximum: 62500000 > + > + vdd-supply: true > + vdd_1_8-supply: true This one seems redundant and should be dropped. But there is also a possible separate reference voltage supply, so we should have a ref-supply property. > + vio-supply: true These chips also have GPIO pins, so we can add the gpio-controller and #gpio-cells properties to the bindings even if we don't implement this in the driver. > + > +required: > + - compatible > + - reg > + - interrupts The chip won't work without vcc-supply and vio-supply so they should be required. ref-supply is clearly optional though. > + > +allOf: > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + compatible = "adi,ad4052"; > + reg = <0>; > + spi-max-frequency = <25000000>; > + > + interrupt-parent = <&gpio>; > + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>, > + <0 1 IRQ_TYPE_EDGE_RISING>; > + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>; > + }; > + }; > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index 06f122cb8bbd15a0076c229dfc89be0b5126f237..fef8adaee888d59e1aa3b3592dda5a8bea0b7677 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1317,6 +1317,12 @@ F: Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml > F: Documentation/iio/ad4030.rst > F: drivers/iio/adc/ad4030.c > > +ANALOG DEVICES INC AD4052 DRIVER > +M: Jorge Marques <jorge.marques@analog.com> > +S: Supported > +W: https://ez.analog.com/linux-software-drivers > +F: Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml > + > ANALOG DEVICES INC AD4130 DRIVER > M: Cosmin Tanislav <cosmin.tanislav@analog.com> > L: linux-iio@vger.kernel.org > > -- > 2.48.1 >
> > + vdd-supply: true > > + vdd_1_8-supply: true > > You're allowed to use . in property names, and the _s should be -s. > That said, vdd and vdd 1.8? Shouldn't both have the voltage in them in > that case? I got curious and opened datasheet. Only seeing two supplies (vdd and vio) So checked driver and that doesn't enable any of them. > > > + vio-supply: true > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + Supplies that are not actually optional (i.e. device can run without them) should be in required list even though the regulator core will provide stubs if they aren't in your dts. The aim is to reflect what should be provided, not what Linux does with it. Jonathan
On Thu, 6 Mar 2025 15:03:15 +0100 Jorge Marques <jorge.marques@analog.com> wrote: > Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058, > low-power with monitor capabilities SAR ADCs. > Contain selectable oversampling and sample rate, the latter for both > oversampling and monitor mode. > The monitor capability is exposed as an IIO threshold either direction > event. > > Signed-off-by: Jorge Marques <jorge.marques@analog.com> > --- > .../devicetree/bindings/iio/adc/adi,ad4052.yaml | 80 ++++++++++++++++++++++ > MAINTAINERS | 6 ++ > 2 files changed, 86 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..4602f1f0184d58f33883852ff6d76933758825f1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml > @@ -0,0 +1,80 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright 2025 Analog Devices Inc. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/adi,ad4052.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices AD4052 ADC family device driver > + > +maintainers: > + - Jorge Marques <jorge.marques@analog.com> > + > +description: | > + Analog Devices AD4052 Single Channel Precision SAR ADC family > + > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf It's this data sheet that I opened. Seems it describes things a bit different that you have here. > + > +properties: > + compatible: > + enum: > + - adi,ad4050 > + - adi,ad4052 > + - adi,ad4056 > + - adi,ad4058 > + > + reg: > + maxItems: 1 > + > + clocks: > + description: > + Reference clock > + maxItems: 1 > + > + interrupts: > + items: > + - description: threshold events. > + - description: device ready and data ready. People have a nasty habit of wiring just one. So use interrupt-names and let them come in any order. The driver can require both or a specific one if it likes, but in future we may need to make it more flexible and the dt-binding should allow that. They seem to be GP0 and GP1 on datasheet and don't have fixed roles like this implies. > + > + cnv-gpios: Not the most self explanatory of names. I'd suggest a bit of help text for this one. > + maxItems: 1 > + > + spi-max-frequency: > + maximum: 62500000 > + > + vdd-supply: true > + vdd_1_8-supply: true As per other thread, supplies like this normally required and this one at least doesn't seem to exist in the datasheet I randomly picked. > + vio-supply: true > + > +required: > + - compatible > + - reg > + - interrupts > +
> > + compatible: > > + enum: > > + - adi,ad4050 > > + - adi,ad4052 > > + - adi,ad4056 > > + - adi,ad4058 > > Can you mention in your commit message what differs between these > devices that makes picking one as the "base"/fallback compatible > unsuitable please? Sure, to be added: Each variant of the family differs in speed and resolution, resulting in different scan types and spi word sizes, that are matched by the compatible with the chip_info. The device contains two required interrupts (gp0, gp1) and one optional gpio (cnv). > > + > > + vdd-supply: true > > + vdd_1_8-supply: true > > You're allowed to use . in property names, and the _s should be -s. > That said, vdd and vdd 1.8? Shouldn't both have the voltage in them in > that case? I overlooked the supplies, the correct are vdd, vio as mandatory, and vref is optional. Jorge
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050.pdf > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf > > The links above don't work for me. Instead... > > https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050-ad4056.pdf > https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052-ad4058.pdf Thanks! > > + clocks: > > + description: > > + Reference clock > > + maxItems: 1 > > I don't see any pins in the datasheet about a "reference clock" input. > Is this for the CNV pin? If this is for the internal clock, then we > don't need a property for it. Indeed, this will be removed. > > > + > > + interrupts: > > + items: > > + - description: threshold events. > > + - description: device ready and data ready. > > + > > Since there are multiple interrupts, we should also have an > interrupt-names property. Also, the interrupts should be named after > the pin they are connected to, not the function. So the interrupt > names should be "rdy", "gp0", and "gp1". Agreed. > > > + cnv-gpios: > > + maxItems: 1 > > Not necessary, but I would not mind having a description that says > that the CNV pin may also be connected to the CS line of the SPI > controller if it is not connected to a GPIO. Included. > > > + > > + spi-max-frequency: > > + maximum: 62500000 > > + > > + vdd-supply: true > > > + vdd_1_8-supply: true > > This one seems redundant and should be dropped. > > But there is also a possible separate reference voltage supply, so we > should have a ref-supply property. > > > + vio-supply: true Yes, I overlooked the supplies, vio, vdd are mandatory, and vref is optional. Also noted Jonathan's comment that the aim is to reflect what should be provided, not what Linux does with it. > > These chips also have GPIO pins, so we can add the gpio-controller and > #gpio-cells properties to the bindings even if we don't implement this > in the driver. Good to know, added as suggested. > > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > The chip won't work without vcc-supply and vio-supply so they should > be required. ref-supply is clearly optional though. Aggreed.
> > +description: | > > + Analog Devices AD4052 Single Channel Precision SAR ADC family > > + > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050.pdf > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf > It's this data sheet that I opened. Seems it describes things a bit > different that you have here. The power supplies were wrong, the correct ones are vdd, vio and vref (optional). > People have a nasty habit of wiring just one. So use > interrupt-names and let them come in any order. The driver can require > both or a specific one if it likes, but in future we may need to make it > more flexible and the dt-binding should allow that. Agreed. > They seem to be GP0 and GP1 on datasheet and don't have fixed roles > like this implies. GP0 and GP1 can output data ready, chop (not implemented), device enabled, interrupt (threshold) or a constant logic state. In the driver implementation, one takes the role of threshold interrupt and the other of data ready. > > > + > > + cnv-gpios: > > Not the most self explanatory of names. I'd suggest a bit of help > text for this one. Noted. > > + vdd-supply: true > > + vdd_1_8-supply: true > As per other thread, supplies like this normally required and this > one at least doesn't seem to exist in the datasheet I randomly picked. Fixed.
On Sun, 9 Mar 2025 20:43:55 +0100 Jorge Marques <gastmaier@gmail.com> wrote: > > > + compatible: > > > + enum: > > > + - adi,ad4050 > > > + - adi,ad4052 > > > + - adi,ad4056 > > > + - adi,ad4058 > > > > Can you mention in your commit message what differs between these > > devices that makes picking one as the "base"/fallback compatible > > unsuitable please? > Sure, to be added: > > Each variant of the family differs in speed and resolution, resulting > in different scan types and spi word sizes, that are matched by the > compatible with the chip_info. > The device contains two required interrupts (gp0, gp1) and one optional > gpio (cnv). Explain why the interrupts are required. That is unusual. Note the driver can be stricter than the binding, so it may make sense to require them in the driver, but leave it flexible in the binding. If someone has a board without them wired, then they can look at adding polling or timing logic to avoid the need for the interrupt lines or at reducing functionality of the driver. > > > > + > > > + vdd-supply: true > > > + vdd_1_8-supply: true > > > > You're allowed to use . in property names, and the _s should be -s. > > That said, vdd and vdd 1.8? Shouldn't both have the voltage in them in > > that case? > I overlooked the supplies, the correct are vdd, vio as mandatory, > and vref is optional. > > Jorge
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml new file mode 100644 index 0000000000000000000000000000000000000000..4602f1f0184d58f33883852ff6d76933758825f1 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml @@ -0,0 +1,80 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright 2025 Analog Devices Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/adi,ad4052.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD4052 ADC family device driver + +maintainers: + - Jorge Marques <jorge.marques@analog.com> + +description: | + Analog Devices AD4052 Single Channel Precision SAR ADC family + + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052.pdf + +properties: + compatible: + enum: + - adi,ad4050 + - adi,ad4052 + - adi,ad4056 + - adi,ad4058 + + reg: + maxItems: 1 + + clocks: + description: + Reference clock + maxItems: 1 + + interrupts: + items: + - description: threshold events. + - description: device ready and data ready. + + cnv-gpios: + maxItems: 1 + + spi-max-frequency: + maximum: 62500000 + + vdd-supply: true + vdd_1_8-supply: true + vio-supply: true + +required: + - compatible + - reg + - interrupts + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad4052"; + reg = <0>; + spi-max-frequency = <25000000>; + + interrupt-parent = <&gpio>; + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>, + <0 1 IRQ_TYPE_EDGE_RISING>; + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>; + }; + }; +... diff --git a/MAINTAINERS b/MAINTAINERS index 06f122cb8bbd15a0076c229dfc89be0b5126f237..fef8adaee888d59e1aa3b3592dda5a8bea0b7677 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1317,6 +1317,12 @@ F: Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml F: Documentation/iio/ad4030.rst F: drivers/iio/adc/ad4030.c +ANALOG DEVICES INC AD4052 DRIVER +M: Jorge Marques <jorge.marques@analog.com> +S: Supported +W: https://ez.analog.com/linux-software-drivers +F: Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml + ANALOG DEVICES INC AD4130 DRIVER M: Cosmin Tanislav <cosmin.tanislav@analog.com> L: linux-iio@vger.kernel.org
Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058, low-power with monitor capabilities SAR ADCs. Contain selectable oversampling and sample rate, the latter for both oversampling and monitor mode. The monitor capability is exposed as an IIO threshold either direction event. Signed-off-by: Jorge Marques <jorge.marques@analog.com> --- .../devicetree/bindings/iio/adc/adi,ad4052.yaml | 80 ++++++++++++++++++++++ MAINTAINERS | 6 ++ 2 files changed, 86 insertions(+)