[PATCH] soundwire: bus: add enumerated slave to device list

[PATCH] soundwire: bus: add enumerated slave to device list

[Srinivas Kandagatla]

Currently slave devices are only added either from device tree or acpi
entries. However lets say, there is wrong or no entry of a slave device
in DT that is enumerated, then there is no way for user to know all
the enumerated devices on the bus.

To fix this add slave device by default if there is no matching dt or
acpi entry, so that we can see this in sysfs entry.

In my case I had a wrong address entry in DT, However I had no way to
know what devices are actually enumerated on the bus!

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/bus.c      | 1 +
 drivers/soundwire/bus.h      | 2 ++
 drivers/soundwire/bus_type.c | 6 ++++++
 drivers/soundwire/slave.c    | 4 ++--
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index e6e0fb9a81b4..55d9c22c4ec5 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -699,6 +699,7 @@ static int sdw_program_device_num(struct sdw_bus *bus)
 
 		if (!found) {
 			/* TODO: Park this device in Group 13 */
+			sdw_slave_add(bus, &id, NULL);
 			dev_err(bus->dev, "Slave Entry not found\n");
 		}
 
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 82484f741168..1517d6789dff 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -19,6 +19,8 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
 int sdw_of_find_slaves(struct sdw_bus *bus);
 void sdw_extract_slave_id(struct sdw_bus *bus,
 			  u64 addr, struct sdw_slave_id *id);
+int sdw_slave_add(struct sdw_bus *bus, struct sdw_slave_id *id,
+		  struct fwnode_handle *fwnode);
 int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
 			  struct fwnode_handle *fwnode);
 int sdw_master_device_del(struct sdw_bus *bus);
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 6fba55898cf0..ac036223046f 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -84,6 +84,12 @@ static int sdw_drv_probe(struct device *dev)
 	const struct sdw_device_id *id;
 	int ret;
 
+	/*
+	 * fw description is mandatory to bind
+	 */
+	if (!dev->fwnode || !dev->of_node)
+		return -ENODEV;
+
 	id = sdw_get_device_id(slave, drv);
 	if (!id)
 		return -ENODEV;
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 0839445ee07b..24a16ebf9ae2 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -20,8 +20,8 @@ struct device_type sdw_slave_type = {
 	.uevent =	sdw_slave_uevent,
 };
 
-static int sdw_slave_add(struct sdw_bus *bus,
-			 struct sdw_slave_id *id, struct fwnode_handle *fwnode)
+int sdw_slave_add(struct sdw_bus *bus,
+		  struct sdw_slave_id *id, struct fwnode_handle *fwnode)
 {
 	struct sdw_slave *slave;
 	int ret;
-- 
2.21.0

[PATCH] soundwire: bus: add enumerated slave to device list

[Srinivas Kandagatla]

Currently slave devices are only added either from device tree or acpi
entries. However lets say, there is wrong or no entry of a slave device
in DT that is enumerated, then there is no way for user to know all
the enumerated devices on the bus.

To fix this add slave device by default if there is no matching dt or
acpi entry, so that we can see this in sysfs entry.

In my case I had a wrong address entry in DT, However I had no way to
know what devices are actually enumerated on the bus!

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/bus.c      | 1 +
 drivers/soundwire/bus.h      | 2 ++
 drivers/soundwire/bus_type.c | 6 ++++++
 drivers/soundwire/slave.c    | 4 ++--
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index e6e0fb9a81b4..55d9c22c4ec5 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -699,6 +699,7 @@ static int sdw_program_device_num(struct sdw_bus *bus)
 
 		if (!found) {
 			/* TODO: Park this device in Group 13 */
+			sdw_slave_add(bus, &id, NULL);
 			dev_err(bus->dev, "Slave Entry not found\n");
 		}
 
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 82484f741168..1517d6789dff 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -19,6 +19,8 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
 int sdw_of_find_slaves(struct sdw_bus *bus);
 void sdw_extract_slave_id(struct sdw_bus *bus,
 			  u64 addr, struct sdw_slave_id *id);
+int sdw_slave_add(struct sdw_bus *bus, struct sdw_slave_id *id,
+		  struct fwnode_handle *fwnode);
 int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
 			  struct fwnode_handle *fwnode);
 int sdw_master_device_del(struct sdw_bus *bus);
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 6fba55898cf0..ac036223046f 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -84,6 +84,12 @@ static int sdw_drv_probe(struct device *dev)
 	const struct sdw_device_id *id;
 	int ret;
 
