Re: [PATCH v3 04/10] dt-bindings: iio: dac: ad3552r: add io-backend support

[Krzysztof Kozlowski <krzk@kernel.org>]

On 24/09/2024 14:27, Nuno Sá wrote:
> On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:
>> On 23/09/2024 17:50, Angelo Dureghello wrote:
>>> Hi Krzysztof,
>>>
>>> On 22/09/24 23:02, Krzysztof Kozlowski wrote:
>>>> On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello wrote:
>>>>> From: Angelo Dureghello <adureghello@baylibre.com>
>>>>>
>>>>> There is a version AXI DAC IP block (for FPGAs) that provides
>>>>> a physical bus for AD3552R and similar chips, and acts as
>>>>> an SPI controller.
>>>>>
>>>>> For this case, the binding is modified to include some
>>>>> additional properties.
>>>>>
>>>>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
>>>>> ---
>>>>>   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42
>>>>> ++++++++++++++++++++++
>>>>>   1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>> b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>> index 41fe00034742..aca4a41c2633 100644
>>>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>> @@ -60,6 +60,18 @@ properties:
>>>>>       $ref: /schemas/types.yaml#/definitions/uint32
>>>>>       enum: [0, 1, 2, 3]
>>>>>   
>>>>> +  io-backends:
>>>>> +    description: The iio backend reference.
>>>>> +      An example backend can be found at
>>>>> +       
>>>>> https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  adi,synchronous-mode:
>>>>> +    description: Enable waiting for external synchronization signal.
>>>>> +      Some AXI IP configuration can implement a dual-IP layout, with
>>>>> internal
>>>>> +      wirings for streaming synchronization.
>>>>> +    type: boolean
>>>>> +
>>>>>     '#address-cells':
>>>>>       const: 1
>>>>>   
>>>>> @@ -128,6 +140,7 @@ patternProperties:
>>>>>             - custom-output-range-config
>>>>>   
>>>>>   allOf:
>>>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>>>>     - if:
>>>>>         properties:
>>>>>           compatible:
>>>>> @@ -238,4 +251,33 @@ examples:
>>>>>               };
>>>>>           };
>>>>>       };
>>>>> +
>>>>> +  - |
>>>>> +    axi_dac: spi@44a70000 {
>>>>> +        compatible = "adi,axi-ad3552r";
>>>> That is either redundant or entire example should go to the parent node,
>>>> if this device is fixed child of complex device (IOW, adi,ad3552r cannot
>>>> be used outside of adi,axi-ad3552r).
>>>
>>> ad3552r can still be used by a generic "classic" spi
>>> controller (SCLK/CS/MISO) but at a slower samplerate, fpga
>>> controller only (axi-ad3552r) can reach 33MUPS.
>>
>> OK, then this is just redundant. Drop the node. Parent example should
>> contain the children, though.
>>>
>>>>
>>>>> +        reg = <0x44a70000 0x1000>;
>>>>> +        dmas = <&dac_tx_dma 0>;
>>>>> +        dma-names = "tx";
>>>>> +        #io-backend-cells = <0>;
>>>>> +        clocks = <&ref_clk>;
>>>>> +
>>>>> +        #address-cells = <1>;
>>>>> +        #size-cells = <0>;
>>>>> +
>>>>> +        dac@0 {
>>>>> +            compatible = "adi,ad3552r";
>>>>> +            reg = <0>;
>>>>> +            reset-gpios = <&gpio0 92 0>;
>>>> Use standard defines for GPIO flags.
>>>
>>> fixed, thanks
>>>
>>>>> +            io-backends = <&axi_dac>;
>>>> Why do you need to point to the parent? How much coupled are these
>>>> devices? Child pointing to parent is not usually expected, because
>>>> that's obvious.
>>>
>>>
>>> "io-backends" is actually the way to refer to the backend module,
>>> (used already for i.e. ad9739a),
>>> it is needed because the backend is not only acting as spi-controller,
>>> but is also providing some APIs for synchronization and bus setup support.
>>
>>
>> But if backend is the parent, then this is redundant. You can take it
>> from the child-parent relationship. Is this pointing to other devices
>> (non-parent) in other ad3552r configurations?
>>
> 
> The backend is a provider-consumer type of API. On the consumer side (which is the
> driver the child node will probe on), we need to call devm_iio_backend_get() to get
> the backend object (which obviously is the parent). For that, 'io-backends' is being

You described the driver, so how does it matter? Driver can call
get_backend_from_parent(), right? Or get_backend_from_fwnode(parent)?

> used. We do have another API called __devm_iio_backend_get_from_fwnode_lookup() that
> could be used with the parent fwnode and should work. However that was only added to
> keep backward compatibility in the first user of the IIO backend framework and it's
> not really meant to be used again. We are aware this is awkward at the very least [1]
> but hopefully still acceptable.

Don't use driver reasons for discussing hardware. Answer my first
question, because it is still ignored.

Best regards,
Krzysztof