mbox series

[v3,0/4] ASoC: codecs: Add WSA881x Smart Speaker amplifier support

Message ID 20190809133407.25918-1-srinivas.kandagatla@linaro.org
Headers show
Series ASoC: codecs: Add WSA881x Smart Speaker amplifier support | expand

Message

Srinivas Kandagatla Aug. 9, 2019, 1:34 p.m. UTC
Thanks for reviewing v2 patchset, here is v3 with addressing the comments in v2.

This patchset adds support to WSA8810/WSA8815 Class-D Smart Speaker
Amplifier which is SoundWire interfaced.
This also adds support to some missing bits in SoundWire bus layer like
Device Tree support.

This patchset along with DB845c machine driver and WCD934x codec driver
has been tested on SDM845 SoC based DragonBoard DB845c with two
WSA8810 speakers.

Most of the code in this driver is rework of Qualcomm downstream drivers
used in Andriod. Credits to Banajit Goswami and Patrick Lai's Team.

TODO:
	Add thermal sensor support in WSA881x.

Thanks,
srini

Changes since v2:
- Updated compatible string to include LinkID.
- udpdated wsa driver to not register/unregister component in SoundWire
 status callbacks.
- Updated few minor coding style review comments.

Changes since v1 RFC:
- bindings document renamed to slave.txt
- fix error code from dt slave parsing

Srinivas Kandagatla (4):
  dt-bindings: soundwire: add slave bindings
  soundwire: core: add device tree support for slave devices
  dt-bindings: ASoC: Add WSA881x bindings
  ASoC: codecs: add wsa881x amplifier support

 .../bindings/sound/qcom,wsa881x.txt           |   24 +
 .../devicetree/bindings/soundwire/slave.txt   |   51 +
 drivers/soundwire/bus.c                       |    2 +
 drivers/soundwire/bus.h                       |    1 +
 drivers/soundwire/slave.c                     |   44 +
 sound/soc/codecs/Kconfig                      |   10 +
 sound/soc/codecs/Makefile                     |    2 +
 sound/soc/codecs/wsa881x.c                    | 1134 +++++++++++++++++
 8 files changed, 1268 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,wsa881x.txt
 create mode 100644 Documentation/devicetree/bindings/soundwire/slave.txt
 create mode 100644 sound/soc/codecs/wsa881x.c

-- 
2.21.0

Comments

Vinod Koul Aug. 21, 2019, 9:09 a.m. UTC | #1
On 09-08-19, 14:34, 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.


Rob does this look good to you, I intent to apply the soundwire parts

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> ---

>  .../devicetree/bindings/soundwire/slave.txt   | 51 +++++++++++++++++++

>  1 file changed, 51 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..201f65d2fafa

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/soundwire/slave.txt

> @@ -0,0 +1,51 @@

> +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 properties:

> +- compatible:	 "sdw<LinkID><VersionID><InstanceID><MFD><PID><CID>".

> +		  Is the textual representation of SoundWire Enumeration

> +		  address along with Link ID. compatible string should contain

> +		  SoundWire Link ID, SoundWire Version ID, Instance ID,

> +		  Manufacturer ID, Part ID and Class ID in order

> +		  represented as above and shall be in lower-case hexadecimal

> +		  with leading zeroes. Vaild sizes of these fields are

> +		  LinkID is 1 nibble,

> +		  Version ID is 1 nibble

> +		  Instance ID in 1 nibble

> +		  MFD in 4 nibbles

> +		  PID in 4 nibbles

> +		  CID is 2 nibbles

> +

> +		  Version number '0x1' represents SoundWire 1.0

> +		  Version number '0x2' represents SoundWire 1.1

> +		  ex: "sdw0110217201000" represents 0 LinkID,

> +		  SoundWire 1.0 version slave with Instance ID 1.

> +		  More Information on detail of encoding of these fields can be

> +		  found in MIPI Alliance DisCo & SoundWire 1.0 Specifications.

> +

> +SoundWire example for Qualcomm's SoundWire controller:

> +

> +soundwire@c2d0000 {

> +	compatible = "qcom,soundwire-v1.5.0"

> +	reg = <0x0c2d0000 0x2000>;

> +

> +	spkr_left:wsa8810-left{

> +		compatible = "sdw0110217201000";

> +		...

> +	};

> +

> +	spkr_right:wsa8810-right{

> +		compatible = "sdw0120217201000";

> +		...

> +	};

> +};

> -- 

> 2.21.0


