Re: [PATCH RFC 4/8] dt-bindings: iio: dac: add adi axi-dac bus property

[Angelo Dureghello <adureghello@baylibre.com>]

Hi Conor,


On 30/08/24 5:33 PM, Conor Dooley wrote:
> On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote:
>> Hi Conor,
>>
>> On 29/08/24 5:46 PM, Conor Dooley wrote:
>>> On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote:
>>>> From: Angelo Dureghello <adureghello@baylibre.com>
>>>>
>>>> Add bus property.
>>> RFC it may be, but you do need to explain what this bus-type actually
>>> describes for commenting on the suitability of the method to be
>>> meaningful.
>> thanks for the feedbacks,
>>
>> a "bus" is intended as a generic interface connected to the target,
>> may be used from a custom IP (fpga) to communicate with the target
>> device (by read/write(reg and value)) using a special custom interface.
>>
>> The bus could also be physically the same of some well-known existing
>> interfaces (as parallel, lvds or other uncommon interfaces), but using
>> an uncommon/custom protocol over it.
>>
>> In concrete, actually bus-type is added to the backend since the
>> ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR
>> parallel bus (interface that i named QSPI, but it's not exactly a QSPI
>> as a protocol), so it's a device-specific interface.
>>
>> With additions in this patchset, other frontends, of course not only
>> DACs, will be able to add specific busses and read/wrtie to the bus
>> as needed.
>>
>>>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml
>>>> index a55e9bfc66d7..a7ce72e1cd81 100644
>>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml
>>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml
>>>> @@ -38,6 +38,15 @@ properties:
>>>>      clocks:
>>>>        maxItems: 1
>>> You mentioned about new compatible strings, does the one currently
>>> listed in this binding support both bus types?
> You didn't answer this, and there's insufficient explanation of the
> "hardware" in this RFC, but I found this which is supposedly the
> backend:
> https://github.com/analogdevicesinc/hdl/tree/main/library/axi_ad3552r
> adi,axi-dac.yaml has a single compatible, and that compatible has
> nothing to do with "axi_ad3552r" as it is "adi,axi-dac-9.1.b". I would
> expect either justification for reuse of the compatible, or a brand new
> compatible for this backend, even if the driver can mostly be reused.
>
> Could you please link to whatever ADI wiki has detailed information on
> how this stuff works so that I can look at it to better understand the
> axes of configuration here?

https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html

that has same structure and register set of the generic ADI AXI-DAC IP:
https://wiki.analog.com/resources/fpga/docs/axi_dac_ip


>>> Making the bus type decision based on compatible only really makes sense
>>> if they're different versions of the IP, but not if they're different
>>> configuration options for a given version.
>>>
>>>> +  bus-type:
>> DAC IP on fpga actually respects same structure and register set, except
>> for a named "custom" register that may use specific bitfields depending
>> on the application of the IP.
> To paraphrase:
> "The register map is the same, except for the bit that is different".
> If ADI is shipping several different configurations of this IP for
> different DACs, I'd be expecting different compatibles for each backend
> to be honest

i am still quite new to this fpga-based implementations, at least for how
such IPs are actually interfacing to the linux subsystem, so i may miss
some point.

About the "adi,axi-dac-9.1.b" compatible, the generic DAC IP register set
is mostly the same structure of this ad3552r IP (links above), except for
bitfields in the DAC_CUSTOM_CTRL register.

My choice for now was to add a bus-type property.

Not an HDL expert, but i think a different bus means, from an hardware 
point of
view, a different IP in terms of internal fpga circuitry, even if not as a
register-set.


> .
> If each DAC specific backend was to have a unique compatible, would the
> type of bus used be determinable from it? Doesn't have to work for all
> devices from now until the heath death of the universe, but at least for
> the devices that you're currently aware of?
>
>>> If, as you mentioned, there are multiple bus types, a non-flag property
>>> does make sense. However, I am really not keen on these "forced" numerical
>>> properties at all, I'd much rather see strings used here.
>>>> +    maxItems: 1
>>>> +    description: |
>>>> +      Configure bus type:
>>>> +        - 0: none
>>>> +        - 1: qspi
> Also, re-reading the cover letter, it says "this platform driver uses a 4
> lanes parallel bus, plus a clock line, similar to a qspi."
> I don't think we should call this "qspi" if it is not actually qspi,
> that's just confusing.

Agree, name should be something different.


> Cheers,
> Conor.

Thanks,
regards,

Angelo


>>>> +    enum: [0, 1]
>>>> +    default: 0
>>>> +
>>>>      '#io-backend-cells':
>>>>        const: 0
>>>>
>>>> -- 
>>>> 2.45.0.rc1
>>>>
>> -- 
>>   ,,,      Angelo Dureghello
>> :: :.     BayLibre -runtime team- Developer
>> :`___:
>>   `____:
>>
-- 
  ,,,      Angelo Dureghello
:: :.     BayLibre -runtime team- Developer
:`___:
  `____: