Re: [PATCH v2 02/16] dt-bindings: iio: adc: ad7768-1: add trigger-sources property

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 28 Jan 2025 09:56:37 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 1/28/25 9:04 AM, Jonathan Santos wrote:
> > On 01/27, David Lechner wrote:  
> >> On 1/27/25 9:11 AM, Jonathan Santos wrote:  
> >>> Add a new trigger-sources property to enable synchronization across
> >>> multiple devices. This property references the main device (or
> >>> trigger provider) responsible for generating the pulse to drive the
> >>> SYNC_IN of all devices in the setup.
> >>>
> >>> In addition to GPIO synchronization, The AD7768-1 also supports
> >>> synchronization over SPI, which use is recommended when the GPIO
> >>> cannot provide a pulse synchronous with the base MCLK signal. It
> >>> consists of looping back the SYNC_OUT to the SYNC_IN pin and send
> >>> a command via SPI to trigger the synchronization.
> >>>
> >>> SPI-based synchronization is enabled in the absence of adi,sync-in-gpios
> >>> property. Since adi,sync-in-gpios is not long the only method, remove it
> >>> from required properties.
> >>>
> >>> While at it, add description to the interrupt property.
> >>>
> >>> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> >>> ---
> >>> v2 Changes:
> >>> * Patch added as replacement for adi,sync-in-spi patch.
> >>> * addressed the request for a description to interrupts property.
> >>> ---
> >>>  .../bindings/iio/adc/adi,ad7768-1.yaml        | 22 +++++++++++++++++--
> >>>  1 file changed, 20 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> >>> index 3ce59d4d065f..3e119cf1754b 100644
> >>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> >>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> >>> @@ -26,7 +26,17 @@ properties:
> >>>    clock-names:
> >>>      const: mclk
> >>>  
> >>> +  trigger-sources:
> >>> +    description:
> >>> +      References the main device responsible for synchronization. In a single
> >>> +      device setup, reference the own node.
> >>> +    maxItems: 1  
> >>
> >> We probably actually need 2 here. One for /SYNC_IN and one for a GPIO3 pin
> >> acting as the /START signal.
> >>  
> >>> +
> >>>    interrupts:
> >>> +    description:
> >>> +      Specifies the interrupt line associated with the ADC. This refers
> >>> +      to the DRDY (Data Ready) pin, which signals when conversion results are
> >>> +      available.
> >>>      maxItems: 1
> >>>  
> >>>    '#address-cells':
> >>> @@ -46,6 +56,8 @@ properties:
> >>>        sampling. A pulse is always required if the configuration is changed
> >>>        in any way, for example if the filter decimation rate changes.
> >>>        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
> >>> +      In the absence of this property, Synchronization over SPI will be
> >>> +      enabled.  
> >>
> >> Isn't /SYNC_OUT connected to /SYNC_IN required for synchronization over SPI?
> >>
> >> If yes, instead of adding this text, I would make the binding have:
> >>  
> > 
> > Yes, but the synchronization over SPI is enabled in the absence of the GPIO.
> > The trigger-sources property would indicate if the sync provider is the
> > own device or not. As i said below, maybe i misunderstood.
> >   
> >> oneOf:
> >>   - required:
> >>       - trigger-sources
> >>   - required:
> >>        - adi,sync-in-gpios
> >>  
> > 
> > Wouldn't be simpler to consider the absence of sync-in-gpio? this way we
> > have less changes in the ABI.  
> 
> Maybe it is me that missed something, but if I'm reading the datasheet
> correctly, then sync over SPI only works if /SYNC_IN is wired to /SYNC_OUT.
> And the chip isn't going to work correctly without some sort of sync. So we
> need something wired to /SYNC_IN no matter what.
> 
> In any case, the DT bindings should just say how the chip is wired up and not
> dictate how the driver should behave. So what I was going for with this is to
> have the bindings say that something has to be wired to /SYNC_IN and we can
> leave it up to the driver to decide what to do with this information.

I was seeing this more like an optional clock / supply etc.
If we don't provide it we assume the driver knows what to do.

So in this case lack of sync in routing off to another device means
to me it is connected to sync out.

I don't mind the self trigger thing if it fits nicely in the binding
though as we are using a trigger that is provided for other chips anyway.

So wired to sync in of this chip and potentially sync in of another
N chips.


> 
> >   
> >>>  
> >>>    reset-gpios:
> >>>      maxItems: 1
> >>> @@ -57,6 +69,9 @@ properties:
> >>>    "#io-channel-cells":
> >>>      const: 1
> >>>  
> >>> +  "#trigger-source-cells":
> >>> +    const: 0
> >>> +
> >>>  required:
> >>>    - compatible
> >>>    - reg
> >>> @@ -65,7 +80,8 @@ required:
> >>>    - vref-supply
> >>>    - spi-cpol
> >>>    - spi-cpha
> >>> -  - adi,sync-in-gpios
> >>> +  - trigger-sources
> >>> +  - #trigger-source-cells
> >>>  
> >>>  patternProperties:
> >>>    "^channel@([0-9]|1[0-5])$":
> >>> @@ -99,7 +115,7 @@ examples:
> >>>          #address-cells = <1>;
> >>>          #size-cells = <0>;
> >>>  
> >>> -        adc@0 {
> >>> +        adc0: adc@0 {
> >>>              compatible = "adi,ad7768-1";
> >>>              reg = <0>;
> >>>              spi-max-frequency = <2000000>;
> >>> @@ -109,6 +125,8 @@ examples:
> >>>              interrupts = <25 IRQ_TYPE_EDGE_RISING>;
> >>>              interrupt-parent = <&gpio>;
> >>>              adi,sync-in-gpios = <&gpio 22 GPIO_ACTIVE_LOW>;  
> >>
> >> Don't we need to drop adi,sync-in-gpios here? I don't think we would have two
> >> things connected to /SYNC_IN at the same time.
> >>  
> > 
> > I guess i misunderstood the use of trigger-sources. I thought it would
> > indicate the trigger provider or main device. Like if it points to other
> > device we should use it to drive the SYNC_IN of all devices.
> > 
> > Then what happens if the trigger-sources points to other node? we would't be
> > able to driver the SYNC_IN in case of any configuration change?  
> 
> I think you understand the trigger-source bindings correctly. 
> 
> The driver doesn't have to support everything that the DT bindings allow. This
> series is big enough already, so we can defer figuring out how to implement
> triggers other than the loopback case later. :-) We just want to make the DT
> bindings as complete as we can now.
> 
> >   
> >>> +            trigger-sources = <&adc0>;
> >>> +            #trigger-source-cells = <0>;
> >>>              reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
> >>>              clocks = <&ad7768_mclk>;
> >>>              clock-names = "mclk";  
> >>  
> 
> 
>