-- 
~Vinod
Rob Herring (Arm) Aug. 21, 2019, 9:44 p.m. UTC | #2
On Fri, Aug 09, 2019 at 02:34:04PM +0100, 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@linaro.org>

> ---

>  .../devicetree/bindings/soundwire/slave.txt   | 51 +++++++++++++++++++

>  1 file changed, 51 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/soundwire/slave.txt


Can you convert this to DT schema given it is a common binding.

What does the host controller look like? You need to define the node 
hierarchy. Bus controller schemas should then include the bus schema. 
See spi-controller.yaml.

> 

> diff --git a/Documentation/devicetree/bindings/soundwire/slave.txt b/Documentation/devicetree/bindings/soundwire/slave.txt

> new file mode 100644

> index 000000000000..201f65d2fafa

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/soundwire/slave.txt

> @@ -0,0 +1,51 @@

> +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 properties:

> +- compatible:	 "sdw<LinkID><VersionID><InstanceID><MFD><PID><CID>".

> +		  Is the textual representation of SoundWire Enumeration

> +		  address along with Link ID. compatible string should contain

> +		  SoundWire Link ID, SoundWire Version ID, Instance ID,

> +		  Manufacturer ID, Part ID and Class ID in order

> +		  represented as above and shall be in lower-case hexadecimal

> +		  with leading zeroes. Vaild sizes of these fields are

> +		  LinkID is 1 nibble,

> +		  Version ID is 1 nibble

> +		  Instance ID in 1 nibble

> +		  MFD in 4 nibbles

> +		  PID in 4 nibbles

> +		  CID is 2 nibbles

> +

> +		  Version number '0x1' represents SoundWire 1.0

> +		  Version number '0x2' represents SoundWire 1.1


This can all be a regex.

> +		  ex: "sdw0110217201000" represents 0 LinkID,

> +		  SoundWire 1.0 version slave with Instance ID 1.

> +		  More Information on detail of encoding of these fields can be

> +		  found in MIPI Alliance DisCo & SoundWire 1.0 Specifications.

> +

> +SoundWire example for Qualcomm's SoundWire controller:

> +

