Re: [alsa-devel] [RESEND PATCH v4 1/4] dt-bindings: soundwire: add slave bindings

[Pierre-Louis Bossart]

On 8/23/19 10:57 AM, Srinivas Kandagatla wrote:
> On 23/08/2019 16:41, Pierre-Louis Bossart wrote:
> > On 8/22/19 6:37 PM, Srinivas Kandagatla wrote:
> > > This patch adds bindings for Soundwire Slave devices that includes how
> > > SoundWire enumeration address and Link ID are used to represented in
> > > SoundWire slave device tree nodes.
> > >
> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > > ---
> > > Â .../soundwire/soundwire-controller.yamlÂÂÂÂÂÂ | 75 +++++++++++++++++++
> > > Â 1 file changed, 75 insertions(+)
> > > Â create mode 100644 
> > > Documentation/devicetree/bindings/soundwire/soundwire-controller.yaml
> > >
> > > diff --git 
> > > a/Documentation/devicetree/bindings/soundwire/soundwire-controller.yaml 
> > > b/Documentation/devicetree/bindings/soundwire/soundwire-controller.yaml
> > > new file mode 100644
> > > index 000000000000..91aa6c6d6266
> > > --- /dev/null
> > > +++ 
> > > b/Documentation/devicetree/bindings/soundwire/soundwire-controller.yaml
> > > @@ -0,0 +1,75 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/soundwire/soundwire-controller.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: SoundWire Controller Generic Binding
> > > +
> > > +maintainers:
> > > +Â - Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > > +
> > > +description: |
> > > +Â SoundWire busses can be described with a node for the SoundWire 
> > > controller
> > > +Â device and a set of child nodes for each SoundWire slave on the bus.
> > > +
> > > +properties:
> > > +Â $nodename:
> > > +ÂÂÂ pattern: "^soundwire(@.*|-[0-9a-f])*$"

re-reading this, it looks like you are defining the controller bindings, 
but there are no real controller-level properties except for the fact 
that they include slave bindings?

In MIPI the notion of controller is that it can deal with multiple 
links, each of which having specific properties (clock speed, clock stop 
properties, etc).

> > > +
> > > +Â "#address-cells":
> > > +ÂÂÂ const: 2
> > > +
> > > +Â "#size-cells":
> > > +ÂÂÂ const: 0
> > > +
> > > +patternProperties:
> > > +Â "^.*@[0-9a-f]+$":
> > > +ÂÂÂ type: object
> > > +
> > > +ÂÂÂ properties:
> > > +ÂÂÂÂÂ compatible:
> > > +ÂÂÂÂÂ pattern: "^sdw[0-9][0-9a-f]{4}[0-9a-f]{4}[0-9a-f]{2}$"
> >
> > So is this a 64-bit value, as in the MIPI spec, or is this part of the 
> > _ADR description?
>
> Rob did not like encoding compatible string exactly like _ADR encoding.
>
> https://lkml.org/lkml/2019/8/22/490

Wondering if we are talking about different concepts?

Rob's point was about the InstanceID

"Assuming you could have more than 1 of the same device on the bus,
then you need some way to distinguish them and the way that's done for
DT is unit-address/reg. And compatible strings should be constant for
each instance."

You can use the MIPI encoding *except* for the InstanceID, that'd be 
fine. It'll just be a bit weird since the Slave device will report the 
48 bits that include the Instance ID, so you'll have to special case 
this field, but if this is a DT requirement then fine.

Rob's point does not apply to the link ID - which is used when you have 
multiple masters in your controller. The Slave device is attached in one 
location and will never move, so that is a constant value.

> > I also don't get why the first item in in base10?
>
> As this corresponds to Soundwire Version, and I have no visibility of 
> version number encoding after reaching number 9 in this field.
>
> This can be updated once we have more info on how the Version encoding 
> will look like in future.
>
> Idea of limiting regex to [0-9] for version is to enforce some checking!

the version is a 4 bit value starting at 1 for SoundWire 1.0. There is 
nothing in the spec that talks about a limit to 9.

It's unlikely we'll ever reach that but you are interpreting a spec 
here. plus just below you mention all fields as being hexadecimal.

> --srini
>
> > > +ÂÂÂÂÂ description:
> > > +ÂÂÂÂÂ Is the textual representation of SoundWire Enumeration
> > > +ÂÂÂÂÂ address. compatible string should contain SoundWire Version ID,
> > > +ÂÂÂÂÂ Manufacturer ID, Part ID and Class ID in order and shall be in
> > > +ÂÂÂÂÂ lower-case hexadecimal with leading zeroes.
> > > +ÂÂÂÂÂ Valid sizes of these fields are
> > > +ÂÂÂÂÂ Version ID is 1 nibble, number '0x1' represents SoundWire 1.0
> > > +ÂÂÂÂÂ and '0x2' represents SoundWire 1.1 and so on.
> > > +ÂÂÂÂÂ MFD is 4 nibbles
> > > +ÂÂÂÂÂ PID is 4 nibbles
> > > +ÂÂÂÂÂ CID is 2 nibbles
> > > +ÂÂÂÂÂ More Information on detail of encoding of these fields can be
> > > +ÂÂÂÂÂ found in MIPI Alliance DisCo & SoundWire 1.0 Specifications.
> > > +
> > > +ÂÂÂÂÂ reg:
> > > +ÂÂÂÂÂÂÂ maxItems: 1
> > > +ÂÂÂÂÂÂÂ description:
> > > +ÂÂÂÂÂÂÂÂÂ Instance ID and Link ID of SoundWire Device Address.
> > > +
> > > +ÂÂÂ required:
> > > +ÂÂÂÂÂ - compatible
> > > +ÂÂÂÂÂ - reg
> > > +
> > > +examples:
> > > +Â - |
> > > +ÂÂÂ soundwire@c2d0000 {
> > > +ÂÂÂÂÂÂÂ #address-cells = <2>;
> > > +ÂÂÂÂÂÂÂ #size-cells = <0>;
> > > +ÂÂÂÂÂÂÂ compatible = "qcom,soundwire-v1.5.0";
> > > +ÂÂÂÂÂÂÂ reg = <0x0c2d0000 0x2000>;
> > > +
> > > +ÂÂÂÂÂÂÂ speaker@1 {
> > > +ÂÂÂÂÂÂÂÂÂÂÂ compatible = "sdw10217201000";
> > > +ÂÂÂÂÂÂÂÂÂÂÂ reg = <1 0>;
> > > +ÂÂÂÂÂÂÂ };
> > > +
> > > +ÂÂÂÂÂÂÂ speaker@2 {
> > > +ÂÂÂÂÂÂÂÂÂÂÂ compatible = "sdw10217201000";
> > > +ÂÂÂÂÂÂÂÂÂÂÂ reg = <2 0>;
> > > +ÂÂÂÂÂÂÂ };
> > > +ÂÂÂ };
> > > +
> > > +...