| 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: <20250614111454.25741044@jic23-huawei>
Date: Sat, 14 Jun 2025 11:14:54 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Jorge Marques <gastmaier@...il.com>
Cc: David Lechner <dlechner@...libre.com>, 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
On Fri, 13 Jun 2025 13:17:46 +0200
Jorge Marques <gastmaier@...il.com> wrote:
> Hi David,
> On Thu, Jun 12, 2025 at 03:20:40PM -0500, David Lechner wrote:
> > On 6/12/25 2:42 PM, Jorge Marques wrote:
> > > 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.
> >
> > The bindings can't be changed later, so no, don't drop the 2nd cell
> > if we are going to add it back later.
> >
> > But considering Jonathan's feedback, I am now questioning if we need
> > the 2nd cell at all. The way trigger-source consumers work currently
> > is that they request a trigger of a certain generic type, like "data
> > ready". So this information could be used to determine what function
> > needs to be assigned to the pin without having to define that in the
> > devicetree.
> >
> Useful for assertion. It is odd to be used for requesting of a certain
> type (gpio role) instead of telling how things are wired.
> > >
> > > - |
> > > #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?
> >
> >
> > Usually, we just have the devicetree describe how things are wired up.
> > Then the driver looks at how things are wired up and decides how to
> > best make use of the available resources. I.e. in the driver add some
> > variables in the driver state struct that keeps track of the function
> > assigned to each GP pin and use that to make decisions.
> >
> > In the driver, we would want to make sure to handle triggers first
> > since those are less flexible (so set up SPI offload first). This
> > would cause one of the GP pins to be assigned to the /RDY function.
> > It doesn't matter which one.
> >
> I will default drdy_gp to g0, until offload request overwrites it,
> either gp0 or gp1.
> > Then later, parse the interrupts property. If we see that one of
> > the GP pins is already assigned to /RDY, then we know we have to
> > use that pin for the /RDY interrupt as well. If both pins are still
> > available, then an arbitrary one can be assigned for /RDY.
> based on drdy_gp, set that gp as drdy, and the remaining is threshold
> either. the interrupt is optional, but setup device gp regardless, since
> the irq may be consumed by other device.
> >
> > Then if there is still an unused GP pin left that is actually
> > wired up to an interrupt, that can be used for the events interrupt.
> >
> > Or we could even consider to have everything on one pin since the
> > /RDY signal would never be needed at the same time as events as long
> > as the events are only ever used in monitor mode.
> >
>
> The threshold event occurs on the rising edge, the data ready on the
> falling edge (it is actually BUSY). Mixing both has a lot of nuances
> involved.
Ok. Mixing them might not make sense - but overall the decision on what
to do with any line that is just wired device to host interrupt is
a driver problem. If it's also wired to another device (including
offload engine) and that requires a specific setting (e.g. data ready)
then that is fair enough to have in DT.
I think that's roughly where this discussion ended up but just wanted
to confirm that.
Jonathan
> > If we find that there is some case though where the driver really
> > can't figure out what to do with the available information, then
> > we could probably justify adding a property like you suggested.
> > It seems like we could possibly do without it at this point though.
>
> With the proposed above, I don't need the cell 0 of trigger-sources. But
> I will keep for assertion since we are inferring
> has?trigger-sources-> -then-> drdy.
>
> Best regards,
> Jorge
Powered by blists - more mailing lists