On Wed, Sep 16, 2020 at 03:15:04PM -0500, Pierre-Louis Bossart wrote:
The MIPI DisCo device properties that are read by the driver from platform firmware, or hard-coded in the driver, should only be provided as sysfs entries when a driver probes successfully.
However the device status and device number is updated even when there is no driver present, and hence can be updated when a Slave device is detected on the bus without being described in platform firmware and without any driver registered/probed.
The attr group added for Slave status and device number is not handled by devm_ routines to avoid errors such as "Resources present before probing". Since device_del() explicitly removes attribute groups, only an init function is provided.
Credits to Vinod Koul for the status_show() function, shared in a separate patch and used as is here. The status table was modified to remove an unnecessary enum and status_show() is handled in a different group attribute than what was suggested by Vinod.
Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Tested-by: Srinivas Kandgatla srinivas.kandagatla@linaro.org Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
.../ABI/testing/sysfs-bus-soundwire-slave | 18 ++++++ drivers/soundwire/slave.c | 2 + drivers/soundwire/sysfs_local.h | 4 ++ drivers/soundwire/sysfs_slave.c | 64 ++++++++++++++++++- 4 files changed, 87 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-slave b/Documentation/ABI/testing/sysfs-bus-soundwire-slave index db4c9511d1aa..425adf7b8aec 100644 --- a/Documentation/ABI/testing/sysfs-bus-soundwire-slave +++ b/Documentation/ABI/testing/sysfs-bus-soundwire-slave @@ -1,3 +1,21 @@ +What: /sys/bus/soundwire/devices/sdw:.../dev-status/status
/sys/bus/soundwire/devices/sdw:.../dev-status/device_number+Date: September 2020
+Contact: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Bard Liao <yung-chuan.liao@linux.intel.com>Vinod Koul <vkoul@kernel.org>+Description: SoundWire Slave status
These properties report the Slave status, e.g. if itis UNATTACHED or not, and in the latter case show thedevice_number. This status information is useful todetect devices exposed by platform firmware but notphysically present on the bus, and conversely devicesnot exposed in platform firmware but enumerated.What: /sys/bus/soundwire/devices/sdw:.../dev-properties/mipi_revision /sys/bus/soundwire/devices/sdw:.../dev-properties/wake_capable /sys/bus/soundwire/devices/sdw:.../dev-properties/test_mode_capable diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index 19b012310c29..ee554a215e44 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -6,6 +6,7 @@ #include <linux/soundwire/sdw.h> #include <linux/soundwire/sdw_type.h> #include "bus.h" +#include "sysfs_local.h"
static void sdw_slave_release(struct device *dev) { @@ -83,6 +84,7 @@ int sdw_slave_add(struct sdw_bus *bus, return ret; } sdw_slave_debugfs_init(slave);
sdw_slave_status_sysfs_init(slave);
return ret;
} diff --git a/drivers/soundwire/sysfs_local.h b/drivers/soundwire/sysfs_local.h index ff60adee3c41..52f2eabf5b4b 100644 --- a/drivers/soundwire/sysfs_local.h +++ b/drivers/soundwire/sysfs_local.h @@ -8,6 +8,10 @@
- SDW sysfs APIs -
*/
+/* basic routine to report status of Slave (attachment, dev_num) */ +int sdw_slave_status_sysfs_init(struct sdw_slave *slave);
+/* additional device-managed properties reported after driver probe */ int sdw_slave_sysfs_init(struct sdw_slave *slave); int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave);
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c index f510071b0add..36eaca343601 100644 --- a/drivers/soundwire/sysfs_slave.c +++ b/drivers/soundwire/sysfs_slave.c @@ -16,10 +16,14 @@
/*
- The sysfs for Slave reflects the MIPI description as given
- in the MIPI DisCo spec
- in the MIPI DisCo spec. dev-status properties come directly
- from the MIPI SoundWire specification.
- Base file is device
- |---- modalias
- |---- dev-status
|---- status
|---- device_number- |---- dev-properties
|---- mipi_revision|---- wake_capable@@ -212,3 +216,61 @@ int sdw_slave_sysfs_init(struct sdw_slave *slave)
return 0; }
+/*
- the status is shown in capital letters for UNATTACHED and RESERVED
- on purpose, to alert users to the fact that these status values are
- not expected.
- */
+static const char *const slave_status[] = {
- [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 ssize_t device_number_show(struct device *dev,
struct device_attribute *attr, char *buf)+{
- struct sdw_slave *slave = dev_to_sdw_dev(dev);
- if (slave->status == SDW_SLAVE_UNATTACHED)
return sprintf(buf, "%s", "N/A");- else
return sprintf(buf, "%d", slave->dev_num);+} +static DEVICE_ATTR_RO(device_number);
+static struct attribute *slave_status_attrs[] = {
- &dev_attr_status.attr,
- &dev_attr_device_number.attr,
- NULL,
+};
+/*
- we don't use ATTRIBUTES_GROUP here since we want to add a subdirectory
- for device-status
- */
+static struct attribute_group sdw_slave_status_attr_group = {
- .attrs = slave_status_attrs,
- .name = "dev-status",
+};
+/*
- We can't use devm_ here, otherwise resources would be added before
- the driver probe. The group is removed in device_del() however.
- */
+int sdw_slave_status_sysfs_init(struct sdw_slave *slave) +{
- return device_add_group(&slave->dev, &sdw_slave_status_attr_group);
DOesn't this race with userspace? Why not make this part of the default set of device attributes and have the driver core create them automatically?
thanks,
greg k-h