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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4571 bytes --]

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?

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

Cheers,
Conor.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]