+	/*
+	 * fw description is mandatory to bind
+	 */
+	if (!dev->fwnode || !dev->of_node)
+		return -ENODEV;
+
 	id = sdw_get_device_id(slave, drv);
 	if (!id)
 		return -ENODEV;
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 0839445ee07b..24a16ebf9ae2 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -20,8 +20,8 @@ struct device_type sdw_slave_type = {
 	.uevent =	sdw_slave_uevent,
 };
 
-static int sdw_slave_add(struct sdw_bus *bus,
-			 struct sdw_slave_id *id, struct fwnode_handle *fwnode)
+int sdw_slave_add(struct sdw_bus *bus,
+		  struct sdw_slave_id *id, struct fwnode_handle *fwnode)
 {
 	struct sdw_slave *slave;
 	int ret;
-- 
2.21.0

Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Pierre-Louis Bossart]

On 9/9/20 3:27 AM, Srinivas Kandagatla wrote:
> Currently slave devices are only added either from device tree or acpi
> entries. However lets say, there is wrong or no entry of a slave device
> in DT that is enumerated, then there is no way for user to know all
> the enumerated devices on the bus.

Sorry Srinivas, I don't understand your point.

The sysfs entries will include all devices that are described in 
platform firmware (be it DT or ACPI).

If you add to sysfs entries unknown devices which happen to be present 
on the bus, then what? How would you identify them from the devices that 
are described in firmware?

Also the sysfs entries describe properties, but if you haven't bound a 
driver then how would this work?

I really feel this deserves more explanations on the problem statement 
and what you are hoping to achieve in this case.

Thanks!

> 
> To fix this add slave device by default if there is no matching dt or
> acpi entry, so that we can see this in sysfs entry.
> 
> In my case I had a wrong address entry in DT, However I had no way to
> know what devices are actually enumerated on the bus!
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   drivers/soundwire/bus.c      | 1 +
>   drivers/soundwire/bus.h      | 2 ++
>   drivers/soundwire/bus_type.c | 6 ++++++
>   drivers/soundwire/slave.c    | 4 ++--
>   4 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index e6e0fb9a81b4..55d9c22c4ec5 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -699,6 +699,7 @@ static int sdw_program_device_num(struct sdw_bus *bus)
>   
>   		if (!found) {
>   			/* TODO: Park this device in Group 13 */
> +			sdw_slave_add(bus, &id, NULL);
>   			dev_err(bus->dev, "Slave Entry not found\n");
>   		}
>   
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 82484f741168..1517d6789dff 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -19,6 +19,8 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
>   int sdw_of_find_slaves(struct sdw_bus *bus);
>   void sdw_extract_slave_id(struct sdw_bus *bus,
>   			  u64 addr, struct sdw_slave_id *id);
> +int sdw_slave_add(struct sdw_bus *bus, struct sdw_slave_id *id,
> +		  struct fwnode_handle *fwnode);
>   int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
>   			  struct fwnode_handle *fwnode);
>   int sdw_master_device_del(struct sdw_bus *bus);
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index 6fba55898cf0..ac036223046f 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -84,6 +84,12 @@ static int sdw_drv_probe(struct device *dev)
>   	const struct sdw_device_id *id;
>   	int ret;
>   
> +	/*
> +	 * fw description is mandatory to bind
> +	 */
> +	if (!dev->fwnode || !dev->of_node)
> +		return -ENODEV;
> +
>   	id = sdw_get_device_id(slave, drv);
>   	if (!id)
>   		return -ENODEV;
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index 0839445ee07b..24a16ebf9ae2 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -20,8 +20,8 @@ struct device_type sdw_slave_type = {
>   	.uevent =	sdw_slave_uevent,
>   };
>   
> -static int sdw_slave_add(struct sdw_bus *bus,
> -			 struct sdw_slave_id *id, struct fwnode_handle *fwnode)
> +int sdw_slave_add(struct sdw_bus *bus,
> +		  struct sdw_slave_id *id, struct fwnode_handle *fwnode)
>   {
>   	struct sdw_slave *slave;
>   	int ret;
> 

Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Srinivas Kandagatla]

On 09/09/2020 14:37, Pierre-Louis Bossart wrote:
> 
> 
> On 9/9/20 3:27 AM, Srinivas Kandagatla wrote:
>> Currently slave devices are only added either from device tree or acpi
>> entries. However lets say, there is wrong or no entry of a slave device
>> in DT that is enumerated, then there is no way for user to know all
>> the enumerated devices on the bus.
> 
> Sorry Srinivas, I don't understand your point.
> 
> The sysfs entries will include all devices that are described in 
> platform firmware (be it DT or ACPI).

yes that is true, but it will not include all the enumerated devices on 
the bus!

In my case on a new board I was trying to figure out what devices are on 
the bus even before even adding any device tree entries!

In second case I had a typo in the device tree entry and sysfs displayed 
devices with that typo rather than actual enumerated device id.

> 
> If you add to sysfs entries unknown devices which happen to be present 
> on the bus, then what? How would you identify them from the devices that 
> are described in firmware?

Both of them should be displayed in sysfs, core should be able to 
differentiate this based on the presence of fw_node or of_node and not bind!

> 
> Also the sysfs entries describe properties, but if you haven't bound a 
> driver then how would this work?

This is would be informative, atleast in cases like me!

All I want to know is the list of enumerated devices on the bus, If 
doing this way is not the right thing, then am happy to try any suggestion!

For now I have managed to figure out enumerated device ids on the bus 
with this patch, I was hoping that other people would also hit such 
issue, so I sent this patch!


thanks,
srini
> 
> I really feel this deserves more explanations on the problem statement 
> and what you are hoping to achieve in this case.
> 
> Thanks!
> 
>>
>> To fix this add slave device by default if there is no matching dt or
>> acpi entry, so that we can see this in sysfs entry.
>>
>> In my case I had a wrong address entry in DT, However I had no way to
>> know what devices are actually enumerated on the bus!
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/soundwire/bus.c      | 1 +
>>   drivers/soundwire/bus.h      | 2 ++
>>   drivers/soundwire/bus_type.c | 6 ++++++
>>   drivers/soundwire/slave.c    | 4 ++--
>>   4 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
>> index e6e0fb9a81b4..55d9c22c4ec5 100644
>> --- a/drivers/soundwire/bus.c
>> +++ b/drivers/soundwire/bus.c
>> @@ -699,6 +699,7 @@ static int sdw_program_device_num(struct sdw_bus 
>> *bus)
>>           if (!found) {
>>               /* TODO: Park this device in Group 13 */
>> +            sdw_slave_add(bus, &id, NULL);
>>               dev_err(bus->dev, "Slave Entry not found\n");
>>           }
>> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
>> index 82484f741168..1517d6789dff 100644
>> --- a/drivers/soundwire/bus.h
>> +++ b/drivers/soundwire/bus.h
>> @@ -19,6 +19,8 @@ static inline int sdw_acpi_find_slaves(struct 
>> sdw_bus *bus)
>>   int sdw_of_find_slaves(struct sdw_bus *bus);
>>   void sdw_extract_slave_id(struct sdw_bus *bus,
>>                 u64 addr, struct sdw_slave_id *id);
>> +int sdw_slave_add(struct sdw_bus *bus, struct sdw_slave_id *id,
>> +          struct fwnode_handle *fwnode);
>>   int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
>>                 struct fwnode_handle *fwnode);
>>   int sdw_master_device_del(struct sdw_bus *bus);
>> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
>> index 6fba55898cf0..ac036223046f 100644
>> --- a/drivers/soundwire/bus_type.c
>> +++ b/drivers/soundwire/bus_type.c
>> @@ -84,6 +84,12 @@ static int sdw_drv_probe(struct device *dev)
>>       const struct sdw_device_id *id;
>>       int ret;
>> +    /*
>> +     * fw description is mandatory to bind
>> +     */
>> +    if (!dev->fwnode || !dev->of_node)
>> +        return -ENODEV;
>> +
>>       id = sdw_get_device_id(slave, drv);
>>       if (!id)
>>           return -ENODEV;
>> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
>> index 0839445ee07b..24a16ebf9ae2 100644
>> --- a/drivers/soundwire/slave.c
>> +++ b/drivers/soundwire/slave.c
>> @@ -20,8 +20,8 @@ struct device_type sdw_slave_type = {
>>       .uevent =    sdw_slave_uevent,
>>   };
>> -static int sdw_slave_add(struct sdw_bus *bus,
>> -             struct sdw_slave_id *id, struct fwnode_handle *fwnode)
>> +int sdw_slave_add(struct sdw_bus *bus,
>> +          struct sdw_slave_id *id, struct fwnode_handle *fwnode)
>>   {
>>       struct sdw_slave *slave;
>>       int ret;
>>

Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Pierre-Louis Bossart]

