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

[Vinod Koul <vkoul@kernel.org>]

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