diff mbox series

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

Message ID 20190808144504.24823-2-srinivas.kandagatla@linaro.org (mailing list archive)
State New, archived
Headers show
Series ASoC: codecs: Add WSA881x Smart Speaker amplifier support | expand

Commit Message

Srinivas Kandagatla Aug. 8, 2019, 2:45 p.m. UTC
This patch adds bindings for Soundwire Slave devices which includes how
SoundWire enumeration address is represented in SoundWire slave device
tree nodes.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 .../devicetree/bindings/soundwire/slave.txt   | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soundwire/slave.txt

Comments

Pierre-Louis Bossart Aug. 8, 2019, 3:58 p.m. UTC | #1
> +++ 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?

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:

soundwire@c2d0000 {
	compatible = "qcom,soundwire-v1.5.0"
	reg = <0x0c2d0000 0x2000>;

	spkr_left:wsa8810-left{
		compatible = "sdw0000100217201000"
	}

	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.
Srinivas Kandagatla Aug. 8, 2019, 4:48 p.m. UTC | #2
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.
Mark Brown Aug. 8, 2019, 7:52 p.m. UTC | #3
On Thu, Aug 08, 2019 at 05:48:56PM +0100, Srinivas Kandagatla wrote:
> On 08/08/2019 16:58, Pierre-Louis Bossart wrote:

> > > +- 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!

If it's mandatory the wording is a bit unclear.  How about something
like:

	Should be ('Instance ID') from the SoundWire Enumeration
	Address.  This must always be provided, if multiple devices
	with the same type or class or attached to the bus each
	instance must have a distinct value.
Vinod Koul Aug. 9, 2019, 4:54 a.m. UTC | #4
On 08-08-19, 20:52, Mark Brown wrote:
> On Thu, Aug 08, 2019 at 05:48:56PM +0100, Srinivas Kandagatla wrote:
> > On 08/08/2019 16:58, Pierre-Louis Bossart wrote:
> 
> > > > +- 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!
> 
> If it's mandatory the wording is a bit unclear.  How about something
> like:
> 
> 	Should be ('Instance ID') from the SoundWire Enumeration
> 	Address.  This must always be provided, if multiple devices
> 	with the same type or class or attached to the bus each
> 	instance must have a distinct value.

That helps to make it clear.

Also the section of properties starts with Mandatory property, it should
be made Mandatory Properties instead, like in other binding docs to make
it clear that properties mentioned in the section are mandatory
Vinod Koul Aug. 9, 2019, 5 a.m. UTC | #5
On 08-08-19, 15:45, Srinivas Kandagatla wrote:
> This patch adds bindings for Soundwire Slave devices which includes how
> SoundWire enumeration address is represented in SoundWire slave device
> tree nodes.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  .../devicetree/bindings/soundwire/slave.txt   | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soundwire/slave.txt
> 
> diff --git a/Documentation/devicetree/bindings/soundwire/slave.txt b/Documentation/devicetree/bindings/soundwire/slave.txt
> new file mode 100644
> index 000000000000..b8e8d34bbc92
> --- /dev/null
> +++ 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

It would help to list them rather than free flowing text

> +have device specific bindings.
> +
> +Required property for SoundWire child node if it is present:

As said earlier, lets make it "Required properties:"

> +- 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.
> +
> +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>;
> +		...
> +	};
> +};
> -- 
> 2.21.0
Srinivas Kandagatla Aug. 9, 2019, 8:25 a.m. UTC | #6
On 09/08/2019 05:54, Vinod Koul wrote:
> On 08-08-19, 20:52, Mark Brown wrote:
>> On Thu, Aug 08, 2019 at 05:48:56PM +0100, Srinivas Kandagatla wrote:
>>> On 08/08/2019 16:58, Pierre-Louis Bossart wrote:
>>
>>>>> +- 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!
>>
>> If it's mandatory the wording is a bit unclear.  How about something
>> like:
>>
>> 	Should be ('Instance ID') from the SoundWire Enumeration
>> 	Address.  This must always be provided, if multiple devices
>> 	with the same type or class or attached to the bus each
>> 	instance must have a distinct value.
> 
> That helps to make it clear.
> 
> Also the section of properties starts with Mandatory property, it should
> be made Mandatory Properties instead, like in other binding docs to make
> it clear that properties mentioned in the section are mandatory

Will update as suggested!

thanks,
srini
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/soundwire/slave.txt b/Documentation/devicetree/bindings/soundwire/slave.txt
new file mode 100644
index 000000000000..b8e8d34bbc92
--- /dev/null
+++ 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.
+
+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.
+
+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>;
+		...
+	};
+};