>>> Currently slave devices are only added either from device tree or acpi
>>> entries. However lets say, there is wrong or no entry of a slave device
>>> in DT that is enumerated, then there is no way for user to know all
>>> the enumerated devices on the bus.
>>
>> Sorry Srinivas, I don't understand your point.
>>
>> The sysfs entries will include all devices that are described in 
>> platform firmware (be it DT or ACPI).
> 
> yes that is true, but it will not include all the enumerated devices on 
> the bus!
> 
> In my case on a new board I was trying to figure out what devices are on 
> the bus even before even adding any device tree entries!

We've seen this before but dynamic debug provides all the information 
you need. see e.g. the logs from 
https://sof-ci.01.org/linuxpr/PR2425/build4447/devicetest/

jf-cml-rvp-sdw-1 kernel: [  289.751974] soundwire sdw-master-0: Slave 
attached, programming device number
jf-cml-rvp-sdw-1 kernel: [  289.752121] soundwire sdw-master-0: SDW 
Slave Addr: 10025d070000 <<< HERE
jf-cml-rvp-sdw-1 kernel: [  289.752122] soundwire sdw-master-0: SDW 
Slave class_id 0, part_id 700, mfg_id 25d, unique_id 0, version 1

  > In second case I had a typo in the device tree entry and sysfs 
displayed
> devices with that typo rather than actual enumerated device id.

That's a feature, not a bug? We use what address the platform firmware 
provides. If it's inaccurate then nothing can work.

>> If you add to sysfs entries unknown devices which happen to be present 
>> on the bus, then what? How would you identify them from the devices 
>> that are described in firmware?
> 
> Both of them should be displayed in sysfs, core should be able to 
> differentiate this based on the presence of fw_node or of_node and not 
> bind!

Core yes but user not so much. If the intent is to list the devices 
present on the bus, your patch still requires manual work.

>> Also the sysfs entries describe properties, but if you haven't bound a 
>> driver then how would this work?
> 
> This is would be informative, atleast in cases like me!
> 
> All I want to know is the list of enumerated devices on the bus, If 
> doing this way is not the right thing, then am happy to try any suggestion!
> 
> For now I have managed to figure out enumerated device ids on the bus 
> with this patch, I was hoping that other people would also hit such 
> issue, so I sent this patch!

Now I get your point but
a) you already have a dynamic debug trace to list all devices
b) adding 'undeclared' devices would make things quite murky and is only 
half of the solution. We already struggle because we already have 
'ghost' devices in sysfs that are not physically present, and no way to 
differentiate between the two. If we did add those entries, then we'd 
need two new sysfs attributes such as
'declared' and 'enumerated'.

Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Srinivas Kandagatla]

On 09/09/2020 15:39, Pierre-Louis Bossart wrote:
> 
>>>> Currently slave devices are only added either from device tree or acpi
>>>> entries. However lets say, there is wrong or no entry of a slave device
>>>> in DT that is enumerated, then there is no way for user to know all
>>>> the enumerated devices on the bus.
>>>
>>> Sorry Srinivas, I don't understand your point.
>>>
>>> The sysfs entries will include all devices that are described in 
>>> platform firmware (be it DT or ACPI).
>>
>> yes that is true, but it will not include all the enumerated devices 
>> on the bus!
>>
>> In my case on a new board I was trying to figure out what devices are 
>> on the bus even before even adding any device tree entries!
> 
> We've seen this before but dynamic debug provides all the information 
> you need. see e.g. the logs from 
> https://sof-ci.01.org/linuxpr/PR2425/build4447/devicetest/
> 
> jf-cml-rvp-sdw-1 kernel: [  289.751974] soundwire sdw-master-0: Slave 
> attached, programming device number
> jf-cml-rvp-sdw-1 kernel: [  289.752121] soundwire sdw-master-0: SDW 
> Slave Addr: 10025d070000 <<< HERE

Yes, I have noticed this too! This will be printed for every call to 
sdw_extract_slave_id()!

...
> 
> Now I get your point but
> a) you already have a dynamic debug trace to list all devices
> b) adding 'undeclared' devices would make things quite murky and is only 
> half of the solution. We already struggle because we already have 
> 'ghost' devices in sysfs that are not physically present, and no way to 
> differentiate between the two. If we did add those entries, then we'd 
> need two new sysfs attributes such as
> 'declared' and 'enumerated'.

I totally agree with you on dealing with the undeclared devices, which 
is unnecessary mess!
May be we could make the enumerated devices discovery bit more verbose!

--srini

Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Pierre-Louis Bossart]

On 9/9/20 10:54 AM, Srinivas Kandagatla wrote:
> 
> 
> On 09/09/2020 15:39, Pierre-Louis Bossart wrote:
>>
>>>>> Currently slave devices are only added either from device tree or acpi
>>>>> entries. However lets say, there is wrong or no entry of a slave 
>>>>> device
>>>>> in DT that is enumerated, then there is no way for user to know all
>>>>> the enumerated devices on the bus.
>>>>
>>>> Sorry Srinivas, I don't understand your point.
>>>>
>>>> The sysfs entries will include all devices that are described in 
>>>> platform firmware (be it DT or ACPI).
>>>
>>> yes that is true, but it will not include all the enumerated devices 
>>> on the bus!
>>>
>>> In my case on a new board I was trying to figure out what devices are 
>>> on the bus even before even adding any device tree entries!
>>
>> We've seen this before but dynamic debug provides all the information 
>> you need. see e.g. the logs from 
>> https://sof-ci.01.org/linuxpr/PR2425/build4447/devicetest/
>>
>> jf-cml-rvp-sdw-1 kernel: [  289.751974] soundwire sdw-master-0: Slave 
>> attached, programming device number
>> jf-cml-rvp-sdw-1 kernel: [  289.752121] soundwire sdw-master-0: SDW 
>> Slave Addr: 10025d070000 <<< HERE
> 
> Yes, I have noticed this too! This will be printed for every call to 
> sdw_extract_slave_id()!
> 
> ...
>>
>> Now I get your point but
>> a) you already have a dynamic debug trace to list all devices
>> b) adding 'undeclared' devices would make things quite murky and is 
>> only half of the solution. We already struggle because we already have 
>> 'ghost' devices in sysfs that are not physically present, and no way 
>> to differentiate between the two. If we did add those entries, then 
>> we'd need two new sysfs attributes such as
>> 'declared' and 'enumerated'.
> 
> I totally agree with you on dealing with the undeclared devices, which 
> is unnecessary mess!

It's not necessarily that bad.
- if the intent is to have a single platform firmware that can deal with 
different boards, it's a good thing.
- but if it's just sloppy platform firmware that just does copy-paste 
from platform to platform then indeed it becomes a mess.

> May be we could make the enumerated devices discovery bit more verbose!

Maybe adding a device number sysfs entry would help, e.g. reporting
NotAttched or a value in [0,11] would tell you if the device is actually 
present.

Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Vinod Koul]

On 09-09-20, 12:00, Pierre-Louis Bossart wrote:
> On 9/9/20 10:54 AM, Srinivas Kandagatla wrote:
> > On 09/09/2020 15:39, Pierre-Louis Bossart wrote:
> > > 
> > > > > > Currently slave devices are only added either from device tree or acpi
> > > > > > entries. However lets say, there is wrong or no entry of
> > > > > > a slave device
> > > > > > in DT that is enumerated, then there is no way for user to know all
> > > > > > the enumerated devices on the bus.
> > > > > 
> > > > > Sorry Srinivas, I don't understand your point.
> > > > > 
> > > > > The sysfs entries will include all devices that are
> > > > > described in platform firmware (be it DT or ACPI).
> > > > 
> > > > yes that is true, but it will not include all the enumerated
> > > > devices on the bus!
> > > > 
> > > > In my case on a new board I was trying to figure out what
> > > > devices are on the bus even before even adding any device tree
> > > > entries!
> > > 
> > > We've seen this before but dynamic debug provides all the
> > > information you need. see e.g. the logs from
> > > https://sof-ci.01.org/linuxpr/PR2425/build4447/devicetest/
> > > 
> > > jf-cml-rvp-sdw-1 kernel: [  289.751974] soundwire sdw-master-0:
> > > Slave attached, programming device number
> > > jf-cml-rvp-sdw-1 kernel: [  289.752121] soundwire sdw-master-0: SDW
> > > Slave Addr: 10025d070000 <<< HERE
> > 
> > Yes, I have noticed this too! This will be printed for every call to
> > sdw_extract_slave_id()!
> > 
> > ...
> > > 
> > > Now I get your point but
> > > a) you already have a dynamic debug trace to list all devices
> > > b) adding 'undeclared' devices would make things quite murky and is
> > > only half of the solution. We already struggle because we already
> > > have 'ghost' devices in sysfs that are not physically present, and
> > > no way to differentiate between the two. If we did add those
> > > entries, then we'd need two new sysfs attributes such as
> > > 'declared' and 'enumerated'.
> > 
> > I totally agree with you on dealing with the undeclared devices, which
> > is unnecessary mess!
> 
> It's not necessarily that bad.
> - if the intent is to have a single platform firmware that can deal with
> different boards, it's a good thing.
> - but if it's just sloppy platform firmware that just does copy-paste from
> platform to platform then indeed it becomes a mess.
> 
> > May be we could make the enumerated devices discovery bit more verbose!
> 
> Maybe adding a device number sysfs entry would help, e.g. reporting
> NotAttched or a value in [0,11] would tell you if the device is actually
> present.

Agreed, I cooked this patch to report verbose device status, let me know
if you are okay with this. I think this would be useful regardless of
current discussion.

On Db845c I see:

root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:1/status
Attached
root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:2/status
Attached

diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c
index f510071b0add..3b2765f10024 100644
--- a/drivers/soundwire/sysfs_slave.c
+++ b/drivers/soundwire/sysfs_slave.c
@@ -97,8 +97,27 @@ static ssize_t modalias_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(modalias);
 
+#define SDW_SLAVE_MAX (SDW_SLAVE_RESERVED + 1)
+
+static const char *const slave_status[SDW_SLAVE_MAX] = {
+	[SDW_SLAVE_UNATTACHED] =  "UNATTACHED",
+	[SDW_SLAVE_ATTACHED] = "Attached",
+	[SDW_SLAVE_ALERT] = "Alert",
+	[SDW_SLAVE_RESERVED] = "Reserved",
+};
+
+static ssize_t status_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+
+	return sprintf(buf, "%s\n", slave_status[slave->status]);
+}
+static DEVICE_ATTR_RO(status);
+
 static struct attribute *slave_attrs[] = {
 	&dev_attr_modalias.attr,
+	&dev_attr_status.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(slave);

-- 
~Vinod

Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Vinod Koul]

On 09-09-20, 12:00, Pierre-Louis Bossart wrote:
> On 9/9/20 10:54 AM, Srinivas Kandagatla wrote:
> > On 09/09/2020 15:39, Pierre-Louis Bossart wrote:
> > > 
> > > > > > Currently slave devices are only added either from device tree or acpi
> > > > > > entries. However lets say, there is wrong or no entry of
> > > > > > a slave device
> > > > > > in DT that is enumerated, then there is no way for user to know all
> > > > > > the enumerated devices on the bus.
> > > > > 
> > > > > Sorry Srinivas, I don't understand your point.
> > > > > 
> > > > > The sysfs entries will include all devices that are
> > > > > described in platform firmware (be it DT or ACPI).
> > > > 
> > > > yes that is true, but it will not include all the enumerated
> > > > devices on the bus!
> > > > 
> > > > In my case on a new board I was trying to figure out what
> > > > devices are on the bus even before even adding any device tree
> > > > entries!
> > > 
> > > We've seen this before but dynamic debug provides all the
> > > information you need. see e.g. the logs from
> > > https://sof-ci.01.org/linuxpr/PR2425/build4447/devicetest/
> > > 
> > > jf-cml-rvp-sdw-1 kernel: [  289.751974] soundwire sdw-master-0:
> > > Slave attached, programming device number
> > > jf-cml-rvp-sdw-1 kernel: [  289.752121] soundwire sdw-master-0: SDW
> > > Slave Addr: 10025d070000 <<< HERE
> > 
> > Yes, I have noticed this too! This will be printed for every call to
> > sdw_extract_slave_id()!
> > 
> > ...
> > > 
> > > Now I get your point but
> > > a) you already have a dynamic debug trace to list all devices
> > > b) adding 'undeclared' devices would make things quite murky and is
> > > only half of the solution. We already struggle because we already
> > > have 'ghost' devices in sysfs that are not physically present, and
> > > no way to differentiate between the two. If we did add those
> > > entries, then we'd need two new sysfs attributes such as
> > > 'declared' and 'enumerated'.
> > 
> > I totally agree with you on dealing with the undeclared devices, which
> > is unnecessary mess!
> 
> It's not necessarily that bad.
> - if the intent is to have a single platform firmware that can deal with
> different boards, it's a good thing.
> - but if it's just sloppy platform firmware that just does copy-paste from
> platform to platform then indeed it becomes a mess.
> 
> > May be we could make the enumerated devices discovery bit more verbose!
> 
> Maybe adding a device number sysfs entry would help, e.g. reporting
> NotAttched or a value in [0,11] would tell you if the device is actually
> present.

