[PATCH v2 0/2] soundwire: sysfs: expose device number and status

[PATCH v2 0/2] soundwire: sysfs: expose device number and status

[Pierre-Louis Bossart]

This patchset combines three contributions:

Srinivas Kandagalta suggested creating a device even it's detected on
the bus but not described in platform firmware. I suggested adding the
device number and status to show the difference with 'ghost' devices,
described in firmware but not physically present. Vinod suggested a
simpler way to report the status.

I did not keep Vinod's patch separate since it was using the same
group attribute as the other properties, which prevents status and
device number from being reported if there is no firmware and no
driver.

These patches were tested on Qualcomm and Intel platforms.

v2: as suggested by GregKH, add attribute group by default by setting
the groups pointer at the device level.

Pierre-Louis Bossart (1):
  soundwire: sysfs: add slave status and device number before probe

Srinivas Kandagatla (1):
  soundwire: bus: add enumerated Slave device to device list

 .../ABI/testing/sysfs-bus-soundwire-slave     | 18 ++++++
 drivers/soundwire/bus.c                       |  9 +++
 drivers/soundwire/bus.h                       |  2 +
 drivers/soundwire/bus_type.c                  |  9 +++
 drivers/soundwire/slave.c                     |  6 +-
 drivers/soundwire/sysfs_local.h               |  4 ++
 drivers/soundwire/sysfs_slave.c               | 59 ++++++++++++++++++-
 7 files changed, 104 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] soundwire: bus: add enumerated Slave device to device list

[Pierre-Louis Bossart]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Currently Slave devices are only added on startup, either from Device
Tree or ACPI entries. However Slave devices that are physically
present on the bus, but not described in platform firmware, will never
be added to the device list. The user/integrator can only know the
list of devices by looking a dynamic debug logs.

This patch suggests adding a Slave device eveb there is no matching DT or
ACPI entry, so that we can see this in sysfs entry.

Initial code from Srinivas. Comments, fixes for ACPI probe and edits
of commit message by Pierre.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c      | 9 +++++++++
 drivers/soundwire/bus.h      | 2 ++
 drivers/soundwire/bus_type.c | 9 +++++++++
 drivers/soundwire/slave.c    | 4 ++--
 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 02574b4bb179..81807b332a12 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -741,6 +741,15 @@ static int sdw_program_device_num(struct sdw_bus *bus)
 
 		if (!found) {
 			/* TODO: Park this device in Group 13 */
+
+			/*
+			 * add Slave device even if there is no platform
+			 * firmware description. There will be no driver probe
+			 * but the user/integration will be able to see the
+			 * device, enumeration status and device number in sysfs
+			 */
+			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 c53345fbc4c7..fd251c1eb925 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..575b9bad99d5 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -84,6 +84,15 @@ 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)
+		return -ENODEV;
+
+	if (!IS_ENABLED(CONFIG_ACPI) && !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 4a250d33de5d..19b012310c29 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.25.1

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

[Pierre-Louis Bossart]

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

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

[Greg KH]

On Thu, Sep 17, 2020 at 11:00:07AM -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.
> 
> 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(-)

Much nicer, thanks for the change:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v2 1/2] soundwire: bus: add enumerated Slave device to device list

[Vinod Koul]

On 17-09-20, 11:00, Pierre-Louis Bossart wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> Currently Slave devices are only added on startup, either from Device
> Tree or ACPI entries. However Slave devices that are physically
> present on the bus, but not described in platform firmware, will never
> be added to the device list. The user/integrator can only know the
> list of devices by looking a dynamic debug logs.
> 
> This patch suggests adding a Slave device eveb there is no matching DT or
> ACPI entry, so that we can see this in sysfs entry.
> 
> Initial code from Srinivas. Comments, fixes for ACPI probe and edits
> of commit message by Pierre.

