Re: [PATCH v3 2/8] dt-bindings: iio: adc: Add adi,ad4052

[David Lechner <dlechner@baylibre.com>]

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@analog.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/

>>> +
>>> +  "#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. 

> 
> 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.

>>> +            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.

>>> +        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>;
>>> +        };
>>> +    };