On Fri, 13 Jun 2025 13:17:46 +0200 Jorge Marques <gastmaier@xxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx/ > > >> > > > > > > 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@xxxxxxxxxx/ > > > exposes GPIO[0..3] through gpio_chip if gpio-controller in dt. > > > > > > https://lore.kernel.org/linux-iio/713fd786010c75858700efaec8bb285274e7057e.1749569957.git.Jonathan.Santos@xxxxxxxxxx/ > > > 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