Agreed, I cooked this patch to report verbose device status, let me know
if you are okay with this. I think this would be useful regardless of
current discussion.

On Db845c I see:

root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:1/status
Attached
root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:2/status
Attached

diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c
index f510071b0add..3b2765f10024 100644
--- a/drivers/soundwire/sysfs_slave.c
+++ b/drivers/soundwire/sysfs_slave.c
@@ -97,8 +97,27 @@ static ssize_t modalias_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(modalias);
 
+#define SDW_SLAVE_MAX (SDW_SLAVE_RESERVED + 1)
+
+static const char *const slave_status[SDW_SLAVE_MAX] = {
+	[SDW_SLAVE_UNATTACHED] =  "UNATTACHED",
+	[SDW_SLAVE_ATTACHED] = "Attached",
+	[SDW_SLAVE_ALERT] = "Alert",
+	[SDW_SLAVE_RESERVED] = "Reserved",
+};
+
+static ssize_t status_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+
+	return sprintf(buf, "%s\n", slave_status[slave->status]);
+}
+static DEVICE_ATTR_RO(status);
+
 static struct attribute *slave_attrs[] = {
 	&dev_attr_modalias.attr,
+	&dev_attr_status.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(slave);

-- 
~Vinod

Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Pierre-Louis Bossart]

>>> May be we could make the enumerated devices discovery bit more verbose!
>>
>> Maybe adding a device number sysfs entry would help, e.g. reporting
>> NotAttched or a value in [0,11] would tell you if the device is actually
>> present.
> 
> Agreed, I cooked this patch to report verbose device status, let me know
> if you are okay with this. I think this would be useful regardless of
> current discussion.
> 
> On Db845c I see:
> 
> root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:1/status
> Attached
> root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:2/status
> Attached

looks like we are all aligned on the idea, I have a similar patch to at 
https://github.com/thesofproject/linux/pull/2426

The difference is that the sysfs status and device_number is added even 
without a driver probe and when there's no firmware description. sysfs 
is currently only added after the driver probe, which wouldn't work for 
the case Srinivas was trying to deal with.

but the way you dealt the status below is better than the switch case I 
used, so will merge this into my code.

Srinivas' patch needs a fix for ACPI platforms, won't probe otherwise 
since we don't have an of_node. If that's alright with everyone I can 
submit a patchset that gathers the 3 contributions.

> 
> diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c
> index f510071b0add..3b2765f10024 100644
> --- a/drivers/soundwire/sysfs_slave.c
> +++ b/drivers/soundwire/sysfs_slave.c
> @@ -97,8 +97,27 @@ static ssize_t modalias_show(struct device *dev,
>   }
>   static DEVICE_ATTR_RO(modalias);
>   
> +#define SDW_SLAVE_MAX (SDW_SLAVE_RESERVED + 1)
> +
> +static const char *const slave_status[SDW_SLAVE_MAX] = {
> +	[SDW_SLAVE_UNATTACHED] =  "UNATTACHED",
> +	[SDW_SLAVE_ATTACHED] = "Attached",
> +	[SDW_SLAVE_ALERT] = "Alert",
> +	[SDW_SLAVE_RESERVED] = "Reserved",
> +};
> +
> +static ssize_t status_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> +	return sprintf(buf, "%s\n", slave_status[slave->status]);
> +}
> +static DEVICE_ATTR_RO(status);
> +
>   static struct attribute *slave_attrs[] = {
>   	&dev_attr_modalias.attr,
> +	&dev_attr_status.attr,
>   	NULL,
>   };
>   ATTRIBUTE_GROUPS(slave);
> 

Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Pierre-Louis Bossart]

>>> May be we could make the enumerated devices discovery bit more verbose!
>>
>> Maybe adding a device number sysfs entry would help, e.g. reporting
>> NotAttched or a value in [0,11] would tell you if the device is actually
>> present.
> 
> Agreed, I cooked this patch to report verbose device status, let me know
> if you are okay with this. I think this would be useful regardless of
> current discussion.
> 
> On Db845c I see:
> 
> root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:1/status
> Attached
> root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:2/status
> Attached

looks like we are all aligned on the idea, I have a similar patch to at 
https://github.com/thesofproject/linux/pull/2426

The difference is that the sysfs status and device_number is added even 
without a driver probe and when there's no firmware description. sysfs 
is currently only added after the driver probe, which wouldn't work for 
the case Srinivas was trying to deal with.

but the way you dealt the status below is better than the switch case I 
used, so will merge this into my code.

