Re: [PATCH v2 2/2] soundwire: sysfs: add slave status and device number before probe

On 17-09-20, 11:00, 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.

As suggested by GregKH, the attribute group for Slave status and device number is is added by default upon device registration.

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.

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 | 59 ++++++++++++++++++- 4 files changed, 82 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 it
  

• is UNATTACHED or not, and in the latter case show the
  

• device_number. This status information is useful to
  

• detect devices exposed by platform firmware but not
  

• physically present on the bus, and conversely devices
  

• not 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..a08f4081c1c4 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) { @@ -51,6 +52,7 @@ int sdw_slave_add(struct sdw_bus *bus, slave->dev.bus = &sdw_bus_type; slave->dev.of_node = of_node_get(to_of_node(fwnode)); slave->dev.type = &sdw_slave_type;

• slave->dev.groups = sdw_slave_status_attr_groups; slave->bus = bus; slave->status = SDW_SLAVE_UNATTACHED; init_completion(&slave->enumeration_complete);

diff --git a/drivers/soundwire/sysfs_local.h b/drivers/soundwire/sysfs_local.h index ff60adee3c41..7268bc24c538 100644 --- a/drivers/soundwire/sysfs_local.h +++ b/drivers/soundwire/sysfs_local.h @@ -8,6 +8,10 @@

• SDW sysfs APIs -

*/

+/* basic attributes to report status of Slave (attachment, dev_num) */ +extern const struct attribute_group *sdw_slave_status_attr_groups[];

+/* 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..99c0bdd4a726 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
    

Any reason why we want this under dev-status.

Both the status and device_number belong to the device, so we can put them under device and use device properties

• |---- dev-properties

• |---- mipi_revision
  

• |---- wake_capable
  

@@ -212,3 +216,56 @@ 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 highligh users to the fact that these status values
• • are not expected.

Thanks for highlighting this, indeed this was intentional

• */

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

Do we really want N/A here, 0 should imply UNATTACHED and then the status_show would tell UNATTACHED.

• 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 const struct attribute_group sdw_slave_status_attr_group = {

• .attrs = slave_status_attrs,
• .name = "dev-status",

+};

+const struct attribute_group *sdw_slave_status_attr_groups[] = {

• &sdw_slave_status_attr_group,
• NULL

+};

2.25.1