[alsa-devel] [RFC PATCH 0/7] soundwire: add sysfs and debugfs support
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
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;
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.oobj-$(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
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?
+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?
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
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
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
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
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?
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
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
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.
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
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.
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...
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.
Am I on the right track or still in the weeds?
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?
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
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;
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.oobj-$(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
+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.
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
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
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
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
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.
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, 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?
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 */
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 Specificationfor SoundWire. They define various properties of the Masterand are used by the bus to configure the Master. clk_stop_modesis a bitmask for simplifications and combines theclock-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
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 Specificationfor SoundWire. They define various properties of the Masterand are used by the bus to configure the Master. clk_stop_modesis a bitmask for simplifications and combines theclock-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
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.
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;
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.oobj-$(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
@@ -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.
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
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.
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?
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);
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
+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.
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.
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);
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
+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.
participants (3)
-
Greg KH -
Pierre-Louis Bossart -
Vinod Koul