Message-ID: <20240906-reveler-waggle-8a7043690633@squawk>
Date: Fri, 6 Sep 2024 09:50:30 +0100
From: Conor Dooley <conor@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Angelo Dureghello <adureghello@...libre.com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Jonathan Cameron <jic23@...nel.org>, 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 Thu, Sep 05, 2024 at 11:50:45AM +0200, Nuno Sá wrote:
> On Fri, 2024-08-30 at 16:33 +0100, 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.
> > 
> 
> Hi Conor,
> 
> So most of these designs have some changes (even if minimal) in the register map
> and the idea (mine actually) with this backend stuff was to keep the backend
> driver (axi-dac/adc) with the generic compatible since all the (different)
> functionality is basically defined by the frontend they connect too and that
> functionality is modeled by IIO backend ops. For some more
> significant/fundamental differences in the IP like this bus controller kind of
> thing, we would add have proper FW properties. The main idea was kind of using
> the frontend + generic backend combo so no need for new compatibles for every
> new design.
> 
> It's still early days (at least upstream) for these IP cores and the backend
> code so if you say that we should have new compatibles for every new design that
> has some differences in the register map (even if minimal), I'm of course fine
> with it. I've done it like this because I was (am) kind of afraid for things to
> get complicated fairly quickly both in the bindings and driver (well maybe not
> in the driver). OTOH, it can simplify things a lot as it's way easier to
> identify different implementations of the IP directly in the driver so we have
> way more flexibility.

Most of my opinion on this from a usability perspective for your
customers, rather than how the kernel is going to handle it. If a user
is inserting a preconfigured instance of the IP, for a specific ADC or
DAC, into their design I think it makes more sense to have a compatible,
rather than expect the user to reverse engineer how the IP has been
configured and which properties they should select. My own policy for
Microchip's stuff is that if something has a name or entry in the IP
catalogue then it should have a dedicated compatible, even if it is just a
preconfigured version of some other IP block and I guess what I am
saying here is an extension of that.

I suspect that in many cases the specific compatible won't be required,
and a fallback to the generic one will suffice for the driver, and it
would only be for cases like this, that have "significant/fundamental
differences" that the driver would need the specific one.

> 
> > 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?
> > 
> > > > 
> > > > 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.
> 
> Yes, pretty much we have a generic core with most of the designs being based on
> it but with some slight differences. At least for the new ones, almost all of
> them have slight deviations from the generic/base core.
> 
> > 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?
> > 
> 
> My original idea was to have a bus controller boolean for this core at least for
> now that we only have one bus type (so we could assume qspi in the driver). If
> the time comes we need to add support for something else, then we would need
> another property to identify the type.

With a specific compatible, you can "easily" add different defaults. So
the other devices could default to no bus when a bus related property is
required and this one could default to qspi. But unless there are
ADCs/DACs that have a backend that can be configured with different
types of bus, a property for this wouldn't be needed - the compatible
and match data would suffice.

> 
> > > > 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.
> > 
> 
> Just by looking at the datasheet it feels like typical qspi to be honest. And,
> fwiw, even if not really qspi, this is how the datasheet names the interface.

Right, just a phrasing issue in the cover letter I guess :)

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists