[RFC PATCH 0/7] soundwire: add sysfs and debugfs support

[RFC PATCH 0/7] soundwire: add sysfs and debugfs support

[Pierre-Louis Bossart]

This patchset is not fully-tested and is not a candidate for any
merge. Since I am not very comfortable with sysfs and debugfs support,
and I am not the initial author for this code, I could use feedback
from smarter people than me. 

This series is intented to be applied on top of the 'soundwire:
corrections to ACPI and DisCo properties' series

Parts of this code was initially written by my Intel colleagues Vinod
Koul, Sanyog Kale, Shreyas Nc and Hardik Shah, who are either no
longer with Intel or no longer involved in SoundWire development. When
relevant, I explictly added a note in commit messages to give them
credit for their hard work, but I removed their signed-off-by tags to
avoid email bounces and avoid spamming them forever with SoundWire
patches.

The sysfs parts essentially expose the values of MIPI DisCo
properties. My contribution here was mainly to align with the
specification, a number of properties from the Intel internal code
were missing. I also split the code to make sure the same attribute
names can be used at different levels, as described in the spec.

One of the main questions I have is whether there is really a need to
add new devices, or if the attributes can be added to the *existing*
ones. For example, the sysfs hierarchy for the SoundWire 0 master
shows as:

/sys/bus/acpi/devices/PRP00001:00/int-sdw.0# ls sdw*
'sdw:0:25d:700:0:0':
bank_delay_support  hda_reg           paging_support           source_ports
ch_prep_timeout     high_PHY_capable  power                    src-dp2
clk_stop_mode1      index_reg         reset_behave             src-dp4
clk_stop_timeout    master_count      simple_clk_stop_capable  subsystem
dp0                 mipi_revision     sink-dp1                 test_mode_capable
driver              modalias          sink-dp3                 uevent
firmware_node       p15_behave        sink_ports               wake_capable

'sdw:0:25d:701:0:0':
bank_delay_support  high_PHY_capable  paging_support           source_ports
ch_prep_timeout     master_count      power                    subsystem
clk_stop_mode1      mipi_revision     reset_behave             test_mode_capable
clk_stop_timeout    modalias          simple_clk_stop_capable  uevent
firmware_node       p15_behave        sink_ports               wake_capable

'sdw-master:0':
clk_stop_modes     default_col         dynamic_frame  power      uevent
clock_frequencies  default_frame_rate  err_threshold  revision
clock_gears        default_row         max_clk_freq   subsystem

For the first two Slaves, this results in pretend-devices being added
below each master, the actual Slave devices are children of the
PRP00001 devices, so here we add a bit of complexity. Likewise, the
'sdw-master:0' is a pretend-device whose purpose is only to expose
property values. Is this the recommended direction? Or should all the
sysfs properties be added to the devices exposed by ACPI?

The debugfs part is mainly to dump the Master and Slave registers, as
well as the Intel-specific parts. One of the main changes from the
previous code was to harden the code with scnprintf

Feedback welcome.
~Pierre

Pierre-Louis Bossart (7):
  soundwire: Add sysfs support for master(s)
  soundwire: add Slave sysfs support
  ABI: testing: Add description of soundwire master sysfs files
  ABI: testing: Add description of soundwire slave sysfs files
  soundwire: add debugfs support
  soundwire: cadence_master: add debugfs register dump
  soundwire: intel: add debugfs register dump

 .../ABI/testing/sysfs-bus-soundwire-master    |  21 +
 .../ABI/testing/sysfs-bus-soundwire-slave     |  85 ++++
 drivers/soundwire/Makefile                    |   4 +-
 drivers/soundwire/bus.c                       |  13 +
 drivers/soundwire/bus.h                       |  30 ++
 drivers/soundwire/bus_type.c                  |   8 +
 drivers/soundwire/cadence_master.c            |  98 +++++
 drivers/soundwire/cadence_master.h            |   5 +
 drivers/soundwire/debugfs.c                   | 214 ++++++++++
 drivers/soundwire/intel.c                     | 115 ++++++
 drivers/soundwire/slave.c                     |   2 +
 drivers/soundwire/sysfs.c                     | 376 ++++++++++++++++++
 drivers/soundwire/sysfs_local.h               |  42 ++
 drivers/soundwire/sysfs_slave_dp0.c           | 112 ++++++
 drivers/soundwire/sysfs_slave_dpn.c           | 168 ++++++++
 include/linux/soundwire/sdw.h                 |  24 ++
 16 files changed, 1316 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-soundwire-master
 create mode 100644 Documentation/ABI/testing/sysfs-bus-soundwire-slave
 create mode 100644 drivers/soundwire/debugfs.c
 create mode 100644 drivers/soundwire/sysfs.c
 create mode 100644 drivers/soundwire/sysfs_local.h
 create mode 100644 drivers/soundwire/sysfs_slave_dp0.c
 create mode 100644 drivers/soundwire/sysfs_slave_dpn.c

-- 
2.17.1

[RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Pierre-Louis Bossart]

For each master N, add a device sdw-master:N and add the
master properties as attributes.

Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/Makefile    |   3 +-
 drivers/soundwire/bus.c       |   6 ++
 drivers/soundwire/sysfs.c     | 162 ++++++++++++++++++++++++++++++++++
 include/linux/soundwire/sdw.h |  10 +++
 4 files changed, 180 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soundwire/sysfs.c

diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index 5817beaca0e1..787f1cbf342c 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -3,7 +3,8 @@
 #
 
 #Bus Objs
-soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o
+soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
+			sysfs.o
 obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
 
 #Cadence Objs
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index fe745830a261..38de7071e135 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -49,6 +49,10 @@ int sdw_add_bus_master(struct sdw_bus *bus)
 		}
 	}
 
+	ret = sdw_sysfs_bus_init(bus);
+	if (ret < 0)
+		dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret);
+
 	/*
 	 * Device numbers in SoundWire are 0 through 15. Enumeration device
 	 * number (0), Broadcast device number (15), Group numbers (12 and
@@ -129,6 +133,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
  */
 void sdw_delete_bus_master(struct sdw_bus *bus)
 {
+	sdw_sysfs_bus_exit(bus);
+
 	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
 }
 EXPORT_SYMBOL(sdw_delete_bus_master);
diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
new file mode 100644
index 000000000000..7b6c3826a73a
--- /dev/null
+++ b/drivers/soundwire/sysfs.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2015-19 Intel Corporation.
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_type.h>
+#include "bus.h"
+
+struct sdw_master_sysfs {
+	struct device dev;
+	struct sdw_bus *bus;
+};
+
+#define to_sdw_device(_dev) \
+	container_of(_dev, struct sdw_master_sysfs, dev)
+
+/*
+ * The sysfs for properties reflects the MIPI description as given
+ * in the MIPI DisCo spec
+ *
+ * Base file is:
+ *	sdw-master-N
+ *      |---- revision
+ *      |---- clk_stop_modes
+ *      |---- max_clk_freq
+ *      |---- clk_freq
+ *      |---- clk_gears
+ *      |---- default_row
+ *      |---- default_col
+ *      |---- dynamic_shape
+ *      |---- err_threshold
+ */
+
+#define sdw_master_attr(field, format_string)				\
+static ssize_t field##_show(struct device *dev,				\
+			    struct device_attribute *attr,		\
+			    char *buf)					\
+{									\
+	struct sdw_master_sysfs *master = to_sdw_device(dev);		\
+	return sprintf(buf, format_string, master->bus->prop.field);	\
+}									\
+static DEVICE_ATTR_RO(field)
+
+sdw_master_attr(revision, "0x%x\n");
+sdw_master_attr(clk_stop_modes, "0x%x\n");
+sdw_master_attr(max_clk_freq, "%d\n");
+sdw_master_attr(default_row, "%d\n");
+sdw_master_attr(default_col, "%d\n");
+sdw_master_attr(default_frame_rate, "%d\n");
+sdw_master_attr(dynamic_frame, "%d\n");
+sdw_master_attr(err_threshold, "%d\n");
+
+static ssize_t clock_frequencies_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct sdw_master_sysfs *master = to_sdw_device(dev);
+	ssize_t size = 0;
+	int i;
+
+	for (i = 0; i < master->bus->prop.num_clk_freq; i++)
+		size += sprintf(buf + size, "%8d ",
+				master->bus->prop.clk_freq[i]);
+	size += sprintf(buf + size, "\n");
+
+	return size;
+}
+static DEVICE_ATTR_RO(clock_frequencies);
+
+static ssize_t clock_gears_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct sdw_master_sysfs *master = to_sdw_device(dev);
+	ssize_t size = 0;
+	int i;
+
+	for (i = 0; i < master->bus->prop.num_clk_gears; i++)
+		size += sprintf(buf + size, "%8d ",
+				master->bus->prop.clk_gears[i]);
+	size += sprintf(buf + size, "\n");
+
+	return size;
+}
+static DEVICE_ATTR_RO(clock_gears);
+
+static struct attribute *master_node_attrs[] = {
+	&dev_attr_revision.attr,
+	&dev_attr_clk_stop_modes.attr,
+	&dev_attr_max_clk_freq.attr,
+	&dev_attr_default_row.attr,
+	&dev_attr_default_col.attr,
+	&dev_attr_default_frame_rate.attr,
+	&dev_attr_dynamic_frame.attr,
+	&dev_attr_err_threshold.attr,
+	&dev_attr_clock_frequencies.attr,
+	&dev_attr_clock_gears.attr,
+	NULL,
+};
+
+static const struct attribute_group sdw_master_node_group = {
+	.attrs = master_node_attrs,
+};
+
+static const struct attribute_group *sdw_master_node_groups[] = {
+	&sdw_master_node_group,
+	NULL
+};
+
+static void sdw_device_release(struct device *dev)
+{
+	struct sdw_master_sysfs *master = to_sdw_device(dev);
+
+	kfree(master);
+}
+
+static struct device_type sdw_device_type = {
+	.name =	"sdw_device",
+	.release = sdw_device_release,
+};
+
+int sdw_sysfs_bus_init(struct sdw_bus *bus)
+{
+	struct sdw_master_sysfs *master;
+	int err;
+
+	if (bus->sysfs) {
+		dev_err(bus->dev, "SDW sysfs is already initialized\n");
+		return -EIO;
+	}
+
+	master = kzalloc(sizeof(*master), GFP_KERNEL);
+	if (!master)
+		return -ENOMEM;
+
+	bus->sysfs = master;
+	master->bus = bus;
+	master->dev.type = &sdw_device_type;
+	master->dev.bus = &sdw_bus_type;
+	master->dev.parent = bus->dev;
+	master->dev.groups = sdw_master_node_groups;
+	dev_set_name(&master->dev, "sdw-master:%x", bus->link_id);
+
+	err = device_register(&master->dev);
+	if (err)
+		put_device(&master->dev);
+
+	return err;
+}
+
+void sdw_sysfs_bus_exit(struct sdw_bus *bus)
+{
+	struct sdw_master_sysfs *master = bus->sysfs;
+
+	if (!master)
+		return;
+
+	master->bus = NULL;
+	put_device(&master->dev);
+	bus->sysfs = NULL;
+}
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 3b231472464a..b64d46fed0c8 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -402,6 +402,14 @@ int sdw_slave_read_dp0(struct sdw_slave *slave,
 		       struct fwnode_handle *port,
 		       struct sdw_dp0_prop *dp0);
 
+/*
+ * SDW sysfs APIs
+ */
+struct sdw_master_sysfs;
+
+int sdw_sysfs_bus_init(struct sdw_bus *bus);
+void sdw_sysfs_bus_exit(struct sdw_bus *bus);
+
 /*
  * SDW Slave Structures and APIs
  */