You should add yours as Co-developed. That is the standard tag for these
things

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/bus.c      | 9 +++++++++
>  drivers/soundwire/bus.h      | 2 ++
>  drivers/soundwire/bus_type.c | 9 +++++++++
>  drivers/soundwire/slave.c    | 4 ++--
>  4 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 02574b4bb179..81807b332a12 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -741,6 +741,15 @@ static int sdw_program_device_num(struct sdw_bus *bus)
>  
>  		if (!found) {
>  			/* TODO: Park this device in Group 13 */
> +
> +			/*
> +			 * add Slave device even if there is no platform
> +			 * firmware description. There will be no driver probe
> +			 * but the user/integration will be able to see the
> +			 * device, enumeration status and device number in sysfs
> +			 */
> +			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 c53345fbc4c7..fd251c1eb925 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..575b9bad99d5 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -84,6 +84,15 @@ 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)
> +		return -ENODEV;
> +
> +	if (!IS_ENABLED(CONFIG_ACPI) && !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 4a250d33de5d..19b012310c29 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.25.1

-- 
~Vinod

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

[Vinod Koul]

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

-- 
~Vinod

Re: [PATCH v2 1/2] soundwire: bus: add enumerated Slave device to device list

[Pierre-Louis Bossart]

On 9/18/20 7:05 AM, Vinod Koul wrote:
> On 17-09-20, 11:00, Pierre-Louis Bossart wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> Currently Slave devices are only added on startup, either from Device
>> Tree or ACPI entries. However Slave devices that are physically
>> present on the bus, but not described in platform firmware, will never
>> be added to the device list. The user/integrator can only know the
>> list of devices by looking a dynamic debug logs.
>>
>> This patch suggests adding a Slave device eveb there is no matching DT or
>> ACPI entry, so that we can see this in sysfs entry.
>>
>> Initial code from Srinivas. Comments, fixes for ACPI probe and edits
>> of commit message by Pierre.
> 
> You should add yours as Co-developed. That is the standard tag for these
> things

ok, I've never used this tag before but it seems appropriate indeed, 
thanks for the suggestion.

Should I add your Co-developed tag to the second patch as well then?

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

[Pierre-Louis Bossart]

>>    * 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

We already use directories for device-level and port-level properties, I 
just thought it be cleaner to continue this model. We might also expand 
the information later on, e.g. provide interrupt status.

I don't mind if we remove the directory and move everything up one 
level, but it wouldn't be consistent with the previous work.

>> +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.

Actually no. If you look at the standard, 'Unattached' is an 'internal 
state of a Slave that indicates that it is not synchronized with to the 
Frame boundaries within the Bitstream'. A Slave device can only become 
attached and report it's presence as Device0 in a PING frame once it's 
ATTACHED - which in turn means the device has been able to sync for 15 
frames. A device number of zero means the device is able to respond to 
command but has not yet been enumerated, or was enumerated previously 
but lost sync or went through a reset sequence and reattached. A device 
number of zero does not mean the device is unattached, the logic is as 
follow:

Attached -> Device 0 or 1..11
Unattached -> No concept of device number (or not an observable value).

We should not overload what 'Device0' means and instead follow the 
standard to the letter. We also don't want the attribute to come and go 
dynamically, so N/A (Not Applicable) is IMHO the only way to convey this 
meaning.

Does this help?

Re: [PATCH v2 1/2] soundwire: bus: add enumerated Slave device to device list

[Vinod Koul]

On 18-09-20, 08:54, Pierre-Louis Bossart wrote:
> 
> 
> On 9/18/20 7:05 AM, Vinod Koul wrote:
> > On 17-09-20, 11:00, Pierre-Louis Bossart wrote:
> > > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > > 
> > > Currently Slave devices are only added on startup, either from Device
> > > Tree or ACPI entries. However Slave devices that are physically
> > > present on the bus, but not described in platform firmware, will never
> > > be added to the device list. The user/integrator can only know the
> > > list of devices by looking a dynamic debug logs.
> > > 
> > > This patch suggests adding a Slave device eveb there is no matching DT or
> > > ACPI entry, so that we can see this in sysfs entry.
> > > 
> > > Initial code from Srinivas. Comments, fixes for ACPI probe and edits
> > > of commit message by Pierre.
> > 
> > You should add yours as Co-developed. That is the standard tag for these
> > things
> 
> ok, I've never used this tag before but it seems appropriate indeed, thanks
> for the suggestion.
> 
> Should I add your Co-developed tag to the second patch as well then?

Sure

-- 
~Vinod

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

[Vinod Koul]

On 18-09-20, 09:21, Pierre-Louis Bossart wrote:
> 
> 
> 
> > >    * 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
> 
> We already use directories for device-level and port-level properties, I
> just thought it be cleaner to continue this model. We might also expand the
> information later on, e.g. provide interrupt status.

Right now we have directories for N ports (needs a dir due to nature of
N ports) and 'properties' derived from Disco/firmware.
So Nport and properties makes sense. But for generic device level stuff
like device number, status and future interrupt or anything should be at
device level.

> I don't mind if we remove the directory and move everything up one level,
> but it wouldn't be consistent with the previous work.

Just because we had directory for a reason, adding a directory to
conform to that does make it better. IMO device files should be at
device directory

> 
> > > +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.
> 
> Actually no. If you look at the standard, 'Unattached' is an 'internal state
> of a Slave that indicates that it is not synchronized with to the Frame
> boundaries within the Bitstream'. A Slave device can only become attached
> and report it's presence as Device0 in a PING frame once it's ATTACHED -
> which in turn means the device has been able to sync for 15 frames. A device
> number of zero means the device is able to respond to command but has not
> yet been enumerated, or was enumerated previously but lost sync or went
> through a reset sequence and reattached. A device number of zero does not
> mean the device is unattached, the logic is as follow:
> 
> Attached -> Device 0 or 1..11
> Unattached -> No concept of device number (or not an observable value).
> 
> We should not overload what 'Device0' means and instead follow the standard
> to the letter. We also don't want the attribute to come and go dynamically,
> so N/A (Not Applicable) is IMHO the only way to convey this meaning.
> 
> Does this help?

Ok lets retain this.

-- 
~Vinod

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

[Pierre-Louis Bossart]

On 9/19/20 6:19 AM, Vinod Koul wrote:
> On 18-09-20, 09:21, Pierre-Louis Bossart wrote:
>>
>>
>>
>>>>     * 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
>>
>> We already use directories for device-level and port-level properties, I
>> just thought it be cleaner to continue this model. We might also expand the
>> information later on, e.g. provide interrupt status.
> 
> Right now we have directories for N ports (needs a dir due to nature of
> N ports) and 'properties' derived from Disco/firmware.
> So Nport and properties makes sense. But for generic device level stuff
> like device number, status and future interrupt or anything should be at
> device level.
> 
>> I don't mind if we remove the directory and move everything up one level,
>> but it wouldn't be consistent with the previous work.
> 
> Just because we had directory for a reason, adding a directory to
> conform to that does make it better. IMO device files should be at
> device directory

We have a "dev-properties" directory, which is added after the driver 
probe, and describes MIPI DisCo values at the device level.

Either we remove this dev-properties and move it to the device level - 
to be consistent with your recommendation - or we keep separate 
directories, one which is populated on device registration and the other 
on driver probe.

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

[Vinod Koul]

On 21-09-20, 09:34, Pierre-Louis Bossart wrote:
> 
> 
> On 9/19/20 6:19 AM, Vinod Koul wrote:
> > On 18-09-20, 09:21, Pierre-Louis Bossart wrote:
> > > 
> > > 
> > > 
> > > > >     * 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
> > > 
> > > We already use directories for device-level and port-level properties, I
> > > just thought it be cleaner to continue this model. We might also expand the
> > > information later on, e.g. provide interrupt status.
> > 
> > Right now we have directories for N ports (needs a dir due to nature of
> > N ports) and 'properties' derived from Disco/firmware.
> > So Nport and properties makes sense. But for generic device level stuff
> > like device number, status and future interrupt or anything should be at
> > device level.
> > 
> > > I don't mind if we remove the directory and move everything up one level,
> > > but it wouldn't be consistent with the previous work.
> > 
> > Just because we had directory for a reason, adding a directory to
> > conform to that does make it better. IMO device files should be at
> > device directory
> 
> We have a "dev-properties" directory, which is added after the driver probe,
> and describes MIPI DisCo values at the device level.

Quite right and the reason to add this after driver probe is to let
driver update the values (hard coded or read from firmware etc)

There is a set of properties which tells us the group of properties
which are coming from MIPI DisCo, so IMO they do belong to a directory

> Either we remove this dev-properties and move it to the device level - to be

It is sysfs ABI it can't be moved, not that I agree with that

> consistent with your recommendation - or we keep separate directories, one
> which is populated on device registration and the other on driver probe.

This is device properties so should not really be a directory!

For example on linux devices we have bunch of files and one specific
directory for power. Here too I would like to see bunch of files and
directory for dp-N and mipi ones

-- 
~Vinod