> +soundwire@c2d0000 {

> +	compatible = "qcom,soundwire-v1.5.0"

> +	reg = <0x0c2d0000 0x2000>;

> +

> +	spkr_left:wsa8810-left{

> +		compatible = "sdw0110217201000";

> +		...

> +	};

> +

> +	spkr_right:wsa8810-right{

> +		compatible = "sdw0120217201000";


The normal way to distinguish instances is with 'reg'. So I think you 
need 'reg' with Instance ID moved there at least. Just guessing, but 
perhaps Link ID, too? And for 2 different classes of device is that 
enough? 

Rob
Srinivas Kandagatla Aug. 22, 2019, 10:12 a.m. UTC | #3
On 21/08/2019 22:44, Rob Herring wrote:
> On Fri, Aug 09, 2019 at 02:34:04PM +0100, 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@linaro.org>

>> ---

>>   .../devicetree/bindings/soundwire/slave.txt   | 51 +++++++++++++++++++

>>   1 file changed, 51 insertions(+)

>>   create mode 100644 Documentation/devicetree/bindings/soundwire/slave.txt

> 

> Can you convert this to DT schema given it is a common binding.

> 


I will give that a go in next version!

> What does the host controller look like? You need to define the node

> hierarchy. Bus controller schemas should then include the bus schema.

> See spi-controller.yaml.


Host controller is always parent of these devices which is represented 
in the example.

In my previous patches, i did put this slave bindings in bus.txt, but 
Vinod suggested to move it to slave.txt.

Are you suggesting to add two yamls here, one for slave and one for bus
Or just document this in one bus bindings?


> 

>>

>> diff --git a/Documentation/devicetree/bindings/soundwire/slave.txt b/Documentation/devicetree/bindings/soundwire/slave.txt

>> new file mode 100644

>> index 000000000000..201f65d2fafa

>> --- /dev/null

>> +++ b/Documentation/devicetree/bindings/soundwire/slave.txt

>> @@ -0,0 +1,51 @@

>> +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 properties:

>> +- compatible:	 "sdw<LinkID><VersionID><InstanceID><MFD><PID><CID>".

>> +		  Is the textual representation of SoundWire Enumeration

>> +		  address along with Link ID. compatible string should contain

>> +		  SoundWire Link ID, SoundWire Version ID, Instance ID,

>> +		  Manufacturer ID, Part ID and Class ID in order

>> +		  represented as above and shall be in lower-case hexadecimal

>> +		  with leading zeroes. Vaild sizes of these fields are

>> +		  LinkID is 1 nibble,

>> +		  Version ID is 1 nibble

>> +		  Instance ID in 1 nibble

>> +		  MFD in 4 nibbles

>> +		  PID in 4 nibbles

>> +		  CID is 2 nibbles

>> +

>> +		  Version number '0x1' represents SoundWire 1.0

>> +		  Version number '0x2' represents SoundWire 1.1

> 

> This can all be a regex.

> 

>> +		  ex: "sdw0110217201000" represents 0 LinkID,

>> +		  SoundWire 1.0 version slave with Instance ID 1.

>> +		  More Information on detail of encoding of these fields can be

>> +		  found in MIPI Alliance DisCo & SoundWire 1.0 Specifications.

>> +

>> +SoundWire example for Qualcomm's SoundWire controller:

>> +

>> +soundwire@c2d0000 {

>> +	compatible = "qcom,soundwire-v1.5.0"

>> +	reg = <0x0c2d0000 0x2000>;

>> +

>> +	spkr_left:wsa8810-left{

>> +		compatible = "sdw0110217201000";

>> +		...

>> +	};

>> +

>> +	spkr_right:wsa8810-right{

>> +		compatible = "sdw0120217201000";

> 

> The normal way to distinguish instances is with 'reg'. So I think you

> need 'reg' with Instance ID moved there at least. Just guessing, but

> perhaps Link ID, too? And for 2 different classes of device is that

> enough?


In previous bindings ( https://lists.gt.net/linux/kernel/3403276 ) we 
did have instance-id as different property, however Pierre had some good 
suggestion to make it align with _ADR encoding as per MIPI DisCo spec.

Do you still think that we should split the instance id to reg property?

Thanks,
srini

> 

> Rob

>
Vinod Koul Aug. 22, 2019, 11:06 a.m. UTC | #4
On 22-08-19, 11:12, Srinivas Kandagatla wrote:
> 

> 

> On 21/08/2019 22:44, Rob Herring wrote:

> > On Fri, Aug 09, 2019 at 02:34:04PM +0100, 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@linaro.org>

> > > ---

> > >   .../devicetree/bindings/soundwire/slave.txt   | 51 +++++++++++++++++++

> > >   1 file changed, 51 insertions(+)

> > >   create mode 100644 Documentation/devicetree/bindings/soundwire/slave.txt

> > 

> > Can you convert this to DT schema given it is a common binding.

> > 

> 

> I will give that a go in next version!

> 

> > What does the host controller look like? You need to define the node

> > hierarchy. Bus controller schemas should then include the bus schema.

> > See spi-controller.yaml.

> 

> Host controller is always parent of these devices which is represented in

> the example.

> 

> In my previous patches, i did put this slave bindings in bus.txt, but Vinod

> suggested to move it to slave.txt.

> 

> Are you suggesting to add two yamls here, one for slave and one for bus

> Or just document this in one bus bindings?


That would be fine by me :-)

-- 
~Vinod
Rob Herring (Arm) Aug. 22, 2019, 12:36 p.m. UTC | #5
On Thu, Aug 22, 2019 at 5:12 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>

>

>

> On 21/08/2019 22:44, Rob Herring wrote:

> > On Fri, Aug 09, 2019 at 02:34:04PM +0100, 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@linaro.org>

> >> ---

> >>   .../devicetree/bindings/soundwire/slave.txt   | 51 +++++++++++++++++++

> >>   1 file changed, 51 insertions(+)

> >>   create mode 100644 Documentation/devicetree/bindings/soundwire/slave.txt

> >

> > Can you convert this to DT schema given it is a common binding.

> >

>

> I will give that a go in next version!

>

> > What does the host controller look like? You need to define the node

> > hierarchy. Bus controller schemas should then include the bus schema.

> > See spi-controller.yaml.

>

> Host controller is always parent of these devices which is represented

> in the example.

>

> In my previous patches, i did put this slave bindings in bus.txt, but

> Vinod suggested to move it to slave.txt.

>

> Are you suggesting to add two yamls here, one for slave and one for bus

> Or just document this in one bus bindings?


One. Like I said, see spi-controller.yaml.

> >> diff --git a/Documentation/devicetree/bindings/soundwire/slave.txt b/Documentation/devicetree/bindings/soundwire/slave.txt

> >> new file mode 100644

> >> index 000000000000..201f65d2fafa

> >> --- /dev/null

> >> +++ b/Documentation/devicetree/bindings/soundwire/slave.txt

> >> @@ -0,0 +1,51 @@

> >> +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 properties:

> >> +- compatible:        "sdw<LinkID><VersionID><InstanceID><MFD><PID><CID>".

> >> +              Is the textual representation of SoundWire Enumeration

> >> +              address along with Link ID. compatible string should contain

> >> +              SoundWire Link ID, SoundWire Version ID, Instance ID,

> >> +              Manufacturer ID, Part ID and Class ID in order

> >> +              represented as above and shall be in lower-case hexadecimal

> >> +              with leading zeroes. Vaild sizes of these fields are

> >> +              LinkID is 1 nibble,

> >> +              Version ID is 1 nibble

> >> +              Instance ID in 1 nibble

> >> +              MFD in 4 nibbles

> >> +              PID in 4 nibbles

> >> +              CID is 2 nibbles

> >> +

> >> +              Version number '0x1' represents SoundWire 1.0

> >> +              Version number '0x2' represents SoundWire 1.1

> >

> > This can all be a regex.

> >

> >> +              ex: "sdw0110217201000" represents 0 LinkID,

> >> +              SoundWire 1.0 version slave with Instance ID 1.

> >> +              More Information on detail of encoding of these fields can be

> >> +              found in MIPI Alliance DisCo & SoundWire 1.0 Specifications.

> >> +

> >> +SoundWire example for Qualcomm's SoundWire controller:

> >> +

> >> +soundwire@c2d0000 {

> >> +    compatible = "qcom,soundwire-v1.5.0"

> >> +    reg = <0x0c2d0000 0x2000>;

> >> +

> >> +    spkr_left:wsa8810-left{

> >> +            compatible = "sdw0110217201000";

> >> +            ...

> >> +    };

> >> +

> >> +    spkr_right:wsa8810-right{

> >> +            compatible = "sdw0120217201000";

> >

> > The normal way to distinguish instances is with 'reg'. So I think you

> > need 'reg' with Instance ID moved there at least. Just guessing, but

> > perhaps Link ID, too? And for 2 different classes of device is that

> > enough?

>

> In previous bindings ( https://lists.gt.net/linux/kernel/3403276 ) we

> did have instance-id as different property, however Pierre had some good

> suggestion to make it align with _ADR encoding as per MIPI DisCo spec.

>

> Do you still think that we should split the instance id to reg property?


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.

Rob
Vinod Koul Aug. 22, 2019, 12:51 p.m. UTC | #6
On 22-08-19, 07:36, Rob Herring wrote:
> On Thu, Aug 22, 2019 at 5:12 AM Srinivas Kandagatla

> <srinivas.kandagatla@linaro.org> wrote:

> >

> >

> >

> > On 21/08/2019 22:44, Rob Herring wrote:

> > > On Fri, Aug 09, 2019 at 02:34:04PM +0100, 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@linaro.org>

> > >> ---

> > >>   .../devicetree/bindings/soundwire/slave.txt   | 51 +++++++++++++++++++

> > >>   1 file changed, 51 insertions(+)

> > >>   create mode 100644 Documentation/devicetree/bindings/soundwire/slave.txt

> > >

> > > Can you convert this to DT schema given it is a common binding.

> > >

> >

> > I will give that a go in next version!

> >

> > > What does the host controller look like? You need to define the node

> > > hierarchy. Bus controller schemas should then include the bus schema.

> > > See spi-controller.yaml.

> >

> > Host controller is always parent of these devices which is represented

> > in the example.

> >

> > In my previous patches, i did put this slave bindings in bus.txt, but

> > Vinod suggested to move it to slave.txt.

> >

> > Are you suggesting to add two yamls here, one for slave and one for bus

> > Or just document this in one bus bindings?

> 

> One. Like I said, see spi-controller.yaml.

> 

> > >> diff --git a/Documentation/devicetree/bindings/soundwire/slave.txt b/Documentation/devicetree/bindings/soundwire/slave.txt

> > >> new file mode 100644

> > >> index 000000000000..201f65d2fafa

> > >> --- /dev/null

> > >> +++ b/Documentation/devicetree/bindings/soundwire/slave.txt

> > >> @@ -0,0 +1,51 @@

> > >> +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 properties:

> > >> +- compatible:        "sdw<LinkID><VersionID><InstanceID><MFD><PID><CID>".

> > >> +              Is the textual representation of SoundWire Enumeration

> > >> +              address along with Link ID. compatible string should contain

> > >> +              SoundWire Link ID, SoundWire Version ID, Instance ID,

> > >> +              Manufacturer ID, Part ID and Class ID in order

> > >> +              represented as above and shall be in lower-case hexadecimal

> > >> +              with leading zeroes. Vaild sizes of these fields are

> > >> +              LinkID is 1 nibble,

> > >> +              Version ID is 1 nibble

> > >> +              Instance ID in 1 nibble

> > >> +              MFD in 4 nibbles

> > >> +              PID in 4 nibbles

> > >> +              CID is 2 nibbles

> > >> +

> > >> +              Version number '0x1' represents SoundWire 1.0

> > >> +              Version number '0x2' represents SoundWire 1.1

> > >

> > > This can all be a regex.

> > >

> > >> +              ex: "sdw0110217201000" represents 0 LinkID,

> > >> +              SoundWire 1.0 version slave with Instance ID 1.

> > >> +              More Information on detail of encoding of these fields can be

> > >> +              found in MIPI Alliance DisCo & SoundWire 1.0 Specifications.

> > >> +

> > >> +SoundWire example for Qualcomm's SoundWire controller:

> > >> +

> > >> +soundwire@c2d0000 {

> > >> +    compatible = "qcom,soundwire-v1.5.0"

> > >> +    reg = <0x0c2d0000 0x2000>;

> > >> +

> > >> +    spkr_left:wsa8810-left{

> > >> +            compatible = "sdw0110217201000";

> > >> +            ...

> > >> +    };

> > >> +

> > >> +    spkr_right:wsa8810-right{

> > >> +            compatible = "sdw0120217201000";

> > >

> > > The normal way to distinguish instances is with 'reg'. So I think you

> > > need 'reg' with Instance ID moved there at least. Just guessing, but

> > > perhaps Link ID, too? And for 2 different classes of device is that

> > > enough?

> >

> > In previous bindings ( https://lists.gt.net/linux/kernel/3403276 ) we

> > did have instance-id as different property, however Pierre had some good

> > suggestion to make it align with _ADR encoding as per MIPI DisCo spec.

> >

> > Do you still think that we should split the instance id to reg property?

> 

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


That does make sense, we can use unit-address/reg as instance id.

Thanks
-- 
~Vinod
Rob Herring (Arm) Aug. 22, 2019, 4:11 p.m. UTC | #7
On Thu, Aug 22, 2019 at 7:56 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>

>

>

> On 22/08/2019 13:36, Rob Herring wrote:

> >>>> +soundwire@c2d0000 {

> >>>> +    compatible = "qcom,soundwire-v1.5.0"

> >>>> +    reg = <0x0c2d0000 0x2000>;

> >>>> +

> >>>> +    spkr_left:wsa8810-left{

> >>>> +            compatible = "sdw0110217201000";

> >>>> +            ...

> >>>> +    };

> >>>> +

> >>>> +    spkr_right:wsa8810-right{

> >>>> +            compatible = "sdw0120217201000";

> >>> The normal way to distinguish instances is with 'reg'. So I think you

> >>> need 'reg' with Instance ID moved there at least. Just guessing, but

> >>> perhaps Link ID, too? And for 2 different classes of device is that

> >>> enough?

> >> In previous bindings (https://lists.gt.net/linux/kernel/3403276  ) we

> >> did have instance-id as different property, however Pierre had some good

> >> suggestion to make it align with _ADR encoding as per MIPI DisCo spec.

> >>

> >> Do you still think that we should split the instance id to reg property?

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

> That is a good point!

> Okay that makes more sense keep compatible string constant.

> Class ID would be constant for given functionality that the driver will

> provide.

>

> So we will end up with some thing like this:

>

> soundwire@c2d0000 {

>         compatible = "qcom,soundwire-v1.5.0"

>         reg = <0x0c2d0000 0x2000>;

>          #address-cells = <1>;

>          #size-cells = <0>;

>

>         spkr_left:skpr@1{

>                 compatible = "sdw10217201000";

>                 reg = <0x1>

>                 sdw-link-id = <0>;


Not really sure what Link ID is, but maybe it should be part of reg
too if it is part of how you address a device.

>                 ...

>         };

>

>         spkr_right:spkr@2{

>                 compatible = "sdw10217201000";

>                 reg = <0x2>

>                 sdw-link-id = <0>;

>         };

> };

>

> I will spin this in next version!

>

> Thanks,

> srini

>

> >

> > Rob