Re: [PATCH v2 1/4] dt-bindings: soundwire: add slave bindings

[Srinivas Kandagatla]

On 08/08/2019 16:58, Pierre-Louis Bossart wrote:
> > +++ b/Documentation/devicetree/bindings/soundwire/slave.txt
> > @@ -0,0 +1,46 @@
> > +SoundWire slave device bindings.
> > +
> > +SoundWire is a 2-pin multi-drop interface with data and clock line.
> > +It facilitates development of low cost, efficient, high performance 
> > systems.
> > +
> > +SoundWire slave devices:
> > +Every SoundWire controller node can contain zero or more child nodes
> > +representing slave devices on the bus. Every SoundWire slave device is
> > +uniquely determined by the enumeration address containing 5 fields:
> > +SoundWire Version, Instance ID, Manufacturer ID, Part ID and Class ID
> > +for a device. Addition to below required properties, child nodes can
> > +have device specific bindings.
>
> In case the controller supports multiple links, what's the encoding then?
> in the MIPI DisCo spec there is a linkId field in the _ADR encoding that 
> helps identify which link the Slave device is connected to
>  >> +
> > +Required property for SoundWire child node if it is present:
> > +- compatible:ÂÂÂÂ "sdwVER,MFD,PID,CID". The textual representation of
> > +ÂÂÂÂÂÂÂÂÂ SoundWire Enumeration address comprising SoundWire
> > +ÂÂÂÂÂÂÂÂÂ Version, Manufacturer ID, Part ID and Class ID,
> > +ÂÂÂÂÂÂÂÂÂ shall be in lower-case hexadecimal with leading
> > +ÂÂÂÂÂÂÂÂÂ zeroes suppressed.
> > +ÂÂÂÂÂÂÂÂÂ Version number '0x10' represents SoundWire 1.0
> > +ÂÂÂÂÂÂÂÂÂ Version number '0x11' represents SoundWire 1.1
> > +ÂÂÂÂÂÂÂÂÂ ex: "sdw10,0217,2010,0"
> > +
> > +- sdw-instance-id: Should be ('Instance ID') from SoundWire
> > +ÂÂÂÂÂÂÂÂÂ Enumeration Address. Instance ID is for the cases
> > +ÂÂÂÂÂÂÂÂÂ where multiple Devices of the same type or Class
> > +ÂÂÂÂÂÂÂÂÂ are attached to the bus.
>
> so it is actually required if you have a single Slave device? Or is it 
> only required when you have more than 1 device of the same type?

This is mandatory for any slave device!

> FWIW in the MIPI DisCo spec we kept the instanceID as part of the _ADR, 
> so it's implicitly mandatory (and ignored by the bus if there is only 
> one device of the same time)
>
> > +
> > +SoundWire example for Qualcomm's SoundWire controller:
> > +
> > +soundwire@c2d0000 {
> > +ÂÂÂ compatible = "qcom,soundwire-v1.5.0"
> > +ÂÂÂ reg = <0x0c2d0000 0x2000>;
> > +
> > +ÂÂÂ spkr_left:wsa8810-left{
> > +ÂÂÂÂÂÂÂ compatible = "sdw10,0217,2010,0";
> > +ÂÂÂÂÂÂÂ sdw-instance-id = <1>;
> > +ÂÂÂÂÂÂÂ ...
> > +ÂÂÂ };
> > +
> > +ÂÂÂ spkr_right:wsa8810-right{
> > +ÂÂÂÂÂÂÂ compatible = "sdw10,0217,2010,0";
> > +ÂÂÂÂÂÂÂ sdw-instance-id = <2>;
>
> Isn't the MIPI encoding reported in the Dev_ID0..5 registers 0-based?
>
> > +ÂÂÂÂÂÂÂ ...
> > +ÂÂÂ };
> > +};
>
> And now that I think of it, wouldn't it be simpler for everyone if we 
> aligned on that MIPI DisCo public spec? e.g. you'd have one property 
> with a 64-bit number that follows the MIPI spec. No special encoding 
> necessary for device tree cases, your DT blob would use this:

Thanks for the suggestion, adding 64 device bits as compatible string 
should take care of linkID too. I will give that a go!

> soundwire@c2d0000 {
>  ÂÂÂÂcompatible = "qcom,soundwire-v1.5.0"
>  ÂÂÂÂreg = <0x0c2d0000 0x2000>;
>
>  ÂÂÂÂspkr_left:wsa8810-left{
>  ÂÂÂÂÂÂÂ compatible = "sdw00 00 10 02 17 20 10 00"
>  ÂÂÂÂ}
>
>  ÂÂÂÂspkr_right:wsa8810-right{
>  ÂÂÂÂÂÂÂ compatible = "sdw0000100217201100"
>  ÂÂÂÂ}
> }
>
> We could use parentheses if it makes people happier, but the information 
> from the MIPI DisCo spec can be used as is, and provide a means for spec 
> changes via reserved bits.