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

[Sabau, Radu bogdan]

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

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

> > +
> > +            interrupts = <12 4>;

Best Regards,
Radu