@@ -731,6 +739,7 @@ struct sdw_master_ops {
  * @m_rt_list: List of Master instance of all stream(s) running on Bus. This
  * is used to compute and program bus bandwidth, clock, frame shape,
  * transport and port parameters
+ * @sysfs: Bus sysfs
  * @defer_msg: Defer message
  * @clk_stop_timeout: Clock stop timeout computed
  * @bank_switch_timeout: Bank switch timeout computed
@@ -750,6 +759,7 @@ struct sdw_bus {
 	struct sdw_bus_params params;
 	struct sdw_master_prop prop;
 	struct list_head m_rt_list;
+	struct sdw_master_sysfs *sysfs;
 	struct sdw_defer defer_msg;
 	unsigned int clk_stop_timeout;
 	u32 bank_switch_timeout;
-- 
2.17.1

[RFC PATCH 2/7] soundwire: add Slave sysfs support

[Pierre-Louis Bossart]

Add DisCo Slave properties as device attributes.
Add a device for Data Port 0 which adds Dp0 properties as attributes.
Add devices for Source and Sink Data Ports, and add Dp-N
properties as attributes.

The Slave, DP0 and DPn cases are intentionally handled in separate
files to avoid conflicts with attributes having the same names at
different levels.

Audio modes are not supported for now. Depending on the discussions
the SoundWire Device Class, we may add it later as is or follow the
new specification.

Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/Makefile          |   2 +-
 drivers/soundwire/bus.c             |   2 +
 drivers/soundwire/bus.h             |   2 +
 drivers/soundwire/bus_type.c        |   5 +
 drivers/soundwire/slave.c           |   1 +
 drivers/soundwire/sysfs.c           | 213 ++++++++++++++++++++++++++++
 drivers/soundwire/sysfs_local.h     |  42 ++++++
 drivers/soundwire/sysfs_slave_dp0.c | 112 +++++++++++++++
 drivers/soundwire/sysfs_slave_dpn.c | 168 ++++++++++++++++++++++
 include/linux/soundwire/sdw.h       |   5 +
 10 files changed, 551 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soundwire/sysfs_local.h
 create mode 100644 drivers/soundwire/sysfs_slave_dp0.c
 create mode 100644 drivers/soundwire/sysfs_slave_dpn.c

diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index 787f1cbf342c..a72a29731a28 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -4,7 +4,7 @@
 
 #Bus Objs
 soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
-			sysfs.o
+			sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o
 obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
 
 #Cadence Objs
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 38de7071e135..dd9181693554 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -113,6 +113,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
 	struct sdw_slave *slave = dev_to_sdw_dev(dev);
 	struct sdw_bus *bus = slave->bus;
 
+	sdw_sysfs_slave_exit(slave);
+
 	mutex_lock(&bus->bus_lock);
 
 	if (slave->dev_num) /* clear dev_num if assigned */
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 3048ca153f22..0707e68a8d21 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -18,6 +18,8 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
 void sdw_extract_slave_id(struct sdw_bus *bus,
 			  u64 addr, struct sdw_slave_id *id);
 
+extern const struct attribute_group *sdw_slave_dev_attr_groups[];
+
 enum {
 	SDW_MSG_FLAG_READ = 0,
 	SDW_MSG_FLAG_WRITE,
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 2655602f0cfb..f68fe45c1037 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -97,6 +97,11 @@ static int sdw_drv_probe(struct device *dev)
 	if (slave->ops && slave->ops->read_prop)
 		slave->ops->read_prop(slave);
 
+	/* init the sysfs as we have properties now */
+	ret = sdw_sysfs_slave_init(slave);
+	if (ret < 0)
+		dev_warn(dev, "Slave sysfs init failed:%d\n", ret);
+
 	/*
 	 * Check for valid clk_stop_timeout, use DisCo worst case value of
 	 * 300ms
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index f39a5815e25d..bad73a267fdd 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -34,6 +34,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
 		     id->class_id, id->unique_id);
 
 	slave->dev.release = sdw_slave_release;
+	slave->dev.groups = sdw_slave_dev_attr_groups;
 	slave->dev.bus = &sdw_bus_type;
 	slave->bus = bus;
 	slave->status = SDW_SLAVE_UNATTACHED;
diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
index 7b6c3826a73a..734e2c8bc5cd 100644
--- a/drivers/soundwire/sysfs.c
+++ b/drivers/soundwire/sysfs.c
@@ -8,6 +8,7 @@
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_type.h>
 #include "bus.h"
+#include "sysfs_local.h"
 
 struct sdw_master_sysfs {
 	struct device dev;
@@ -160,3 +161,215 @@ void sdw_sysfs_bus_exit(struct sdw_bus *bus)
 	put_device(&master->dev);
 	bus->sysfs = NULL;
 }
+
+/*
+ * Slave sysfs
+ */
+
+/*
+ * The sysfs for Slave reflects the MIPI description as given
+ * in the MIPI DisCo spec
+ *
+ * Base file is device
+ *	|---- mipi_revision
+ *	|---- wake_capable
+ *	|---- test_mode_capable
+ *	|---- simple_clk_stop_capable
+ *	|---- clk_stop_timeout
+ *	|---- ch_prep_timeout
+ *	|---- reset_behave
+ *	|---- high_PHY_capable
+ *	|---- paging_support
+ *	|---- bank_delay_support
+ *	|---- p15_behave
+ *	|---- master_count
+ *	|---- source_ports
+ *	|---- sink_ports
+ *	|---- dp0
+ *		|---- max_word
+ *		|---- min_word
+ *		|---- words
+ *		|---- flow_controlled
+ *		|---- simple_ch_prep_sm
+ *		|---- device_interrupts
+ *	|---- dpN
+ *		|---- max_word
+ *		|---- min_word
+ *		|---- words
+ *		|---- type
+ *		|---- max_grouping
+ *		|---- simple_ch_prep_sm
+ *		|---- ch_prep_timeout
+ *		|---- device_interrupts
+ *		|---- max_ch
+ *		|---- min_ch
+ *		|---- ch
+ *		|---- ch_combinations
+ *		|---- modes
+ *		|---- max_async_buffer
+ *		|---- block_pack_mode
+ *		|---- port_encoding
+ *		|---- audio_modeM
+ *				|---- bus_min_freq
+ *				|---- bus_max_freq
+ *				|---- bus_freq
+ *				|---- max_freq
+ *				|---- min_freq
+ *				|---- freq
+ *				|---- prep_ch_behave
+ *				|---- glitchless
+ *
+ */
+
+#define sdw_slave_attr(field, format_string)			\
+static ssize_t field##_show(struct device *dev,			\
+			    struct device_attribute *attr,	\
+			    char *buf)				\
+{								\
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);		\
+	return sprintf(buf, format_string, slave->prop.field);	\
+}								\
+static DEVICE_ATTR_RO(field)
+
+sdw_slave_attr(mipi_revision, "0x%x\n");
+sdw_slave_attr(wake_capable, "%d\n");
+sdw_slave_attr(test_mode_capable, "%d\n");
+sdw_slave_attr(clk_stop_mode1, "%d\n");
+sdw_slave_attr(simple_clk_stop_capable, "%d\n");
+sdw_slave_attr(clk_stop_timeout, "%d\n");
+sdw_slave_attr(ch_prep_timeout, "%d\n");
+sdw_slave_attr(reset_behave, "%d\n");
+sdw_slave_attr(high_PHY_capable, "%d\n");
+sdw_slave_attr(paging_support, "%d\n");
+sdw_slave_attr(bank_delay_support, "%d\n");
+sdw_slave_attr(p15_behave, "%d\n");
+sdw_slave_attr(master_count, "%d\n");
+sdw_slave_attr(source_ports, "%d\n");
+sdw_slave_attr(sink_ports, "%d\n");
+
+static ssize_t modalias_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+
+	return sdw_slave_modalias(slave, buf, 256);
+}
+static DEVICE_ATTR_RO(modalias);
+
+static struct attribute *slave_dev_attrs[] = {
+	&dev_attr_mipi_revision.attr,
+	&dev_attr_wake_capable.attr,
+	&dev_attr_test_mode_capable.attr,
+	&dev_attr_clk_stop_mode1.attr,
+	&dev_attr_simple_clk_stop_capable.attr,
+	&dev_attr_clk_stop_timeout.attr,
+	&dev_attr_ch_prep_timeout.attr,
+	&dev_attr_reset_behave.attr,
+	&dev_attr_high_PHY_capable.attr,
+	&dev_attr_paging_support.attr,
+	&dev_attr_bank_delay_support.attr,
+	&dev_attr_p15_behave.attr,
+	&dev_attr_master_count.attr,
+	&dev_attr_source_ports.attr,
+	&dev_attr_sink_ports.attr,
+	&dev_attr_modalias.attr,
+	NULL,
+};
+
+static struct attribute_group sdw_slave_dev_attr_group = {
+	.attrs	= slave_dev_attrs,
+};
+
+const struct attribute_group *sdw_slave_dev_attr_groups[] = {
+	&sdw_slave_dev_attr_group,
+	NULL
+};
+
+int sdw_sysfs_slave_init(struct sdw_slave *slave)
+{
+	struct sdw_slave_sysfs *sysfs;
+	unsigned int src_dpns, sink_dpns, i, j;
+	int err;
+
+	if (slave->sysfs) {
+		dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
+		err = -EIO;
+		goto err_ret;
+	}
+
+	sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
+	if (!sysfs) {
+		err = -ENOMEM;
+		goto err_ret;
+	}
+
+	slave->sysfs = sysfs;
+	sysfs->slave = slave;
+
+	if (slave->prop.dp0_prop) {
+		sysfs->dp0 = sdw_sysfs_slave_dp0_init(slave,
+						      slave->prop.dp0_prop);
+		if (!sysfs->dp0) {
+			err = -ENOMEM;
+			goto err_free_sysfs;
+		}
+	}
+
+	src_dpns = hweight32(slave->prop.source_ports);
+	sink_dpns = hweight32(slave->prop.sink_ports);
+	sysfs->num_dpns = src_dpns + sink_dpns;
+
+	sysfs->dpns = kcalloc(sysfs->num_dpns,
+			      sizeof(**sysfs->dpns), GFP_KERNEL);
+	if (!sysfs->dpns) {
+		err = -ENOMEM;
+		goto err_dp0;
+	}
+
+	for (i = 0; i < src_dpns; i++) {
+		sysfs->dpns[i] =
+			sdw_sysfs_slave_dpn_init(slave,
+						 &slave->prop.src_dpn_prop[i],
+						 true);
+		if (!sysfs->dpns[i]) {
+			err = -ENOMEM;
+			goto err_dpn;
+		}
+	}
+
+	for (j = 0; j < sink_dpns; j++) {
+		sysfs->dpns[i + j] =
+			sdw_sysfs_slave_dpn_init(slave,
+						 &slave->prop.sink_dpn_prop[j],
+						 false);
+		if (!sysfs->dpns[i + j]) {
+			err = -ENOMEM;
+			goto err_dpn;
+		}
+	}
+	return 0;
+
+err_dpn:
+	sdw_sysfs_slave_dpn_exit(sysfs);
+err_dp0:
+	sdw_sysfs_slave_dp0_exit(sysfs);
+err_free_sysfs:
+	kfree(sysfs);
+	sysfs->slave = NULL;
+err_ret:
+	return err;
+}
+
+void sdw_sysfs_slave_exit(struct sdw_slave *slave)
+{
+	struct sdw_slave_sysfs *sysfs = slave->sysfs;
+
+	if (!sysfs)
+		return;
+
+	sdw_sysfs_slave_dpn_exit(sysfs);
+	kfree(sysfs->dpns);
+	sdw_sysfs_slave_dp0_exit(sysfs);
+	kfree(sysfs);
+	slave->sysfs = NULL;
+}
diff --git a/drivers/soundwire/sysfs_local.h b/drivers/soundwire/sysfs_local.h
new file mode 100644
index 000000000000..dd037890117d
--- /dev/null
+++ b/drivers/soundwire/sysfs_local.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
+/* Copyright(c) 2015-19 Intel Corporation. */
+
+#ifndef __SDW_SYSFS_LOCAL_H
+#define __SDW_SYSFS_LOCAL_H
+
+struct sdw_dp0_sysfs {
+	struct device dev;
+	struct sdw_dp0_prop *dp0_prop;
+};
+
+extern struct device_type sdw_dp0_type;
+
+struct sdw_dp0_sysfs
+*sdw_sysfs_slave_dp0_init(struct sdw_slave *slave,
+			  struct sdw_dp0_prop *prop);
+
+void sdw_sysfs_slave_dp0_exit(struct sdw_slave_sysfs *sysfs);
+
+struct sdw_dpn_sysfs {
+	struct device dev;
+	struct sdw_dpn_prop *dpn_prop;
+};
+
+extern struct device_type sdw_dpn_type;
+
+struct sdw_dpn_sysfs
+*sdw_sysfs_slave_dpn_init(struct sdw_slave *slave,
+			  struct sdw_dpn_prop *prop,
+			  bool src);
+
+void sdw_sysfs_slave_dpn_exit(struct sdw_slave_sysfs *sysfs);
+
+struct sdw_slave_sysfs {
+	struct sdw_slave *slave;
+	struct sdw_dp0_sysfs *dp0;
+	unsigned int num_dpns;
+	struct sdw_dpn_sysfs **dpns;
+
+};
+
+#endif /* __SDW_SYSFS_LOCAL_H */
diff --git a/drivers/soundwire/sysfs_slave_dp0.c b/drivers/soundwire/sysfs_slave_dp0.c
new file mode 100644
index 000000000000..4c9994a19199
--- /dev/null
+++ b/drivers/soundwire/sysfs_slave_dp0.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2015-19 Intel Corporation.
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_type.h>
+#include "bus.h"
+#include "sysfs_local.h"
+
+/*
+ * DP0 sysfs
+ */
+
+#define to_sdw_dp0(_dev) \
+	container_of(_dev, struct sdw_dp0_sysfs, dev)
+
+#define sdw_dp0_attr(field, format_string)				\
+static ssize_t field##_show(struct device *dev,				\
+			    struct device_attribute *attr,		\
+			    char *buf)					\
+{									\
+	struct sdw_dp0_sysfs *sysfs = to_sdw_dp0(dev);			\
+	return sprintf(buf, format_string, sysfs->dp0_prop->field);	\
+}									\
+static DEVICE_ATTR_RO(field)
+
+sdw_dp0_attr(min_word, "%d\n");
+sdw_dp0_attr(max_word, "%d\n");
+sdw_dp0_attr(BRA_flow_controlled, "%d\n");
+sdw_dp0_attr(simple_ch_prep_sm, "%d\n");
+sdw_dp0_attr(imp_def_interrupts, "0x%x\n");
+
+static ssize_t word_bits_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct sdw_dp0_sysfs *sysfs = to_sdw_dp0(dev);
+	ssize_t size = 0;
+	int i;
+
+	for (i = 0; i < sysfs->dp0_prop->num_words; i++)
+		size += sprintf(buf + size, "%d ", sysfs->dp0_prop->words[i]);
+	size += sprintf(buf + size, "\n");
+
+	return size;
+}
+static DEVICE_ATTR_RO(word_bits);
+
+static struct attribute *dp0_attrs[] = {
+	&dev_attr_min_word.attr,
+	&dev_attr_max_word.attr,
+	&dev_attr_BRA_flow_controlled.attr,
+	&dev_attr_simple_ch_prep_sm.attr,
+	&dev_attr_imp_def_interrupts.attr,
+	&dev_attr_word_bits.attr,
+	NULL,
+};
+
+static const struct attribute_group dp0_group = {
+	.attrs = dp0_attrs,
+};
+
+static const struct attribute_group *dp0_groups[] = {
+	&dp0_group,
+	NULL
+};
+
+static void sdw_dp0_release(struct device *dev)
+{
+	struct sdw_dp0_sysfs *sysfs = to_sdw_dp0(dev);
+
+	kfree(sysfs);
+}
+
+struct device_type sdw_dp0_type = {
+	.name =	"sdw_dp0",
+	.release = sdw_dp0_release,
+};
+
+struct sdw_dp0_sysfs
+*sdw_sysfs_slave_dp0_init(struct sdw_slave *slave,
+			  struct sdw_dp0_prop *prop)
+{
+	struct sdw_dp0_sysfs *dp0;
+	int err;
+
+	dp0 = kzalloc(sizeof(*dp0), GFP_KERNEL);
+	if (!dp0)
+		return NULL;
+
+	dp0->dev.type = &sdw_dp0_type;
+	dp0->dev.parent = &slave->dev;
+	dp0->dev.groups = dp0_groups;
+	dev_set_name(&dp0->dev, "dp0");
+	dp0->dp0_prop = prop;
+
+	err = device_register(&dp0->dev);
+	if (err) {
+		put_device(&dp0->dev);
+		dp0 = NULL;
+	}
+
+	return dp0;
+}
+
+void sdw_sysfs_slave_dp0_exit(struct sdw_slave_sysfs *sysfs)
+{
+	if (sysfs->dp0)
+		put_device(&sysfs->dp0->dev);
+}
diff --git a/drivers/soundwire/sysfs_slave_dpn.c b/drivers/soundwire/sysfs_slave_dpn.c
new file mode 100644
index 000000000000..a200898e8b5f
--- /dev/null
+++ b/drivers/soundwire/sysfs_slave_dpn.c
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2015-19 Intel Corporation.
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_type.h>
+#include "bus.h"
+#include "sysfs_local.h"
+
+/*
+ * DP-N properties
+ */
+
+#define to_sdw_dpn(_dev) \
+	container_of(_dev, struct sdw_dpn_sysfs, dev)
+
+#define sdw_dpn_attr(field, format_string)				\
+static ssize_t field##_show(struct device *dev,				\
+			    struct device_attribute *attr,		\
+			    char *buf)					\
+{									\
+	struct sdw_dpn_sysfs *sysfs = to_sdw_dpn(dev);			\
+	return sprintf(buf, format_string, sysfs->dpn_prop->field);	\
+}									\
+static DEVICE_ATTR_RO(field)
+
+sdw_dpn_attr(max_word, "%d\n");
+sdw_dpn_attr(min_word, "%d\n");
+sdw_dpn_attr(max_grouping, "%d\n");
+sdw_dpn_attr(imp_def_interrupts, "%d\n");
+sdw_dpn_attr(max_ch, "%d\n");
+sdw_dpn_attr(min_ch, "%d\n");
+sdw_dpn_attr(modes, "%d\n");
+sdw_dpn_attr(max_async_buffer, "%d\n");
+sdw_dpn_attr(block_pack_mode, "%d\n");
+sdw_dpn_attr(port_encoding, "%d\n");
+sdw_dpn_attr(simple_ch_prep_sm, "%d\n");
+sdw_dpn_attr(ch_prep_timeout, "%d\n");
+
+static ssize_t words_show(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	struct sdw_dpn_sysfs *sysfs = to_sdw_dpn(dev);
+	ssize_t size = 0;
+	int i;
+
+	for (i = 0; i < sysfs->dpn_prop->num_words; i++)
+		size += sprintf(buf + size, "%d ", sysfs->dpn_prop->words[i]);
+	size += sprintf(buf + size, "\n");
+
+	return size;
+}
+static DEVICE_ATTR_RO(words);
+
+static ssize_t channels_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct sdw_dpn_sysfs *sysfs = to_sdw_dpn(dev);
+	ssize_t size = 0;
+	int i;
+
+	for (i = 0; i < sysfs->dpn_prop->num_ch; i++)
+		size += sprintf(buf + size, "%d ", sysfs->dpn_prop->ch[i]);
+	size += sprintf(buf + size, "\n");
+
+	return size;
+}
+static DEVICE_ATTR_RO(channels);
+
+static ssize_t ch_combinations_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct sdw_dpn_sysfs *sysfs = to_sdw_dpn(dev);
+	ssize_t size = 0;
+	int i;
+
+	for (i = 0; i < sysfs->dpn_prop->num_ch_combinations; i++)
+		size += sprintf(buf + size, "%d ",
+				sysfs->dpn_prop->ch_combinations[i]);
+	size += sprintf(buf + size, "\n");
+
+	return size;
+}
+static DEVICE_ATTR_RO(ch_combinations);
+
+static struct attribute *dpn_attrs[] = {
+	&dev_attr_max_word.attr,
+	&dev_attr_min_word.attr,
+	&dev_attr_max_grouping.attr,
+	&dev_attr_imp_def_interrupts.attr,
+	&dev_attr_max_ch.attr,
+	&dev_attr_min_ch.attr,
+	&dev_attr_modes.attr,
+	&dev_attr_max_async_buffer.attr,
+	&dev_attr_block_pack_mode.attr,
+	&dev_attr_port_encoding.attr,
+	&dev_attr_simple_ch_prep_sm.attr,
+	&dev_attr_ch_prep_timeout.attr,
+	&dev_attr_words.attr,
+	&dev_attr_channels.attr,
+	&dev_attr_ch_combinations.attr,
+	NULL,
+};
+
+static const struct attribute_group dpn_group = {
+	.attrs = dpn_attrs,
+};
+
+static const struct attribute_group *dpn_groups[] = {
+	&dpn_group,
+	NULL
+};
+
+static void sdw_dpn_release(struct device *dev)
+{
+	struct sdw_dpn_sysfs *sysfs = to_sdw_dpn(dev);
+
+	kfree(sysfs);
+}
+
+struct device_type sdw_dpn_type = {
+	.name =	"sdw_dpn",
+	.release = sdw_dpn_release,
+};
+
+struct sdw_dpn_sysfs
+*sdw_sysfs_slave_dpn_init(struct sdw_slave *slave,
+			  struct sdw_dpn_prop *prop,
+			  bool src)
+{
+	struct sdw_dpn_sysfs *dpn;
+	int err;
+
+	dpn = kzalloc(sizeof(*dpn), GFP_KERNEL);
+	if (!dpn)
+		return NULL;
+
+	dpn->dev.type = &sdw_dpn_type;
+	dpn->dev.parent = &slave->dev;
+	dpn->dev.groups = dpn_groups;
+	dpn->dpn_prop = prop;
+
+	if (src)
+		dev_set_name(&dpn->dev, "src-dp%x", prop->num);
+	else
+		dev_set_name(&dpn->dev, "sink-dp%x", prop->num);
+
+	err = device_register(&dpn->dev);
+	if (err) {
+		put_device(&dpn->dev);
+		dpn = NULL;
+	}
+
+	return dpn;
+}
+
+void sdw_sysfs_slave_dpn_exit(struct sdw_slave_sysfs *sysfs)
+{
+	int i;
+
+	for (i = 0; i < sysfs->num_dpns; i++) {
+		if (sysfs->dpns[i])
+			put_device(&sysfs->dpns[i]->dev);
+	}
+}
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index b64d46fed0c8..fae3a3ad6e68 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -405,10 +405,13 @@ int sdw_slave_read_dp0(struct sdw_slave *slave,
 /*
  * SDW sysfs APIs
  */
+struct sdw_slave_sysfs;
 struct sdw_master_sysfs;
 
 int sdw_sysfs_bus_init(struct sdw_bus *bus);
 void sdw_sysfs_bus_exit(struct sdw_bus *bus);
+int sdw_sysfs_slave_init(struct sdw_slave *slave);
+void sdw_sysfs_slave_exit(struct sdw_slave *slave);
 
 /*
  * SDW Slave Structures and APIs
@@ -552,6 +555,7 @@ struct sdw_slave_ops {
  * @bus: Bus handle
  * @ops: Slave callback ops
  * @prop: Slave properties
+ * @sysfs: Sysfs interface
  * @node: node for bus list
  * @port_ready: Port ready completion flag for each Slave port
  * @dev_num: Device Number assigned by Bus
@@ -563,6 +567,7 @@ struct sdw_slave {
 	struct sdw_bus *bus;
 	const struct sdw_slave_ops *ops;
 	struct sdw_slave_prop prop;
+	struct sdw_slave_sysfs *sysfs;
 	struct list_head node;
 	struct completion *port_ready;
 	u16 dev_num;
-- 
2.17.1

[RFC PATCH 3/7] ABI: testing: Add description of soundwire master sysfs files

[Pierre-Louis Bossart]

The description is directly derived from the MIPI DisCo specification.

Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 .../ABI/testing/sysfs-bus-soundwire-master    | 21 +++++++++++++++++++
 drivers/soundwire/sysfs.c                     |  1 +
 2 files changed, 22 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-soundwire-master

diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-master b/Documentation/ABI/testing/sysfs-bus-soundwire-master
new file mode 100644
index 000000000000..69cadf31049d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-soundwire-master
@@ -0,0 +1,21 @@
+What:		/sys/bus/soundwire/devices/sdw-master-N/revision
+		/sys/bus/soundwire/devices/sdw-master-N/clk_stop_modes
+		/sys/bus/soundwire/devices/sdw-master-N/clk_freq
+		/sys/bus/soundwire/devices/sdw-master-N/clk_gears
+		/sys/bus/soundwire/devices/sdw-master-N/default_col
+		/sys/bus/soundwire/devices/sdw-master-N/default_frame_rate
+		/sys/bus/soundwire/devices/sdw-master-N/default_row
+		/sys/bus/soundwire/devices/sdw-master-N/dynamic_shape
+		/sys/bus/soundwire/devices/sdw-master-N/err_threshold
+		/sys/bus/soundwire/devices/sdw-master-N/max_clk_freq
+
+Date:		May 2019
+
+Contact:	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
+
+Description:	SoundWire Master-N DisCo properties.
+		These properties are defined by MIPI DisCo Specification
+		for SoundWire. They define various properties of the Master
+		and are used by the bus to configure the Master. clk_stop_modes
+		is a bitmask for simplifications and combines the
+		clock-stop-mode0 and clock-stop-mode1 properties.
diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
index 734e2c8bc5cd..c2e5b7ad42fb 100644
--- a/drivers/soundwire/sysfs.c
+++ b/drivers/soundwire/sysfs.c
@@ -31,6 +31,7 @@ struct sdw_master_sysfs {
  *      |---- clk_gears
  *      |---- default_row
  *      |---- default_col
+ *      |---- default_frame_shape
  *      |---- dynamic_shape
  *      |---- err_threshold
  */
-- 
2.17.1

[RFC PATCH 4/7] ABI: testing: Add description of soundwire slave sysfs files

[Pierre-Louis Bossart]

The description is directly derived from the MIPI DisCo specification.

Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 .../ABI/testing/sysfs-bus-soundwire-slave     | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-soundwire-slave

diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-slave b/Documentation/ABI/testing/sysfs-bus-soundwire-slave
new file mode 100644
index 000000000000..db2a0177f68f
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-soundwire-slave
@@ -0,0 +1,85 @@
+What:		/sys/bus/soundwire/devices/sdw:.../bank_delay_support
+		/sys/bus/soundwire/devices/sdw:.../ch_prep_timeout
+		/sys/bus/soundwire/devices/sdw:.../clk_stop_mode1
+		/sys/bus/soundwire/devices/sdw:.../clk_stop_timeout
+		/sys/bus/soundwire/devices/sdw:.../high_PHY_capable
+		/sys/bus/soundwire/devices/sdw:.../master_count
+		/sys/bus/soundwire/devices/sdw:.../mipi_revision
+		/sys/bus/soundwire/devices/sdw:.../p15_behave
+		/sys/bus/soundwire/devices/sdw:.../paging_support
+		/sys/bus/soundwire/devices/sdw:.../reset_behave
+		/sys/bus/soundwire/devices/sdw:.../simple_clk_stop_capable
+		/sys/bus/soundwire/devices/sdw:.../sink_ports
+		/sys/bus/soundwire/devices/sdw:.../source_ports
+		/sys/bus/soundwire/devices/sdw:.../test_mode_capable
+		/sys/bus/soundwire/devices/sdw:.../wake_capable
+
+Date:		May 2019
+
+Contact:	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
+
+Description:	SoundWire Slave DisCo properties.
+		These properties are defined by MIPI DisCo Specification
+		for SoundWire. They define various properties of the
+		SoundWire Slave and are used by the bus to configure
+		the Slave
+
+
+What:		/sys/bus/soundwire/devices/sdw:.../dp0/BRA_flow_controlled
+		/sys/bus/soundwire/devices/sdw:.../dp0/imp_def_interrupts
+		/sys/bus/soundwire/devices/sdw:.../dp0/max_word
+		/sys/bus/soundwire/devices/sdw:.../dp0/min_word
+		/sys/bus/soundwire/devices/sdw:.../dp0/simple_ch_prep_sm
+		/sys/bus/soundwire/devices/sdw:.../dp0/word_bits
+
+Date:		May 2019
+
+Contact:	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
+
+Description:	SoundWire Slave Data Port-0 DisCo properties.
+		These properties are defined by MIPI DisCo Specification
+		for the SoundWire. They define various properties of the
+		Data port 0 are used by the bus to configure the Data Port 0.
+
+
+What:		/sys/bus/soundwire/devices/sdw:.../src-dpN/block_pack_mode
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/channels
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/ch_combinations
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/ch_prep_timeout
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/imp_def_interrupts
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/max_async_buffer
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/max_ch
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/max_grouping
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/max_word
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/min_ch
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/min_word
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/modes
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/port_encoding
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/simple_ch_prep_sm
+		/sys/bus/soundwire/devices/sdw:.../src-dpN/words
+
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/block_pack_mode
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/channels
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/ch_combinations
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/ch_prep_timeout
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/imp_def_interrupts
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/max_async_buffer
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/max_ch
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/max_grouping
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/max_word
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/min_ch
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/min_word
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/modes
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/port_encoding
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/simple_ch_prep_sm
+		/sys/bus/soundwire/devices/sdw:.../sink-dpN/words
+
+Date:		May 2019
+
+Contact:	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
+
+Description:	SoundWire Slave Data Source/Sink Port-N DisCo properties.
+		These properties are defined by MIPI DisCo Specification
+		for SoundWire. They define various properties of the
+		Source/Sink Data port N and are used by the bus to configure
+		the Data Port N.
-- 
2.17.1

[RFC PATCH 5/7] soundwire: add debugfs support

[Pierre-Louis Bossart]

Add base debugfs mechanism for SoundWire bus by creating soundwire
root and master-N and slave-x hierarchy.

Also add SDW Slave SCP, DP0 and DP-N register debug file.

Registers not implemented will print as "XX"

Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
is the use of scnprintf to avoid known issues with snprintf.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/Makefile    |   3 +-
 drivers/soundwire/bus.c       |   5 +
 drivers/soundwire/bus.h       |  28 +++++
 drivers/soundwire/bus_type.c  |   3 +
 drivers/soundwire/debugfs.c   | 214 ++++++++++++++++++++++++++++++++++
 drivers/soundwire/slave.c     |   1 +
 include/linux/soundwire/sdw.h |   9 ++
 7 files changed, 262 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soundwire/debugfs.c

diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index a72a29731a28..f2c425fa15bd 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -4,7 +4,8 @@
 
 #Bus Objs
 soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
-			sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o
+			sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o  \
+			debugfs.o
 obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
 
 #Cadence Objs
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index dd9181693554..b79eba321b71 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -53,6 +53,8 @@ int sdw_add_bus_master(struct sdw_bus *bus)
 	if (ret < 0)
 		dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret);
 
