| 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: <zd4fvyjbfurgsp3rpslo2ubpxzxn7bh5b2vh5j4j7outxdrcd7@firxlr6bfkic>
Date: Thu, 12 Jun 2025 21:42:44 +0200
From: Jorge Marques <gastmaier@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
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>, 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
Hi David,
thank you for chiming in
On Thu, Jun 12, 2025 at 10:03:37AM -0500, David Lechner wrote:
> On 6/12/25 5:11 AM, Jorge Marques wrote:
> > 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:
> >>
>
> ...
>
> >>> + trigger-sources:
> >>> + minItems: 1
> >>> + maxItems: 2
> >>> + description:
> >>> + Describes the output pin and event associated.
>
> trigger-sources would be an input pin connected to an external trigger.
> For example, the CNV pin could be connected to a trigger-source
> provider to trigger a conversion. But there aren't any other digital
> inputs, so I don't know what the 2nd source would be here.
>
> As an example, see [1]. We could potentially use the same gpio
> trigger-source for the conversion pin here. There is already
> a similar binding for pwm triggers, so we could drop the separate
> pwms binding as well an just have a single trigger-sources
> property for the CNV pin that works for both gpio and pwm.
>
> [1]: https://lore.kernel.org/linux-iio/cover.1749569957.git.Jonathan.Santos@analog.com/
>
Quick summary to familiarize myself with this part and driver.
On ad7768-1:
ad7768-1.SYNC_OUT is a digital output, ad7768-1.SYNC_IN input, and
ad7768-1.GPIO3 (START) configured as input. ad7768-1.GPIO[0..3] are
configurable GPIO, GPIO3 as START, or in PIN control mode, the input
GPIO[3:0] sets the power mode and modulator freq (MODEx).
On that thread:
https://lore.kernel.org/linux-iio/8abca580f43cb31d7088d07a7414b5f7efe91ead.1749569957.git.Jonathan.Santos@analog.com/
exposes GPIO[0..3] through gpio_chip if gpio-controller in dt.
https://lore.kernel.org/linux-iio/713fd786010c75858700efaec8bb285274e7057e.1749569957.git.Jonathan.Santos@analog.com/
trigger-sources-cells: the cell define the type of signal but *not* its
origin, because {DRDY, SYNC_OUT, GPIO3(START)} are dedicated pins, *so
there is no need to do so*.
> >>> +
> >>> + "#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.
>
> That sounds wrong. This would only make sense if an output of the
> ADC was wired back to itself.
>
The issue is the lack of way of describing to the driver the function of
each gpio, when configurable. Perhaps it is better to use
trigger-source-cells to only describe the topology at that node
receiving the trigger, e.g.
trigger-sources = <&adc AD4052_TRIGGER_PIN_GP0>;
Below I continue the discussion.
> >
> > 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).
> >>
>
> ,,,
>
> >>> +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>;
>
> This doesn't make sense for the reason given above. These outputs
> aren't wired back to inputs on the ADC. They are wired to interrupts
> on the MCU, which is already described below.
>
Below.
> >>> + 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";
>
> The dmas aren't related to the ADC, so can be left out of the example.
>
Ack.
> >>> + 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>;
>
> Same as above - the GP pins aren't wired back to the ADC itself.
>
> >>> + 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>;
> >>> + };
> >>> + };
Considering the discussion above. As is, in this series GP0 is event
Either threshold and GP1 Data ready. A future series would aim to make
it truly configurable.
For this series then, do we then drop the second cell of trigger cell
and do not provide a way of describing the function of each gpio? e.g.
- |
#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>;
trigger-sources = <&adc AD4052_TRIGGER_PIN_GP0>;
adc@0 {
compatible = "adi,ad4052";
reg = <0>;
vdd-supply = <&vdd>;
vio-supply = <&vio>;
spi-max-frequency = <83333333>;
pwms = <&adc_trigger 0 10000 0>;
// --- Other thought ------
//adi,gpio-role = <AD4052_TRIGGER_EVENT_EITHER_THRESH
// AD4052_TRIGGER_EVENT_DATA_READY>;
// ------------------------
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>;
};
};
Other thought is to add an "adi,gpio-role" property to define gpio
function (as commented in the example above, matched with index of
interrupts-names). If no interrupt-name.gp0 but trigger-source.GP0,
assume role Data ready (no irq for raw read, only buffer offload).
What is your opinion on this?
Best regards,
Jorge
Powered by blists - more mailing lists