Re: [PATCH v3 1/4] dt-bindings: iio: adc: add bindings for AD4691 family

[David Lechner]

On 3/16/26 7:39 AM, Sabau, Radu bogdan wrote:
> 
> 
>> -----Original Message-----
>> From: David Lechner <dlechner@xxxxxxxxxxxx>
>> Sent: Saturday, March 14, 2026 5:30 PM
>> On 3/13/26 5:07 AM, Radu Sabau via B4 Relay wrote:
> 
> ...
> 
>>> +
>>> +  clocks:
>>> +    description: Reference clock for PWM timing in CNV Clock Mode.
>>> +    maxItems: 1
>>
>> I feel like I asked this already, but which pin is this clock connected to?
>> It sounds like it is the clock for the PWM, not the ADC. So it does not belong
>> here.
>>
> 
> The pin is connected to the CNV pin of the ADC, which in CNV Clock Mode
> replaces the internal oscillator. 
> 
>>> +
>>> +  pwms:
>>> +    description:
>>> +      PWM connected to the CNV pin. When present, selects CNV Clock Mode
> 
> ...
> 
>>> +      Two cells are required:
>>> +        - First cell: Trigger event type (0 = BUSY, 1 = DATA_READY)
>>
>> I'm wondering if we really need to specify the event type. For interrupts,
>> we we just specify the pin and not the function when the pin has more than
>> one possible function.
>>
>> I know that we have done something like this on some of the previous SPI
>> offload devices. So maybe there was a good reason for it. Or maybe I just
>> had tunnel vision at the time.
>>
>> I suggest we try implementing this with just one cell that specifies the
>> physical pin. In the driver, when SPI_OFFLOAD_TRIGGER_DATA_READY is
>> requested in the driver, we can use that to program the function of the
>> pin accordingly.
> 
> I agree with this, since only DATA_READY will be used anyway as an interrupt
> in CNV_CLOCK mode.
> In fact, I am now thinking of removing ADC_BUSY entirely, since its used in
> just two cases, which none of them perhaps make sense :
> 
> 1. Manual Mode,where ADC_BUSY is selected for GPx, though is not used as
> an interrupt or 'feedback' of anyway.
> 2. Autonomous Mode, where in theory it would be used to see when each
> channel was sampled, but this mode is used for just once channel single
> shot reading, so again, not actually used.
> 
> The implementation would see the enum removed and just initializing
> the GPx pin used as DATA READY using a macro. 
> 
> What are your thoughts on this?


We should try to consider every reasonable possible wiring situation.
The only case I can think where the devicetree might need to know the
requested function in addition to which pin is if the pin is wired to
something not controlled by Linux. That is an odd enough situation though
that we could defer considering that. I think we could add support for such
a thing later if we needed to without breaking the existing bindings.

So hopefully I am thinking clearly enough about this to say, yes, we
should just go with #trigger-source-cells = <1>; where the cell is the
GP pin number.

> 
>>
>>> +        - Second cell: GPIO pin number (only 0 = GP0 is supported)
>>
>> If GP0 is the only possible pin for an output, we should omit the cell. If
>> there are more possible pins, we should document them (even if the driver
>> doesn't support it).
> 
> You are also right about this, other pins can be used as DATA_READY, and so
> the DT should perhaps indicate which of those pins is actually used, so
> that we know at probe (gpio_setup would make a comeback?) which
> value should be written to the GPIO registers.
> 
>>
>>> +
>>> +      Macros are available in dt-bindings/iio/adc/adi,ad4691.h:
>>> +        AD4691_TRIGGER_EVENT_BUSY,
> 
> ...
> 
>>> +
>>> +            clocks = <&ref_clk>;
>>> +
>>> +            pwms = <&pwm_gen 0 0>;
>>> +            pwm-names = "cnv";
>>
>> Should we also include the trigger in this example?
>>
> 
> In this example, I would say this is needed since the CNV PWM is
> not only starting the conversion on the ADC, but also controlling
> the sampling rate, making custom sampling rates available in
> comparison to the internal oscillator used by AUTONOMOUS.

The point was to have an example that shows SPI offload usage.
I assume this would be more common that PWM without SPI offload.

> 
>>> +
>>> +            interrupts = <12 4>;
> 
> Best Regards,
> Radu