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@xxxxxxxxxxxx> > > > > > > > > > > > > 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@xxxxxxxxxxxx> > > > > > > --- > > > > > > 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á > > > > >
