Message-ID: <a6e57474-6eff-4f7b-8204-8137f95a33e7@baylibre.com>
Date: Tue, 28 Jan 2025 09:56:37 -0600
From: David Lechner <dlechner@...libre.com>
To: 0044dd4b-01ce-4ca0-9855-8c239b9bfb6f@...libre.com
Cc: Jonathan Santos <Jonathan.Santos@...log.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, lars@...afoo.de,
 Michael.Hennerich@...log.com, marcelo.schmitt@...log.com, jic23@...nel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 marcelo.schmitt1@...il.com
Subject: Re: [PATCH v2 02/16] dt-bindings: iio: adc: ad7768-1: add
 trigger-sources property

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

> 
>>>  
>>>    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";
>>

Powered by blists - more mailing lists