Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 13 Jan 2025 21:41:04 -0300
Jonathan Santos <jonath4nns@gmail.com> wrote:

> On 01/12, Jonathan Cameron wrote:
> > On Fri, 10 Jan 2025 18:51:06 -0300
> > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> >   
> > > On 01/07, Jonathan Santos wrote:  
> > > > Add adi,sync-in-spi property to enable synchronization over SPI.
> > > > This should be used in the case when the GPIO cannot provide a
> > > > pulse synchronous with the base MCLK signal.
> > > > 
> > > > User can choose between SPI, GPIO synchronization or neither of them,
> > > > but only if a external pulse can be provided, for example, by another
> > > > device in a multidevice setup.
> > > > 
> > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > > ---
> > > >  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
> > > >  1 file changed, 23 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > > index 3ce59d4d065f..55cec27bfe60 100644
> > > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > > @@ -47,6 +47,15 @@ properties:
> > > >        in any way, for example if the filter decimation rate changes.
> > > >        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
> > > >  
> > > > +  adi,sync-in-spi:
> > > > +    description:
> > > > +      Enables synchronization of multiple devices over SPI. This property is
> > > > +      used when a signal synchronous to the base MCLK signal cannot be provided
> > > > +      via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin
> > > > +      on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device
> > > > +      should be routed to the SYNC_IN pins of the other devices.    
> > > So, if I'm getting it right,  
> > 
> > Datasheet on this is indeed complex!
> >   
> > >/SYNC_IN may be driven by a GPIO (ADAQ7768-1 datasheet Figure 131),  
> > 
> > For that we expose a gpio binding already. If that's present we know what is going on.
> >   
> > >/SYNC_IN may be driven by own device /SYNC_OUT (ADAQ7768-1 datasheet Figure 133),  
> > 
> > This is the default - no information provided so it isn't wired externally.
> > We don't normally bother to describe required chip to chip connections.
> > I couldn't entirely figure out if this is 'required' if we aren't driving explicitly
> > from GPIO or another chip but i think it is(?).
> >  
> 
> It is. If the device is not being driven by a GPIO or an external
> device, the user should connect the /SYNC_OUT to /SYNC_IN. We could
> assume that this is the case if the GPIO is not present in the
> devicetree, maybe put that into the description. The sync-out-sync-in
> property as a boolean can be quite redundant, since we also need to
> remove the GPIO.
> 
> > >/SYNC_IN may be driven by other AD7768-1 > /SYNC_OUT pin (also Figure 133).  
> > This is only case we are about for sync in I think.
> > 
> > As long as there isn't a valid 'not connected' It think we are fine with a boolean.
> >   
> 
> If opt for a single node instace, thats ok; otherwise, David's
> trigger-sources suggestion seems to be the best approach. 
That probably works on assumption that if no gpio, or trigger source etc
bindings are present then we assume the pins are wired together and it's
simple SPI triggering (so what we'd have if none of this complexity exists!)

Jonathan

> 
> > > That is too much to describe with a boolean.
> > > 
> > > If David's suggestion of using a trigger-source doesn't fit, this property
> > > should at least become an enum or string.
> > >   
> > > > +    type: boolean
> > > > +
> > > >    reset-gpios:
> > > >      maxItems: 1
> > > >  
> > > > @@ -65,7 +74,6 @@ required:
> > > >    - vref-supply
> > > >    - spi-cpol
> > > >    - spi-cpha
> > > > -  - adi,sync-in-gpios
> > > >  
> > > >  patternProperties:
> > > >    "^channel@([0-9]|1[0-5])$":
> > > > @@ -89,6 +97,20 @@ patternProperties:
> > > >  allOf:
> > > >    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > >  
> > > > +  # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid)
> > > > +  - if:
> > > > +      required:
> > > > +        - adi,sync-in-gpios
> > > > +    then:
> > > > +      properties:
> > > > +        adi,sync-in-spi: false
> > > > +  - if:
> > > > +      required:
> > > > +        - adi,sync-in-spi
> > > > +    then:
> > > > +      properties:
> > > > +        adi,sync-in-gpios: false
> > > > +
> > > >  unevaluatedProperties: false
> > > >  
> > > >  examples:
> > > > -- 
> > > > 2.34.1
> > > >     
> >