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

["Nuno Sá" <noname.nuno@gmail.com>]

On Fri, 2024-09-06 at 09:50 +0100, Conor Dooley wrote:
> 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@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.
> > > 
> > 
> > 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.
> 

Hmm, indeed the above makes sense...

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

Hopefully yes :)

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

Agreed...

- Nuno Sá

> > > > >