+	bus->debugfs = sdw_bus_debugfs_init(bus);
+
 	/*
 	 * Device numbers in SoundWire are 0 through 15. Enumeration device
 	 * number (0), Broadcast device number (15), Group numbers (12 and
@@ -114,6 +116,7 @@ static int sdw_delete_slave(struct device *dev, void *data)
 	struct sdw_bus *bus = slave->bus;
 
 	sdw_sysfs_slave_exit(slave);
+	sdw_slave_debugfs_exit(slave->debugfs);
 
 	mutex_lock(&bus->bus_lock);
 
@@ -136,6 +139,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
 void sdw_delete_bus_master(struct sdw_bus *bus)
 {
 	sdw_sysfs_bus_exit(bus);
+	if (bus->debugfs)
+		sdw_bus_debugfs_exit(bus->debugfs);
 
 	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
 }
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 0707e68a8d21..f0b1f4f9d7b2 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -20,6 +20,34 @@ void sdw_extract_slave_id(struct sdw_bus *bus,
 
 extern const struct attribute_group *sdw_slave_dev_attr_groups[];
 
+#ifdef CONFIG_DEBUG_FS
+struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus);
+void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d);
+struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d);
+struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave);
+void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d);
+void sdw_debugfs_init(void);
+void sdw_debugfs_exit(void);
+#else
+struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{ return NULL; }
+
+void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d) {}
+
+struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
+{ return NULL; }
+
+struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
+{ return NULL; }
+
+void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d) {}
+
+void sdw_debugfs_init(void) {}
+
+void sdw_debugfs_exit(void) {}
+
+#endif
+
 enum {
 	SDW_MSG_FLAG_READ = 0,
 	SDW_MSG_FLAG_WRITE,
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index f68fe45c1037..f28c1a2446af 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -6,6 +6,7 @@
 #include <linux/pm_domain.h>
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_type.h>
+#include "bus.h"
 
 /**
  * sdw_get_device_id - find the matching SoundWire device id
@@ -182,11 +183,13 @@ EXPORT_SYMBOL_GPL(sdw_unregister_driver);
 
 static int __init sdw_bus_init(void)
 {
+	sdw_debugfs_init();
 	return bus_register(&sdw_bus_type);
 }
 
 static void __exit sdw_bus_exit(void)
 {
+	sdw_debugfs_exit();
 	bus_unregister(&sdw_bus_type);
 }
 
diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
new file mode 100644
index 000000000000..8a8b4df95c46
--- /dev/null
+++ b/drivers/soundwire/debugfs.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2017-19 Intel Corporation.
+
+#include <linux/device.h>
+#include <linux/debugfs.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_registers.h>
+#include "bus.h"
+
+#ifdef CONFIG_DEBUG_FS
+struct dentry *sdw_debugfs_root;
+#endif
+
+struct sdw_bus_debugfs {
+	struct sdw_bus *bus;
+
+	struct dentry *fs;
+};
+
+struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{
+	struct sdw_bus_debugfs *d;
+	char name[16];
+
+	if (!sdw_debugfs_root)
+		return NULL;
+
+	d = kzalloc(sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return NULL;
+
+	/* create the debugfs master-N */
+	snprintf(name, sizeof(name), "master-%d", bus->link_id);
+	d->fs = debugfs_create_dir(name, sdw_debugfs_root);
+	if (IS_ERR_OR_NULL(d->fs)) {
+		dev_err(bus->dev, "debugfs root creation failed\n");
+		goto err;
+	}
+
+	d->bus = bus;
+
+	return d;
+
+err:
+	kfree(d);
+	return NULL;
+}
+
+void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d)
+{
+	debugfs_remove_recursive(d->fs);
+	kfree(d);
+}
+
+struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
+{
+	if (d)
+		return d->fs;
+	return NULL;
+}
+EXPORT_SYMBOL(sdw_bus_debugfs_get_root);
+
+struct sdw_slave_debugfs {
+	struct sdw_slave *slave;
+
+	struct dentry *fs;
+};
+
+#define RD_BUF (3 * PAGE_SIZE)
+
+static ssize_t sdw_sprintf(struct sdw_slave *slave,
+			   char *buf, size_t pos, unsigned int reg)
+{
+	int value;
+
+	value = sdw_read(slave, reg);
+
+	if (value < 0)
+		return scnprintf(buf + pos, RD_BUF - pos, "%3x\tXX\n", reg);
+	else
+		return scnprintf(buf + pos, RD_BUF - pos,
+				"%3x\t%2x\n", reg, value);
+}
+
+static ssize_t sdw_slave_reg_read(struct file *file, char __user *user_buf,
+				  size_t count, loff_t *ppos)
+{
+	struct sdw_slave *slave = file->private_data;
+	unsigned int reg;
+	char *buf;
+	ssize_t ret;
+	int i, j;
+
+	buf = kzalloc(RD_BUF, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = scnprintf(buf, RD_BUF, "Register  Value\n");
+	ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP0\n");
+
+	for (i = 0; i < 6; i++)
+		ret += sdw_sprintf(slave, buf, ret, i);
+
+	ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+	ret += sdw_sprintf(slave, buf, ret, SDW_DP0_CHANNELEN);
+	for (i = SDW_DP0_SAMPLECTRL1; i <= SDW_DP0_LANECTRL; i++)
+		ret += sdw_sprintf(slave, buf, ret, i);
+
+	ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+	ret += sdw_sprintf(slave, buf, ret,
+			SDW_DP0_CHANNELEN + SDW_BANK1_OFFSET);
+	for (i = SDW_DP0_SAMPLECTRL1 + SDW_BANK1_OFFSET;
+			i <= SDW_DP0_LANECTRL + SDW_BANK1_OFFSET; i++)
+		ret += sdw_sprintf(slave, buf, ret, i);
+
+	ret += scnprintf(buf + ret, RD_BUF - ret, "\nSCP\n");
+	for (i = SDW_SCP_INT1; i <= SDW_SCP_BANKDELAY; i++)
+		ret += sdw_sprintf(slave, buf, ret, i);
+	for (i = SDW_SCP_DEVID_0; i <= SDW_SCP_DEVID_5; i++)
+		ret += sdw_sprintf(slave, buf, ret, i);
+
+	ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+	ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B0);
+	ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B0);
+
+	ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+	ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B1);
+	ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B1);
+
+	for (i = 1; i < 14; i++) {
+		ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP%d\n", i);
+		reg = SDW_DPN_INT(i);
+		for (j = 0; j < 6; j++)
+			ret += sdw_sprintf(slave, buf, ret, reg + j);
+
+		ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+		reg = SDW_DPN_CHANNELEN_B0(i);
+		for (j = 0; j < 9; j++)
+			ret += sdw_sprintf(slave, buf, ret, reg + j);
+
+		ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+		reg = SDW_DPN_CHANNELEN_B1(i);
+		for (j = 0; j < 9; j++)
+			ret += sdw_sprintf(slave, buf, ret, reg + j);
+	}
+
+	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+	kfree(buf);
+
+	return ret;
+}
+
+static const struct file_operations sdw_slave_reg_fops = {
+	.open = simple_open,
+	.read = sdw_slave_reg_read,
+	.llseek = default_llseek,
+};
+
+struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
+{
+	struct sdw_bus_debugfs *master;
+	struct sdw_slave_debugfs *d;
+	char name[32];
+
+	master = slave->bus->debugfs;
+	if (!master)
+		return NULL;
+
+	d = kzalloc(sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return NULL;
+
+	/* create the debugfs slave-name */
+	snprintf(name, sizeof(name), "%s", dev_name(&slave->dev));
+	d->fs = debugfs_create_dir(name, master->fs);
+	if (IS_ERR_OR_NULL(d->fs)) {
+		dev_err(&slave->dev, "slave debugfs root creation failed\n");
+		goto err;
+	}
+
+	d->slave = slave;
+
+	debugfs_create_file("registers", 0400, d->fs,
+			    slave, &sdw_slave_reg_fops);
+
+	return d;
+
+err:
+	kfree(d);
+	return NULL;
+}
+
+void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d)
+{
+	debugfs_remove_recursive(d->fs);
+	kfree(d);
+}
+
+void sdw_debugfs_init(void)
+{
+	sdw_debugfs_root = debugfs_create_dir("soundwire", NULL);
+	if (IS_ERR_OR_NULL(sdw_debugfs_root)) {
+		pr_warn("SoundWire: Failed to create debugfs directory\n");
+		sdw_debugfs_root = NULL;
+		return;
+	}
+}
+
+void sdw_debugfs_exit(void)
+{
+	debugfs_remove_recursive(sdw_debugfs_root);
+}
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index bad73a267fdd..e41332a7f340 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -57,6 +57,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
 		mutex_unlock(&bus->bus_lock);
 		put_device(&slave->dev);
 	}
+	slave->debugfs = sdw_slave_debugfs_init(slave);
 
 	return ret;
 }
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index fae3a3ad6e68..3684ca106408 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -547,6 +547,8 @@ struct sdw_slave_ops {
 			 enum sdw_port_prep_ops pre_ops);
 };
 
+struct sdw_slave_debugfs;
+
 /**
  * struct sdw_slave - SoundWire Slave
  * @id: MIPI device ID
@@ -556,6 +558,7 @@ struct sdw_slave_ops {
  * @ops: Slave callback ops
  * @prop: Slave properties
  * @sysfs: Sysfs interface
+ * @debugfs: Slave debugfs
  * @node: node for bus list
  * @port_ready: Port ready completion flag for each Slave port
  * @dev_num: Device Number assigned by Bus
@@ -568,6 +571,7 @@ struct sdw_slave {
 	const struct sdw_slave_ops *ops;
 	struct sdw_slave_prop prop;
 	struct sdw_slave_sysfs *sysfs;
+	struct sdw_slave_debugfs *debugfs;
 	struct list_head node;
 	struct completion *port_ready;
 	u16 dev_num;
@@ -728,6 +732,8 @@ struct sdw_master_ops {
 
 };
 
+struct sdw_bus_debugfs;
+
 /**
  * struct sdw_bus - SoundWire bus
  * @dev: Master linux device
@@ -742,8 +748,10 @@ struct sdw_master_ops {
  * @params: Current bus parameters
  * @prop: Master properties
  * @m_rt_list: List of Master instance of all stream(s) running on Bus. This
+ * @rt_list: List of Master instance of all stream(s) running on Bus. This
  * is used to compute and program bus bandwidth, clock, frame shape,
  * transport and port parameters
+ * @debugfs: Bus debugfs
  * @sysfs: Bus sysfs
  * @defer_msg: Defer message
  * @clk_stop_timeout: Clock stop timeout computed
@@ -765,6 +773,7 @@ struct sdw_bus {
 	struct sdw_master_prop prop;
 	struct list_head m_rt_list;
 	struct sdw_master_sysfs *sysfs;
+	struct sdw_bus_debugfs *debugfs;
 	struct sdw_defer defer_msg;
 	unsigned int clk_stop_timeout;
 	u32 bank_switch_timeout;
-- 
2.17.1

[RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump

[Pierre-Louis Bossart]

Add debugfs file to dump the Cadence master registers

Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
is the use of scnprintf to avoid known issues with snprintf.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 98 ++++++++++++++++++++++++++++++
 drivers/soundwire/cadence_master.h |  5 ++
 2 files changed, 103 insertions(+)

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 682789bb8ab3..e9c30f18ce25 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -8,6 +8,7 @@
 
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/debugfs.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
@@ -222,6 +223,103 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value)
 	return -EAGAIN;
 }
 
+/*
+ * debugfs
+ */
+
+#define RD_BUF (2 * PAGE_SIZE)
+
+static ssize_t cdns_sprintf(struct sdw_cdns *cdns,
+			    char *buf, size_t pos, unsigned int reg)
+{
+	return scnprintf(buf + pos, RD_BUF - pos,
+			 "%4x\t%4x\n", reg, cdns_readl(cdns, reg));
+}
+
+static ssize_t cdns_reg_read(struct file *file, char __user *user_buf,
+			     size_t count, loff_t *ppos)
+{
+	struct sdw_cdns *cdns = file->private_data;
+	char *buf;
+	ssize_t ret;
+	int i, j;
+
+	buf = kzalloc(RD_BUF, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = scnprintf(buf, RD_BUF, "Register  Value\n");
+	ret += scnprintf(buf + ret, RD_BUF - ret, "\nMCP Registers\n");
+	for (i = 0; i < 8; i++) /* 8 MCP registers */
+		ret += cdns_sprintf(cdns, buf, ret, i * 4);
+
+	ret += scnprintf(buf + ret, RD_BUF - ret,
+			 "\nStatus & Intr Registers\n");
+	for (i = 0; i < 13; i++) /* 13 Status & Intr registers */
+		ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_STAT + i * 4);
+
+	ret += scnprintf(buf + ret, RD_BUF - ret,
+			 "\nSSP & Clk ctrl Registers\n");
+	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL0);
+	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL1);
+	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL0);
+	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL1);
+
+	ret += scnprintf(buf + ret, RD_BUF - ret,
+			 "\nDPn B0 Registers\n");
+	for (i = 0; i < 7; i++) {
+		ret += scnprintf(buf + ret, RD_BUF - ret,
+				 "\nDP-%d\n", i);
+		for (j = 0; j < 6; j++)
+			ret += cdns_sprintf(cdns, buf, ret,
+					CDNS_DPN_B0_CONFIG(i) + j * 4);
+	}
+
+	ret += scnprintf(buf + ret, RD_BUF - ret,
+			 "\nDPn B1 Registers\n");
+	for (i = 0; i < 7; i++) {
+		ret += scnprintf(buf + ret, RD_BUF - ret,
+				 "\nDP-%d\n", i);
+
+		for (j = 0; j < 6; j++)
+			ret += cdns_sprintf(cdns, buf, ret,
+					CDNS_DPN_B1_CONFIG(i) + j * 4);
+	}
+
+	ret += scnprintf(buf + ret, RD_BUF - ret,
+			 "\nDPn Control Registers\n");
+	for (i = 0; i < 7; i++)
+		ret += cdns_sprintf(cdns, buf, ret,
+				CDNS_PORTCTRL + i * CDNS_PORT_OFFSET);
+
+	ret += scnprintf(buf + ret, RD_BUF - ret,
+			 "\nPDIn Config Registers\n");
+	for (i = 0; i < 7; i++)
+		ret += cdns_sprintf(cdns, buf, ret, CDNS_PDI_CONFIG(i));
+
+	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+	kfree(buf);
+
+	return ret;
+}
+
+static const struct file_operations cdns_reg_fops = {
+	.open = simple_open,
+	.read = cdns_reg_read,
+	.llseek = default_llseek,
+};
+
+/**
+ * sdw_cdns_debugfs_init() - Cadence debugfs init
+ * @cdns: Cadence instance
+ * @root: debugfs root
+ */
+void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
+{
+	debugfs_create_file("cdns-registers", 0400, root, cdns, &cdns_reg_fops);
+}
+EXPORT_SYMBOL(sdw_cdns_debugfs_init);
+
 /*
  * IO Calls
  */
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
index fe2af62958b1..d375bbfead18 100644
--- a/drivers/soundwire/cadence_master.h
+++ b/drivers/soundwire/cadence_master.h
@@ -163,6 +163,11 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
 		      struct sdw_cdns_stream_config config);
 int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
 
+void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
+
+int sdw_cdns_suspend(struct sdw_cdns *cdns);
+bool sdw_cdns_check_resume_status(struct sdw_cdns *cdns);
+
 int sdw_cdns_get_stream(struct sdw_cdns *cdns,
 			struct sdw_cdns_streams *stream,
 			u32 ch, u32 dir);
-- 
2.17.1

[RFC PATCH 7/7] soundwire: intel: add debugfs register dump

[Pierre-Louis Bossart]

Add debugfs file to dump the Intel SoundWire registers

Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
is the use of scnprintf to avoid known issues with snprintf.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 115 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 4ac141730b13..7fb2cd6d5bb5 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
@@ -16,6 +17,7 @@
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_intel.h>
 #include "cadence_master.h"
+#include "bus.h"
 #include "intel.h"
 
 /* Intel SHIM Registers Definition */
@@ -98,6 +100,7 @@ struct sdw_intel {
 	struct sdw_cdns cdns;
 	int instance;
 	struct sdw_intel_link_res *res;
+	struct dentry *fs;
 };
 
 #define cdns_to_intel(_cdns) container_of(_cdns, struct sdw_intel, cdns)
@@ -161,6 +164,115 @@ static int intel_set_bit(void __iomem *base, int offset, u32 value, u32 mask)
 	return -EAGAIN;
 }
 
+/*
+ * debugfs
+ */
+
+#define RD_BUF (2 * PAGE_SIZE)
+
+static ssize_t intel_sprintf(void __iomem *mem, bool l,
+			     char *buf, size_t pos, unsigned int reg)
+{
+	int value;
+
+	if (l)
+		value = intel_readl(mem, reg);
+	else
+		value = intel_readw(mem, reg);
+
+	return scnprintf(buf + pos, RD_BUF - pos, "%4x\t%4x\n", reg, value);
+}
+
+static ssize_t intel_reg_read(struct file *file, char __user *user_buf,
+			      size_t count, loff_t *ppos)
+{
+	struct sdw_intel *sdw = file->private_data;
+	void __iomem *s = sdw->res->shim;
+	void __iomem *a = sdw->res->alh;
+	char *buf;
+	ssize_t ret;
+	int i, j;
+	unsigned int links, reg;
+
+	buf = kzalloc(RD_BUF, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	links = intel_readl(s, SDW_SHIM_LCAP) & GENMASK(2, 0);
+
+	ret = scnprintf(buf, RD_BUF, "Register  Value\n");
+	ret += scnprintf(buf + ret, RD_BUF - ret, "\nShim\n");
+
+	for (i = 0; i < 4; i++) {
+		reg = SDW_SHIM_LCAP + i * 4;
+		ret += intel_sprintf(s, true, buf, ret, reg);
+	}
+
+	for (i = 0; i < links; i++) {
+		ret += scnprintf(buf + ret, RD_BUF - ret, "\nLink%d\n", i);
+		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLSCAP(i));
+		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS0CM(i));
+		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS1CM(i));
+		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS2CM(i));
+		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS3CM(i));
+		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_PCMSCAP(i));
+
+		for (j = 0; j < 8; j++) {
+			ret += intel_sprintf(s, false, buf, ret,
+					SDW_SHIM_PCMSYCHM(i, j));
+			ret += intel_sprintf(s, false, buf, ret,
+					SDW_SHIM_PCMSYCHC(i, j));
+		}
+
+		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_PDMSCAP(i));
+		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_IOCTL(i));
+		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTMCTL(i));
+	}
+
+	ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_WAKEEN);
+	ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_WAKESTS);
+
+	ret += scnprintf(buf + ret, RD_BUF - ret, "\nALH\n");
+	for (i = 0; i < 8; i++)
+		ret += intel_sprintf(a, true, buf, ret, SDW_ALH_STRMZCFG(i));
+
+	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+	kfree(buf);
+
+	return ret;
+}
+
+static const struct file_operations intel_reg_fops = {
+	.open = simple_open,
+	.read = intel_reg_read,
+	.llseek = default_llseek,
+};
+
+static void intel_debugfs_init(struct sdw_intel *sdw)
+{
+	struct dentry *root = sdw_bus_debugfs_get_root(sdw->cdns.bus.debugfs);
+
+	if (!root)
+		return;
+
+	sdw->fs = debugfs_create_dir("intel-sdw", root);
+	if (IS_ERR_OR_NULL(sdw->fs)) {
+		dev_err(sdw->cdns.dev, "debugfs root creation failed\n");
+		sdw->fs = NULL;
+		return;
+	}
+
+	debugfs_create_file("intel-registers", 0400, sdw->fs, sdw,
+			    &intel_reg_fops);
+
+	sdw_cdns_debugfs_init(&sdw->cdns, sdw->fs);
+}
+
+static void intel_debugfs_exit(struct sdw_intel *sdw)
+{
+	debugfs_remove_recursive(sdw->fs);
+}
+
 /*
  * shim ops
  */
@@ -890,6 +1002,8 @@ static int intel_probe(struct platform_device *pdev)
 		goto err_dai;
 	}
 
+	intel_debugfs_init(sdw);
+
 	return 0;
 
 err_dai:
@@ -906,6 +1020,7 @@ static int intel_remove(struct platform_device *pdev)
 
 	sdw = platform_get_drvdata(pdev);
 
+	intel_debugfs_exit(sdw);
 	free_irq(sdw->res->irq, sdw);
 	snd_soc_unregister_component(sdw->cdns.dev);
 	sdw_delete_bus_master(&sdw->cdns.bus);
-- 
2.17.1

Re: [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Greg KH]

On Fri, May 03, 2019 at 08:00:24PM -0500, Pierre-Louis Bossart wrote:
> For each master N, add a device sdw-master:N and add the
> master properties as attributes.
> 
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/Makefile    |   3 +-
>  drivers/soundwire/bus.c       |   6 ++
>  drivers/soundwire/sysfs.c     | 162 ++++++++++++++++++++++++++++++++++
>  include/linux/soundwire/sdw.h |  10 +++
>  4 files changed, 180 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/soundwire/sysfs.c
> 
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index 5817beaca0e1..787f1cbf342c 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -3,7 +3,8 @@
>  #
>  
>  #Bus Objs
> -soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o
> +soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
> +			sysfs.o
>  obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
>  
>  #Cadence Objs
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index fe745830a261..38de7071e135 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -49,6 +49,10 @@ int sdw_add_bus_master(struct sdw_bus *bus)
>  		}
>  	}
>  
> +	ret = sdw_sysfs_bus_init(bus);
> +	if (ret < 0)
> +		dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret);
> +
>  	/*
>  	 * Device numbers in SoundWire are 0 through 15. Enumeration device
>  	 * number (0), Broadcast device number (15), Group numbers (12 and
> @@ -129,6 +133,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
>   */
>  void sdw_delete_bus_master(struct sdw_bus *bus)
>  {
> +	sdw_sysfs_bus_exit(bus);
> +
>  	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
>  }
>  EXPORT_SYMBOL(sdw_delete_bus_master);
> diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
> new file mode 100644
> index 000000000000..7b6c3826a73a
> --- /dev/null
> +++ b/drivers/soundwire/sysfs.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2015-19 Intel Corporation.
> +
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/soundwire/sdw.h>
> +#include <linux/soundwire/sdw_type.h>
> +#include "bus.h"
> +
> +struct sdw_master_sysfs {
> +	struct device dev;
> +	struct sdw_bus *bus;
> +};
> +
> +#define to_sdw_device(_dev) \
> +	container_of(_dev, struct sdw_master_sysfs, dev)
> +
> +/*
> + * The sysfs for properties reflects the MIPI description as given
> + * in the MIPI DisCo spec
> + *
> + * Base file is:
> + *	sdw-master-N
> + *      |---- revision
> + *      |---- clk_stop_modes
> + *      |---- max_clk_freq
> + *      |---- clk_freq
> + *      |---- clk_gears
> + *      |---- default_row
> + *      |---- default_col
> + *      |---- dynamic_shape
> + *      |---- err_threshold
> + */
> +
> +#define sdw_master_attr(field, format_string)				\
> +static ssize_t field##_show(struct device *dev,				\
> +			    struct device_attribute *attr,		\
> +			    char *buf)					\
> +{									\
> +	struct sdw_master_sysfs *master = to_sdw_device(dev);		\
> +	return sprintf(buf, format_string, master->bus->prop.field);	\
> +}									\
> +static DEVICE_ATTR_RO(field)
> +
> +sdw_master_attr(revision, "0x%x\n");
> +sdw_master_attr(clk_stop_modes, "0x%x\n");
> +sdw_master_attr(max_clk_freq, "%d\n");
> +sdw_master_attr(default_row, "%d\n");
> +sdw_master_attr(default_col, "%d\n");
> +sdw_master_attr(default_frame_rate, "%d\n");
> +sdw_master_attr(dynamic_frame, "%d\n");
> +sdw_master_attr(err_threshold, "%d\n");
> +
> +static ssize_t clock_frequencies_show(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct sdw_master_sysfs *master = to_sdw_device(dev);
> +	ssize_t size = 0;
> +	int i;
> +
> +	for (i = 0; i < master->bus->prop.num_clk_freq; i++)
> +		size += sprintf(buf + size, "%8d ",
> +				master->bus->prop.clk_freq[i]);
> +	size += sprintf(buf + size, "\n");
> +
> +	return size;
> +}
> +static DEVICE_ATTR_RO(clock_frequencies);
> +
> +static ssize_t clock_gears_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct sdw_master_sysfs *master = to_sdw_device(dev);
> +	ssize_t size = 0;
> +	int i;
> +
> +	for (i = 0; i < master->bus->prop.num_clk_gears; i++)
> +		size += sprintf(buf + size, "%8d ",
> +				master->bus->prop.clk_gears[i]);
> +	size += sprintf(buf + size, "\n");
> +
> +	return size;
> +}
> +static DEVICE_ATTR_RO(clock_gears);
> +
> +static struct attribute *master_node_attrs[] = {
> +	&dev_attr_revision.attr,
> +	&dev_attr_clk_stop_modes.attr,
> +	&dev_attr_max_clk_freq.attr,
> +	&dev_attr_default_row.attr,
> +	&dev_attr_default_col.attr,
> +	&dev_attr_default_frame_rate.attr,
> +	&dev_attr_dynamic_frame.attr,
> +	&dev_attr_err_threshold.attr,
> +	&dev_attr_clock_frequencies.attr,
> +	&dev_attr_clock_gears.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group sdw_master_node_group = {
> +	.attrs = master_node_attrs,
> +};
> +
> +static const struct attribute_group *sdw_master_node_groups[] = {
> +	&sdw_master_node_group,
> +	NULL
> +};

Minor nit, you can use the ATTRIBUTE_GROUPS() macro here to save you a
few lines.

> +
> +static void sdw_device_release(struct device *dev)
> +{
> +	struct sdw_master_sysfs *master = to_sdw_device(dev);
> +
> +	kfree(master);
> +}
> +
> +static struct device_type sdw_device_type = {
> +	.name =	"sdw_device",
> +	.release = sdw_device_release,
> +};
> +
> +int sdw_sysfs_bus_init(struct sdw_bus *bus)
> +{
> +	struct sdw_master_sysfs *master;
> +	int err;
> +
> +	if (bus->sysfs) {
> +		dev_err(bus->dev, "SDW sysfs is already initialized\n");
> +		return -EIO;
> +	}
> +
> +	master = kzalloc(sizeof(*master), GFP_KERNEL);
> +	if (!master)
> +		return -ENOMEM;

Why are you creating a whole new device to put all of this under?  Is
this needed?  What will the sysfs tree look like when you do this?  Why
can't the "bus" device just get all of these attributes and no second
device be created?

thanks,

greg k-h

Re: [RFC PATCH 3/7] ABI: testing: Add description of soundwire master sysfs files

[Greg KH]

On Fri, May 03, 2019 at 08:00:26PM -0500, Pierre-Louis Bossart wrote:
> The description is directly derived from the MIPI DisCo specification.
> 
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  .../ABI/testing/sysfs-bus-soundwire-master    | 21 +++++++++++++++++++
>  drivers/soundwire/sysfs.c                     |  1 +
>  2 files changed, 22 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-soundwire-master
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-master b/Documentation/ABI/testing/sysfs-bus-soundwire-master
> new file mode 100644
> index 000000000000..69cadf31049d
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-soundwire-master
> @@ -0,0 +1,21 @@
> +What:		/sys/bus/soundwire/devices/sdw-master-N/revision
> +		/sys/bus/soundwire/devices/sdw-master-N/clk_stop_modes
> +		/sys/bus/soundwire/devices/sdw-master-N/clk_freq
> +		/sys/bus/soundwire/devices/sdw-master-N/clk_gears
> +		/sys/bus/soundwire/devices/sdw-master-N/default_col
> +		/sys/bus/soundwire/devices/sdw-master-N/default_frame_rate
> +		/sys/bus/soundwire/devices/sdw-master-N/default_row
> +		/sys/bus/soundwire/devices/sdw-master-N/dynamic_shape
> +		/sys/bus/soundwire/devices/sdw-master-N/err_threshold
> +		/sys/bus/soundwire/devices/sdw-master-N/max_clk_freq
> +
> +Date:		May 2019
> +
> +Contact:	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> +
> +Description:	SoundWire Master-N DisCo properties.
> +		These properties are defined by MIPI DisCo Specification
> +		for SoundWire. They define various properties of the Master
> +		and are used by the bus to configure the Master. clk_stop_modes
> +		is a bitmask for simplifications and combines the
> +		clock-stop-mode0 and clock-stop-mode1 properties.
> diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
> index 734e2c8bc5cd..c2e5b7ad42fb 100644
> --- a/drivers/soundwire/sysfs.c
> +++ b/drivers/soundwire/sysfs.c
> @@ -31,6 +31,7 @@ struct sdw_master_sysfs {
>   *      |---- clk_gears
>   *      |---- default_row
>   *      |---- default_col
> + *      |---- default_frame_shape
>   *      |---- dynamic_shape
>   *      |---- err_threshold
>   */

This last chunk should go in patch 1 of this series, right?

thanks,

greg k-h

Re: [RFC PATCH 2/7] soundwire: add Slave sysfs support

[Greg KH]

On Fri, May 03, 2019 at 08:00:25PM -0500, Pierre-Louis Bossart wrote:
> Add DisCo Slave properties as device attributes.
> Add a device for Data Port 0 which adds Dp0 properties as attributes.
> Add devices for Source and Sink Data Ports, and add Dp-N
> properties as attributes.
> 
> The Slave, DP0 and DPn cases are intentionally handled in separate
> files to avoid conflicts with attributes having the same names at
> different levels.
> 
> Audio modes are not supported for now. Depending on the discussions
> the SoundWire Device Class, we may add it later as is or follow the
> new specification.
> 
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/Makefile          |   2 +-
>  drivers/soundwire/bus.c             |   2 +
>  drivers/soundwire/bus.h             |   2 +
>  drivers/soundwire/bus_type.c        |   5 +
>  drivers/soundwire/slave.c           |   1 +
>  drivers/soundwire/sysfs.c           | 213 ++++++++++++++++++++++++++++
>  drivers/soundwire/sysfs_local.h     |  42 ++++++
>  drivers/soundwire/sysfs_slave_dp0.c | 112 +++++++++++++++
>  drivers/soundwire/sysfs_slave_dpn.c | 168 ++++++++++++++++++++++
>  include/linux/soundwire/sdw.h       |   5 +
>  10 files changed, 551 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/soundwire/sysfs_local.h
>  create mode 100644 drivers/soundwire/sysfs_slave_dp0.c
>  create mode 100644 drivers/soundwire/sysfs_slave_dpn.c
> 
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index 787f1cbf342c..a72a29731a28 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -4,7 +4,7 @@
>  
>  #Bus Objs
>  soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
> -			sysfs.o
> +			sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o
>  obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
>  
>  #Cadence Objs
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 38de7071e135..dd9181693554 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -113,6 +113,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
>  	struct sdw_slave *slave = dev_to_sdw_dev(dev);
>  	struct sdw_bus *bus = slave->bus;
>  
> +	sdw_sysfs_slave_exit(slave);
> +
>  	mutex_lock(&bus->bus_lock);
>  
>  	if (slave->dev_num) /* clear dev_num if assigned */
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 3048ca153f22..0707e68a8d21 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -18,6 +18,8 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
>  void sdw_extract_slave_id(struct sdw_bus *bus,
>  			  u64 addr, struct sdw_slave_id *id);
>  
> +extern const struct attribute_group *sdw_slave_dev_attr_groups[];
> +
>  enum {
>  	SDW_MSG_FLAG_READ = 0,
>  	SDW_MSG_FLAG_WRITE,
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index 2655602f0cfb..f68fe45c1037 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -97,6 +97,11 @@ static int sdw_drv_probe(struct device *dev)
>  	if (slave->ops && slave->ops->read_prop)
>  		slave->ops->read_prop(slave);
>  
> +	/* init the sysfs as we have properties now */
> +	ret = sdw_sysfs_slave_init(slave);
> +	if (ret < 0)
> +		dev_warn(dev, "Slave sysfs init failed:%d\n", ret);
> +
>  	/*
>  	 * Check for valid clk_stop_timeout, use DisCo worst case value of
>  	 * 300ms
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index f39a5815e25d..bad73a267fdd 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -34,6 +34,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
>  		     id->class_id, id->unique_id);
>  
>  	slave->dev.release = sdw_slave_release;
> +	slave->dev.groups = sdw_slave_dev_attr_groups;
>  	slave->dev.bus = &sdw_bus_type;
>  	slave->bus = bus;
>  	slave->status = SDW_SLAVE_UNATTACHED;
> diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
> index 7b6c3826a73a..734e2c8bc5cd 100644
> --- a/drivers/soundwire/sysfs.c
> +++ b/drivers/soundwire/sysfs.c
> @@ -8,6 +8,7 @@
>  #include <linux/soundwire/sdw.h>
>  #include <linux/soundwire/sdw_type.h>
>  #include "bus.h"
> +#include "sysfs_local.h"
>  
>  struct sdw_master_sysfs {
>  	struct device dev;
> @@ -160,3 +161,215 @@ void sdw_sysfs_bus_exit(struct sdw_bus *bus)
>  	put_device(&master->dev);
>  	bus->sysfs = NULL;
>  }
> +
> +/*
> + * Slave sysfs
> + */
> +
> +/*
> + * The sysfs for Slave reflects the MIPI description as given
> + * in the MIPI DisCo spec
> + *
> + * Base file is device
> + *	|---- mipi_revision
> + *	|---- wake_capable
> + *	|---- test_mode_capable
> + *	|---- simple_clk_stop_capable
> + *	|---- clk_stop_timeout
> + *	|---- ch_prep_timeout
> + *	|---- reset_behave
> + *	|---- high_PHY_capable
> + *	|---- paging_support
> + *	|---- bank_delay_support
> + *	|---- p15_behave
> + *	|---- master_count
> + *	|---- source_ports
> + *	|---- sink_ports
> + *	|---- dp0
> + *		|---- max_word
> + *		|---- min_word
> + *		|---- words
> + *		|---- flow_controlled
> + *		|---- simple_ch_prep_sm
> + *		|---- device_interrupts
> + *	|---- dpN
> + *		|---- max_word
> + *		|---- min_word
> + *		|---- words
> + *		|---- type
> + *		|---- max_grouping
> + *		|---- simple_ch_prep_sm
> + *		|---- ch_prep_timeout
> + *		|---- device_interrupts
> + *		|---- max_ch
> + *		|---- min_ch
> + *		|---- ch
> + *		|---- ch_combinations
> + *		|---- modes
> + *		|---- max_async_buffer
> + *		|---- block_pack_mode
> + *		|---- port_encoding
> + *		|---- audio_modeM
> + *				|---- bus_min_freq
> + *				|---- bus_max_freq
> + *				|---- bus_freq
> + *				|---- max_freq
> + *				|---- min_freq
> + *				|---- freq
> + *				|---- prep_ch_behave
> + *				|---- glitchless
> + *
> + */
> +
> +#define sdw_slave_attr(field, format_string)			\
> +static ssize_t field##_show(struct device *dev,			\
> +			    struct device_attribute *attr,	\
> +			    char *buf)				\
> +{								\
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);		\
> +	return sprintf(buf, format_string, slave->prop.field);	\
> +}								\
> +static DEVICE_ATTR_RO(field)
> +
> +sdw_slave_attr(mipi_revision, "0x%x\n");
> +sdw_slave_attr(wake_capable, "%d\n");
> +sdw_slave_attr(test_mode_capable, "%d\n");
> +sdw_slave_attr(clk_stop_mode1, "%d\n");
> +sdw_slave_attr(simple_clk_stop_capable, "%d\n");
> +sdw_slave_attr(clk_stop_timeout, "%d\n");
> +sdw_slave_attr(ch_prep_timeout, "%d\n");
> +sdw_slave_attr(reset_behave, "%d\n");
> +sdw_slave_attr(high_PHY_capable, "%d\n");
> +sdw_slave_attr(paging_support, "%d\n");
> +sdw_slave_attr(bank_delay_support, "%d\n");
> +sdw_slave_attr(p15_behave, "%d\n");
> +sdw_slave_attr(master_count, "%d\n");
> +sdw_slave_attr(source_ports, "%d\n");
> +sdw_slave_attr(sink_ports, "%d\n");
> +
> +static ssize_t modalias_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> +	return sdw_slave_modalias(slave, buf, 256);
> +}
> +static DEVICE_ATTR_RO(modalias);
> +
> +static struct attribute *slave_dev_attrs[] = {
> +	&dev_attr_mipi_revision.attr,
> +	&dev_attr_wake_capable.attr,
> +	&dev_attr_test_mode_capable.attr,
> +	&dev_attr_clk_stop_mode1.attr,
> +	&dev_attr_simple_clk_stop_capable.attr,
> +	&dev_attr_clk_stop_timeout.attr,
> +	&dev_attr_ch_prep_timeout.attr,
> +	&dev_attr_reset_behave.attr,
> +	&dev_attr_high_PHY_capable.attr,
> +	&dev_attr_paging_support.attr,
> +	&dev_attr_bank_delay_support.attr,
> +	&dev_attr_p15_behave.attr,
> +	&dev_attr_master_count.attr,
> +	&dev_attr_source_ports.attr,
> +	&dev_attr_sink_ports.attr,
> +	&dev_attr_modalias.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group sdw_slave_dev_attr_group = {
> +	.attrs	= slave_dev_attrs,
> +};
> +
> +const struct attribute_group *sdw_slave_dev_attr_groups[] = {
> +	&sdw_slave_dev_attr_group,
> +	NULL
> +};

ATTRIBUTE_GROUP()?


> +
> +int sdw_sysfs_slave_init(struct sdw_slave *slave)
> +{
> +	struct sdw_slave_sysfs *sysfs;
> +	unsigned int src_dpns, sink_dpns, i, j;
> +	int err;
> +
> +	if (slave->sysfs) {
> +		dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
> +		err = -EIO;
> +		goto err_ret;
> +	}
> +
> +	sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);

Same question as patch 1, why a new device?

thanks,

greg k-h

Re: [RFC PATCH 5/7] soundwire: add debugfs support

[Greg KH]

On Fri, May 03, 2019 at 08:00:28PM -0500, Pierre-Louis Bossart wrote:
> Add base debugfs mechanism for SoundWire bus by creating soundwire
> root and master-N and slave-x hierarchy.
> 
> Also add SDW Slave SCP, DP0 and DP-N register debug file.
> 
> Registers not implemented will print as "XX"
> 
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
> is the use of scnprintf to avoid known issues with snprintf.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/Makefile    |   3 +-
>  drivers/soundwire/bus.c       |   5 +
>  drivers/soundwire/bus.h       |  28 +++++
>  drivers/soundwire/bus_type.c  |   3 +
>  drivers/soundwire/debugfs.c   | 214 ++++++++++++++++++++++++++++++++++
>  drivers/soundwire/slave.c     |   1 +
>  include/linux/soundwire/sdw.h |   9 ++
>  7 files changed, 262 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/soundwire/debugfs.c
> 
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index a72a29731a28..f2c425fa15bd 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -4,7 +4,8 @@
>  
>  #Bus Objs
>  soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
> -			sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o
> +			sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o  \
> +			debugfs.o
>  obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
>  
>  #Cadence Objs
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index dd9181693554..b79eba321b71 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -53,6 +53,8 @@ int sdw_add_bus_master(struct sdw_bus *bus)
>  	if (ret < 0)
>  		dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret);
>  
> +	bus->debugfs = sdw_bus_debugfs_init(bus);
> +
>  	/*
>  	 * Device numbers in SoundWire are 0 through 15. Enumeration device
>  	 * number (0), Broadcast device number (15), Group numbers (12 and
> @@ -114,6 +116,7 @@ static int sdw_delete_slave(struct device *dev, void *data)
>  	struct sdw_bus *bus = slave->bus;
>  
>  	sdw_sysfs_slave_exit(slave);
> +	sdw_slave_debugfs_exit(slave->debugfs);
>  
>  	mutex_lock(&bus->bus_lock);
>  
> @@ -136,6 +139,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
>  void sdw_delete_bus_master(struct sdw_bus *bus)
>  {
>  	sdw_sysfs_bus_exit(bus);
> +	if (bus->debugfs)
> +		sdw_bus_debugfs_exit(bus->debugfs);

No need to check, just call it.

>  
>  	device_for_each_child(bus->dev, NULL, sdw_delete_slave);
>  }
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 0707e68a8d21..f0b1f4f9d7b2 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -20,6 +20,34 @@ void sdw_extract_slave_id(struct sdw_bus *bus,
>  
>  extern const struct attribute_group *sdw_slave_dev_attr_groups[];
>  
> +#ifdef CONFIG_DEBUG_FS
> +struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus);
> +void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d);
> +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d);
> +struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave);
> +void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d);
> +void sdw_debugfs_init(void);
> +void sdw_debugfs_exit(void);
> +#else
> +struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus)
> +{ return NULL; }
> +
> +void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d) {}
> +
> +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
> +{ return NULL; }
> +
> +struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
> +{ return NULL; }
> +
> +void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d) {}
> +
> +void sdw_debugfs_init(void) {}
> +
> +void sdw_debugfs_exit(void) {}
> +
> +#endif
> +
>  enum {
>  	SDW_MSG_FLAG_READ = 0,
>  	SDW_MSG_FLAG_WRITE,
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index f68fe45c1037..f28c1a2446af 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -6,6 +6,7 @@
>  #include <linux/pm_domain.h>
>  #include <linux/soundwire/sdw.h>
>  #include <linux/soundwire/sdw_type.h>
> +#include "bus.h"
>  
>  /**
>   * sdw_get_device_id - find the matching SoundWire device id
> @@ -182,11 +183,13 @@ EXPORT_SYMBOL_GPL(sdw_unregister_driver);
>  
>  static int __init sdw_bus_init(void)
>  {
> +	sdw_debugfs_init();
>  	return bus_register(&sdw_bus_type);
>  }
>  
>  static void __exit sdw_bus_exit(void)
>  {
> +	sdw_debugfs_exit();
>  	bus_unregister(&sdw_bus_type);
>  }
>  
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> new file mode 100644
> index 000000000000..8a8b4df95c46
> --- /dev/null
> +++ b/drivers/soundwire/debugfs.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2017-19 Intel Corporation.
> +
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/slab.h>
> +#include <linux/soundwire/sdw.h>
> +#include <linux/soundwire/sdw_registers.h>
> +#include "bus.h"
> +
> +#ifdef CONFIG_DEBUG_FS
> +struct dentry *sdw_debugfs_root;
> +#endif
> +
> +struct sdw_bus_debugfs {
> +	struct sdw_bus *bus;

Why do you need to save this pointer?

> +	struct dentry *fs;

This really is all you need to have around, right?

> +};
> +
> +struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus)
> +{
> +	struct sdw_bus_debugfs *d;
> +	char name[16];
> +
> +	if (!sdw_debugfs_root)
> +		return NULL;
> +
> +	d = kzalloc(sizeof(*d), GFP_KERNEL);
> +	if (!d)
> +		return NULL;
> +
> +	/* create the debugfs master-N */
> +	snprintf(name, sizeof(name), "master-%d", bus->link_id);
> +	d->fs = debugfs_create_dir(name, sdw_debugfs_root);
> +	if (IS_ERR_OR_NULL(d->fs)) {
> +		dev_err(bus->dev, "debugfs root creation failed\n");
> +		goto err;
> +	}
> +
> +	d->bus = bus;
> +
> +	return d;
> +
> +err:
> +	kfree(d);
> +	return NULL;
> +}
> +
> +void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d)
> +{
> +	debugfs_remove_recursive(d->fs);
> +	kfree(d);
> +}
> +
> +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
> +{
> +	if (d)
> +		return d->fs;
> +	return NULL;
> +}
> +EXPORT_SYMBOL(sdw_bus_debugfs_get_root);

_GPL()?

But why is this exported at all?  No one calls this function.

