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);