Message-ID: <20240903201825.442502bb@jic23-huawei>
Date: Tue, 3 Sep 2024 20:18:25 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Angelo Dureghello <adureghello@...libre.com>
Cc: Conor Dooley <conor@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>, Nuno Sá
 <nuno.sa@...log.com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Olivier Moysan
 <olivier.moysan@...s.st.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 dlechner@...libre.com
Subject: Re: [PATCH RFC 4/8] dt-bindings: iio: dac: add adi axi-dac bus
 property

On Mon, 2 Sep 2024 11:32:37 +0200
Angelo Dureghello <adureghello@...libre.com> wrote:

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

Whilst I'm not sure we should be hiding the bus element away (rather than
doing something closer to an spi bus + offloads) if we do end up like this
maybe encode the bus type in the compatible. It's definitely the
adi,axi-dac-qspi* specific variant if there isn't an explicit register to
program to tell it not use a parallel bus or similar.

Jonathan


> 
> 
> > .
> > 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
> >> :`___:
> >>   `____:
> >>  

Powered by blists - more mailing lists