> +struct sdw_slave_debugfs {
> +	struct sdw_slave *slave;

Same question as above, why do you need this pointer?

And meta-comment, if you _EVER_ save off a pointer to a reference
counted object (like this and the above one), you HAVE to grab a
reference to it, otherwise it can go away at any point in time as that
is the point of reference counted objects.

So even if you do need/want this, you have to properly handle the
reference count by incrementing/decrementing it as needed.

> +
> +	struct dentry *fs;
> +};
> +
> +#define RD_BUF (3 * PAGE_SIZE)
> +
> +static ssize_t sdw_sprintf(struct sdw_slave *slave,
> +			   char *buf, size_t pos, unsigned int reg)
> +{
> +	int value;
> +
> +	value = sdw_read(slave, reg);
> +
> +	if (value < 0)
> +		return scnprintf(buf + pos, RD_BUF - pos, "%3x\tXX\n", reg);
> +	else
> +		return scnprintf(buf + pos, RD_BUF - pos,
> +				"%3x\t%2x\n", reg, value);
> +}
> +
> +static ssize_t sdw_slave_reg_read(struct file *file, char __user *user_buf,
> +				  size_t count, loff_t *ppos)
> +{
> +	struct sdw_slave *slave = file->private_data;
> +	unsigned int reg;
> +	char *buf;
> +	ssize_t ret;
> +	int i, j;
> +
> +	buf = kzalloc(RD_BUF, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = scnprintf(buf, RD_BUF, "Register  Value\n");
> +	ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP0\n");
> +
> +	for (i = 0; i < 6; i++)
> +		ret += sdw_sprintf(slave, buf, ret, i);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
> +	ret += sdw_sprintf(slave, buf, ret, SDW_DP0_CHANNELEN);
> +	for (i = SDW_DP0_SAMPLECTRL1; i <= SDW_DP0_LANECTRL; i++)
> +		ret += sdw_sprintf(slave, buf, ret, i);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
> +	ret += sdw_sprintf(slave, buf, ret,
> +			SDW_DP0_CHANNELEN + SDW_BANK1_OFFSET);
> +	for (i = SDW_DP0_SAMPLECTRL1 + SDW_BANK1_OFFSET;
> +			i <= SDW_DP0_LANECTRL + SDW_BANK1_OFFSET; i++)
> +		ret += sdw_sprintf(slave, buf, ret, i);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret, "\nSCP\n");
> +	for (i = SDW_SCP_INT1; i <= SDW_SCP_BANKDELAY; i++)
> +		ret += sdw_sprintf(slave, buf, ret, i);
> +	for (i = SDW_SCP_DEVID_0; i <= SDW_SCP_DEVID_5; i++)
> +		ret += sdw_sprintf(slave, buf, ret, i);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
> +	ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B0);
> +	ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B0);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
> +	ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B1);
> +	ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B1);
> +
> +	for (i = 1; i < 14; i++) {
> +		ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP%d\n", i);
> +		reg = SDW_DPN_INT(i);
> +		for (j = 0; j < 6; j++)
> +			ret += sdw_sprintf(slave, buf, ret, reg + j);
> +
> +		ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
> +		reg = SDW_DPN_CHANNELEN_B0(i);
> +		for (j = 0; j < 9; j++)
> +			ret += sdw_sprintf(slave, buf, ret, reg + j);
> +
> +		ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
> +		reg = SDW_DPN_CHANNELEN_B1(i);
> +		for (j = 0; j < 9; j++)
> +			ret += sdw_sprintf(slave, buf, ret, reg + j);
> +	}
> +
> +	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> +	kfree(buf);
> +
> +	return ret;
> +}
> +
> +static const struct file_operations sdw_slave_reg_fops = {
> +	.open = simple_open,
> +	.read = sdw_slave_reg_read,
> +	.llseek = default_llseek,
> +};
> +
> +struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
> +{
> +	struct sdw_bus_debugfs *master;
> +	struct sdw_slave_debugfs *d;
> +	char name[32];
> +
> +	master = slave->bus->debugfs;
> +	if (!master)
> +		return NULL;
> +
> +	d = kzalloc(sizeof(*d), GFP_KERNEL);
> +	if (!d)
> +		return NULL;
> +
> +	/* create the debugfs slave-name */
> +	snprintf(name, sizeof(name), "%s", dev_name(&slave->dev));
> +	d->fs = debugfs_create_dir(name, master->fs);
> +	if (IS_ERR_OR_NULL(d->fs)) {
> +		dev_err(&slave->dev, "slave debugfs root creation failed\n");
> +		goto err;
> +	}

You never care about the return value of a debugfs call.  I have a 100+
patch series stripping all of this out of the kernel, please don't force
me to add another one to it :)

Just call debugfs and move on, you can always put the return value of
one call into another one just fine, and your function logic should
never change if debugfs returns an error or not, you do not care.

> +
> +	d->slave = slave;
> +
> +	debugfs_create_file("registers", 0400, d->fs,
> +			    slave, &sdw_slave_reg_fops);
> +
> +	return d;
> +
> +err:
> +	kfree(d);
> +	return NULL;
> +}
> +
> +void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d)
> +{
> +	debugfs_remove_recursive(d->fs);
> +	kfree(d);
> +}
> +
> +void sdw_debugfs_init(void)
> +{
> +	sdw_debugfs_root = debugfs_create_dir("soundwire", NULL);
> +	if (IS_ERR_OR_NULL(sdw_debugfs_root)) {
> +		pr_warn("SoundWire: Failed to create debugfs directory\n");
> +		sdw_debugfs_root = NULL;
> +		return;

Same here, just call the function and return.

thanks,

greg k-h

Re: [RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump

[Greg KH]

On Fri, May 03, 2019 at 08:00:29PM -0500, Pierre-Louis Bossart wrote:
> Add debugfs file to dump the Cadence master registers
> 
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
> is the use of scnprintf to avoid known issues with snprintf.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/cadence_master.c | 98 ++++++++++++++++++++++++++++++
>  drivers/soundwire/cadence_master.h |  5 ++
>  2 files changed, 103 insertions(+)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 682789bb8ab3..e9c30f18ce25 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/delay.h>
>  #include <linux/device.h>
> +#include <linux/debugfs.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> @@ -222,6 +223,103 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value)
>  	return -EAGAIN;
>  }
>  
> +/*
> + * debugfs
> + */
> +
> +#define RD_BUF (2 * PAGE_SIZE)
> +
> +static ssize_t cdns_sprintf(struct sdw_cdns *cdns,
> +			    char *buf, size_t pos, unsigned int reg)
> +{
> +	return scnprintf(buf + pos, RD_BUF - pos,
> +			 "%4x\t%4x\n", reg, cdns_readl(cdns, reg));
> +}
> +
> +static ssize_t cdns_reg_read(struct file *file, char __user *user_buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	struct sdw_cdns *cdns = file->private_data;
> +	char *buf;
> +	ssize_t ret;
> +	int i, j;
> +
> +	buf = kzalloc(RD_BUF, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = scnprintf(buf, RD_BUF, "Register  Value\n");
> +	ret += scnprintf(buf + ret, RD_BUF - ret, "\nMCP Registers\n");
> +	for (i = 0; i < 8; i++) /* 8 MCP registers */
> +		ret += cdns_sprintf(cdns, buf, ret, i * 4);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nStatus & Intr Registers\n");
> +	for (i = 0; i < 13; i++) /* 13 Status & Intr registers */
> +		ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_STAT + i * 4);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nSSP & Clk ctrl Registers\n");
> +	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL0);
> +	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL1);
> +	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL0);
> +	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL1);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nDPn B0 Registers\n");
> +	for (i = 0; i < 7; i++) {
> +		ret += scnprintf(buf + ret, RD_BUF - ret,
> +				 "\nDP-%d\n", i);
> +		for (j = 0; j < 6; j++)
> +			ret += cdns_sprintf(cdns, buf, ret,
> +					CDNS_DPN_B0_CONFIG(i) + j * 4);
> +	}
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nDPn B1 Registers\n");
> +	for (i = 0; i < 7; i++) {
> +		ret += scnprintf(buf + ret, RD_BUF - ret,
> +				 "\nDP-%d\n", i);
> +
> +		for (j = 0; j < 6; j++)
> +			ret += cdns_sprintf(cdns, buf, ret,
> +					CDNS_DPN_B1_CONFIG(i) + j * 4);
> +	}
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nDPn Control Registers\n");
> +	for (i = 0; i < 7; i++)
> +		ret += cdns_sprintf(cdns, buf, ret,
> +				CDNS_PORTCTRL + i * CDNS_PORT_OFFSET);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nPDIn Config Registers\n");
> +	for (i = 0; i < 7; i++)
> +		ret += cdns_sprintf(cdns, buf, ret, CDNS_PDI_CONFIG(i));
> +
> +	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> +	kfree(buf);
> +
> +	return ret;
> +}
> +
> +static const struct file_operations cdns_reg_fops = {
> +	.open = simple_open,
> +	.read = cdns_reg_read,
> +	.llseek = default_llseek,
> +};
> +
> +/**
> + * sdw_cdns_debugfs_init() - Cadence debugfs init
> + * @cdns: Cadence instance
> + * @root: debugfs root
> + */
> +void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
> +{
> +	debugfs_create_file("cdns-registers", 0400, root, cdns, &cdns_reg_fops);
> +}
> +EXPORT_SYMBOL(sdw_cdns_debugfs_init);

Don't wrap debugfs calls with export symbol without using
EXPORT_SYMBOL_GPL() or you will get grumpy emails from me :)

thanks,

greg k-h

Re: [RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump

[Greg KH]

On Fri, May 03, 2019 at 08:00:29PM -0500, Pierre-Louis Bossart wrote:
> Add debugfs file to dump the Cadence master registers
> 
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
> is the use of scnprintf to avoid known issues with snprintf.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/cadence_master.c | 98 ++++++++++++++++++++++++++++++
>  drivers/soundwire/cadence_master.h |  5 ++
>  2 files changed, 103 insertions(+)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 682789bb8ab3..e9c30f18ce25 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/delay.h>
>  #include <linux/device.h>
> +#include <linux/debugfs.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> @@ -222,6 +223,103 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value)
>  	return -EAGAIN;
>  }
>  
> +/*
> + * debugfs
> + */
> +
> +#define RD_BUF (2 * PAGE_SIZE)
> +
> +static ssize_t cdns_sprintf(struct sdw_cdns *cdns,
> +			    char *buf, size_t pos, unsigned int reg)
> +{
> +	return scnprintf(buf + pos, RD_BUF - pos,
> +			 "%4x\t%4x\n", reg, cdns_readl(cdns, reg));
> +}
> +
> +static ssize_t cdns_reg_read(struct file *file, char __user *user_buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	struct sdw_cdns *cdns = file->private_data;
> +	char *buf;
> +	ssize_t ret;
> +	int i, j;
> +
> +	buf = kzalloc(RD_BUF, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = scnprintf(buf, RD_BUF, "Register  Value\n");
> +	ret += scnprintf(buf + ret, RD_BUF - ret, "\nMCP Registers\n");
> +	for (i = 0; i < 8; i++) /* 8 MCP registers */
> +		ret += cdns_sprintf(cdns, buf, ret, i * 4);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nStatus & Intr Registers\n");
> +	for (i = 0; i < 13; i++) /* 13 Status & Intr registers */
> +		ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_STAT + i * 4);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nSSP & Clk ctrl Registers\n");
> +	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL0);
> +	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL1);
> +	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL0);
> +	ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL1);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nDPn B0 Registers\n");
> +	for (i = 0; i < 7; i++) {
> +		ret += scnprintf(buf + ret, RD_BUF - ret,
> +				 "\nDP-%d\n", i);
> +		for (j = 0; j < 6; j++)
> +			ret += cdns_sprintf(cdns, buf, ret,
> +					CDNS_DPN_B0_CONFIG(i) + j * 4);
> +	}
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nDPn B1 Registers\n");
> +	for (i = 0; i < 7; i++) {
> +		ret += scnprintf(buf + ret, RD_BUF - ret,
> +				 "\nDP-%d\n", i);
> +
> +		for (j = 0; j < 6; j++)
> +			ret += cdns_sprintf(cdns, buf, ret,
> +					CDNS_DPN_B1_CONFIG(i) + j * 4);
> +	}
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nDPn Control Registers\n");
> +	for (i = 0; i < 7; i++)
> +		ret += cdns_sprintf(cdns, buf, ret,
> +				CDNS_PORTCTRL + i * CDNS_PORT_OFFSET);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret,
> +			 "\nPDIn Config Registers\n");
> +	for (i = 0; i < 7; i++)
> +		ret += cdns_sprintf(cdns, buf, ret, CDNS_PDI_CONFIG(i));
> +
> +	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> +	kfree(buf);
> +
> +	return ret;
> +}
> +
> +static const struct file_operations cdns_reg_fops = {
> +	.open = simple_open,
> +	.read = cdns_reg_read,
> +	.llseek = default_llseek,
> +};
> +
> +/**
> + * sdw_cdns_debugfs_init() - Cadence debugfs init
> + * @cdns: Cadence instance
> + * @root: debugfs root
> + */
> +void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
> +{
> +	debugfs_create_file("cdns-registers", 0400, root, cdns, &cdns_reg_fops);
> +}
> +EXPORT_SYMBOL(sdw_cdns_debugfs_init);

Wait, why is this exported at all?  No one is calling it.

Re: [RFC PATCH 7/7] soundwire: intel: add debugfs register dump

[Greg KH]

