| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <xqkr3rq6ikuiz5wcbxmto4gp7wnccmmogklf2ux2edauotufim@pcuhddxdzjxi>
Date: Thu, 12 Jun 2025 12:11:41 +0200
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>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Uwe Kleine-König <ukleinek@...nel.org>,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-doc@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v3 2/8] dt-bindings: iio: adc: Add adi,ad4052
On Wed, Jun 11, 2025 at 06:18:18PM +0100, Jonathan Cameron wrote:
> On Tue, 10 Jun 2025 09:34:35 +0200
> Jorge Marques <jorge.marques@...log.com> wrote:
>
> > Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058,
> > low-power with monitor capabilities SAR ADCs. 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 bit about what the drive does with this doesn't really belong in a DT
> binding patch description. Stick to just something like.
>
> Each variant of the family differs in speed and resolution, reflected
> in word size for SPI messages.
>
> > The device contains one input (cnv) and two outputs (gp0, gp1).
> > The outputs can be configured for range of options, such as threshold
> > and data ready.
> > The spi-max-frequency refers to the configuration mode maximum access
> > speed. The ADC mode speed depends on the vio input voltage.
> >
> > Signed-off-by: Jorge Marques <jorge.marques@...log.com>
> > ---
> > .../devicetree/bindings/iio/adc/adi,ad4052.yaml | 167 +++++++++++++++++++++
> > MAINTAINERS | 6 +
> > include/dt-bindings/iio/adc/adi,ad4052.h | 17 +++
> > 3 files changed, 190 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..2cf197e2d872d9a3d4f7210121a1e38f784f92dc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
> > @@ -0,0 +1,167 @@
> > +
> > + interrupts:
> > + items:
> > + - description: Signal coming from the GP0 pin.
> Description would be better in interrupt-names than here.
> > + - description: Signal coming from the GP1 pin.
>
> Also minItems should be specified to allow for just one of these
> being wired I think.
>
Ack, then maxItems is also required, or dt_binding_check complains
> "interrupts: [[...], [...]] is too long"
since I don't have the items to infer length, that means, the updated
yaml is:
interrupts:
minItems: 1
maxItems: 2
interrupt-names:
items:
- const: gp0
description: Signal coming from the GP0 pin.
- const: gp1
description: Signal coming from the GP1 pin.
> > +
> > + interrupt-names:
> > + items:
> > + - const: gp0
> > + - const: gp1
> > +
> > + cnv-gpios:
> > + description: The Convert Input (CNV). If omitted, CNV is tied to SPI CS.
> > + maxItems: 1
> > +
> > + pwms:
> > + maxItems: 1
> > + description: PWM connected to the CNV pin.
> > +
> > + trigger-sources:
> > + minItems: 1
> > + maxItems: 2
> > + description:
> > + Describes the output pin and event associated.
> > +
> > + "#trigger-source-cells":
> > + const: 2
> > + description: |
> > + Output pins used as trigger source.
> > +
> > + Cell 0 defines the event:
> > + * 0 = Data ready
> > + * 1 = Min threshold
> > + * 2 = Max threshold
> > + * 3 = Either threshold
> > + * 4 = CHOP control
> > + * 5 = Device enable
> > + * 6 = Device ready (only GP1)
>
> Hmm. I'm a bit dubious on why 'what the offload trigger is'
> is a DT thing? Is that because the IP needs to comprehend
> this? I guess only data ready is actually supported in
> practice?
A trigger can be connected to trigger something other than a spi
offload, it is in the DT because it describes how the device is
connected. When using spi offload, the trigger-source at the spi handle
describes which gpio and event is routed to the offload trigger input.
At the ADC node, trigger-source-cells describe the source gpio and event
for the device driver.
In practice, in this series, one gpio is Data ready, triggering offload
when buffer enabled, and raw reads, when disabled. And the other is
Either threshold, propagated as an IIO event. Fancy logic can be added
to the driver in future patches to allow other combinations.
It is also worth to mention that the trigger-source is duplicated for
each node that uses it, as seen in the second dts example:
&adc AD4052_TRIGGER_EVENT_DATA_READY AD4052_TRIGGER_PIN_GP1
Is repeated on both adc and spi node.
One last thing, on the driver, for v3, I should handle -ENOENT:
ret = of_parse_phandle_with_args(np, "trigger-sources",
"#trigger-source-cells", i,
&trigger_sources);
if (ret)
return ret == -ENOENT ? 0 : ret;
To assert only when present, since the nodes are not required.
Or, in the driver,
require AD4052_TRIGGER_PIN_GP0 if irq_get_byname finds gp0, and
require AD4052_TRIGGER_PIN_GP1 if irq_get_byname finds gp1?
(I would go with the first option).
>
> What would the use of Device enable or device ready or chop
> control actually be?
>
Device enable is the default register value, it could signal the
conclusion of the reset or exit of low power mode. The CHOP signal is
used to synchronize an auto-zero amplifier's chopping and the adc's
sampling and is an example of when the trigger is connected to something
other than spi. In that particular topology, the user would then set at
the devicetree to use the gpio for that, at the hypothetical chop
consumer node. Still, in the driver of this series, only the Data ready
and Either threshold is supported.
Best regards,
Jorge
> The thresholds are unusual but those I can sort of understand.
>
> Jonathan
>
> > +
> > + Cell 1 defines which pin:
> > + * 0 = GP0
> > + * 1 = GP1
> > +
> > + For convenience, macros for these values are available in
> > + dt-bindings/iio/adc/adi,ad4052.h.
> > +
> > + spi-max-frequency:
> > + maximum: 83333333
> > +
> > + vdd-supply:
> > + description: Analog power supply.
> > +
> > + vio-supply:
> > + description: Digital interface logic power supply.
> > +
> > + ref-supply:
> > + description: |
>
> Don't need the | as no need to preserve anything about formatting of
> a single paragraph like this.
>
Ack.
>
> > + Reference voltage to set the ADC full-scale range. If not present,
> > + vdd-supply is used as the reference voltage.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - vdd-supply
> > + - vio-supply
> > +
> > +allOf:
> > + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + #include <dt-bindings/iio/adc/adi,ad4052.h>
> > +
> > + spi {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + adc@0 {
> > + compatible = "adi,ad4052";
> > + reg = <0>;
> > + vdd-supply = <&vdd>;
> > + vio-supply = <&vio>;
> > + ref-supply = <&ref>;
> > + spi-max-frequency = <83333333>;
> > +
> > + #trigger-source-cells = <2>;
> > + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> > + AD4052_TRIGGER_PIN_GP0
> > + &adc AD4052_TRIGGER_EVENT_DATA_READY
> > + AD4052_TRIGGER_PIN_GP1>;
> > + interrupt-parent = <&gpio>;
> > + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> > + <0 1 IRQ_TYPE_EDGE_FALLING>;
> > + interrupt-names = "gp0", "gp1";
> > + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> > + };
> > + };
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + #include <dt-bindings/iio/adc/adi,ad4052.h>
> > +
> > + rx_dma {
> > + #dma-cells = <1>;
> > + };
> > +
> > + spi {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + dmas = <&rx_dma 0>;
> > + dma-names = "offload0-rx";
> > + trigger-sources = <&adc AD4052_TRIGGER_EVENT_DATA_READY
> > + AD4052_TRIGGER_PIN_GP1>;
> > +
> > + adc@0 {
> > + compatible = "adi,ad4052";
> > + reg = <0>;
> > + vdd-supply = <&vdd>;
> > + vio-supply = <&vio>;
> > + spi-max-frequency = <83333333>;
> > + pwms = <&adc_trigger 0 10000 0>;
> > +
> > + #trigger-source-cells = <2>;
> > + trigger-sources = <&adc AD4052_TRIGGER_EVENT_EITHER_THRESH
> > + AD4052_TRIGGER_PIN_GP0
> > + &adc AD4052_TRIGGER_EVENT_DATA_READY
> > + AD4052_TRIGGER_PIN_GP1>;
> > + interrupt-parent = <&gpio>;
> > + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>,
> > + <0 1 IRQ_TYPE_EDGE_FALLING>;
> > + interrupt-names = "gp0", "gp1";
> > + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> > + };
> > + };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c02d83560058f7ea75e24509b4d87ef293df6773..d000c7de7ff9eba390f87593bc2b1847f966f48b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1337,6 +1337,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@...log.com>
> > +S: Supported
> > +W: https://ez.analog.com/linux-software-drivers
> > +F: Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml
> > +
> > ANALOG DEVICES INC AD4080 DRIVER
> > M: Antoniu Miclaus <antoniu.miclaus@...log.com>
> > L: linux-iio@...r.kernel.org
> > diff --git a/include/dt-bindings/iio/adc/adi,ad4052.h b/include/dt-bindings/iio/adc/adi,ad4052.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..37db5d9d10e788d5e7fb715c4ba9077e555131d5
> > --- /dev/null
> > +++ b/include/dt-bindings/iio/adc/adi,ad4052.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +
> > +#ifndef _DT_BINDINGS_ADI_AD4052_H
> > +#define _DT_BINDINGS_ADI_AD4052_H
> > +
> > +#define AD4052_TRIGGER_EVENT_DATA_READY 0
> > +#define AD4052_TRIGGER_EVENT_MIN_THRESH 1
> > +#define AD4052_TRIGGER_EVENT_MAX_THRESH 2
> > +#define AD4052_TRIGGER_EVENT_EITHER_THRESH 3
> > +#define AD4052_TRIGGER_EVENT_CHOP 4
> > +#define AD4052_TRIGGER_EVENT_DEV_ENABLED 5
> > +#define AD4052_TRIGGER_EVENT_DEV_READY 6
> > +
> > +#define AD4052_TRIGGER_PIN_GP0 0
> > +#define AD4052_TRIGGER_PIN_GP1 1
> > +
> > +#endif /* _DT_BINDINGS_ADI_AD4052_H */
> >
>
Powered by blists - more mailing lists