Srinivas' patch needs a fix for ACPI platforms, won't probe otherwise 
since we don't have an of_node. If that's alright with everyone I can 
submit a patchset that gathers the 3 contributions.

> 
> diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c
> index f510071b0add..3b2765f10024 100644
> --- a/drivers/soundwire/sysfs_slave.c
> +++ b/drivers/soundwire/sysfs_slave.c
> @@ -97,8 +97,27 @@ static ssize_t modalias_show(struct device *dev,
>   }
>   static DEVICE_ATTR_RO(modalias);
>   
> +#define SDW_SLAVE_MAX (SDW_SLAVE_RESERVED + 1)
> +
> +static const char *const slave_status[SDW_SLAVE_MAX] = {
> +	[SDW_SLAVE_UNATTACHED] =  "UNATTACHED",
> +	[SDW_SLAVE_ATTACHED] = "Attached",
> +	[SDW_SLAVE_ALERT] = "Alert",
> +	[SDW_SLAVE_RESERVED] = "Reserved",
> +};
> +
> +static ssize_t status_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> +	return sprintf(buf, "%s\n", slave_status[slave->status]);
> +}
> +static DEVICE_ATTR_RO(status);
> +
>   static struct attribute *slave_attrs[] = {
>   	&dev_attr_modalias.attr,
> +	&dev_attr_status.attr,
>   	NULL,
>   };
>   ATTRIBUTE_GROUPS(slave);
> 

Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Vinod Koul]

On 10-09-20, 09:02, Pierre-Louis Bossart wrote:
> 
> > > > May be we could make the enumerated devices discovery bit more verbose!
> > > 
> > > Maybe adding a device number sysfs entry would help, e.g. reporting
> > > NotAttched or a value in [0,11] would tell you if the device is actually
> > > present.
> > 
> > Agreed, I cooked this patch to report verbose device status, let me know
> > if you are okay with this. I think this would be useful regardless of
> > current discussion.
> > 
> > On Db845c I see:
> > 
> > root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:1/status
> > Attached
> > root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:2/status
> > Attached
> 
> looks like we are all aligned on the idea, I have a similar patch to at
> https://github.com/thesofproject/linux/pull/2426
> 
> The difference is that the sysfs status and device_number is added even
> without a driver probe and when there's no firmware description. sysfs is
> currently only added after the driver probe, which wouldn't work for the
> case Srinivas was trying to deal with.

Okay sound fine

> but the way you dealt the status below is better than the switch case I
> used, so will merge this into my code.

Why merge? That patch can remain independent and you can add
device_number patch on top and another one for moving sysfs creation
without a driver probe, these three sound like three different patches
to me.

> Srinivas' patch needs a fix for ACPI platforms, won't probe otherwise since
> we don't have an of_node. If that's alright with everyone I can submit a
> patchset that gathers the 3 contributions.

Yes one series should be good, but lets keep one change in a patch
please

-- 
~Vinod

Re: [PATCH] soundwire: bus: add enumerated slave to device list

[Vinod Koul]

On 10-09-20, 09:02, Pierre-Louis Bossart wrote:
> 
> > > > May be we could make the enumerated devices discovery bit more verbose!
> > > 
> > > Maybe adding a device number sysfs entry would help, e.g. reporting
> > > NotAttched or a value in [0,11] would tell you if the device is actually
> > > present.
> > 
> > Agreed, I cooked this patch to report verbose device status, let me know
> > if you are okay with this. I think this would be useful regardless of
> > current discussion.
> > 
> > On Db845c I see:
> > 
> > root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:1/status
> > Attached
> > root@linaro-alip:/sys/bus/soundwire/devices# cat sdw:0:217:2010:0:2/status
> > Attached
> 
> looks like we are all aligned on the idea, I have a similar patch to at
> https://github.com/thesofproject/linux/pull/2426
> 
> The difference is that the sysfs status and device_number is added even
> without a driver probe and when there's no firmware description. sysfs is
> currently only added after the driver probe, which wouldn't work for the
> case Srinivas was trying to deal with.

Okay sound fine

> but the way you dealt the status below is better than the switch case I
> used, so will merge this into my code.

Why merge? That patch can remain independent and you can add
device_number patch on top and another one for moving sysfs creation
without a driver probe, these three sound like three different patches
to me.

> Srinivas' patch needs a fix for ACPI platforms, won't probe otherwise since
> we don't have an of_node. If that's alright with everyone I can submit a
> patchset that gathers the 3 contributions.

Yes one series should be good, but lets keep one change in a patch
please

-- 
~Vinod