On Fri, May 03, 2019 at 08:00:30PM -0500, Pierre-Louis Bossart wrote:
> Add debugfs file to dump the Intel SoundWire registers
> 
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
> is the use of scnprintf to avoid known issues with snprintf.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/intel.c | 115 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
> 
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 4ac141730b13..7fb2cd6d5bb5 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
> @@ -16,6 +17,7 @@
>  #include <linux/soundwire/sdw.h>
>  #include <linux/soundwire/sdw_intel.h>
>  #include "cadence_master.h"
> +#include "bus.h"
>  #include "intel.h"
>  
>  /* Intel SHIM Registers Definition */
> @@ -98,6 +100,7 @@ struct sdw_intel {
>  	struct sdw_cdns cdns;
>  	int instance;
>  	struct sdw_intel_link_res *res;
> +	struct dentry *fs;
>  };
>  
>  #define cdns_to_intel(_cdns) container_of(_cdns, struct sdw_intel, cdns)
> @@ -161,6 +164,115 @@ static int intel_set_bit(void __iomem *base, int offset, u32 value, u32 mask)
>  	return -EAGAIN;
>  }
>  
> +/*
> + * debugfs
> + */
> +
> +#define RD_BUF (2 * PAGE_SIZE)
> +
> +static ssize_t intel_sprintf(void __iomem *mem, bool l,
> +			     char *buf, size_t pos, unsigned int reg)
> +{
> +	int value;
> +
> +	if (l)
> +		value = intel_readl(mem, reg);
> +	else
> +		value = intel_readw(mem, reg);
> +
> +	return scnprintf(buf + pos, RD_BUF - pos, "%4x\t%4x\n", reg, value);
> +}
> +
> +static ssize_t intel_reg_read(struct file *file, char __user *user_buf,
> +			      size_t count, loff_t *ppos)
> +{
> +	struct sdw_intel *sdw = file->private_data;
> +	void __iomem *s = sdw->res->shim;
> +	void __iomem *a = sdw->res->alh;
> +	char *buf;
> +	ssize_t ret;
> +	int i, j;
> +	unsigned int links, reg;
> +
> +	buf = kzalloc(RD_BUF, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	links = intel_readl(s, SDW_SHIM_LCAP) & GENMASK(2, 0);
> +
> +	ret = scnprintf(buf, RD_BUF, "Register  Value\n");
> +	ret += scnprintf(buf + ret, RD_BUF - ret, "\nShim\n");
> +
> +	for (i = 0; i < 4; i++) {
> +		reg = SDW_SHIM_LCAP + i * 4;
> +		ret += intel_sprintf(s, true, buf, ret, reg);
> +	}
> +
> +	for (i = 0; i < links; i++) {
> +		ret += scnprintf(buf + ret, RD_BUF - ret, "\nLink%d\n", i);
> +		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLSCAP(i));
> +		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS0CM(i));
> +		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS1CM(i));
> +		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS2CM(i));
> +		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS3CM(i));
> +		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_PCMSCAP(i));
> +
> +		for (j = 0; j < 8; j++) {
> +			ret += intel_sprintf(s, false, buf, ret,
> +					SDW_SHIM_PCMSYCHM(i, j));
> +			ret += intel_sprintf(s, false, buf, ret,
> +					SDW_SHIM_PCMSYCHC(i, j));
> +		}
> +
> +		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_PDMSCAP(i));
> +		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_IOCTL(i));
> +		ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTMCTL(i));
> +	}
> +
> +	ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_WAKEEN);
> +	ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_WAKESTS);
> +
> +	ret += scnprintf(buf + ret, RD_BUF - ret, "\nALH\n");
> +	for (i = 0; i < 8; i++)
> +		ret += intel_sprintf(a, true, buf, ret, SDW_ALH_STRMZCFG(i));
> +
> +	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> +	kfree(buf);
> +
> +	return ret;
> +}
> +
> +static const struct file_operations intel_reg_fops = {
> +	.open = simple_open,
> +	.read = intel_reg_read,
> +	.llseek = default_llseek,
> +};
> +
> +static void intel_debugfs_init(struct sdw_intel *sdw)
> +{
> +	struct dentry *root = sdw_bus_debugfs_get_root(sdw->cdns.bus.debugfs);
> +
> +	if (!root)
> +		return;
> +
> +	sdw->fs = debugfs_create_dir("intel-sdw", root);
> +	if (IS_ERR_OR_NULL(sdw->fs)) {

Again, you do not care, do not check this.

thanks,

greg k-h

Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support

[Pierre-Louis Bossart]

>> +static struct attribute_group sdw_slave_dev_attr_group = {
>> +	.attrs	= slave_dev_attrs,
>> +};
>> +
>> +const struct attribute_group *sdw_slave_dev_attr_groups[] = {
>> +	&sdw_slave_dev_attr_group,
>> +	NULL
>> +};
> 
> ATTRIBUTE_GROUP()?

yes.

> 
> 
>> +
>> +int sdw_sysfs_slave_init(struct sdw_slave *slave)
>> +{
>> +	struct sdw_slave_sysfs *sysfs;
>> +	unsigned int src_dpns, sink_dpns, i, j;
>> +	int err;
>> +
>> +	if (slave->sysfs) {
>> +		dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
>> +		err = -EIO;
>> +		goto err_ret;
>> +	}
>> +
>> +	sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
> 
> Same question as patch 1, why a new device?

yes it's the same open. In this case, the slave devices are defined at a 
different level so it's also confusing to create a device to represent 
the slave properties. The code works but I am not sure the initial 
directions are correct.

Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support

[Pierre-Louis Bossart]

>> @@ -136,6 +139,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
>>   void sdw_delete_bus_master(struct sdw_bus *bus)
>>   {
>>   	sdw_sysfs_bus_exit(bus);
>> +	if (bus->debugfs)
>> +		sdw_bus_debugfs_exit(bus->debugfs);
> 
> No need to check, just call it.

That was on my todo list, will remove.


>> +struct sdw_bus_debugfs {
>> +	struct sdw_bus *bus;
> 
> Why do you need to save this pointer?
> 
>> +	struct dentry *fs;
> 
> This really is all you need to have around, right?

will check.

>> +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
>> +{
>> +	if (d)
>> +		return d->fs;
>> +	return NULL;
>> +}
>> +EXPORT_SYMBOL(sdw_bus_debugfs_get_root);
> 
> _GPL()?

Oops, that's a big miss. will fix, thanks for spotting this.

> 
> But why is this exported at all?  No one calls this function.

I will have to check.

> 
>> +struct sdw_slave_debugfs {
>> +	struct sdw_slave *slave;
> 
> Same question as above, why do you need this pointer?

will check.

> 
> And meta-comment, if you _EVER_ save off a pointer to a reference
> counted object (like this and the above one), you HAVE to grab a
> reference to it, otherwise it can go away at any point in time as that
> is the point of reference counted objects.
> 
> So even if you do need/want this, you have to properly handle the
> reference count by incrementing/decrementing it as needed.

good comment, thank you for the guidance.

>> +struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
>> +{
>> +	struct sdw_bus_debugfs *master;
>> +	struct sdw_slave_debugfs *d;
>> +	char name[32];
>> +
>> +	master = slave->bus->debugfs;
>> +	if (!master)
>> +		return NULL;
>> +
>> +	d = kzalloc(sizeof(*d), GFP_KERNEL);
>> +	if (!d)
>> +		return NULL;
>> +
>> +	/* create the debugfs slave-name */
>> +	snprintf(name, sizeof(name), "%s", dev_name(&slave->dev));
>> +	d->fs = debugfs_create_dir(name, master->fs);
>> +	if (IS_ERR_OR_NULL(d->fs)) {
>> +		dev_err(&slave->dev, "slave debugfs root creation failed\n");
>> +		goto err;
>> +	}
> 
> You never care about the return value of a debugfs call.  I have a 100+
> patch series stripping all of this out of the kernel, please don't force
> me to add another one to it :)
> 
> Just call debugfs and move on, you can always put the return value of
> one call into another one just fine, and your function logic should
> never change if debugfs returns an error or not, you do not care.

Yes, it's agreed that we should not depend on debugfs or fail here. will 
fix, no worries.

>
>> +void sdw_debugfs_init(void)
>> +{
>> +	sdw_debugfs_root = debugfs_create_dir("soundwire", NULL);
>> +	if (IS_ERR_OR_NULL(sdw_debugfs_root)) {
>> +		pr_warn("SoundWire: Failed to create debugfs directory\n");
>> +		sdw_debugfs_root = NULL;
>> +		return;
> 
> Same here, just call the function and return.

yep, will do.

Re: [alsa-devel] [RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump

[Pierre-Louis Bossart]

>> +void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
>> +{
>> +	debugfs_create_file("cdns-registers", 0400, root, cdns, &cdns_reg_fops);
>> +}
>> +EXPORT_SYMBOL(sdw_cdns_debugfs_init);
> 
> Don't wrap debugfs calls with export symbol without using
> EXPORT_SYMBOL_GPL() or you will get grumpy emails from me :)

Yes, that's a miss. I've already seen this comment and fixed it for SOF, 
I should have thought about it for SoundWire, my bad.

Re: [alsa-devel] [RFC PATCH 7/7] soundwire: intel: add debugfs register dump

[Pierre-Louis Bossart]

>> +static void intel_debugfs_init(struct sdw_intel *sdw)
>> +{
>> +	struct dentry *root = sdw_bus_debugfs_get_root(sdw->cdns.bus.debugfs);
>> +
>> +	if (!root)
>> +		return;
>> +
>> +	sdw->fs = debugfs_create_dir("intel-sdw", root);
>> +	if (IS_ERR_OR_NULL(sdw->fs)) {
> 
> Again, you do not care, do not check this.

yes will check all this.
Thanks for all the comments, much appreciated.

Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support

[Greg KH]

On Mon, May 06, 2019 at 09:42:35AM -0500, Pierre-Louis Bossart wrote:
> > > +
> > > +int sdw_sysfs_slave_init(struct sdw_slave *slave)
> > > +{
> > > +	struct sdw_slave_sysfs *sysfs;
> > > +	unsigned int src_dpns, sink_dpns, i, j;
> > > +	int err;
> > > +
> > > +	if (slave->sysfs) {
> > > +		dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
> > > +		err = -EIO;
> > > +		goto err_ret;
> > > +	}
> > > +
> > > +	sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
> > 
> > Same question as patch 1, why a new device?
> 
> yes it's the same open. In this case, the slave devices are defined at a
> different level so it's also confusing to create a device to represent the
> slave properties. The code works but I am not sure the initial directions
> are correct.

You can just make a subdir for your attributes by using the attribute
group name, if a subdirectory is needed just to keep things a bit more
organized.

Otherwise, you need to mess with having multiple "types" of struct
device all associated with the same bus.  It is possible, and not that
hard, but I don't think you are doing that here.

thnaks,

greg k-h

Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support

[Vinod Koul]

On 06-05-19, 17:19, Greg KH wrote:
> On Mon, May 06, 2019 at 09:42:35AM -0500, Pierre-Louis Bossart wrote:
> > > > +
> > > > +int sdw_sysfs_slave_init(struct sdw_slave *slave)
> > > > +{
> > > > +	struct sdw_slave_sysfs *sysfs;
> > > > +	unsigned int src_dpns, sink_dpns, i, j;
> > > > +	int err;
> > > > +
> > > > +	if (slave->sysfs) {
> > > > +		dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
> > > > +		err = -EIO;
> > > > +		goto err_ret;
> > > > +	}
> > > > +
> > > > +	sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
> > > 
> > > Same question as patch 1, why a new device?
> > 
> > yes it's the same open. In this case, the slave devices are defined at a
> > different level so it's also confusing to create a device to represent the
> > slave properties. The code works but I am not sure the initial directions
> > are correct.
> 
> You can just make a subdir for your attributes by using the attribute
> group name, if a subdirectory is needed just to keep things a bit more
> organized.

The key here is 'a subdir' which is not the case here. We did discuss
this in the initial patches for SoundWire which had sysfs :)

The way MIPI disco spec organized properties, we have dp0 and dpN
properties each of them requires to have a subdir of their own and that
was the reason why I coded it to be creating a device.

Do we have a better way to handle this?

> Otherwise, you need to mess with having multiple "types" of struct
> device all associated with the same bus.  It is possible, and not that
> hard, but I don't think you are doing that here.
> 
> thnaks,
> 
> greg k-h

-- 
~Vinod

Re: [RFC PATCH 3/7] ABI: testing: Add description of soundwire master sysfs files

[Vinod Koul]

On 03-05-19, 20:00, Pierre-Louis Bossart wrote:
> The description is directly derived from the MIPI DisCo specification.
> 
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  .../ABI/testing/sysfs-bus-soundwire-master    | 21 +++++++++++++++++++
>  drivers/soundwire/sysfs.c                     |  1 +
>  2 files changed, 22 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-soundwire-master
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-master b/Documentation/ABI/testing/sysfs-bus-soundwire-master
> new file mode 100644
> index 000000000000..69cadf31049d
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-soundwire-master
> @@ -0,0 +1,21 @@
> +What:		/sys/bus/soundwire/devices/sdw-master-N/revision
> +		/sys/bus/soundwire/devices/sdw-master-N/clk_stop_modes
> +		/sys/bus/soundwire/devices/sdw-master-N/clk_freq
> +		/sys/bus/soundwire/devices/sdw-master-N/clk_gears
> +		/sys/bus/soundwire/devices/sdw-master-N/default_col
> +		/sys/bus/soundwire/devices/sdw-master-N/default_frame_rate
> +		/sys/bus/soundwire/devices/sdw-master-N/default_row
> +		/sys/bus/soundwire/devices/sdw-master-N/dynamic_shape
> +		/sys/bus/soundwire/devices/sdw-master-N/err_threshold
> +		/sys/bus/soundwire/devices/sdw-master-N/max_clk_freq
> +
> +Date:		May 2019
> +
> +Contact:	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

As the author of original code, it would be great if you can add me as a
contact as well.

> +
> +Description:	SoundWire Master-N DisCo properties.
> +		These properties are defined by MIPI DisCo Specification
> +		for SoundWire. They define various properties of the Master
> +		and are used by the bus to configure the Master. clk_stop_modes
> +		is a bitmask for simplifications and combines the
> +		clock-stop-mode0 and clock-stop-mode1 properties.
> diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
> index 734e2c8bc5cd..c2e5b7ad42fb 100644
> --- a/drivers/soundwire/sysfs.c
> +++ b/drivers/soundwire/sysfs.c
> @@ -31,6 +31,7 @@ struct sdw_master_sysfs {
>   *      |---- clk_gears
>   *      |---- default_row
>   *      |---- default_col
> + *      |---- default_frame_shape

This should be folded into 1st patch

>   *      |---- dynamic_shape
>   *      |---- err_threshold
>   */
> -- 
> 2.17.1

-- 
~Vinod

Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support

[Vinod Koul]

On 06-05-19, 09:48, Pierre-Louis Bossart wrote:

> > > +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
> > > +{
> > > +	if (d)
> > > +		return d->fs;
> > > +	return NULL;
> > > +}
> > > +EXPORT_SYMBOL(sdw_bus_debugfs_get_root);
> > 
> > _GPL()?
> 
> Oops, that's a big miss. will fix, thanks for spotting this.

Not really. The Soundwire code is dual licensed. Many of the soundwire
symbols are indeed exported as EXPORT_SYMBOL. But I agree this one is
'linux' specific so can be made _GPL.

Pierre, does Intel still care about this being dual licensed or not?

> 
> > 
> > But why is this exported at all?  No one calls this function.
> 
> I will have to check.

It is used by codec driver which are not upstream yet. So my suggestion
would be NOT to export this and only do so when we have users for it
That would be true for other APIs exported out as well.

> > 
> > > +struct sdw_slave_debugfs {
> > > +	struct sdw_slave *slave;
> > 
> > Same question as above, why do you need this pointer?
> 
> will check.

The deubugfs code does hold a ref to slave object to read the data and
dump, this particular instance might be able to get rid of (should be
doable)

> > And meta-comment, if you _EVER_ save off a pointer to a reference
> > counted object (like this and the above one), you HAVE to grab a
> > reference to it, otherwise it can go away at any point in time as that
> > is the point of reference counted objects.
> > 
> > So even if you do need/want this, you have to properly handle the
> > reference count by incrementing/decrementing it as needed.

Yes, but then device exit routine is supposed to do debugfs cleanup as
well, so that would ensure these references are dropped at that point of
time. Greg should that not take care of it or we *should* always do
refcounting.

Thanks
-- 
~Vinod

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Pierre-Louis Bossart]

Thanks for the quick feedback Greg!

>> +static const struct attribute_group sdw_master_node_group = {
>> +	.attrs = master_node_attrs,
>> +};
>> +
>> +static const struct attribute_group *sdw_master_node_groups[] = {
>> +	&sdw_master_node_group,
>> +	NULL
>> +};
> 
> Minor nit, you can use the ATTRIBUTE_GROUPS() macro here to save you a
> few lines.

will do.

>> +
>> +static void sdw_device_release(struct device *dev)
>> +{
>> +	struct sdw_master_sysfs *master = to_sdw_device(dev);
>> +
>> +	kfree(master);
>> +}
>> +
>> +static struct device_type sdw_device_type = {
>> +	.name =	"sdw_device",
>> +	.release = sdw_device_release,
>> +};
>> +
>> +int sdw_sysfs_bus_init(struct sdw_bus *bus)
>> +{
>> +	struct sdw_master_sysfs *master;
>> +	int err;
>> +
>> +	if (bus->sysfs) {
>> +		dev_err(bus->dev, "SDW sysfs is already initialized\n");
>> +		return -EIO;
>> +	}
>> +
>> +	master = kzalloc(sizeof(*master), GFP_KERNEL);
>> +	if (!master)
>> +		return -ENOMEM;
> 
> Why are you creating a whole new device to put all of this under?  Is
> this needed?  What will the sysfs tree look like when you do this?  Why
> can't the "bus" device just get all of these attributes and no second
> device be created?

This is indeed my main question on this code (see cover letter) and why 
I tagged the series as RFC. I find it odd to create an int-sdw.0 
platform device to model the SoundWire master, and a sdw-master:0 device 
whose purpose is only to expose the properties of that master. it'd be 
simpler if all the properties were exposed one level up.

Vinod and Sanyog might be able to shed some light on this?

Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support

[Pierre-Louis Bossart]

On 5/6/19 11:22 AM, Vinod Koul wrote:
> On 06-05-19, 17:19, Greg KH wrote:
>> On Mon, May 06, 2019 at 09:42:35AM -0500, Pierre-Louis Bossart wrote:
>>>>> +
>>>>> +int sdw_sysfs_slave_init(struct sdw_slave *slave)
>>>>> +{
>>>>> +	struct sdw_slave_sysfs *sysfs;
>>>>> +	unsigned int src_dpns, sink_dpns, i, j;
>>>>> +	int err;
>>>>> +
>>>>> +	if (slave->sysfs) {
>>>>> +		dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
>>>>> +		err = -EIO;
>>>>> +		goto err_ret;
>>>>> +	}
>>>>> +
>>>>> +	sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
>>>>
>>>> Same question as patch 1, why a new device?
>>>
>>> yes it's the same open. In this case, the slave devices are defined at a
>>> different level so it's also confusing to create a device to represent the
>>> slave properties. The code works but I am not sure the initial directions
>>> are correct.
>>
>> You can just make a subdir for your attributes by using the attribute
>> group name, if a subdirectory is needed just to keep things a bit more
>> organized.
> 
> The key here is 'a subdir' which is not the case here. We did discuss
> this in the initial patches for SoundWire which had sysfs :)
> 
> The way MIPI disco spec organized properties, we have dp0 and dpN
> properties each of them requires to have a subdir of their own and that
> was the reason why I coded it to be creating a device.

Vinod, the question was not for dp0 and dpN, it's fine to have 
subdirectories there, but rather why we need separate devices for the 
master and slave properties.

> 
> Do we have a better way to handle this?
> 
>> Otherwise, you need to mess with having multiple "types" of struct
>> device all associated with the same bus.  It is possible, and not that
>> hard, but I don't think you are doing that here.
>>
>> thnaks,
>>
>> greg k-h
> 

Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support

[Pierre-Louis Bossart]

On 5/6/19 11:38 AM, Vinod Koul wrote:
> On 06-05-19, 09:48, Pierre-Louis Bossart wrote:
> 
>>>> +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
>>>> +{
>>>> +	if (d)
>>>> +		return d->fs;
>>>> +	return NULL;
>>>> +}
>>>> +EXPORT_SYMBOL(sdw_bus_debugfs_get_root);
>>>
>>> _GPL()?
>>
>> Oops, that's a big miss. will fix, thanks for spotting this.
> 
> Not really. The Soundwire code is dual licensed. Many of the soundwire
> symbols are indeed exported as EXPORT_SYMBOL. But I agree this one is
> 'linux' specific so can be made _GPL.
> 
> Pierre, does Intel still care about this being dual licensed or not?

Debugfs was never in scope for the dual-licensed parts, we've already 
agreed for SOF to move to _GPL.

> 
>>
>>>
>>> But why is this exported at all?  No one calls this function.
>>
>> I will have to check.
> 
> It is used by codec driver which are not upstream yet. So my suggestion
> would be NOT to export this and only do so when we have users for it
> That would be true for other APIs exported out as well.

It'll just make the first codec driver patchset more complicated but fine.

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Pierre-Louis Bossart]

>> +int sdw_sysfs_bus_init(struct sdw_bus *bus)
>> +{
>> +	struct sdw_master_sysfs *master;
>> +	int err;
>> +
>> +	if (bus->sysfs) {
>> +		dev_err(bus->dev, "SDW sysfs is already initialized\n");
>> +		return -EIO;
>> +	}
>> +
>> +	master = kzalloc(sizeof(*master), GFP_KERNEL);
>> +	if (!master)
>> +		return -ENOMEM;
> 
> Why are you creating a whole new device to put all of this under?  Is
> this needed?  What will the sysfs tree look like when you do this?  Why
> can't the "bus" device just get all of these attributes and no second
> device be created?

I tried a quick hack and indeed we could simplify the code with 
something as simple as:

[attributes omitted]

static const struct attribute_group sdw_master_node_group = {
	.attrs = master_node_attrs,
	.name = "mipi-disco"
};

int sdw_sysfs_bus_init(struct sdw_bus *bus)
{
	return sysfs_create_group(&bus->dev->kobj, &sdw_master_node_group);
}

void sdw_sysfs_bus_exit(struct sdw_bus *bus)
{
	sysfs_remove_group(&bus->dev->kobj, &sdw_master_node_group);	
}

which gives me a simpler structure and doesn't require additional 
pretend-devices:

/sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# ls
clock_gears
/sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# more clock_gears
8086

The issue I have is that for the _show() functions, I don't see a way to 
go from the device argument to bus. In the example above I forced the 
output but would need a helper.

static ssize_t clock_gears_show(struct device *dev,
				struct device_attribute *attr, char *buf)
{
	struct sdw_bus *bus; // this is what I need to find from dev
	ssize_t size = 0;
	int i;

	return sprintf(buf, "%d \n", 8086);
}

my brain is starting to fry, but I don't see how container_of() would 
work here since the bus structure contains a pointer to the device. I 
don't also see a way to check for all devices for the bus_type soundwire.
For the slaves we do have a macro based on container_of(), so wondering 
if we made a mistake in the bus definition? Vinod, any thoughts?

Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support

[Vinod Koul]

On 06-05-19, 11:46, Pierre-Louis Bossart wrote:
> On 5/6/19 11:22 AM, Vinod Koul wrote:
> > On 06-05-19, 17:19, Greg KH wrote:
> > > On Mon, May 06, 2019 at 09:42:35AM -0500, Pierre-Louis Bossart wrote:
> > > > > > +
> > > > > > +int sdw_sysfs_slave_init(struct sdw_slave *slave)
> > > > > > +{
> > > > > > +	struct sdw_slave_sysfs *sysfs;
> > > > > > +	unsigned int src_dpns, sink_dpns, i, j;
> > > > > > +	int err;
> > > > > > +
> > > > > > +	if (slave->sysfs) {
> > > > > > +		dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
> > > > > > +		err = -EIO;
> > > > > > +		goto err_ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
> > > > > 
> > > > > Same question as patch 1, why a new device?
> > > > 
> > > > yes it's the same open. In this case, the slave devices are defined at a
> > > > different level so it's also confusing to create a device to represent the
> > > > slave properties. The code works but I am not sure the initial directions
> > > > are correct.
> > > 
> > > You can just make a subdir for your attributes by using the attribute
> > > group name, if a subdirectory is needed just to keep things a bit more
> > > organized.
> > 
> > The key here is 'a subdir' which is not the case here. We did discuss
> > this in the initial patches for SoundWire which had sysfs :)
> > 
> > The way MIPI disco spec organized properties, we have dp0 and dpN
> > properties each of them requires to have a subdir of their own and that
> > was the reason why I coded it to be creating a device.
> 
> Vinod, the question was not for dp0 and dpN, it's fine to have
> subdirectories there, but rather why we need separate devices for the master
> and slave properties.

Slave does not have a separate device. IIRC the properties for Slave are
in /sys/bus/soundwire/device/<slave>/...

For master yes we can skip the device creation, it was done for
consistency sake of having these properties ties into sys/bus/soundwire/

I don't mind if they are shown up in respective device node (PCI/platform
etc) /sys/bus/foo/device/<> 

But for creating subdirectories you would need the new dpX devices.

HTH

> 
> > 
> > Do we have a better way to handle this?
> > 
> > > Otherwise, you need to mess with having multiple "types" of struct
> > > device all associated with the same bus.  It is possible, and not that
> > > hard, but I don't think you are doing that here.
> > > 
> > > thnaks,
> > > 
> > > greg k-h
> > 

-- 
~Vinod

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Vinod Koul]

On 06-05-19, 21:24, Pierre-Louis Bossart wrote:
> 
> > > +int sdw_sysfs_bus_init(struct sdw_bus *bus)
> > > +{
> > > +	struct sdw_master_sysfs *master;
> > > +	int err;
> > > +
> > > +	if (bus->sysfs) {
> > > +		dev_err(bus->dev, "SDW sysfs is already initialized\n");
> > > +		return -EIO;
> > > +	}
> > > +
> > > +	master = kzalloc(sizeof(*master), GFP_KERNEL);
> > > +	if (!master)
> > > +		return -ENOMEM;
> > 
> > Why are you creating a whole new device to put all of this under?  Is
> > this needed?  What will the sysfs tree look like when you do this?  Why
> > can't the "bus" device just get all of these attributes and no second
> > device be created?
> 
> I tried a quick hack and indeed we could simplify the code with something as
> simple as:
> 
> [attributes omitted]
> 
> static const struct attribute_group sdw_master_node_group = {
> 	.attrs = master_node_attrs,
> 	.name = "mipi-disco"
> };
> 
> int sdw_sysfs_bus_init(struct sdw_bus *bus)
> {
> 	return sysfs_create_group(&bus->dev->kobj, &sdw_master_node_group);
> }
> 
> void sdw_sysfs_bus_exit(struct sdw_bus *bus)
> {
> 	sysfs_remove_group(&bus->dev->kobj, &sdw_master_node_group);	
> }
> 
> which gives me a simpler structure and doesn't require additional
> pretend-devices:
> 
> /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# ls
> clock_gears
> /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# more clock_gears
> 8086
> 
> The issue I have is that for the _show() functions, I don't see a way to go
> from the device argument to bus. In the example above I forced the output
> but would need a helper.
> 
> static ssize_t clock_gears_show(struct device *dev,
> 				struct device_attribute *attr, char *buf)
> {
> 	struct sdw_bus *bus; // this is what I need to find from dev
> 	ssize_t size = 0;
> 	int i;
> 
> 	return sprintf(buf, "%d \n", 8086);
> }
> 
> my brain is starting to fry, but I don't see how container_of() would work
> here since the bus structure contains a pointer to the device. I don't also
> see a way to check for all devices for the bus_type soundwire.
> For the slaves we do have a macro based on container_of(), so wondering if
> we made a mistake in the bus definition? Vinod, any thoughts?

yeah I dont recall a way to get bus fed into create_group, I did look at
the other examples back then and IIRC and most of them were using a
global to do the trick (I didn't want to go down that route).

I think that was the reason I wrote it this way...

BTW if you do use psedo-device you can create your own struct foo which
embeds device and then then you can use container approach to get foo
(and foo contains bus as a member).

Greg, any thoughts?

Thanks
-- 
~Vinod

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Greg KH]

On Tue, May 07, 2019 at 10:57:32AM +0530, Vinod Koul wrote:
> On 06-05-19, 21:24, Pierre-Louis Bossart wrote:
> > 
> > > > +int sdw_sysfs_bus_init(struct sdw_bus *bus)
> > > > +{
> > > > +	struct sdw_master_sysfs *master;
> > > > +	int err;
> > > > +
> > > > +	if (bus->sysfs) {
> > > > +		dev_err(bus->dev, "SDW sysfs is already initialized\n");
> > > > +		return -EIO;
> > > > +	}
> > > > +
> > > > +	master = kzalloc(sizeof(*master), GFP_KERNEL);
> > > > +	if (!master)
> > > > +		return -ENOMEM;
> > > 
> > > Why are you creating a whole new device to put all of this under?  Is
> > > this needed?  What will the sysfs tree look like when you do this?  Why
> > > can't the "bus" device just get all of these attributes and no second
> > > device be created?
> > 
> > I tried a quick hack and indeed we could simplify the code with something as
> > simple as:
> > 
> > [attributes omitted]
> > 
> > static const struct attribute_group sdw_master_node_group = {
> > 	.attrs = master_node_attrs,
> > 	.name = "mipi-disco"
> > };
> > 
> > int sdw_sysfs_bus_init(struct sdw_bus *bus)
> > {
> > 	return sysfs_create_group(&bus->dev->kobj, &sdw_master_node_group);
> > }
> > 
> > void sdw_sysfs_bus_exit(struct sdw_bus *bus)
> > {
> > 	sysfs_remove_group(&bus->dev->kobj, &sdw_master_node_group);	
> > }
> > 
> > which gives me a simpler structure and doesn't require additional
> > pretend-devices:
> > 
> > /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# ls
> > clock_gears
> > /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# more clock_gears
> > 8086
> > 
> > The issue I have is that for the _show() functions, I don't see a way to go
> > from the device argument to bus. In the example above I forced the output
> > but would need a helper.
> > 
> > static ssize_t clock_gears_show(struct device *dev,
> > 				struct device_attribute *attr, char *buf)
> > {
> > 	struct sdw_bus *bus; // this is what I need to find from dev
> > 	ssize_t size = 0;
> > 	int i;
> > 
> > 	return sprintf(buf, "%d \n", 8086);
> > }
> > 
> > my brain is starting to fry, but I don't see how container_of() would work
> > here since the bus structure contains a pointer to the device. I don't also
> > see a way to check for all devices for the bus_type soundwire.
> > For the slaves we do have a macro based on container_of(), so wondering if
> > we made a mistake in the bus definition? Vinod, any thoughts?
> 
> yeah I dont recall a way to get bus fed into create_group, I did look at
> the other examples back then and IIRC and most of them were using a
> global to do the trick (I didn't want to go down that route).
> 
> I think that was the reason I wrote it this way...
> 
> BTW if you do use psedo-device you can create your own struct foo which
> embeds device and then then you can use container approach to get foo
> (and foo contains bus as a member).
> 
> Greg, any thoughts?

Why would you have "bus" attributes on a device?  I don't think you are
using "bus" here like the driver model uses the term "bus", right?

What are you really trying to show here?

And if you need to know the bus pointer from the device, why don't you
have a pointer to it in your device-specific structure?

thanks,

greg k-h

Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support

[Greg KH]

On Mon, May 06, 2019 at 10:08:10PM +0530, Vinod Koul wrote:
> Yes, but then device exit routine is supposed to do debugfs cleanup as
> well, so that would ensure these references are dropped at that point of
> time. Greg should that not take care of it or we *should* always do
> refcounting.

Always do refcounting.  How else can you "guarantee" that it is safe?

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Vinod Koul]

On 07-05-19, 07:54, Greg KH wrote:
> On Tue, May 07, 2019 at 10:57:32AM +0530, Vinod Koul wrote:
> > On 06-05-19, 21:24, Pierre-Louis Bossart wrote:
> > > 
> > > > > +int sdw_sysfs_bus_init(struct sdw_bus *bus)
> > > > > +{
> > > > > +	struct sdw_master_sysfs *master;
> > > > > +	int err;
> > > > > +
> > > > > +	if (bus->sysfs) {
> > > > > +		dev_err(bus->dev, "SDW sysfs is already initialized\n");
> > > > > +		return -EIO;
> > > > > +	}
> > > > > +
> > > > > +	master = kzalloc(sizeof(*master), GFP_KERNEL);
> > > > > +	if (!master)
> > > > > +		return -ENOMEM;
> > > > 
> > > > Why are you creating a whole new device to put all of this under?  Is
> > > > this needed?  What will the sysfs tree look like when you do this?  Why
> > > > can't the "bus" device just get all of these attributes and no second
> > > > device be created?
> > > 
> > > I tried a quick hack and indeed we could simplify the code with something as
> > > simple as:
> > > 
> > > [attributes omitted]
> > > 
> > > static const struct attribute_group sdw_master_node_group = {
> > > 	.attrs = master_node_attrs,
> > > 	.name = "mipi-disco"
> > > };
> > > 
> > > int sdw_sysfs_bus_init(struct sdw_bus *bus)
> > > {
> > > 	return sysfs_create_group(&bus->dev->kobj, &sdw_master_node_group);
> > > }
> > > 
> > > void sdw_sysfs_bus_exit(struct sdw_bus *bus)
> > > {
> > > 	sysfs_remove_group(&bus->dev->kobj, &sdw_master_node_group);	
> > > }
> > > 
> > > which gives me a simpler structure and doesn't require additional
> > > pretend-devices:
> > > 
> > > /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# ls
> > > clock_gears
> > > /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# more clock_gears
> > > 8086
> > > 
> > > The issue I have is that for the _show() functions, I don't see a way to go
> > > from the device argument to bus. In the example above I forced the output
> > > but would need a helper.
> > > 
> > > static ssize_t clock_gears_show(struct device *dev,
> > > 				struct device_attribute *attr, char *buf)
> > > {
> > > 	struct sdw_bus *bus; // this is what I need to find from dev
> > > 	ssize_t size = 0;
> > > 	int i;
> > > 
> > > 	return sprintf(buf, "%d \n", 8086);
> > > }
> > > 
> > > my brain is starting to fry, but I don't see how container_of() would work
> > > here since the bus structure contains a pointer to the device. I don't also
> > > see a way to check for all devices for the bus_type soundwire.
> > > For the slaves we do have a macro based on container_of(), so wondering if
> > > we made a mistake in the bus definition? Vinod, any thoughts?
> > 
> > yeah I dont recall a way to get bus fed into create_group, I did look at
> > the other examples back then and IIRC and most of them were using a
> > global to do the trick (I didn't want to go down that route).
> > 
> > I think that was the reason I wrote it this way...
> > 
> > BTW if you do use psedo-device you can create your own struct foo which
> > embeds device and then then you can use container approach to get foo
> > (and foo contains bus as a member).
> > 
> > Greg, any thoughts?
> 
> Why would you have "bus" attributes on a device?  I don't think you are
> using "bus" here like the driver model uses the term "bus", right?
> 
> What are you really trying to show here?
> 
> And if you need to know the bus pointer from the device, why don't you
> have a pointer to it in your device-specific structure?

The model here is that Master device is PCI or Platform device and then
creates a bus instance which has soundwire slave devices.

So for any attribute on Master device (which has properties as well and
representation in sysfs), device specfic struct (PCI/platfrom doesn't
help). For slave that is not a problem as sdw_slave structure takes care
if that.

So, the solution was to create the psedo sdw_master device for the
representation and have device-specific structure.

Thanks
-- 
~Vinod

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Greg KH]

On Tue, May 07, 2019 at 04:33:31PM +0530, Vinod Koul wrote:
> On 07-05-19, 07:54, Greg KH wrote:
> > On Tue, May 07, 2019 at 10:57:32AM +0530, Vinod Koul wrote:
> > > On 06-05-19, 21:24, Pierre-Louis Bossart wrote:
> > > > 
> > > > > > +int sdw_sysfs_bus_init(struct sdw_bus *bus)
> > > > > > +{
> > > > > > +	struct sdw_master_sysfs *master;
> > > > > > +	int err;
> > > > > > +
> > > > > > +	if (bus->sysfs) {
> > > > > > +		dev_err(bus->dev, "SDW sysfs is already initialized\n");
> > > > > > +		return -EIO;
> > > > > > +	}
> > > > > > +
> > > > > > +	master = kzalloc(sizeof(*master), GFP_KERNEL);
> > > > > > +	if (!master)
> > > > > > +		return -ENOMEM;
> > > > > 
> > > > > Why are you creating a whole new device to put all of this under?  Is
> > > > > this needed?  What will the sysfs tree look like when you do this?  Why
> > > > > can't the "bus" device just get all of these attributes and no second
> > > > > device be created?
> > > > 
> > > > I tried a quick hack and indeed we could simplify the code with something as
> > > > simple as:
> > > > 
> > > > [attributes omitted]
> > > > 
> > > > static const struct attribute_group sdw_master_node_group = {
> > > > 	.attrs = master_node_attrs,
> > > > 	.name = "mipi-disco"
> > > > };
> > > > 
> > > > int sdw_sysfs_bus_init(struct sdw_bus *bus)
> > > > {
> > > > 	return sysfs_create_group(&bus->dev->kobj, &sdw_master_node_group);
> > > > }
> > > > 
> > > > void sdw_sysfs_bus_exit(struct sdw_bus *bus)
> > > > {
> > > > 	sysfs_remove_group(&bus->dev->kobj, &sdw_master_node_group);	
> > > > }
> > > > 
> > > > which gives me a simpler structure and doesn't require additional
> > > > pretend-devices:
> > > > 
> > > > /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# ls
> > > > clock_gears
> > > > /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# more clock_gears
> > > > 8086
> > > > 
> > > > The issue I have is that for the _show() functions, I don't see a way to go
> > > > from the device argument to bus. In the example above I forced the output
> > > > but would need a helper.
> > > > 
> > > > static ssize_t clock_gears_show(struct device *dev,
> > > > 				struct device_attribute *attr, char *buf)
> > > > {
> > > > 	struct sdw_bus *bus; // this is what I need to find from dev
> > > > 	ssize_t size = 0;
> > > > 	int i;
> > > > 
> > > > 	return sprintf(buf, "%d \n", 8086);
> > > > }
> > > > 
> > > > my brain is starting to fry, but I don't see how container_of() would work
> > > > here since the bus structure contains a pointer to the device. I don't also
> > > > see a way to check for all devices for the bus_type soundwire.
> > > > For the slaves we do have a macro based on container_of(), so wondering if
> > > > we made a mistake in the bus definition? Vinod, any thoughts?
> > > 
> > > yeah I dont recall a way to get bus fed into create_group, I did look at
> > > the other examples back then and IIRC and most of them were using a
> > > global to do the trick (I didn't want to go down that route).
> > > 
> > > I think that was the reason I wrote it this way...
> > > 
> > > BTW if you do use psedo-device you can create your own struct foo which
> > > embeds device and then then you can use container approach to get foo
> > > (and foo contains bus as a member).
> > > 
> > > Greg, any thoughts?
> > 
> > Why would you have "bus" attributes on a device?  I don't think you are
> > using "bus" here like the driver model uses the term "bus", right?
> > 
> > What are you really trying to show here?
> > 
> > And if you need to know the bus pointer from the device, why don't you
> > have a pointer to it in your device-specific structure?
> 
> The model here is that Master device is PCI or Platform device and then
> creates a bus instance which has soundwire slave devices.
> 
> So for any attribute on Master device (which has properties as well and
> representation in sysfs), device specfic struct (PCI/platfrom doesn't
> help). For slave that is not a problem as sdw_slave structure takes care
> if that.
> 
> So, the solution was to create the psedo sdw_master device for the
> representation and have device-specific structure.

Ok, much like the "USB host controller" type device.  That's fine, make
such a device, add it to your bus, and set the type correctly.  And keep
a pointer to that structure in your device-specific structure if you
really need to get to anything in it.

thanks,

greg k-h

Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support

[Pierre-Louis Bossart]

On 5/7/19 12:19 AM, Vinod Koul wrote:
> On 06-05-19, 11:46, Pierre-Louis Bossart wrote:
>> On 5/6/19 11:22 AM, Vinod Koul wrote:
>>> On 06-05-19, 17:19, Greg KH wrote:
>>>> On Mon, May 06, 2019 at 09:42:35AM -0500, Pierre-Louis Bossart wrote:
>>>>>>> +
>>>>>>> +int sdw_sysfs_slave_init(struct sdw_slave *slave)
>>>>>>> +{
>>>>>>> +	struct sdw_slave_sysfs *sysfs;
>>>>>>> +	unsigned int src_dpns, sink_dpns, i, j;
>>>>>>> +	int err;
>>>>>>> +
>>>>>>> +	if (slave->sysfs) {
>>>>>>> +		dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
>>>>>>> +		err = -EIO;
>>>>>>> +		goto err_ret;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
>>>>>>
>>>>>> Same question as patch 1, why a new device?
>>>>>
>>>>> yes it's the same open. In this case, the slave devices are defined at a
>>>>> different level so it's also confusing to create a device to represent the
>>>>> slave properties. The code works but I am not sure the initial directions
>>>>> are correct.
>>>>
>>>> You can just make a subdir for your attributes by using the attribute
>>>> group name, if a subdirectory is needed just to keep things a bit more
>>>> organized.
>>>
>>> The key here is 'a subdir' which is not the case here. We did discuss
>>> this in the initial patches for SoundWire which had sysfs :)
>>>
>>> The way MIPI disco spec organized properties, we have dp0 and dpN
>>> properties each of them requires to have a subdir of their own and that
>>> was the reason why I coded it to be creating a device.
>>
>> Vinod, the question was not for dp0 and dpN, it's fine to have
>> subdirectories there, but rather why we need separate devices for the master
>> and slave properties.
> 
> Slave does not have a separate device. IIRC the properties for Slave are
> in /sys/bus/soundwire/device/<slave>/...

I am not sure this is correct

ACPI defines the slaves devices under
/sys/bus/acpi/PRP0001, e.g.

/sys/bus/acpi/devices/PRP00001:00/device:17# ls
adr                                 mipi-sdw-dp-5-sink-subproperties
intel-endpoint-descriptor-0         mipi-sdw-dp-6-source-subproperties
intel-endpoint-descriptor-1         mipi-sdw-dp-7-sink-subproperties
mipi-sdw-dp-0-subproperties         mipi-sdw-dp-8-source-subproperties
mipi-sdw-dp-1-sink-subproperties    path
mipi-sdw-dp-1-source-subproperties  physical_node
mipi-sdw-dp-2-sink-subproperties    power
mipi-sdw-dp-2-source-subproperties  subsystem
mipi-sdw-dp-3-sink-subproperties    uevent
mipi-sdw-dp-4-source-subproperties

but the sysfs for slaves is shown as
/sys/bus/acpi/devices/PRP00001:00/int-sdw.0/sdw:0:25d:700:0:0# ls
bank_delay_support  master_count             sink_ports
ch_prep_timeout     mipi_revision            source_ports
clk_stop_mode1      modalias                 src-dp2
clk_stop_timeout    p15_behave               src-dp4
dp0                 paging_support           subsystem
driver              power                    test_mode_capable
firmware_node       reset_behave             uevent
hda_reg             simple_clk_stop_capable  wake_capable
high_PHY_capable    sink-dp1
index_reg           sink-dp3

and in sys/bus/soundwire/devices/sdw:0:25d:700:0:0# ls
bank_delay_support  master_count             sink_ports
ch_prep_timeout     mipi_revision            source_ports
clk_stop_mode1      modalias                 src-dp2
clk_stop_timeout    p15_behave               src-dp4
dp0                 paging_support           subsystem
driver              power                    test_mode_capable
firmware_node       reset_behave             uevent
hda_reg             simple_clk_stop_capable  wake_capable
high_PHY_capable    sink-dp1
index_reg           sink-dp3

So I would think we *do* create a new device for each slave instead of 
using the one that's already exposed by ACPI.

> 
> For master yes we can skip the device creation, it was done for
> consistency sake of having these properties ties into sys/bus/soundwire/
> 
> I don't mind if they are shown up in respective device node (PCI/platform
> etc) /sys/bus/foo/device/<>
> 
> But for creating subdirectories you would need the new dpX devices.

yes, that's agreed.

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Pierre-Louis Bossart]

>> The model here is that Master device is PCI or Platform device and then
>> creates a bus instance which has soundwire slave devices.
>>
>> So for any attribute on Master device (which has properties as well and
>> representation in sysfs), device specfic struct (PCI/platfrom doesn't
>> help). For slave that is not a problem as sdw_slave structure takes care
>> if that.
>>
>> So, the solution was to create the psedo sdw_master device for the
>> representation and have device-specific structure.
> 
> Ok, much like the "USB host controller" type device.  That's fine, make
> such a device, add it to your bus, and set the type correctly.  And keep
> a pointer to that structure in your device-specific structure if you
> really need to get to anything in it.

humm, you lost me on the last sentence. Did you mean using 
set_drv/platform_data during the init and retrieving the bus information 
with get_drv/platform_data as needed later? Or something else I badly 
need to learn?

Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support

[Vinod Koul]

On 07-05-19, 08:54, Pierre-Louis Bossart wrote:
> On 5/7/19 12:19 AM, Vinod Koul wrote:
> > On 06-05-19, 11:46, Pierre-Louis Bossart wrote:
> > > On 5/6/19 11:22 AM, Vinod Koul wrote:
> > > > On 06-05-19, 17:19, Greg KH wrote:
> > > > > On Mon, May 06, 2019 at 09:42:35AM -0500, Pierre-Louis Bossart wrote:
> > > > > > > > +
> > > > > > > > +int sdw_sysfs_slave_init(struct sdw_slave *slave)
> > > > > > > > +{
> > > > > > > > +	struct sdw_slave_sysfs *sysfs;
> > > > > > > > +	unsigned int src_dpns, sink_dpns, i, j;
> > > > > > > > +	int err;
> > > > > > > > +
> > > > > > > > +	if (slave->sysfs) {
> > > > > > > > +		dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
> > > > > > > > +		err = -EIO;
> > > > > > > > +		goto err_ret;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
> > > > > > > 
> > > > > > > Same question as patch 1, why a new device?
> > > > > > 
> > > > > > yes it's the same open. In this case, the slave devices are defined at a
> > > > > > different level so it's also confusing to create a device to represent the
> > > > > > slave properties. The code works but I am not sure the initial directions
> > > > > > are correct.
> > > > > 
> > > > > You can just make a subdir for your attributes by using the attribute
> > > > > group name, if a subdirectory is needed just to keep things a bit more
> > > > > organized.
> > > > 
> > > > The key here is 'a subdir' which is not the case here. We did discuss
> > > > this in the initial patches for SoundWire which had sysfs :)
> > > > 
> > > > The way MIPI disco spec organized properties, we have dp0 and dpN
> > > > properties each of them requires to have a subdir of their own and that
> > > > was the reason why I coded it to be creating a device.
> > > 
> > > Vinod, the question was not for dp0 and dpN, it's fine to have
> > > subdirectories there, but rather why we need separate devices for the master
> > > and slave properties.
> > 
> > Slave does not have a separate device. IIRC the properties for Slave are
> > in /sys/bus/soundwire/device/<slave>/...
> 
> I am not sure this is correct
> 
> ACPI defines the slaves devices under
> /sys/bus/acpi/PRP0001, e.g.

Yes the bus will create 'soundwire slave' device type (In acpi case
created from ACPI walk) and we do link the ACPI as the firmware node.
This is 'not' created for properties but for soundwire representation of
slave devices. This is the one code driver attaches to.
 
> /sys/bus/acpi/devices/PRP00001:00/device:17# ls

Yes this would the companion ACPI device

> adr                                 mipi-sdw-dp-5-sink-subproperties
> intel-endpoint-descriptor-0         mipi-sdw-dp-6-source-subproperties
> intel-endpoint-descriptor-1         mipi-sdw-dp-7-sink-subproperties
> mipi-sdw-dp-0-subproperties         mipi-sdw-dp-8-source-subproperties
> mipi-sdw-dp-1-sink-subproperties    path
> mipi-sdw-dp-1-source-subproperties  physical_node
> mipi-sdw-dp-2-sink-subproperties    power
> mipi-sdw-dp-2-source-subproperties  subsystem
> mipi-sdw-dp-3-sink-subproperties    uevent
> mipi-sdw-dp-4-source-subproperties
> 
> but the sysfs for slaves is shown as
> /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/sdw:0:25d:700:0:0# ls
> bank_delay_support  master_count             sink_ports
> ch_prep_timeout     mipi_revision            source_ports
> clk_stop_mode1      modalias                 src-dp2
> clk_stop_timeout    p15_behave               src-dp4
> dp0                 paging_support           subsystem
> driver              power                    test_mode_capable
> firmware_node       reset_behave             uevent
> hda_reg             simple_clk_stop_capable  wake_capable
> high_PHY_capable    sink-dp1
> index_reg           sink-dp3
> 
> and in sys/bus/soundwire/devices/sdw:0:25d:700:0:0# ls

I think both are same nodes. Since the SoundWire slave is a child of
master it appears under int-sdw.0 as well

> bank_delay_support  master_count             sink_ports
> ch_prep_timeout     mipi_revision            source_ports
> clk_stop_mode1      modalias                 src-dp2
> clk_stop_timeout    p15_behave               src-dp4
> dp0                 paging_support           subsystem
> driver              power                    test_mode_capable
> firmware_node       reset_behave             uevent
> hda_reg             simple_clk_stop_capable  wake_capable
> high_PHY_capable    sink-dp1
> index_reg           sink-dp3
> 
> So I would think we *do* create a new device for each slave instead of using
> the one that's already exposed by ACPI.
> 
> > 
> > For master yes we can skip the device creation, it was done for
> > consistency sake of having these properties ties into sys/bus/soundwire/
> > 
> > I don't mind if they are shown up in respective device node (PCI/platform
> > etc) /sys/bus/foo/device/<>
> > 
> > But for creating subdirectories you would need the new dpX devices.
> 
> yes, that's agreed.

-- 
~Vinod

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Vinod Koul]

On 07-05-19, 17:49, Pierre-Louis Bossart wrote:
> 
> > > The model here is that Master device is PCI or Platform device and then
> > > creates a bus instance which has soundwire slave devices.
> > > 
> > > So for any attribute on Master device (which has properties as well and
> > > representation in sysfs), device specfic struct (PCI/platfrom doesn't
> > > help). For slave that is not a problem as sdw_slave structure takes care
> > > if that.
> > > 
> > > So, the solution was to create the psedo sdw_master device for the
> > > representation and have device-specific structure.
> > 
> > Ok, much like the "USB host controller" type device.  That's fine, make
> > such a device, add it to your bus, and set the type correctly.  And keep
> > a pointer to that structure in your device-specific structure if you
> > really need to get to anything in it.
> 
> humm, you lost me on the last sentence. Did you mean using
> set_drv/platform_data during the init and retrieving the bus information
> with get_drv/platform_data as needed later? Or something else I badly need
> to learn?

IIUC Greg meant we should represent a soundwire master device type and
use that here. Just like we have soundwire slave device type. Something
like:

struct sdw_master {
        struct device dev;
        struct sdw_master_prop *prop;
        ...
};

In show function you get master from dev (container of) and then use
that to access the master properties. So int.sdw.0 can be of this type.

Thanks
-- 
~Vinod

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Greg KH]

On Wed, May 08, 2019 at 01:16:06PM +0530, Vinod Koul wrote:
> On 07-05-19, 17:49, Pierre-Louis Bossart wrote:
> > 
> > > > The model here is that Master device is PCI or Platform device and then
> > > > creates a bus instance which has soundwire slave devices.
> > > > 
> > > > So for any attribute on Master device (which has properties as well and
> > > > representation in sysfs), device specfic struct (PCI/platfrom doesn't
> > > > help). For slave that is not a problem as sdw_slave structure takes care
> > > > if that.
> > > > 
> > > > So, the solution was to create the psedo sdw_master device for the
> > > > representation and have device-specific structure.
> > > 
> > > Ok, much like the "USB host controller" type device.  That's fine, make
> > > such a device, add it to your bus, and set the type correctly.  And keep
> > > a pointer to that structure in your device-specific structure if you
> > > really need to get to anything in it.
> > 
> > humm, you lost me on the last sentence. Did you mean using
> > set_drv/platform_data during the init and retrieving the bus information
> > with get_drv/platform_data as needed later? Or something else I badly need
> > to learn?
> 
> IIUC Greg meant we should represent a soundwire master device type and
> use that here. Just like we have soundwire slave device type. Something
> like:
> 
> struct sdw_master {
>         struct device dev;
>         struct sdw_master_prop *prop;
>         ...
> };
> 
> In show function you get master from dev (container of) and then use
> that to access the master properties. So int.sdw.0 can be of this type.

Yes, you need to represent the master device type if you are going to be
having an internal representation of it.

thanks,

greg k-h

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Pierre-Louis Bossart]

On 5/8/19 4:16 AM, Greg KH wrote:
> On Wed, May 08, 2019 at 01:16:06PM +0530, Vinod Koul wrote:
>> On 07-05-19, 17:49, Pierre-Louis Bossart wrote:
>>>
>>>>> The model here is that Master device is PCI or Platform device and then
>>>>> creates a bus instance which has soundwire slave devices.
>>>>>
>>>>> So for any attribute on Master device (which has properties as well and
>>>>> representation in sysfs), device specfic struct (PCI/platfrom doesn't
>>>>> help). For slave that is not a problem as sdw_slave structure takes care
>>>>> if that.
>>>>>
>>>>> So, the solution was to create the psedo sdw_master device for the
>>>>> representation and have device-specific structure.
>>>>
>>>> Ok, much like the "USB host controller" type device.  That's fine, make
>>>> such a device, add it to your bus, and set the type correctly.  And keep
>>>> a pointer to that structure in your device-specific structure if you
>>>> really need to get to anything in it.
>>>
>>> humm, you lost me on the last sentence. Did you mean using
>>> set_drv/platform_data during the init and retrieving the bus information
>>> with get_drv/platform_data as needed later? Or something else I badly need
>>> to learn?
>>
>> IIUC Greg meant we should represent a soundwire master device type and
>> use that here. Just like we have soundwire slave device type. Something
>> like:
>>
>> struct sdw_master {
>>          struct device dev;
>>          struct sdw_master_prop *prop;
>>          ...
>> };
>>
>> In show function you get master from dev (container of) and then use
>> that to access the master properties. So int.sdw.0 can be of this type.
> 
> Yes, you need to represent the master device type if you are going to be
> having an internal representation of it.

Humm, confused...In the existing code bus and master are synonyms, see 
e.g. following code excerpts:

  * sdw_add_bus_master() - add a bus Master instance
  * @bus: bus instance
  *
  * Initializes the bus instance, read properties and create child
  * devices.

struct sdw_bus {
	struct device *dev; <<< pointer here
	unsigned int link_id;
	struct list_head slaves;
	DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
	struct mutex bus_lock;
	struct mutex msg_lock;
	const struct sdw_master_ops *ops;
	const struct sdw_master_port_ops *port_ops;
	struct sdw_bus_params params;
	struct sdw_master_prop prop;

The existing code creates a platform_device in 
drivers/soundwire/intel_init.c, and it's assigned by the following code:

static int intel_probe(struct platform_device *pdev)
{
	struct sdw_cdns_stream_config config;
	struct sdw_intel *sdw;
	int ret;

	sdw = devm_kzalloc(&pdev->dev, sizeof(*sdw), GFP_KERNEL);
[snip]
	sdw->cdns.dev = &pdev->dev;
	sdw->cdns.bus.dev = &pdev->dev;

I really don't see what you are hinting at, sorry, unless we are talking 
about major surgery in the code.

Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support

[Pierre-Louis Bossart]

>>>> Vinod, the question was not for dp0 and dpN, it's fine to have
>>>> subdirectories there, but rather why we need separate devices for the master
>>>> and slave properties.
>>>
>>> Slave does not have a separate device. IIRC the properties for Slave are
>>> in /sys/bus/soundwire/device/<slave>/...
>>
>> I am not sure this is correct
>>
>> ACPI defines the slaves devices under
>> /sys/bus/acpi/PRP0001, e.g.
> 
> Yes the bus will create 'soundwire slave' device type (In acpi case
> created from ACPI walk) and we do link the ACPI as the firmware node.
> This is 'not' created for properties but for soundwire representation of
> slave devices. This is the one code driver attaches to.
>   
>> /sys/bus/acpi/devices/PRP00001:00/device:17# ls
> 
> Yes this would the companion ACPI device

I see, I must admit I missed this part.

I guess it's not technically broken but was is really necessary though 
to use this notion of companion ACPI device? For the controller it makes 
sense, that's how to match ACPI and PCI, but since Soundwire slaves are 
not fully enumerable, precisely why we need all these _DSD properties, 
couldn't we just use ACPI devices directly?

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Greg KH]

On Wed, May 08, 2019 at 11:42:15AM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 5/8/19 4:16 AM, Greg KH wrote:
> > On Wed, May 08, 2019 at 01:16:06PM +0530, Vinod Koul wrote:
> > > On 07-05-19, 17:49, Pierre-Louis Bossart wrote:
> > > > 
> > > > > > The model here is that Master device is PCI or Platform device and then
> > > > > > creates a bus instance which has soundwire slave devices.
> > > > > > 
> > > > > > So for any attribute on Master device (which has properties as well and
> > > > > > representation in sysfs), device specfic struct (PCI/platfrom doesn't
> > > > > > help). For slave that is not a problem as sdw_slave structure takes care
> > > > > > if that.
> > > > > > 
> > > > > > So, the solution was to create the psedo sdw_master device for the
> > > > > > representation and have device-specific structure.
> > > > > 
> > > > > Ok, much like the "USB host controller" type device.  That's fine, make
> > > > > such a device, add it to your bus, and set the type correctly.  And keep
> > > > > a pointer to that structure in your device-specific structure if you
> > > > > really need to get to anything in it.
> > > > 
> > > > humm, you lost me on the last sentence. Did you mean using
> > > > set_drv/platform_data during the init and retrieving the bus information
> > > > with get_drv/platform_data as needed later? Or something else I badly need
> > > > to learn?
> > > 
> > > IIUC Greg meant we should represent a soundwire master device type and
> > > use that here. Just like we have soundwire slave device type. Something
> > > like:
> > > 
> > > struct sdw_master {
> > >          struct device dev;
> > >          struct sdw_master_prop *prop;
> > >          ...
> > > };
> > > 
> > > In show function you get master from dev (container of) and then use
> > > that to access the master properties. So int.sdw.0 can be of this type.
> > 
> > Yes, you need to represent the master device type if you are going to be
> > having an internal representation of it.
> 
> Humm, confused...In the existing code bus and master are synonyms, see e.g.
> following code excerpts:
> 
>  * sdw_add_bus_master() - add a bus Master instance
>  * @bus: bus instance
>  *
>  * Initializes the bus instance, read properties and create child
>  * devices.
> 
> struct sdw_bus {
> 	struct device *dev; <<< pointer here

That's the pointer to what?  The device that the bus is "attached to"
(i.e. parent, like a platform device or a pci device)?

Why isn't this a "real" device in itself?

I thought I asked that a long time ago when first reviewing these
patches...

> 	unsigned int link_id;
> 	struct list_head slaves;
> 	DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
> 	struct mutex bus_lock;
> 	struct mutex msg_lock;
> 	const struct sdw_master_ops *ops;
> 	const struct sdw_master_port_ops *port_ops;
> 	struct sdw_bus_params params;
> 	struct sdw_master_prop prop;
> 
> The existing code creates a platform_device in
> drivers/soundwire/intel_init.c, and it's assigned by the following code:

The core creates a platform device, don't assume you can "take it over"
:)

That platform device lives on the platform bus, you need a "master"
device that lives on your soundbus bus.

Again, look at how USB does this.  Or better yet, greybus, as that code
is a lot smaller and simpler.

> 
> static int intel_probe(struct platform_device *pdev)
> {
> 	struct sdw_cdns_stream_config config;
> 	struct sdw_intel *sdw;
> 	int ret;
> 
> 	sdw = devm_kzalloc(&pdev->dev, sizeof(*sdw), GFP_KERNEL);
> [snip]
> 	sdw->cdns.dev = &pdev->dev;
> 	sdw->cdns.bus.dev = &pdev->dev;

Gotta love the lack of reference counting :(

> I really don't see what you are hinting at, sorry, unless we are talking
> about major surgery in the code.

It sounds like you need a device on your bus that represents the master,
as you have attributes associated with it, and other things.  You can't
put attributes on a random pci or platform device, as you do not "own"
that device.

does that help?

greg k-h

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Vinod Koul]

On 08-05-19, 15:57, Pierre-Louis Bossart wrote:
> 
> 
> On 5/8/19 11:59 AM, Greg KH wrote:
> > On Wed, May 08, 2019 at 11:42:15AM -0500, Pierre-Louis Bossart wrote:
> > > 
> > > 
> > > On 5/8/19 4:16 AM, Greg KH wrote:
> > > > On Wed, May 08, 2019 at 01:16:06PM +0530, Vinod Koul wrote:
> > > > > On 07-05-19, 17:49, Pierre-Louis Bossart wrote:
> > > > > > 
> > > > > > > > The model here is that Master device is PCI or Platform device and then
> > > > > > > > creates a bus instance which has soundwire slave devices.
> > > > > > > > 
> > > > > > > > So for any attribute on Master device (which has properties as well and
> > > > > > > > representation in sysfs), device specfic struct (PCI/platfrom doesn't
> > > > > > > > help). For slave that is not a problem as sdw_slave structure takes care
> > > > > > > > if that.
> > > > > > > > 
> > > > > > > > So, the solution was to create the psedo sdw_master device for the
> > > > > > > > representation and have device-specific structure.
> > > > > > > 
> > > > > > > Ok, much like the "USB host controller" type device.  That's fine, make
> > > > > > > such a device, add it to your bus, and set the type correctly.  And keep
> > > > > > > a pointer to that structure in your device-specific structure if you
> > > > > > > really need to get to anything in it.
> > > > > > 
> > > > > > humm, you lost me on the last sentence. Did you mean using
> > > > > > set_drv/platform_data during the init and retrieving the bus information
> > > > > > with get_drv/platform_data as needed later? Or something else I badly need
> > > > > > to learn?
> > > > > 
> > > > > IIUC Greg meant we should represent a soundwire master device type and
> > > > > use that here. Just like we have soundwire slave device type. Something
> > > > > like:
> > > > > 
> > > > > struct sdw_master {
> > > > >           struct device dev;
> > > > >           struct sdw_master_prop *prop;
> > > > >           ...
> > > > > };
> > > > > 
> > > > > In show function you get master from dev (container of) and then use
> > > > > that to access the master properties. So int.sdw.0 can be of this type.
> > > > 
> > > > Yes, you need to represent the master device type if you are going to be
> > > > having an internal representation of it.
> > > 
> > > Humm, confused...In the existing code bus and master are synonyms, see e.g.
> > > following code excerpts:
> > > 
> > >   * sdw_add_bus_master() - add a bus Master instance
> > >   * @bus: bus instance
> > >   *
> > >   * Initializes the bus instance, read properties and create child
> > >   * devices.
> > > 
> > > struct sdw_bus {
> > > 	struct device *dev; <<< pointer here
> > 
> > That's the pointer to what?  The device that the bus is "attached to"
> > (i.e. parent, like a platform device or a pci device)?
> > 
> > Why isn't this a "real" device in itself?

Correct, I am revisiting this and I think I have a fair idea of
expectations here (looking at usb and greybus model), will hack
something up

> Allow me to provide a bit of background. I am not trying to be pedantic but
> make sure we are on the same page.
> 
> The SoundWire spec only defines a Master and Slaves attached to that Master.
> 
> In real applications, there is a need to have multiple links, which can
> possibly operate in synchronized ways, so Intel came up with the concept of
> Controller, which expose multiple Master interfaces that are in sync (two
> streams can start at exactly the same clock edge of different links).
> 
> The Controller is exposed in ACPI as a child of the HDAudio controller (ACPI
> companion of a PCI device). The controller exposes a 'master-count' and a
> set of link-specific properties needed for bandwidth/clock scaling.
> 
> For some reason, our Windows friends did not want to have a device for each
> Master interface, likely because they did not want to load a driver per
> Master interface or have 'yellow bangs'.
> 
> So the net result is that we have the following hierarchy in ACPI
> 
> Device(HDAS) // HDaudio controller
>   Device(SNDW) // SoundWire Controller
>     Device(SDW0) { // Slave0
> 	_ADR(link0, vendorX, partY...)
>     }
>     Device(SDW1) { // Slave0
> 	_ADR(link0, vendorX, partY...)
>     }
>     Device(SDW2) { // Slave0
> 	_ADR(link1, vendorX, partY...)
>     }
>     Device(SDWM) { // Slave0
> 	_ADR(linkM, vendorX, partY...)
>     }
> 
> There is no master device represented in ACPI and the only way by which we
> know to which Master a Slave device is attached by looking up the _ADR which
> contains the link information.
> 
> So, coming back to the plot, when we parse the Controller properties, we
> find out how many Master interfaces we have, create a platform_device for
> each of them, then initialize all the bus stuff.

So the idea here would be to go back and create a sdw_master device and
use that in the bus instance. I think it should be doable..

> > I thought I asked that a long time ago when first reviewing these
> > patches...

Sorry my fault, I should have fixed it back then.

> > 
> > > 	unsigned int link_id;
> > > 	struct list_head slaves;
> > > 	DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
> > > 	struct mutex bus_lock;
> > > 	struct mutex msg_lock;
> > > 	const struct sdw_master_ops *ops;
> > > 	const struct sdw_master_port_ops *port_ops;
> > > 	struct sdw_bus_params params;
> > > 	struct sdw_master_prop prop;
> > > 
> > > The existing code creates a platform_device in
> > > drivers/soundwire/intel_init.c, and it's assigned by the following code:
> > 
> > The core creates a platform device, don't assume you can "take it over"
> > :)
> > 
> > That platform device lives on the platform bus, you need a "master"
> > device that lives on your soundbus bus.
> > 
> > Again, look at how USB does this.  Or better yet, greybus, as that code
> > is a lot smaller and simpler.
> 
> The learning curve is not small here...
> 
> > > 
> > > static int intel_probe(struct platform_device *pdev)
> > > {
> > > 	struct sdw_cdns_stream_config config;
> > > 	struct sdw_intel *sdw;
> > > 	int ret;
> > > 
> > > 	sdw = devm_kzalloc(&pdev->dev, sizeof(*sdw), GFP_KERNEL);
> > > [snip]
> > > 	sdw->cdns.dev = &pdev->dev;
> > > 	sdw->cdns.bus.dev = &pdev->dev;
> > 
> > Gotta love the lack of reference counting :(
> > 
> > > I really don't see what you are hinting at, sorry, unless we are talking
> > > about major surgery in the code.

Not really we have object here which should contain a real device for
master and need plumbing for it..

> > It sounds like you need a device on your bus that represents the master,
> > as you have attributes associated with it, and other things.  You can't
> > put attributes on a random pci or platform device, as you do not "own"
> > that device.
> > 
> > does that help?
> 
> Looks like we are doing things wrong at multiple levels.
> 
> It might be better to have a more 'self-contained' solution where the bus
> initialization creates/registers a master device instead of having this
> proxy platform_device. That would avoid all these refcount issues and make
> the translation from device to bus straightforward.

yes that is my thinking as well. We still need to link to
platform/pci/whatever device you have and grab a refcount to that one.

> Am I on the right track or still in the weeds?


-- 
~Vinod

Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Greg KH]

On Wed, May 08, 2019 at 03:57:49PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 5/8/19 11:59 AM, Greg KH wrote:
> > On Wed, May 08, 2019 at 11:42:15AM -0500, Pierre-Louis Bossart wrote:
> > > 
> > > 
> > > On 5/8/19 4:16 AM, Greg KH wrote:
> > > > On Wed, May 08, 2019 at 01:16:06PM +0530, Vinod Koul wrote:
> > > > > On 07-05-19, 17:49, Pierre-Louis Bossart wrote:
> > > > > > 
> > > > > > > > The model here is that Master device is PCI or Platform device and then
> > > > > > > > creates a bus instance which has soundwire slave devices.
> > > > > > > > 
> > > > > > > > So for any attribute on Master device (which has properties as well and
> > > > > > > > representation in sysfs), device specfic struct (PCI/platfrom doesn't
> > > > > > > > help). For slave that is not a problem as sdw_slave structure takes care
> > > > > > > > if that.
> > > > > > > > 
> > > > > > > > So, the solution was to create the psedo sdw_master device for the
> > > > > > > > representation and have device-specific structure.
> > > > > > > 
> > > > > > > Ok, much like the "USB host controller" type device.  That's fine, make
> > > > > > > such a device, add it to your bus, and set the type correctly.  And keep
> > > > > > > a pointer to that structure in your device-specific structure if you
> > > > > > > really need to get to anything in it.
> > > > > > 
> > > > > > humm, you lost me on the last sentence. Did you mean using
> > > > > > set_drv/platform_data during the init and retrieving the bus information
> > > > > > with get_drv/platform_data as needed later? Or something else I badly need
> > > > > > to learn?
> > > > > 
> > > > > IIUC Greg meant we should represent a soundwire master device type and
> > > > > use that here. Just like we have soundwire slave device type. Something
> > > > > like:
> > > > > 
> > > > > struct sdw_master {
> > > > >           struct device dev;
> > > > >           struct sdw_master_prop *prop;
> > > > >           ...
> > > > > };
> > > > > 
> > > > > In show function you get master from dev (container of) and then use
> > > > > that to access the master properties. So int.sdw.0 can be of this type.
> > > > 
> > > > Yes, you need to represent the master device type if you are going to be
> > > > having an internal representation of it.
> > > 
> > > Humm, confused...In the existing code bus and master are synonyms, see e.g.
> > > following code excerpts:
> > > 
> > >   * sdw_add_bus_master() - add a bus Master instance
> > >   * @bus: bus instance
> > >   *
> > >   * Initializes the bus instance, read properties and create child
> > >   * devices.
> > > 
> > > struct sdw_bus {
> > > 	struct device *dev; <<< pointer here
> > 
> > That's the pointer to what?  The device that the bus is "attached to"
> > (i.e. parent, like a platform device or a pci device)?
> > 
> > Why isn't this a "real" device in itself?
> 
> Allow me to provide a bit of background. I am not trying to be pedantic but
> make sure we are on the same page.
> 
> The SoundWire spec only defines a Master and Slaves attached to that Master.
> 
> In real applications, there is a need to have multiple links, which can
> possibly operate in synchronized ways, so Intel came up with the concept of
> Controller, which expose multiple Master interfaces that are in sync (two
> streams can start at exactly the same clock edge of different links).
> 
> The Controller is exposed in ACPI as a child of the HDAudio controller (ACPI
> companion of a PCI device). The controller exposes a 'master-count' and a
> set of link-specific properties needed for bandwidth/clock scaling.
> 
> For some reason, our Windows friends did not want to have a device for each
> Master interface, likely because they did not want to load a driver per
> Master interface or have 'yellow bangs'.
> 
> So the net result is that we have the following hierarchy in ACPI
> 
> Device(HDAS) // HDaudio controller
>   Device(SNDW) // SoundWire Controller
>     Device(SDW0) { // Slave0
> 	_ADR(link0, vendorX, partY...)
>     }
>     Device(SDW1) { // Slave0
> 	_ADR(link0, vendorX, partY...)
>     }
>     Device(SDW2) { // Slave0
> 	_ADR(link1, vendorX, partY...)
>     }
>     Device(SDWM) { // Slave0
> 	_ADR(linkM, vendorX, partY...)
>     }
> 
> There is no master device represented in ACPI and the only way by which we
> know to which Master a Slave device is attached by looking up the _ADR which
> contains the link information.
> 
> So, coming back to the plot, when we parse the Controller properties, we
> find out how many Master interfaces we have, create a platform_device for
> each of them, then initialize all the bus stuff.

So soundwire is creating platform devices?  Ick, no!  Don't do that,
create a "real" device that is the root of your bus and attach that to
the platform/pci/whatever device that is the parent of that device's
resources.

> > I thought I asked that a long time ago when first reviewing these
> > patches...
> > 
> > > 	unsigned int link_id;
> > > 	struct list_head slaves;
> > > 	DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
> > > 	struct mutex bus_lock;
> > > 	struct mutex msg_lock;
> > > 	const struct sdw_master_ops *ops;
> > > 	const struct sdw_master_port_ops *port_ops;
> > > 	struct sdw_bus_params params;
> > > 	struct sdw_master_prop prop;
> > > 
> > > The existing code creates a platform_device in
> > > drivers/soundwire/intel_init.c, and it's assigned by the following code:
> > 
> > The core creates a platform device, don't assume you can "take it over"
> > :)
> > 
> > That platform device lives on the platform bus, you need a "master"
> > device that lives on your soundbus bus.
> > 
> > Again, look at how USB does this.  Or better yet, greybus, as that code
> > is a lot smaller and simpler.
> 
> The learning curve is not small here...

kernel programming is hard.  Writing a new bus subsystem is even harder :(

> > > static int intel_probe(struct platform_device *pdev)
> > > {
> > > 	struct sdw_cdns_stream_config config;
> > > 	struct sdw_intel *sdw;
> > > 	int ret;
> > > 
> > > 	sdw = devm_kzalloc(&pdev->dev, sizeof(*sdw), GFP_KERNEL);
> > > [snip]
> > > 	sdw->cdns.dev = &pdev->dev;
> > > 	sdw->cdns.bus.dev = &pdev->dev;
> > 
> > Gotta love the lack of reference counting :(
> > 
> > > I really don't see what you are hinting at, sorry, unless we are talking
> > > about major surgery in the code.
> > 
> > It sounds like you need a device on your bus that represents the master,
> > as you have attributes associated with it, and other things.  You can't
> > put attributes on a random pci or platform device, as you do not "own"
> > that device.
> > 
> > does that help?
> 
> Looks like we are doing things wrong at multiple levels.
> 
> It might be better to have a more 'self-contained' solution where the bus
> initialization creates/registers a master device instead of having this
> proxy platform_device. That would avoid all these refcount issues and make
> the translation from device to bus straightforward.

Yes, that sounds better.  Don't mess with a platform device unless you
really have no other possible choice.  And even then, don't do it and
try to do something else.  Platform devices are really abused, don't
perpetuate it.

thanks,

greg k-h