* [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-04 1:00 [RFC PATCH 0/7] soundwire: add sysfs and debugfs support Pierre-Louis Bossart
@ 2019-05-04 1:00 ` Pierre-Louis Bossart
2019-05-04 6:52 ` Greg KH
2019-05-04 1:00 ` [RFC PATCH 2/7] soundwire: add Slave sysfs support Pierre-Louis Bossart
` (5 subsequent siblings)
6 siblings, 1 reply; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-04 1:00 UTC (permalink / raw)
To: alsa-devel
Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Pierre-Louis Bossart,
Sanyog Kale
For each master N, add a device sdw-master:N and add the
master properties as attributes.
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
drivers/soundwire/Makefile | 3 +-
drivers/soundwire/bus.c | 6 ++
drivers/soundwire/sysfs.c | 162 ++++++++++++++++++++++++++++++++++
include/linux/soundwire/sdw.h | 10 +++
4 files changed, 180 insertions(+), 1 deletion(-)
create mode 100644 drivers/soundwire/sysfs.c
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index 5817beaca0e1..787f1cbf342c 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -3,7 +3,8 @@
#
#Bus Objs
-soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o
+soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
+ sysfs.o
obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
#Cadence Objs
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index fe745830a261..38de7071e135 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -49,6 +49,10 @@ int sdw_add_bus_master(struct sdw_bus *bus)
}
}
+ ret = sdw_sysfs_bus_init(bus);
+ if (ret < 0)
+ dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret);
+
/*
* Device numbers in SoundWire are 0 through 15. Enumeration device
* number (0), Broadcast device number (15), Group numbers (12 and
@@ -129,6 +133,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
*/
void sdw_delete_bus_master(struct sdw_bus *bus)
{
+ sdw_sysfs_bus_exit(bus);
+
device_for_each_child(bus->dev, NULL, sdw_delete_slave);
}
EXPORT_SYMBOL(sdw_delete_bus_master);
diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
new file mode 100644
index 000000000000..7b6c3826a73a
--- /dev/null
+++ b/drivers/soundwire/sysfs.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2015-19 Intel Corporation.
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_type.h>
+#include "bus.h"
+
+struct sdw_master_sysfs {
+ struct device dev;
+ struct sdw_bus *bus;
+};
+
+#define to_sdw_device(_dev) \
+ container_of(_dev, struct sdw_master_sysfs, dev)
+
+/*
+ * The sysfs for properties reflects the MIPI description as given
+ * in the MIPI DisCo spec
+ *
+ * Base file is:
+ * sdw-master-N
+ * |---- revision
+ * |---- clk_stop_modes
+ * |---- max_clk_freq
+ * |---- clk_freq
+ * |---- clk_gears
+ * |---- default_row
+ * |---- default_col
+ * |---- dynamic_shape
+ * |---- err_threshold
+ */
+
+#define sdw_master_attr(field, format_string) \
+static ssize_t field##_show(struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+{ \
+ struct sdw_master_sysfs *master = to_sdw_device(dev); \
+ return sprintf(buf, format_string, master->bus->prop.field); \
+} \
+static DEVICE_ATTR_RO(field)
+
+sdw_master_attr(revision, "0x%x\n");
+sdw_master_attr(clk_stop_modes, "0x%x\n");
+sdw_master_attr(max_clk_freq, "%d\n");
+sdw_master_attr(default_row, "%d\n");
+sdw_master_attr(default_col, "%d\n");
+sdw_master_attr(default_frame_rate, "%d\n");
+sdw_master_attr(dynamic_frame, "%d\n");
+sdw_master_attr(err_threshold, "%d\n");
+
+static ssize_t clock_frequencies_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct sdw_master_sysfs *master = to_sdw_device(dev);
+ ssize_t size = 0;
+ int i;
+
+ for (i = 0; i < master->bus->prop.num_clk_freq; i++)
+ size += sprintf(buf + size, "%8d ",
+ master->bus->prop.clk_freq[i]);
+ size += sprintf(buf + size, "\n");
+
+ return size;
+}
+static DEVICE_ATTR_RO(clock_frequencies);
+
+static ssize_t clock_gears_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct sdw_master_sysfs *master = to_sdw_device(dev);
+ ssize_t size = 0;
+ int i;
+
+ for (i = 0; i < master->bus->prop.num_clk_gears; i++)
+ size += sprintf(buf + size, "%8d ",
+ master->bus->prop.clk_gears[i]);
+ size += sprintf(buf + size, "\n");
+
+ return size;
+}
+static DEVICE_ATTR_RO(clock_gears);
+
+static struct attribute *master_node_attrs[] = {
+ &dev_attr_revision.attr,
+ &dev_attr_clk_stop_modes.attr,
+ &dev_attr_max_clk_freq.attr,
+ &dev_attr_default_row.attr,
+ &dev_attr_default_col.attr,
+ &dev_attr_default_frame_rate.attr,
+ &dev_attr_dynamic_frame.attr,
+ &dev_attr_err_threshold.attr,
+ &dev_attr_clock_frequencies.attr,
+ &dev_attr_clock_gears.attr,
+ NULL,
+};
+
+static const struct attribute_group sdw_master_node_group = {
+ .attrs = master_node_attrs,
+};
+
+static const struct attribute_group *sdw_master_node_groups[] = {
+ &sdw_master_node_group,
+ NULL
+};
+
+static void sdw_device_release(struct device *dev)
+{
+ struct sdw_master_sysfs *master = to_sdw_device(dev);
+
+ kfree(master);
+}
+
+static struct device_type sdw_device_type = {
+ .name = "sdw_device",
+ .release = sdw_device_release,
+};
+
+int sdw_sysfs_bus_init(struct sdw_bus *bus)
+{
+ struct sdw_master_sysfs *master;
+ int err;
+
+ if (bus->sysfs) {
+ dev_err(bus->dev, "SDW sysfs is already initialized\n");
+ return -EIO;
+ }
+
+ master = kzalloc(sizeof(*master), GFP_KERNEL);
+ if (!master)
+ return -ENOMEM;
+
+ bus->sysfs = master;
+ master->bus = bus;
+ master->dev.type = &sdw_device_type;
+ master->dev.bus = &sdw_bus_type;
+ master->dev.parent = bus->dev;
+ master->dev.groups = sdw_master_node_groups;
+ dev_set_name(&master->dev, "sdw-master:%x", bus->link_id);
+
+ err = device_register(&master->dev);
+ if (err)
+ put_device(&master->dev);
+
+ return err;
+}
+
+void sdw_sysfs_bus_exit(struct sdw_bus *bus)
+{
+ struct sdw_master_sysfs *master = bus->sysfs;
+
+ if (!master)
+ return;
+
+ master->bus = NULL;
+ put_device(&master->dev);
+ bus->sysfs = NULL;
+}
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 3b231472464a..b64d46fed0c8 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -402,6 +402,14 @@ int sdw_slave_read_dp0(struct sdw_slave *slave,
struct fwnode_handle *port,
struct sdw_dp0_prop *dp0);
+/*
+ * SDW sysfs APIs
+ */
+struct sdw_master_sysfs;
+
+int sdw_sysfs_bus_init(struct sdw_bus *bus);
+void sdw_sysfs_bus_exit(struct sdw_bus *bus);
+
/*
* SDW Slave Structures and APIs
*/
@@ -731,6 +739,7 @@ struct sdw_master_ops {
* @m_rt_list: List of Master instance of all stream(s) running on Bus. This
* is used to compute and program bus bandwidth, clock, frame shape,
* transport and port parameters
+ * @sysfs: Bus sysfs
* @defer_msg: Defer message
* @clk_stop_timeout: Clock stop timeout computed
* @bank_switch_timeout: Bank switch timeout computed
@@ -750,6 +759,7 @@ struct sdw_bus {
struct sdw_bus_params params;
struct sdw_master_prop prop;
struct list_head m_rt_list;
+ struct sdw_master_sysfs *sysfs;
struct sdw_defer defer_msg;
unsigned int clk_stop_timeout;
u32 bank_switch_timeout;
--
2.17.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-04 1:00 ` [RFC PATCH 1/7] soundwire: Add sysfs support for master(s) Pierre-Louis Bossart
@ 2019-05-04 6:52 ` Greg KH
2019-05-06 16:43 ` [alsa-devel] " Pierre-Louis Bossart
2019-05-07 2:24 ` Pierre-Louis Bossart
0 siblings, 2 replies; 43+ messages in thread
From: Greg KH @ 2019-05-04 6:52 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, linux-kernel, tiwai, broonie, vkoul, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Sanyog Kale
On Fri, May 03, 2019 at 08:00:24PM -0500, Pierre-Louis Bossart wrote:
> For each master N, add a device sdw-master:N and add the
> master properties as attributes.
>
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
> drivers/soundwire/Makefile | 3 +-
> drivers/soundwire/bus.c | 6 ++
> drivers/soundwire/sysfs.c | 162 ++++++++++++++++++++++++++++++++++
> include/linux/soundwire/sdw.h | 10 +++
> 4 files changed, 180 insertions(+), 1 deletion(-)
> create mode 100644 drivers/soundwire/sysfs.c
>
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index 5817beaca0e1..787f1cbf342c 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -3,7 +3,8 @@
> #
>
> #Bus Objs
> -soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o
> +soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
> + sysfs.o
> obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
>
> #Cadence Objs
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index fe745830a261..38de7071e135 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -49,6 +49,10 @@ int sdw_add_bus_master(struct sdw_bus *bus)
> }
> }
>
> + ret = sdw_sysfs_bus_init(bus);
> + if (ret < 0)
> + dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret);
> +
> /*
> * Device numbers in SoundWire are 0 through 15. Enumeration device
> * number (0), Broadcast device number (15), Group numbers (12 and
> @@ -129,6 +133,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
> */
> void sdw_delete_bus_master(struct sdw_bus *bus)
> {
> + sdw_sysfs_bus_exit(bus);
> +
> device_for_each_child(bus->dev, NULL, sdw_delete_slave);
> }
> EXPORT_SYMBOL(sdw_delete_bus_master);
> diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
> new file mode 100644
> index 000000000000..7b6c3826a73a
> --- /dev/null
> +++ b/drivers/soundwire/sysfs.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2015-19 Intel Corporation.
> +
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/soundwire/sdw.h>
> +#include <linux/soundwire/sdw_type.h>
> +#include "bus.h"
> +
> +struct sdw_master_sysfs {
> + struct device dev;
> + struct sdw_bus *bus;
> +};
> +
> +#define to_sdw_device(_dev) \
> + container_of(_dev, struct sdw_master_sysfs, dev)
> +
> +/*
> + * The sysfs for properties reflects the MIPI description as given
> + * in the MIPI DisCo spec
> + *
> + * Base file is:
> + * sdw-master-N
> + * |---- revision
> + * |---- clk_stop_modes
> + * |---- max_clk_freq
> + * |---- clk_freq
> + * |---- clk_gears
> + * |---- default_row
> + * |---- default_col
> + * |---- dynamic_shape
> + * |---- err_threshold
> + */
> +
> +#define sdw_master_attr(field, format_string) \
> +static ssize_t field##_show(struct device *dev, \
> + struct device_attribute *attr, \
> + char *buf) \
> +{ \
> + struct sdw_master_sysfs *master = to_sdw_device(dev); \
> + return sprintf(buf, format_string, master->bus->prop.field); \
> +} \
> +static DEVICE_ATTR_RO(field)
> +
> +sdw_master_attr(revision, "0x%x\n");
> +sdw_master_attr(clk_stop_modes, "0x%x\n");
> +sdw_master_attr(max_clk_freq, "%d\n");
> +sdw_master_attr(default_row, "%d\n");
> +sdw_master_attr(default_col, "%d\n");
> +sdw_master_attr(default_frame_rate, "%d\n");
> +sdw_master_attr(dynamic_frame, "%d\n");
> +sdw_master_attr(err_threshold, "%d\n");
> +
> +static ssize_t clock_frequencies_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct sdw_master_sysfs *master = to_sdw_device(dev);
> + ssize_t size = 0;
> + int i;
> +
> + for (i = 0; i < master->bus->prop.num_clk_freq; i++)
> + size += sprintf(buf + size, "%8d ",
> + master->bus->prop.clk_freq[i]);
> + size += sprintf(buf + size, "\n");
> +
> + return size;
> +}
> +static DEVICE_ATTR_RO(clock_frequencies);
> +
> +static ssize_t clock_gears_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct sdw_master_sysfs *master = to_sdw_device(dev);
> + ssize_t size = 0;
> + int i;
> +
> + for (i = 0; i < master->bus->prop.num_clk_gears; i++)
> + size += sprintf(buf + size, "%8d ",
> + master->bus->prop.clk_gears[i]);
> + size += sprintf(buf + size, "\n");
> +
> + return size;
> +}
> +static DEVICE_ATTR_RO(clock_gears);
> +
> +static struct attribute *master_node_attrs[] = {
> + &dev_attr_revision.attr,
> + &dev_attr_clk_stop_modes.attr,
> + &dev_attr_max_clk_freq.attr,
> + &dev_attr_default_row.attr,
> + &dev_attr_default_col.attr,
> + &dev_attr_default_frame_rate.attr,
> + &dev_attr_dynamic_frame.attr,
> + &dev_attr_err_threshold.attr,
> + &dev_attr_clock_frequencies.attr,
> + &dev_attr_clock_gears.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group sdw_master_node_group = {
> + .attrs = master_node_attrs,
> +};
> +
> +static const struct attribute_group *sdw_master_node_groups[] = {
> + &sdw_master_node_group,
> + NULL
> +};
Minor nit, you can use the ATTRIBUTE_GROUPS() macro here to save you a
few lines.
> +
> +static void sdw_device_release(struct device *dev)
> +{
> + struct sdw_master_sysfs *master = to_sdw_device(dev);
> +
> + kfree(master);
> +}
> +
> +static struct device_type sdw_device_type = {
> + .name = "sdw_device",
> + .release = sdw_device_release,
> +};
> +
> +int sdw_sysfs_bus_init(struct sdw_bus *bus)
> +{
> + struct sdw_master_sysfs *master;
> + int err;
> +
> + if (bus->sysfs) {
> + dev_err(bus->dev, "SDW sysfs is already initialized\n");
> + return -EIO;
> + }
> +
> + master = kzalloc(sizeof(*master), GFP_KERNEL);
> + if (!master)
> + return -ENOMEM;
Why are you creating a whole new device to put all of this under? Is
this needed? What will the sysfs tree look like when you do this? Why
can't the "bus" device just get all of these attributes and no second
device be created?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-04 6:52 ` Greg KH
@ 2019-05-06 16:43 ` Pierre-Louis Bossart
2019-05-07 2:24 ` Pierre-Louis Bossart
1 sibling, 0 replies; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 16:43 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, vkoul, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
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?
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-04 6:52 ` Greg KH
2019-05-06 16:43 ` [alsa-devel] " Pierre-Louis Bossart
@ 2019-05-07 2:24 ` Pierre-Louis Bossart
2019-05-07 5:27 ` Vinod Koul
1 sibling, 1 reply; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-07 2:24 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, vkoul, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
>> +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?
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-07 2:24 ` Pierre-Louis Bossart
@ 2019-05-07 5:27 ` Vinod Koul
2019-05-07 5:54 ` Greg KH
0 siblings, 1 reply; 43+ messages in thread
From: Vinod Koul @ 2019-05-07 5:27 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Greg KH, alsa-devel, tiwai, linux-kernel, liam.r.girdwood,
broonie, srinivas.kandagatla, jank, joe, Sanyog Kale
On 06-05-19, 21:24, Pierre-Louis Bossart wrote:
>
> > > +int sdw_sysfs_bus_init(struct sdw_bus *bus)
> > > +{
> > > + struct sdw_master_sysfs *master;
> > > + int err;
> > > +
> > > + if (bus->sysfs) {
> > > + dev_err(bus->dev, "SDW sysfs is already initialized\n");
> > > + return -EIO;
> > > + }
> > > +
> > > + master = kzalloc(sizeof(*master), GFP_KERNEL);
> > > + if (!master)
> > > + return -ENOMEM;
> >
> > Why are you creating a whole new device to put all of this under? Is
> > this needed? What will the sysfs tree look like when you do this? Why
> > can't the "bus" device just get all of these attributes and no second
> > device be created?
>
> I tried a quick hack and indeed we could simplify the code with something as
> simple as:
>
> [attributes omitted]
>
> static const struct attribute_group sdw_master_node_group = {
> .attrs = master_node_attrs,
> .name = "mipi-disco"
> };
>
> int sdw_sysfs_bus_init(struct sdw_bus *bus)
> {
> return sysfs_create_group(&bus->dev->kobj, &sdw_master_node_group);
> }
>
> void sdw_sysfs_bus_exit(struct sdw_bus *bus)
> {
> sysfs_remove_group(&bus->dev->kobj, &sdw_master_node_group);
> }
>
> which gives me a simpler structure and doesn't require additional
> pretend-devices:
>
> /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# ls
> clock_gears
> /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# more clock_gears
> 8086
>
> The issue I have is that for the _show() functions, I don't see a way to go
> from the device argument to bus. In the example above I forced the output
> but would need a helper.
>
> static ssize_t clock_gears_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct sdw_bus *bus; // this is what I need to find from dev
> ssize_t size = 0;
> int i;
>
> return sprintf(buf, "%d \n", 8086);
> }
>
> my brain is starting to fry, but I don't see how container_of() would work
> here since the bus structure contains a pointer to the device. I don't also
> see a way to check for all devices for the bus_type soundwire.
> For the slaves we do have a macro based on container_of(), so wondering if
> we made a mistake in the bus definition? Vinod, any thoughts?
yeah I dont recall a way to get bus fed into create_group, I did look at
the other examples back then and IIRC and most of them were using a
global to do the trick (I didn't want to go down that route).
I think that was the reason I wrote it this way...
BTW if you do use psedo-device you can create your own struct foo which
embeds device and then then you can use container approach to get foo
(and foo contains bus as a member).
Greg, any thoughts?
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-07 5:27 ` Vinod Koul
@ 2019-05-07 5:54 ` Greg KH
2019-05-07 11:03 ` Vinod Koul
0 siblings, 1 reply; 43+ messages in thread
From: Greg KH @ 2019-05-07 5:54 UTC (permalink / raw)
To: Vinod Koul
Cc: Pierre-Louis Bossart, alsa-devel, tiwai, linux-kernel,
liam.r.girdwood, broonie, srinivas.kandagatla, jank, joe,
Sanyog Kale
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-07 5:54 ` Greg KH
@ 2019-05-07 11:03 ` Vinod Koul
2019-05-07 11:19 ` Greg KH
0 siblings, 1 reply; 43+ messages in thread
From: Vinod Koul @ 2019-05-07 11:03 UTC (permalink / raw)
To: Greg KH
Cc: Pierre-Louis Bossart, alsa-devel, tiwai, linux-kernel,
liam.r.girdwood, broonie, srinivas.kandagatla, jank, joe,
Sanyog Kale
On 07-05-19, 07:54, Greg KH wrote:
> On Tue, May 07, 2019 at 10:57:32AM +0530, Vinod Koul wrote:
> > On 06-05-19, 21:24, Pierre-Louis Bossart wrote:
> > >
> > > > > +int sdw_sysfs_bus_init(struct sdw_bus *bus)
> > > > > +{
> > > > > + struct sdw_master_sysfs *master;
> > > > > + int err;
> > > > > +
> > > > > + if (bus->sysfs) {
> > > > > + dev_err(bus->dev, "SDW sysfs is already initialized\n");
> > > > > + return -EIO;
> > > > > + }
> > > > > +
> > > > > + master = kzalloc(sizeof(*master), GFP_KERNEL);
> > > > > + if (!master)
> > > > > + return -ENOMEM;
> > > >
> > > > Why are you creating a whole new device to put all of this under? Is
> > > > this needed? What will the sysfs tree look like when you do this? Why
> > > > can't the "bus" device just get all of these attributes and no second
> > > > device be created?
> > >
> > > I tried a quick hack and indeed we could simplify the code with something as
> > > simple as:
> > >
> > > [attributes omitted]
> > >
> > > static const struct attribute_group sdw_master_node_group = {
> > > .attrs = master_node_attrs,
> > > .name = "mipi-disco"
> > > };
> > >
> > > int sdw_sysfs_bus_init(struct sdw_bus *bus)
> > > {
> > > return sysfs_create_group(&bus->dev->kobj, &sdw_master_node_group);
> > > }
> > >
> > > void sdw_sysfs_bus_exit(struct sdw_bus *bus)
> > > {
> > > sysfs_remove_group(&bus->dev->kobj, &sdw_master_node_group);
> > > }
> > >
> > > which gives me a simpler structure and doesn't require additional
> > > pretend-devices:
> > >
> > > /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# ls
> > > clock_gears
> > > /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# more clock_gears
> > > 8086
> > >
> > > The issue I have is that for the _show() functions, I don't see a way to go
> > > from the device argument to bus. In the example above I forced the output
> > > but would need a helper.
> > >
> > > static ssize_t clock_gears_show(struct device *dev,
> > > struct device_attribute *attr, char *buf)
> > > {
> > > struct sdw_bus *bus; // this is what I need to find from dev
> > > ssize_t size = 0;
> > > int i;
> > >
> > > return sprintf(buf, "%d \n", 8086);
> > > }
> > >
> > > my brain is starting to fry, but I don't see how container_of() would work
> > > here since the bus structure contains a pointer to the device. I don't also
> > > see a way to check for all devices for the bus_type soundwire.
> > > For the slaves we do have a macro based on container_of(), so wondering if
> > > we made a mistake in the bus definition? Vinod, any thoughts?
> >
> > yeah I dont recall a way to get bus fed into create_group, I did look at
> > the other examples back then and IIRC and most of them were using a
> > global to do the trick (I didn't want to go down that route).
> >
> > I think that was the reason I wrote it this way...
> >
> > BTW if you do use psedo-device you can create your own struct foo which
> > embeds device and then then you can use container approach to get foo
> > (and foo contains bus as a member).
> >
> > Greg, any thoughts?
>
> Why would you have "bus" attributes on a device? I don't think you are
> using "bus" here like the driver model uses the term "bus", right?
>
> What are you really trying to show here?
>
> And if you need to know the bus pointer from the device, why don't you
> have a pointer to it in your device-specific structure?
The model here is that Master device is PCI or Platform device and then
creates a bus instance which has soundwire slave devices.
So for any attribute on Master device (which has properties as well and
representation in sysfs), device specfic struct (PCI/platfrom doesn't
help). For slave that is not a problem as sdw_slave structure takes care
if that.
So, the solution was to create the psedo sdw_master device for the
representation and have device-specific structure.
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-07 11:03 ` Vinod Koul
@ 2019-05-07 11:19 ` Greg KH
2019-05-07 22:49 ` Pierre-Louis Bossart
0 siblings, 1 reply; 43+ messages in thread
From: Greg KH @ 2019-05-07 11:19 UTC (permalink / raw)
To: Vinod Koul
Cc: Pierre-Louis Bossart, alsa-devel, tiwai, linux-kernel,
liam.r.girdwood, broonie, srinivas.kandagatla, jank, joe,
Sanyog Kale
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-07 11:19 ` Greg KH
@ 2019-05-07 22:49 ` Pierre-Louis Bossart
2019-05-08 7:46 ` Vinod Koul
0 siblings, 1 reply; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-07 22:49 UTC (permalink / raw)
To: Greg KH, Vinod Koul
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
>> 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?
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-07 22:49 ` Pierre-Louis Bossart
@ 2019-05-08 7:46 ` Vinod Koul
2019-05-08 9:16 ` Greg KH
0 siblings, 1 reply; 43+ messages in thread
From: Vinod Koul @ 2019-05-08 7:46 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Greg KH, alsa-devel, tiwai, linux-kernel, liam.r.girdwood,
broonie, srinivas.kandagatla, jank, joe, Sanyog Kale
On 07-05-19, 17:49, Pierre-Louis Bossart wrote:
>
> > > The model here is that Master device is PCI or Platform device and then
> > > creates a bus instance which has soundwire slave devices.
> > >
> > > So for any attribute on Master device (which has properties as well and
> > > representation in sysfs), device specfic struct (PCI/platfrom doesn't
> > > help). For slave that is not a problem as sdw_slave structure takes care
> > > if that.
> > >
> > > So, the solution was to create the psedo sdw_master device for the
> > > representation and have device-specific structure.
> >
> > Ok, much like the "USB host controller" type device. That's fine, make
> > such a device, add it to your bus, and set the type correctly. And keep
> > a pointer to that structure in your device-specific structure if you
> > really need to get to anything in it.
>
> humm, you lost me on the last sentence. Did you mean using
> set_drv/platform_data during the init and retrieving the bus information
> with get_drv/platform_data as needed later? Or something else I badly need
> to learn?
IIUC Greg meant we should represent a soundwire master device type and
use that here. Just like we have soundwire slave device type. Something
like:
struct sdw_master {
struct device dev;
struct sdw_master_prop *prop;
...
};
In show function you get master from dev (container of) and then use
that to access the master properties. So int.sdw.0 can be of this type.
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-08 7:46 ` Vinod Koul
@ 2019-05-08 9:16 ` Greg KH
2019-05-08 16:42 ` Pierre-Louis Bossart
0 siblings, 1 reply; 43+ messages in thread
From: Greg KH @ 2019-05-08 9:16 UTC (permalink / raw)
To: Vinod Koul
Cc: Pierre-Louis Bossart, alsa-devel, tiwai, linux-kernel,
liam.r.girdwood, broonie, srinivas.kandagatla, jank, joe,
Sanyog Kale
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-08 9:16 ` Greg KH
@ 2019-05-08 16:42 ` Pierre-Louis Bossart
2019-05-08 16:59 ` Greg KH
0 siblings, 1 reply; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-08 16:42 UTC (permalink / raw)
To: Greg KH, Vinod Koul
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)
2019-05-08 16:42 ` Pierre-Louis Bossart
@ 2019-05-08 16:59 ` Greg KH
[not found] ` <0b8d5238-6894-e2b4-5522-28636e40dd63@linux.intel.com>
0 siblings, 1 reply; 43+ messages in thread
From: Greg KH @ 2019-05-08 16:59 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Vinod Koul, alsa-devel, tiwai, linux-kernel, liam.r.girdwood,
broonie, srinivas.kandagatla, jank, joe, Sanyog Kale
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC PATCH 2/7] soundwire: add Slave sysfs support
2019-05-04 1:00 [RFC PATCH 0/7] soundwire: add sysfs and debugfs support Pierre-Louis Bossart
2019-05-04 1:00 ` [RFC PATCH 1/7] soundwire: Add sysfs support for master(s) Pierre-Louis Bossart
@ 2019-05-04 1:00 ` Pierre-Louis Bossart
2019-05-04 6:54 ` Greg KH
2019-05-04 1:00 ` [RFC PATCH 3/7] ABI: testing: Add description of soundwire master sysfs files Pierre-Louis Bossart
` (4 subsequent siblings)
6 siblings, 1 reply; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-04 1:00 UTC (permalink / raw)
To: alsa-devel
Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Pierre-Louis Bossart,
Sanyog Kale
Add DisCo Slave properties as device attributes.
Add a device for Data Port 0 which adds Dp0 properties as attributes.
Add devices for Source and Sink Data Ports, and add Dp-N
properties as attributes.
The Slave, DP0 and DPn cases are intentionally handled in separate
files to avoid conflicts with attributes having the same names at
different levels.
Audio modes are not supported for now. Depending on the discussions
the SoundWire Device Class, we may add it later as is or follow the
new specification.
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
drivers/soundwire/Makefile | 2 +-
drivers/soundwire/bus.c | 2 +
drivers/soundwire/bus.h | 2 +
drivers/soundwire/bus_type.c | 5 +
drivers/soundwire/slave.c | 1 +
drivers/soundwire/sysfs.c | 213 ++++++++++++++++++++++++++++
drivers/soundwire/sysfs_local.h | 42 ++++++
drivers/soundwire/sysfs_slave_dp0.c | 112 +++++++++++++++
drivers/soundwire/sysfs_slave_dpn.c | 168 ++++++++++++++++++++++
include/linux/soundwire/sdw.h | 5 +
10 files changed, 551 insertions(+), 1 deletion(-)
create mode 100644 drivers/soundwire/sysfs_local.h
create mode 100644 drivers/soundwire/sysfs_slave_dp0.c
create mode 100644 drivers/soundwire/sysfs_slave_dpn.c
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index 787f1cbf342c..a72a29731a28 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -4,7 +4,7 @@
#Bus Objs
soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
- sysfs.o
+ sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o
obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
#Cadence Objs
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 38de7071e135..dd9181693554 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -113,6 +113,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
struct sdw_slave *slave = dev_to_sdw_dev(dev);
struct sdw_bus *bus = slave->bus;
+ sdw_sysfs_slave_exit(slave);
+
mutex_lock(&bus->bus_lock);
if (slave->dev_num) /* clear dev_num if assigned */
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 3048ca153f22..0707e68a8d21 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -18,6 +18,8 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
void sdw_extract_slave_id(struct sdw_bus *bus,
u64 addr, struct sdw_slave_id *id);
+extern const struct attribute_group *sdw_slave_dev_attr_groups[];
+
enum {
SDW_MSG_FLAG_READ = 0,
SDW_MSG_FLAG_WRITE,
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 2655602f0cfb..f68fe45c1037 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -97,6 +97,11 @@ static int sdw_drv_probe(struct device *dev)
if (slave->ops && slave->ops->read_prop)
slave->ops->read_prop(slave);
+ /* init the sysfs as we have properties now */
+ ret = sdw_sysfs_slave_init(slave);
+ if (ret < 0)
+ dev_warn(dev, "Slave sysfs init failed:%d\n", ret);
+
/*
* Check for valid clk_stop_timeout, use DisCo worst case value of
* 300ms
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index f39a5815e25d..bad73a267fdd 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -34,6 +34,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
id->class_id, id->unique_id);
slave->dev.release = sdw_slave_release;
+ slave->dev.groups = sdw_slave_dev_attr_groups;
slave->dev.bus = &sdw_bus_type;
slave->bus = bus;
slave->status = SDW_SLAVE_UNATTACHED;
diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
index 7b6c3826a73a..734e2c8bc5cd 100644
--- a/drivers/soundwire/sysfs.c
+++ b/drivers/soundwire/sysfs.c
@@ -8,6 +8,7 @@
#include <linux/soundwire/sdw.h>
#include <linux/soundwire/sdw_type.h>
#include "bus.h"
+#include "sysfs_local.h"
struct sdw_master_sysfs {
struct device dev;
@@ -160,3 +161,215 @@ void sdw_sysfs_bus_exit(struct sdw_bus *bus)
put_device(&master->dev);
bus->sysfs = NULL;
}
+
+/*
+ * Slave sysfs
+ */
+
+/*
+ * The sysfs for Slave reflects the MIPI description as given
+ * in the MIPI DisCo spec
+ *
+ * Base file is device
+ * |---- mipi_revision
+ * |---- wake_capable
+ * |---- test_mode_capable
+ * |---- simple_clk_stop_capable
+ * |---- clk_stop_timeout
+ * |---- ch_prep_timeout
+ * |---- reset_behave
+ * |---- high_PHY_capable
+ * |---- paging_support
+ * |---- bank_delay_support
+ * |---- p15_behave
+ * |---- master_count
+ * |---- source_ports
+ * |---- sink_ports
+ * |---- dp0
+ * |---- max_word
+ * |---- min_word
+ * |---- words
+ * |---- flow_controlled
+ * |---- simple_ch_prep_sm
+ * |---- device_interrupts
+ * |---- dpN
+ * |---- max_word
+ * |---- min_word
+ * |---- words
+ * |---- type
+ * |---- max_grouping
+ * |---- simple_ch_prep_sm
+ * |---- ch_prep_timeout
+ * |---- device_interrupts
+ * |---- max_ch
+ * |---- min_ch
+ * |---- ch
+ * |---- ch_combinations
+ * |---- modes
+ * |---- max_async_buffer
+ * |---- block_pack_mode
+ * |---- port_encoding
+ * |---- audio_modeM
+ * |---- bus_min_freq
+ * |---- bus_max_freq
+ * |---- bus_freq
+ * |---- max_freq
+ * |---- min_freq
+ * |---- freq
+ * |---- prep_ch_behave
+ * |---- glitchless
+ *
+ */
+
+#define sdw_slave_attr(field, format_string) \
+static ssize_t field##_show(struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+{ \
+ struct sdw_slave *slave = dev_to_sdw_dev(dev); \
+ return sprintf(buf, format_string, slave->prop.field); \
+} \
+static DEVICE_ATTR_RO(field)
+
+sdw_slave_attr(mipi_revision, "0x%x\n");
+sdw_slave_attr(wake_capable, "%d\n");
+sdw_slave_attr(test_mode_capable, "%d\n");
+sdw_slave_attr(clk_stop_mode1, "%d\n");
+sdw_slave_attr(simple_clk_stop_capable, "%d\n");
+sdw_slave_attr(clk_stop_timeout, "%d\n");
+sdw_slave_attr(ch_prep_timeout, "%d\n");
+sdw_slave_attr(reset_behave, "%d\n");
+sdw_slave_attr(high_PHY_capable, "%d\n");
+sdw_slave_attr(paging_support, "%d\n");
+sdw_slave_attr(bank_delay_support, "%d\n");
+sdw_slave_attr(p15_behave, "%d\n");
+sdw_slave_attr(master_count, "%d\n");
+sdw_slave_attr(source_ports, "%d\n");
+sdw_slave_attr(sink_ports, "%d\n");
+
+static ssize_t modalias_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct sdw_slave *slave = dev_to_sdw_dev(dev);
+
+ return sdw_slave_modalias(slave, buf, 256);
+}
+static DEVICE_ATTR_RO(modalias);
+
+static struct attribute *slave_dev_attrs[] = {
+ &dev_attr_mipi_revision.attr,
+ &dev_attr_wake_capable.attr,
+ &dev_attr_test_mode_capable.attr,
+ &dev_attr_clk_stop_mode1.attr,
+ &dev_attr_simple_clk_stop_capable.attr,
+ &dev_attr_clk_stop_timeout.attr,
+ &dev_attr_ch_prep_timeout.attr,
+ &dev_attr_reset_behave.attr,
+ &dev_attr_high_PHY_capable.attr,
+ &dev_attr_paging_support.attr,
+ &dev_attr_bank_delay_support.attr,
+ &dev_attr_p15_behave.attr,
+ &dev_attr_master_count.attr,
+ &dev_attr_source_ports.attr,
+ &dev_attr_sink_ports.attr,
+ &dev_attr_modalias.attr,
+ NULL,
+};
+
+static struct attribute_group sdw_slave_dev_attr_group = {
+ .attrs = slave_dev_attrs,
+};
+
+const struct attribute_group *sdw_slave_dev_attr_groups[] = {
+ &sdw_slave_dev_attr_group,
+ NULL
+};
+
+int sdw_sysfs_slave_init(struct sdw_slave *slave)
+{
+ struct sdw_slave_sysfs *sysfs;
+ unsigned int src_dpns, sink_dpns, i, j;
+ int err;
+
+ if (slave->sysfs) {
+ dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
+ err = -EIO;
+ goto err_ret;
+ }
+
+ sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
+ if (!sysfs) {
+ err = -ENOMEM;
+ goto err_ret;
+ }
+
+ slave->sysfs = sysfs;
+ sysfs->slave = slave;
+
+ if (slave->prop.dp0_prop) {
+ sysfs->dp0 = sdw_sysfs_slave_dp0_init(slave,
+ slave->prop.dp0_prop);
+ if (!sysfs->dp0) {
+ err = -ENOMEM;
+ goto err_free_sysfs;
+ }
+ }
+
+ src_dpns = hweight32(slave->prop.source_ports);
+ sink_dpns = hweight32(slave->prop.sink_ports);
+ sysfs->num_dpns = src_dpns + sink_dpns;
+
+ sysfs->dpns = kcalloc(sysfs->num_dpns,
+ sizeof(**sysfs->dpns), GFP_KERNEL);
+ if (!sysfs->dpns) {
+ err = -ENOMEM;
+ goto err_dp0;
+ }
+
+ for (i = 0; i < src_dpns; i++) {
+ sysfs->dpns[i] =
+ sdw_sysfs_slave_dpn_init(slave,
+ &slave->prop.src_dpn_prop[i],
+ true);
+ if (!sysfs->dpns[i]) {
+ err = -ENOMEM;
+ goto err_dpn;
+ }
+ }
+
+ for (j = 0; j < sink_dpns; j++) {
+ sysfs->dpns[i + j] =
+ sdw_sysfs_slave_dpn_init(slave,
+ &slave->prop.sink_dpn_prop[j],
+ false);
+ if (!sysfs->dpns[i + j]) {
+ err = -ENOMEM;
+ goto err_dpn;
+ }
+ }
+ return 0;
+
+err_dpn:
+ sdw_sysfs_slave_dpn_exit(sysfs);
+err_dp0:
+ sdw_sysfs_slave_dp0_exit(sysfs);
+err_free_sysfs:
+ kfree(sysfs);
+ sysfs->slave = NULL;
+err_ret:
+ return err;
+}
+
+void sdw_sysfs_slave_exit(struct sdw_slave *slave)
+{
+ struct sdw_slave_sysfs *sysfs = slave->sysfs;
+
+ if (!sysfs)
+ return;
+
+ sdw_sysfs_slave_dpn_exit(sysfs);
+ kfree(sysfs->dpns);
+ sdw_sysfs_slave_dp0_exit(sysfs);
+ kfree(sysfs);
+ slave->sysfs = NULL;
+}
diff --git a/drivers/soundwire/sysfs_local.h b/drivers/soundwire/sysfs_local.h
new file mode 100644
index 000000000000..dd037890117d
--- /dev/null
+++ b/drivers/soundwire/sysfs_local.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
+/* Copyright(c) 2015-19 Intel Corporation. */
+
+#ifndef __SDW_SYSFS_LOCAL_H
+#define __SDW_SYSFS_LOCAL_H
+
+struct sdw_dp0_sysfs {
+ struct device dev;
+ struct sdw_dp0_prop *dp0_prop;
+};
+
+extern struct device_type sdw_dp0_type;
+
+struct sdw_dp0_sysfs
+*sdw_sysfs_slave_dp0_init(struct sdw_slave *slave,
+ struct sdw_dp0_prop *prop);
+
+void sdw_sysfs_slave_dp0_exit(struct sdw_slave_sysfs *sysfs);
+
+struct sdw_dpn_sysfs {
+ struct device dev;
+ struct sdw_dpn_prop *dpn_prop;
+};
+
+extern struct device_type sdw_dpn_type;
+
+struct sdw_dpn_sysfs
+*sdw_sysfs_slave_dpn_init(struct sdw_slave *slave,
+ struct sdw_dpn_prop *prop,
+ bool src);
+
+void sdw_sysfs_slave_dpn_exit(struct sdw_slave_sysfs *sysfs);
+
+struct sdw_slave_sysfs {
+ struct sdw_slave *slave;
+ struct sdw_dp0_sysfs *dp0;
+ unsigned int num_dpns;
+ struct sdw_dpn_sysfs **dpns;
+
+};
+
+#endif /* __SDW_SYSFS_LOCAL_H */
diff --git a/drivers/soundwire/sysfs_slave_dp0.c b/drivers/soundwire/sysfs_slave_dp0.c
new file mode 100644
index 000000000000..4c9994a19199
--- /dev/null
+++ b/drivers/soundwire/sysfs_slave_dp0.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2015-19 Intel Corporation.
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_type.h>
+#include "bus.h"
+#include "sysfs_local.h"
+
+/*
+ * DP0 sysfs
+ */
+
+#define to_sdw_dp0(_dev) \
+ container_of(_dev, struct sdw_dp0_sysfs, dev)
+
+#define sdw_dp0_attr(field, format_string) \
+static ssize_t field##_show(struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+{ \
+ struct sdw_dp0_sysfs *sysfs = to_sdw_dp0(dev); \
+ return sprintf(buf, format_string, sysfs->dp0_prop->field); \
+} \
+static DEVICE_ATTR_RO(field)
+
+sdw_dp0_attr(min_word, "%d\n");
+sdw_dp0_attr(max_word, "%d\n");
+sdw_dp0_attr(BRA_flow_controlled, "%d\n");
+sdw_dp0_attr(simple_ch_prep_sm, "%d\n");
+sdw_dp0_attr(imp_def_interrupts, "0x%x\n");
+
+static ssize_t word_bits_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct sdw_dp0_sysfs *sysfs = to_sdw_dp0(dev);
+ ssize_t size = 0;
+ int i;
+
+ for (i = 0; i < sysfs->dp0_prop->num_words; i++)
+ size += sprintf(buf + size, "%d ", sysfs->dp0_prop->words[i]);
+ size += sprintf(buf + size, "\n");
+
+ return size;
+}
+static DEVICE_ATTR_RO(word_bits);
+
+static struct attribute *dp0_attrs[] = {
+ &dev_attr_min_word.attr,
+ &dev_attr_max_word.attr,
+ &dev_attr_BRA_flow_controlled.attr,
+ &dev_attr_simple_ch_prep_sm.attr,
+ &dev_attr_imp_def_interrupts.attr,
+ &dev_attr_word_bits.attr,
+ NULL,
+};
+
+static const struct attribute_group dp0_group = {
+ .attrs = dp0_attrs,
+};
+
+static const struct attribute_group *dp0_groups[] = {
+ &dp0_group,
+ NULL
+};
+
+static void sdw_dp0_release(struct device *dev)
+{
+ struct sdw_dp0_sysfs *sysfs = to_sdw_dp0(dev);
+
+ kfree(sysfs);
+}
+
+struct device_type sdw_dp0_type = {
+ .name = "sdw_dp0",
+ .release = sdw_dp0_release,
+};
+
+struct sdw_dp0_sysfs
+*sdw_sysfs_slave_dp0_init(struct sdw_slave *slave,
+ struct sdw_dp0_prop *prop)
+{
+ struct sdw_dp0_sysfs *dp0;
+ int err;
+
+ dp0 = kzalloc(sizeof(*dp0), GFP_KERNEL);
+ if (!dp0)
+ return NULL;
+
+ dp0->dev.type = &sdw_dp0_type;
+ dp0->dev.parent = &slave->dev;
+ dp0->dev.groups = dp0_groups;
+ dev_set_name(&dp0->dev, "dp0");
+ dp0->dp0_prop = prop;
+
+ err = device_register(&dp0->dev);
+ if (err) {
+ put_device(&dp0->dev);
+ dp0 = NULL;
+ }
+
+ return dp0;
+}
+
+void sdw_sysfs_slave_dp0_exit(struct sdw_slave_sysfs *sysfs)
+{
+ if (sysfs->dp0)
+ put_device(&sysfs->dp0->dev);
+}
diff --git a/drivers/soundwire/sysfs_slave_dpn.c b/drivers/soundwire/sysfs_slave_dpn.c
new file mode 100644
index 000000000000..a200898e8b5f
--- /dev/null
+++ b/drivers/soundwire/sysfs_slave_dpn.c
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2015-19 Intel Corporation.
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_type.h>
+#include "bus.h"
+#include "sysfs_local.h"
+
+/*
+ * DP-N properties
+ */
+
+#define to_sdw_dpn(_dev) \
+ container_of(_dev, struct sdw_dpn_sysfs, dev)
+
+#define sdw_dpn_attr(field, format_string) \
+static ssize_t field##_show(struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+{ \
+ struct sdw_dpn_sysfs *sysfs = to_sdw_dpn(dev); \
+ return sprintf(buf, format_string, sysfs->dpn_prop->field); \
+} \
+static DEVICE_ATTR_RO(field)
+
+sdw_dpn_attr(max_word, "%d\n");
+sdw_dpn_attr(min_word, "%d\n");
+sdw_dpn_attr(max_grouping, "%d\n");
+sdw_dpn_attr(imp_def_interrupts, "%d\n");
+sdw_dpn_attr(max_ch, "%d\n");
+sdw_dpn_attr(min_ch, "%d\n");
+sdw_dpn_attr(modes, "%d\n");
+sdw_dpn_attr(max_async_buffer, "%d\n");
+sdw_dpn_attr(block_pack_mode, "%d\n");
+sdw_dpn_attr(port_encoding, "%d\n");
+sdw_dpn_attr(simple_ch_prep_sm, "%d\n");
+sdw_dpn_attr(ch_prep_timeout, "%d\n");
+
+static ssize_t words_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct sdw_dpn_sysfs *sysfs = to_sdw_dpn(dev);
+ ssize_t size = 0;
+ int i;
+
+ for (i = 0; i < sysfs->dpn_prop->num_words; i++)
+ size += sprintf(buf + size, "%d ", sysfs->dpn_prop->words[i]);
+ size += sprintf(buf + size, "\n");
+
+ return size;
+}
+static DEVICE_ATTR_RO(words);
+
+static ssize_t channels_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct sdw_dpn_sysfs *sysfs = to_sdw_dpn(dev);
+ ssize_t size = 0;
+ int i;
+
+ for (i = 0; i < sysfs->dpn_prop->num_ch; i++)
+ size += sprintf(buf + size, "%d ", sysfs->dpn_prop->ch[i]);
+ size += sprintf(buf + size, "\n");
+
+ return size;
+}
+static DEVICE_ATTR_RO(channels);
+
+static ssize_t ch_combinations_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct sdw_dpn_sysfs *sysfs = to_sdw_dpn(dev);
+ ssize_t size = 0;
+ int i;
+
+ for (i = 0; i < sysfs->dpn_prop->num_ch_combinations; i++)
+ size += sprintf(buf + size, "%d ",
+ sysfs->dpn_prop->ch_combinations[i]);
+ size += sprintf(buf + size, "\n");
+
+ return size;
+}
+static DEVICE_ATTR_RO(ch_combinations);
+
+static struct attribute *dpn_attrs[] = {
+ &dev_attr_max_word.attr,
+ &dev_attr_min_word.attr,
+ &dev_attr_max_grouping.attr,
+ &dev_attr_imp_def_interrupts.attr,
+ &dev_attr_max_ch.attr,
+ &dev_attr_min_ch.attr,
+ &dev_attr_modes.attr,
+ &dev_attr_max_async_buffer.attr,
+ &dev_attr_block_pack_mode.attr,
+ &dev_attr_port_encoding.attr,
+ &dev_attr_simple_ch_prep_sm.attr,
+ &dev_attr_ch_prep_timeout.attr,
+ &dev_attr_words.attr,
+ &dev_attr_channels.attr,
+ &dev_attr_ch_combinations.attr,
+ NULL,
+};
+
+static const struct attribute_group dpn_group = {
+ .attrs = dpn_attrs,
+};
+
+static const struct attribute_group *dpn_groups[] = {
+ &dpn_group,
+ NULL
+};
+
+static void sdw_dpn_release(struct device *dev)
+{
+ struct sdw_dpn_sysfs *sysfs = to_sdw_dpn(dev);
+
+ kfree(sysfs);
+}
+
+struct device_type sdw_dpn_type = {
+ .name = "sdw_dpn",
+ .release = sdw_dpn_release,
+};
+
+struct sdw_dpn_sysfs
+*sdw_sysfs_slave_dpn_init(struct sdw_slave *slave,
+ struct sdw_dpn_prop *prop,
+ bool src)
+{
+ struct sdw_dpn_sysfs *dpn;
+ int err;
+
+ dpn = kzalloc(sizeof(*dpn), GFP_KERNEL);
+ if (!dpn)
+ return NULL;
+
+ dpn->dev.type = &sdw_dpn_type;
+ dpn->dev.parent = &slave->dev;
+ dpn->dev.groups = dpn_groups;
+ dpn->dpn_prop = prop;
+
+ if (src)
+ dev_set_name(&dpn->dev, "src-dp%x", prop->num);
+ else
+ dev_set_name(&dpn->dev, "sink-dp%x", prop->num);
+
+ err = device_register(&dpn->dev);
+ if (err) {
+ put_device(&dpn->dev);
+ dpn = NULL;
+ }
+
+ return dpn;
+}
+
+void sdw_sysfs_slave_dpn_exit(struct sdw_slave_sysfs *sysfs)
+{
+ int i;
+
+ for (i = 0; i < sysfs->num_dpns; i++) {
+ if (sysfs->dpns[i])
+ put_device(&sysfs->dpns[i]->dev);
+ }
+}
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index b64d46fed0c8..fae3a3ad6e68 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -405,10 +405,13 @@ int sdw_slave_read_dp0(struct sdw_slave *slave,
/*
* SDW sysfs APIs
*/
+struct sdw_slave_sysfs;
struct sdw_master_sysfs;
int sdw_sysfs_bus_init(struct sdw_bus *bus);
void sdw_sysfs_bus_exit(struct sdw_bus *bus);
+int sdw_sysfs_slave_init(struct sdw_slave *slave);
+void sdw_sysfs_slave_exit(struct sdw_slave *slave);
/*
* SDW Slave Structures and APIs
@@ -552,6 +555,7 @@ struct sdw_slave_ops {
* @bus: Bus handle
* @ops: Slave callback ops
* @prop: Slave properties
+ * @sysfs: Sysfs interface
* @node: node for bus list
* @port_ready: Port ready completion flag for each Slave port
* @dev_num: Device Number assigned by Bus
@@ -563,6 +567,7 @@ struct sdw_slave {
struct sdw_bus *bus;
const struct sdw_slave_ops *ops;
struct sdw_slave_prop prop;
+ struct sdw_slave_sysfs *sysfs;
struct list_head node;
struct completion *port_ready;
u16 dev_num;
--
2.17.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 2/7] soundwire: add Slave sysfs support
2019-05-04 1:00 ` [RFC PATCH 2/7] soundwire: add Slave sysfs support Pierre-Louis Bossart
@ 2019-05-04 6:54 ` Greg KH
2019-05-06 14:42 ` [alsa-devel] " Pierre-Louis Bossart
0 siblings, 1 reply; 43+ messages in thread
From: Greg KH @ 2019-05-04 6:54 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, linux-kernel, tiwai, broonie, vkoul, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Sanyog Kale
On Fri, May 03, 2019 at 08:00:25PM -0500, Pierre-Louis Bossart wrote:
> Add DisCo Slave properties as device attributes.
> Add a device for Data Port 0 which adds Dp0 properties as attributes.
> Add devices for Source and Sink Data Ports, and add Dp-N
> properties as attributes.
>
> The Slave, DP0 and DPn cases are intentionally handled in separate
> files to avoid conflicts with attributes having the same names at
> different levels.
>
> Audio modes are not supported for now. Depending on the discussions
> the SoundWire Device Class, we may add it later as is or follow the
> new specification.
>
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
> drivers/soundwire/Makefile | 2 +-
> drivers/soundwire/bus.c | 2 +
> drivers/soundwire/bus.h | 2 +
> drivers/soundwire/bus_type.c | 5 +
> drivers/soundwire/slave.c | 1 +
> drivers/soundwire/sysfs.c | 213 ++++++++++++++++++++++++++++
> drivers/soundwire/sysfs_local.h | 42 ++++++
> drivers/soundwire/sysfs_slave_dp0.c | 112 +++++++++++++++
> drivers/soundwire/sysfs_slave_dpn.c | 168 ++++++++++++++++++++++
> include/linux/soundwire/sdw.h | 5 +
> 10 files changed, 551 insertions(+), 1 deletion(-)
> create mode 100644 drivers/soundwire/sysfs_local.h
> create mode 100644 drivers/soundwire/sysfs_slave_dp0.c
> create mode 100644 drivers/soundwire/sysfs_slave_dpn.c
>
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index 787f1cbf342c..a72a29731a28 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -4,7 +4,7 @@
>
> #Bus Objs
> soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
> - sysfs.o
> + sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o
> obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
>
> #Cadence Objs
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 38de7071e135..dd9181693554 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -113,6 +113,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
> struct sdw_slave *slave = dev_to_sdw_dev(dev);
> struct sdw_bus *bus = slave->bus;
>
> + sdw_sysfs_slave_exit(slave);
> +
> mutex_lock(&bus->bus_lock);
>
> if (slave->dev_num) /* clear dev_num if assigned */
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 3048ca153f22..0707e68a8d21 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -18,6 +18,8 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
> void sdw_extract_slave_id(struct sdw_bus *bus,
> u64 addr, struct sdw_slave_id *id);
>
> +extern const struct attribute_group *sdw_slave_dev_attr_groups[];
> +
> enum {
> SDW_MSG_FLAG_READ = 0,
> SDW_MSG_FLAG_WRITE,
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index 2655602f0cfb..f68fe45c1037 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -97,6 +97,11 @@ static int sdw_drv_probe(struct device *dev)
> if (slave->ops && slave->ops->read_prop)
> slave->ops->read_prop(slave);
>
> + /* init the sysfs as we have properties now */
> + ret = sdw_sysfs_slave_init(slave);
> + if (ret < 0)
> + dev_warn(dev, "Slave sysfs init failed:%d\n", ret);
> +
> /*
> * Check for valid clk_stop_timeout, use DisCo worst case value of
> * 300ms
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index f39a5815e25d..bad73a267fdd 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -34,6 +34,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
> id->class_id, id->unique_id);
>
> slave->dev.release = sdw_slave_release;
> + slave->dev.groups = sdw_slave_dev_attr_groups;
> slave->dev.bus = &sdw_bus_type;
> slave->bus = bus;
> slave->status = SDW_SLAVE_UNATTACHED;
> diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
> index 7b6c3826a73a..734e2c8bc5cd 100644
> --- a/drivers/soundwire/sysfs.c
> +++ b/drivers/soundwire/sysfs.c
> @@ -8,6 +8,7 @@
> #include <linux/soundwire/sdw.h>
> #include <linux/soundwire/sdw_type.h>
> #include "bus.h"
> +#include "sysfs_local.h"
>
> struct sdw_master_sysfs {
> struct device dev;
> @@ -160,3 +161,215 @@ void sdw_sysfs_bus_exit(struct sdw_bus *bus)
> put_device(&master->dev);
> bus->sysfs = NULL;
> }
> +
> +/*
> + * Slave sysfs
> + */
> +
> +/*
> + * The sysfs for Slave reflects the MIPI description as given
> + * in the MIPI DisCo spec
> + *
> + * Base file is device
> + * |---- mipi_revision
> + * |---- wake_capable
> + * |---- test_mode_capable
> + * |---- simple_clk_stop_capable
> + * |---- clk_stop_timeout
> + * |---- ch_prep_timeout
> + * |---- reset_behave
> + * |---- high_PHY_capable
> + * |---- paging_support
> + * |---- bank_delay_support
> + * |---- p15_behave
> + * |---- master_count
> + * |---- source_ports
> + * |---- sink_ports
> + * |---- dp0
> + * |---- max_word
> + * |---- min_word
> + * |---- words
> + * |---- flow_controlled
> + * |---- simple_ch_prep_sm
> + * |---- device_interrupts
> + * |---- dpN
> + * |---- max_word
> + * |---- min_word
> + * |---- words
> + * |---- type
> + * |---- max_grouping
> + * |---- simple_ch_prep_sm
> + * |---- ch_prep_timeout
> + * |---- device_interrupts
> + * |---- max_ch
> + * |---- min_ch
> + * |---- ch
> + * |---- ch_combinations
> + * |---- modes
> + * |---- max_async_buffer
> + * |---- block_pack_mode
> + * |---- port_encoding
> + * |---- audio_modeM
> + * |---- bus_min_freq
> + * |---- bus_max_freq
> + * |---- bus_freq
> + * |---- max_freq
> + * |---- min_freq
> + * |---- freq
> + * |---- prep_ch_behave
> + * |---- glitchless
> + *
> + */
> +
> +#define sdw_slave_attr(field, format_string) \
> +static ssize_t field##_show(struct device *dev, \
> + struct device_attribute *attr, \
> + char *buf) \
> +{ \
> + struct sdw_slave *slave = dev_to_sdw_dev(dev); \
> + return sprintf(buf, format_string, slave->prop.field); \
> +} \
> +static DEVICE_ATTR_RO(field)
> +
> +sdw_slave_attr(mipi_revision, "0x%x\n");
> +sdw_slave_attr(wake_capable, "%d\n");
> +sdw_slave_attr(test_mode_capable, "%d\n");
> +sdw_slave_attr(clk_stop_mode1, "%d\n");
> +sdw_slave_attr(simple_clk_stop_capable, "%d\n");
> +sdw_slave_attr(clk_stop_timeout, "%d\n");
> +sdw_slave_attr(ch_prep_timeout, "%d\n");
> +sdw_slave_attr(reset_behave, "%d\n");
> +sdw_slave_attr(high_PHY_capable, "%d\n");
> +sdw_slave_attr(paging_support, "%d\n");
> +sdw_slave_attr(bank_delay_support, "%d\n");
> +sdw_slave_attr(p15_behave, "%d\n");
> +sdw_slave_attr(master_count, "%d\n");
> +sdw_slave_attr(source_ports, "%d\n");
> +sdw_slave_attr(sink_ports, "%d\n");
> +
> +static ssize_t modalias_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> + return sdw_slave_modalias(slave, buf, 256);
> +}
> +static DEVICE_ATTR_RO(modalias);
> +
> +static struct attribute *slave_dev_attrs[] = {
> + &dev_attr_mipi_revision.attr,
> + &dev_attr_wake_capable.attr,
> + &dev_attr_test_mode_capable.attr,
> + &dev_attr_clk_stop_mode1.attr,
> + &dev_attr_simple_clk_stop_capable.attr,
> + &dev_attr_clk_stop_timeout.attr,
> + &dev_attr_ch_prep_timeout.attr,
> + &dev_attr_reset_behave.attr,
> + &dev_attr_high_PHY_capable.attr,
> + &dev_attr_paging_support.attr,
> + &dev_attr_bank_delay_support.attr,
> + &dev_attr_p15_behave.attr,
> + &dev_attr_master_count.attr,
> + &dev_attr_source_ports.attr,
> + &dev_attr_sink_ports.attr,
> + &dev_attr_modalias.attr,
> + NULL,
> +};
> +
> +static struct attribute_group sdw_slave_dev_attr_group = {
> + .attrs = slave_dev_attrs,
> +};
> +
> +const struct attribute_group *sdw_slave_dev_attr_groups[] = {
> + &sdw_slave_dev_attr_group,
> + NULL
> +};
ATTRIBUTE_GROUP()?
> +
> +int sdw_sysfs_slave_init(struct sdw_slave *slave)
> +{
> + struct sdw_slave_sysfs *sysfs;
> + unsigned int src_dpns, sink_dpns, i, j;
> + int err;
> +
> + if (slave->sysfs) {
> + dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
> + err = -EIO;
> + goto err_ret;
> + }
> +
> + sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
Same question as patch 1, why a new device?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support
2019-05-04 6:54 ` Greg KH
@ 2019-05-06 14:42 ` Pierre-Louis Bossart
2019-05-06 15:19 ` Greg KH
0 siblings, 1 reply; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 14:42 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, vkoul, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
>> +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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support
2019-05-06 14:42 ` [alsa-devel] " Pierre-Louis Bossart
@ 2019-05-06 15:19 ` Greg KH
2019-05-06 16:22 ` Vinod Koul
0 siblings, 1 reply; 43+ messages in thread
From: Greg KH @ 2019-05-06 15:19 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, vkoul, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support
2019-05-06 15:19 ` Greg KH
@ 2019-05-06 16:22 ` Vinod Koul
2019-05-06 16:46 ` Pierre-Louis Bossart
0 siblings, 1 reply; 43+ messages in thread
From: Vinod Koul @ 2019-05-06 16:22 UTC (permalink / raw)
To: Greg KH
Cc: Pierre-Louis Bossart, alsa-devel, tiwai, linux-kernel,
liam.r.girdwood, broonie, srinivas.kandagatla, jank, joe,
Sanyog Kale
On 06-05-19, 17:19, Greg KH wrote:
> On Mon, May 06, 2019 at 09:42:35AM -0500, Pierre-Louis Bossart wrote:
> > > > +
> > > > +int sdw_sysfs_slave_init(struct sdw_slave *slave)
> > > > +{
> > > > + struct sdw_slave_sysfs *sysfs;
> > > > + unsigned int src_dpns, sink_dpns, i, j;
> > > > + int err;
> > > > +
> > > > + if (slave->sysfs) {
> > > > + dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
> > > > + err = -EIO;
> > > > + goto err_ret;
> > > > + }
> > > > +
> > > > + sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
> > >
> > > Same question as patch 1, why a new device?
> >
> > yes it's the same open. In this case, the slave devices are defined at a
> > different level so it's also confusing to create a device to represent the
> > slave properties. The code works but I am not sure the initial directions
> > are correct.
>
> You can just make a subdir for your attributes by using the attribute
> group name, if a subdirectory is needed just to keep things a bit more
> organized.
The key here is 'a subdir' which is not the case here. We did discuss
this in the initial patches for SoundWire which had sysfs :)
The way MIPI disco spec organized properties, we have dp0 and dpN
properties each of them requires to have a subdir of their own and that
was the reason why I coded it to be creating a device.
Do we have a better way to handle this?
> Otherwise, you need to mess with having multiple "types" of struct
> device all associated with the same bus. It is possible, and not that
> hard, but I don't think you are doing that here.
>
> thnaks,
>
> greg k-h
--
~Vinod
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support
2019-05-06 16:22 ` Vinod Koul
@ 2019-05-06 16:46 ` Pierre-Louis Bossart
2019-05-07 5:19 ` Vinod Koul
0 siblings, 1 reply; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 16:46 UTC (permalink / raw)
To: Vinod Koul, Greg KH
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
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
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support
2019-05-06 16:46 ` Pierre-Louis Bossart
@ 2019-05-07 5:19 ` Vinod Koul
2019-05-07 13:54 ` Pierre-Louis Bossart
0 siblings, 1 reply; 43+ messages in thread
From: Vinod Koul @ 2019-05-07 5:19 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Greg KH, alsa-devel, tiwai, linux-kernel, liam.r.girdwood,
broonie, srinivas.kandagatla, jank, joe, Sanyog Kale
On 06-05-19, 11:46, Pierre-Louis Bossart wrote:
> On 5/6/19 11:22 AM, Vinod Koul wrote:
> > On 06-05-19, 17:19, Greg KH wrote:
> > > On Mon, May 06, 2019 at 09:42:35AM -0500, Pierre-Louis Bossart wrote:
> > > > > > +
> > > > > > +int sdw_sysfs_slave_init(struct sdw_slave *slave)
> > > > > > +{
> > > > > > + struct sdw_slave_sysfs *sysfs;
> > > > > > + unsigned int src_dpns, sink_dpns, i, j;
> > > > > > + int err;
> > > > > > +
> > > > > > + if (slave->sysfs) {
> > > > > > + dev_err(&slave->dev, "SDW Slave sysfs is already initialized\n");
> > > > > > + err = -EIO;
> > > > > > + goto err_ret;
> > > > > > + }
> > > > > > +
> > > > > > + sysfs = kzalloc(sizeof(*sysfs), GFP_KERNEL);
> > > > >
> > > > > Same question as patch 1, why a new device?
> > > >
> > > > yes it's the same open. In this case, the slave devices are defined at a
> > > > different level so it's also confusing to create a device to represent the
> > > > slave properties. The code works but I am not sure the initial directions
> > > > are correct.
> > >
> > > You can just make a subdir for your attributes by using the attribute
> > > group name, if a subdirectory is needed just to keep things a bit more
> > > organized.
> >
> > The key here is 'a subdir' which is not the case here. We did discuss
> > this in the initial patches for SoundWire which had sysfs :)
> >
> > The way MIPI disco spec organized properties, we have dp0 and dpN
> > properties each of them requires to have a subdir of their own and that
> > was the reason why I coded it to be creating a device.
>
> Vinod, the question was not for dp0 and dpN, it's fine to have
> subdirectories there, but rather why we need separate devices for the master
> and slave properties.
Slave does not have a separate device. IIRC the properties for Slave are
in /sys/bus/soundwire/device/<slave>/...
For master yes we can skip the device creation, it was done for
consistency sake of having these properties ties into sys/bus/soundwire/
I don't mind if they are shown up in respective device node (PCI/platform
etc) /sys/bus/foo/device/<>
But for creating subdirectories you would need the new dpX devices.
HTH
>
> >
> > Do we have a better way to handle this?
> >
> > > Otherwise, you need to mess with having multiple "types" of struct
> > > device all associated with the same bus. It is possible, and not that
> > > hard, but I don't think you are doing that here.
> > >
> > > thnaks,
> > >
> > > greg k-h
> >
--
~Vinod
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support
2019-05-07 5:19 ` Vinod Koul
@ 2019-05-07 13:54 ` Pierre-Louis Bossart
2019-05-08 7:40 ` Vinod Koul
0 siblings, 1 reply; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-07 13:54 UTC (permalink / raw)
To: Vinod Koul
Cc: alsa-devel, tiwai, Greg KH, linux-kernel, liam.r.girdwood,
broonie, srinivas.kandagatla, jank, joe, Sanyog Kale
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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support
2019-05-07 13:54 ` Pierre-Louis Bossart
@ 2019-05-08 7:40 ` Vinod Koul
2019-05-08 16:51 ` Pierre-Louis Bossart
0 siblings, 1 reply; 43+ messages in thread
From: Vinod Koul @ 2019-05-08 7:40 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, tiwai, Greg KH, linux-kernel, liam.r.girdwood,
broonie, srinivas.kandagatla, jank, joe, Sanyog Kale
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 2/7] soundwire: add Slave sysfs support
2019-05-08 7:40 ` Vinod Koul
@ 2019-05-08 16:51 ` Pierre-Louis Bossart
0 siblings, 0 replies; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-08 16:51 UTC (permalink / raw)
To: Vinod Koul
Cc: alsa-devel, tiwai, Greg KH, linux-kernel, liam.r.girdwood,
broonie, srinivas.kandagatla, jank, joe, Sanyog Kale
>>>> 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?
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC PATCH 3/7] ABI: testing: Add description of soundwire master sysfs files
2019-05-04 1:00 [RFC PATCH 0/7] soundwire: add sysfs and debugfs support Pierre-Louis Bossart
2019-05-04 1:00 ` [RFC PATCH 1/7] soundwire: Add sysfs support for master(s) Pierre-Louis Bossart
2019-05-04 1:00 ` [RFC PATCH 2/7] soundwire: add Slave sysfs support Pierre-Louis Bossart
@ 2019-05-04 1:00 ` Pierre-Louis Bossart
2019-05-04 6:53 ` Greg KH
2019-05-06 16:24 ` Vinod Koul
2019-05-04 1:00 ` [RFC PATCH 4/7] ABI: testing: Add description of soundwire slave " Pierre-Louis Bossart
` (3 subsequent siblings)
6 siblings, 2 replies; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-04 1:00 UTC (permalink / raw)
To: alsa-devel
Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Pierre-Louis Bossart,
Sanyog Kale
The description is directly derived from the MIPI DisCo specification.
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
.../ABI/testing/sysfs-bus-soundwire-master | 21 +++++++++++++++++++
drivers/soundwire/sysfs.c | 1 +
2 files changed, 22 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-soundwire-master
diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-master b/Documentation/ABI/testing/sysfs-bus-soundwire-master
new file mode 100644
index 000000000000..69cadf31049d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-soundwire-master
@@ -0,0 +1,21 @@
+What: /sys/bus/soundwire/devices/sdw-master-N/revision
+ /sys/bus/soundwire/devices/sdw-master-N/clk_stop_modes
+ /sys/bus/soundwire/devices/sdw-master-N/clk_freq
+ /sys/bus/soundwire/devices/sdw-master-N/clk_gears
+ /sys/bus/soundwire/devices/sdw-master-N/default_col
+ /sys/bus/soundwire/devices/sdw-master-N/default_frame_rate
+ /sys/bus/soundwire/devices/sdw-master-N/default_row
+ /sys/bus/soundwire/devices/sdw-master-N/dynamic_shape
+ /sys/bus/soundwire/devices/sdw-master-N/err_threshold
+ /sys/bus/soundwire/devices/sdw-master-N/max_clk_freq
+
+Date: May 2019
+
+Contact: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
+
+Description: SoundWire Master-N DisCo properties.
+ These properties are defined by MIPI DisCo Specification
+ for SoundWire. They define various properties of the Master
+ and are used by the bus to configure the Master. clk_stop_modes
+ is a bitmask for simplifications and combines the
+ clock-stop-mode0 and clock-stop-mode1 properties.
diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
index 734e2c8bc5cd..c2e5b7ad42fb 100644
--- a/drivers/soundwire/sysfs.c
+++ b/drivers/soundwire/sysfs.c
@@ -31,6 +31,7 @@ struct sdw_master_sysfs {
* |---- clk_gears
* |---- default_row
* |---- default_col
+ * |---- default_frame_shape
* |---- dynamic_shape
* |---- err_threshold
*/
--
2.17.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 3/7] ABI: testing: Add description of soundwire master sysfs files
2019-05-04 1:00 ` [RFC PATCH 3/7] ABI: testing: Add description of soundwire master sysfs files Pierre-Louis Bossart
@ 2019-05-04 6:53 ` Greg KH
2019-05-06 16:24 ` Vinod Koul
1 sibling, 0 replies; 43+ messages in thread
From: Greg KH @ 2019-05-04 6:53 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, linux-kernel, tiwai, broonie, vkoul, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Sanyog Kale
On Fri, May 03, 2019 at 08:00:26PM -0500, Pierre-Louis Bossart wrote:
> The description is directly derived from the MIPI DisCo specification.
>
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
> .../ABI/testing/sysfs-bus-soundwire-master | 21 +++++++++++++++++++
> drivers/soundwire/sysfs.c | 1 +
> 2 files changed, 22 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-soundwire-master
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-master b/Documentation/ABI/testing/sysfs-bus-soundwire-master
> new file mode 100644
> index 000000000000..69cadf31049d
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-soundwire-master
> @@ -0,0 +1,21 @@
> +What: /sys/bus/soundwire/devices/sdw-master-N/revision
> + /sys/bus/soundwire/devices/sdw-master-N/clk_stop_modes
> + /sys/bus/soundwire/devices/sdw-master-N/clk_freq
> + /sys/bus/soundwire/devices/sdw-master-N/clk_gears
> + /sys/bus/soundwire/devices/sdw-master-N/default_col
> + /sys/bus/soundwire/devices/sdw-master-N/default_frame_rate
> + /sys/bus/soundwire/devices/sdw-master-N/default_row
> + /sys/bus/soundwire/devices/sdw-master-N/dynamic_shape
> + /sys/bus/soundwire/devices/sdw-master-N/err_threshold
> + /sys/bus/soundwire/devices/sdw-master-N/max_clk_freq
> +
> +Date: May 2019
> +
> +Contact: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> +
> +Description: SoundWire Master-N DisCo properties.
> + These properties are defined by MIPI DisCo Specification
> + for SoundWire. They define various properties of the Master
> + and are used by the bus to configure the Master. clk_stop_modes
> + is a bitmask for simplifications and combines the
> + clock-stop-mode0 and clock-stop-mode1 properties.
> diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
> index 734e2c8bc5cd..c2e5b7ad42fb 100644
> --- a/drivers/soundwire/sysfs.c
> +++ b/drivers/soundwire/sysfs.c
> @@ -31,6 +31,7 @@ struct sdw_master_sysfs {
> * |---- clk_gears
> * |---- default_row
> * |---- default_col
> + * |---- default_frame_shape
> * |---- dynamic_shape
> * |---- err_threshold
> */
This last chunk should go in patch 1 of this series, right?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 3/7] ABI: testing: Add description of soundwire master sysfs files
2019-05-04 1:00 ` [RFC PATCH 3/7] ABI: testing: Add description of soundwire master sysfs files Pierre-Louis Bossart
2019-05-04 6:53 ` Greg KH
@ 2019-05-06 16:24 ` Vinod Koul
1 sibling, 0 replies; 43+ messages in thread
From: Vinod Koul @ 2019-05-06 16:24 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, linux-kernel, tiwai, broonie, gregkh,
liam.r.girdwood, jank, joe, srinivas.kandagatla, Sanyog Kale
On 03-05-19, 20:00, Pierre-Louis Bossart wrote:
> The description is directly derived from the MIPI DisCo specification.
>
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
> .../ABI/testing/sysfs-bus-soundwire-master | 21 +++++++++++++++++++
> drivers/soundwire/sysfs.c | 1 +
> 2 files changed, 22 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-soundwire-master
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-master b/Documentation/ABI/testing/sysfs-bus-soundwire-master
> new file mode 100644
> index 000000000000..69cadf31049d
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-soundwire-master
> @@ -0,0 +1,21 @@
> +What: /sys/bus/soundwire/devices/sdw-master-N/revision
> + /sys/bus/soundwire/devices/sdw-master-N/clk_stop_modes
> + /sys/bus/soundwire/devices/sdw-master-N/clk_freq
> + /sys/bus/soundwire/devices/sdw-master-N/clk_gears
> + /sys/bus/soundwire/devices/sdw-master-N/default_col
> + /sys/bus/soundwire/devices/sdw-master-N/default_frame_rate
> + /sys/bus/soundwire/devices/sdw-master-N/default_row
> + /sys/bus/soundwire/devices/sdw-master-N/dynamic_shape
> + /sys/bus/soundwire/devices/sdw-master-N/err_threshold
> + /sys/bus/soundwire/devices/sdw-master-N/max_clk_freq
> +
> +Date: May 2019
> +
> +Contact: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
As the author of original code, it would be great if you can add me as a
contact as well.
> +
> +Description: SoundWire Master-N DisCo properties.
> + These properties are defined by MIPI DisCo Specification
> + for SoundWire. They define various properties of the Master
> + and are used by the bus to configure the Master. clk_stop_modes
> + is a bitmask for simplifications and combines the
> + clock-stop-mode0 and clock-stop-mode1 properties.
> diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c
> index 734e2c8bc5cd..c2e5b7ad42fb 100644
> --- a/drivers/soundwire/sysfs.c
> +++ b/drivers/soundwire/sysfs.c
> @@ -31,6 +31,7 @@ struct sdw_master_sysfs {
> * |---- clk_gears
> * |---- default_row
> * |---- default_col
> + * |---- default_frame_shape
This should be folded into 1st patch
> * |---- dynamic_shape
> * |---- err_threshold
> */
> --
> 2.17.1
--
~Vinod
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC PATCH 4/7] ABI: testing: Add description of soundwire slave sysfs files
2019-05-04 1:00 [RFC PATCH 0/7] soundwire: add sysfs and debugfs support Pierre-Louis Bossart
` (2 preceding siblings ...)
2019-05-04 1:00 ` [RFC PATCH 3/7] ABI: testing: Add description of soundwire master sysfs files Pierre-Louis Bossart
@ 2019-05-04 1:00 ` Pierre-Louis Bossart
2019-05-04 1:00 ` [RFC PATCH 5/7] soundwire: add debugfs support Pierre-Louis Bossart
` (2 subsequent siblings)
6 siblings, 0 replies; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-04 1:00 UTC (permalink / raw)
To: alsa-devel
Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Pierre-Louis Bossart
The description is directly derived from the MIPI DisCo specification.
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
.../ABI/testing/sysfs-bus-soundwire-slave | 85 +++++++++++++++++++
1 file changed, 85 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-soundwire-slave
diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-slave b/Documentation/ABI/testing/sysfs-bus-soundwire-slave
new file mode 100644
index 000000000000..db2a0177f68f
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-soundwire-slave
@@ -0,0 +1,85 @@
+What: /sys/bus/soundwire/devices/sdw:.../bank_delay_support
+ /sys/bus/soundwire/devices/sdw:.../ch_prep_timeout
+ /sys/bus/soundwire/devices/sdw:.../clk_stop_mode1
+ /sys/bus/soundwire/devices/sdw:.../clk_stop_timeout
+ /sys/bus/soundwire/devices/sdw:.../high_PHY_capable
+ /sys/bus/soundwire/devices/sdw:.../master_count
+ /sys/bus/soundwire/devices/sdw:.../mipi_revision
+ /sys/bus/soundwire/devices/sdw:.../p15_behave
+ /sys/bus/soundwire/devices/sdw:.../paging_support
+ /sys/bus/soundwire/devices/sdw:.../reset_behave
+ /sys/bus/soundwire/devices/sdw:.../simple_clk_stop_capable
+ /sys/bus/soundwire/devices/sdw:.../sink_ports
+ /sys/bus/soundwire/devices/sdw:.../source_ports
+ /sys/bus/soundwire/devices/sdw:.../test_mode_capable
+ /sys/bus/soundwire/devices/sdw:.../wake_capable
+
+Date: May 2019
+
+Contact: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
+
+Description: SoundWire Slave DisCo properties.
+ These properties are defined by MIPI DisCo Specification
+ for SoundWire. They define various properties of the
+ SoundWire Slave and are used by the bus to configure
+ the Slave
+
+
+What: /sys/bus/soundwire/devices/sdw:.../dp0/BRA_flow_controlled
+ /sys/bus/soundwire/devices/sdw:.../dp0/imp_def_interrupts
+ /sys/bus/soundwire/devices/sdw:.../dp0/max_word
+ /sys/bus/soundwire/devices/sdw:.../dp0/min_word
+ /sys/bus/soundwire/devices/sdw:.../dp0/simple_ch_prep_sm
+ /sys/bus/soundwire/devices/sdw:.../dp0/word_bits
+
+Date: May 2019
+
+Contact: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
+
+Description: SoundWire Slave Data Port-0 DisCo properties.
+ These properties are defined by MIPI DisCo Specification
+ for the SoundWire. They define various properties of the
+ Data port 0 are used by the bus to configure the Data Port 0.
+
+
+What: /sys/bus/soundwire/devices/sdw:.../src-dpN/block_pack_mode
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/channels
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/ch_combinations
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/ch_prep_timeout
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/imp_def_interrupts
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/max_async_buffer
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/max_ch
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/max_grouping
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/max_word
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/min_ch
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/min_word
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/modes
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/port_encoding
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/simple_ch_prep_sm
+ /sys/bus/soundwire/devices/sdw:.../src-dpN/words
+
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/block_pack_mode
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/channels
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/ch_combinations
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/ch_prep_timeout
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/imp_def_interrupts
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/max_async_buffer
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/max_ch
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/max_grouping
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/max_word
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/min_ch
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/min_word
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/modes
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/port_encoding
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/simple_ch_prep_sm
+ /sys/bus/soundwire/devices/sdw:.../sink-dpN/words
+
+Date: May 2019
+
+Contact: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
+
+Description: SoundWire Slave Data Source/Sink Port-N DisCo properties.
+ These properties are defined by MIPI DisCo Specification
+ for SoundWire. They define various properties of the
+ Source/Sink Data port N and are used by the bus to configure
+ the Data Port N.
--
2.17.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [RFC PATCH 5/7] soundwire: add debugfs support
2019-05-04 1:00 [RFC PATCH 0/7] soundwire: add sysfs and debugfs support Pierre-Louis Bossart
` (3 preceding siblings ...)
2019-05-04 1:00 ` [RFC PATCH 4/7] ABI: testing: Add description of soundwire slave " Pierre-Louis Bossart
@ 2019-05-04 1:00 ` Pierre-Louis Bossart
2019-05-04 7:03 ` Greg KH
2019-05-04 1:00 ` [RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump Pierre-Louis Bossart
2019-05-04 1:00 ` [RFC PATCH 7/7] soundwire: intel: " Pierre-Louis Bossart
6 siblings, 1 reply; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-04 1:00 UTC (permalink / raw)
To: alsa-devel
Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Pierre-Louis Bossart,
Sanyog Kale
Add base debugfs mechanism for SoundWire bus by creating soundwire
root and master-N and slave-x hierarchy.
Also add SDW Slave SCP, DP0 and DP-N register debug file.
Registers not implemented will print as "XX"
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
is the use of scnprintf to avoid known issues with snprintf.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
drivers/soundwire/Makefile | 3 +-
drivers/soundwire/bus.c | 5 +
drivers/soundwire/bus.h | 28 +++++
drivers/soundwire/bus_type.c | 3 +
drivers/soundwire/debugfs.c | 214 ++++++++++++++++++++++++++++++++++
drivers/soundwire/slave.c | 1 +
include/linux/soundwire/sdw.h | 9 ++
7 files changed, 262 insertions(+), 1 deletion(-)
create mode 100644 drivers/soundwire/debugfs.c
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index a72a29731a28..f2c425fa15bd 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -4,7 +4,8 @@
#Bus Objs
soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
- sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o
+ sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o \
+ debugfs.o
obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
#Cadence Objs
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index dd9181693554..b79eba321b71 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -53,6 +53,8 @@ int sdw_add_bus_master(struct sdw_bus *bus)
if (ret < 0)
dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret);
+ bus->debugfs = sdw_bus_debugfs_init(bus);
+
/*
* Device numbers in SoundWire are 0 through 15. Enumeration device
* number (0), Broadcast device number (15), Group numbers (12 and
@@ -114,6 +116,7 @@ static int sdw_delete_slave(struct device *dev, void *data)
struct sdw_bus *bus = slave->bus;
sdw_sysfs_slave_exit(slave);
+ sdw_slave_debugfs_exit(slave->debugfs);
mutex_lock(&bus->bus_lock);
@@ -136,6 +139,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
void sdw_delete_bus_master(struct sdw_bus *bus)
{
sdw_sysfs_bus_exit(bus);
+ if (bus->debugfs)
+ sdw_bus_debugfs_exit(bus->debugfs);
device_for_each_child(bus->dev, NULL, sdw_delete_slave);
}
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 0707e68a8d21..f0b1f4f9d7b2 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -20,6 +20,34 @@ void sdw_extract_slave_id(struct sdw_bus *bus,
extern const struct attribute_group *sdw_slave_dev_attr_groups[];
+#ifdef CONFIG_DEBUG_FS
+struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus);
+void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d);
+struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d);
+struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave);
+void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d);
+void sdw_debugfs_init(void);
+void sdw_debugfs_exit(void);
+#else
+struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{ return NULL; }
+
+void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d) {}
+
+struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
+{ return NULL; }
+
+struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
+{ return NULL; }
+
+void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d) {}
+
+void sdw_debugfs_init(void) {}
+
+void sdw_debugfs_exit(void) {}
+
+#endif
+
enum {
SDW_MSG_FLAG_READ = 0,
SDW_MSG_FLAG_WRITE,
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index f68fe45c1037..f28c1a2446af 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -6,6 +6,7 @@
#include <linux/pm_domain.h>
#include <linux/soundwire/sdw.h>
#include <linux/soundwire/sdw_type.h>
+#include "bus.h"
/**
* sdw_get_device_id - find the matching SoundWire device id
@@ -182,11 +183,13 @@ EXPORT_SYMBOL_GPL(sdw_unregister_driver);
static int __init sdw_bus_init(void)
{
+ sdw_debugfs_init();
return bus_register(&sdw_bus_type);
}
static void __exit sdw_bus_exit(void)
{
+ sdw_debugfs_exit();
bus_unregister(&sdw_bus_type);
}
diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
new file mode 100644
index 000000000000..8a8b4df95c46
--- /dev/null
+++ b/drivers/soundwire/debugfs.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2017-19 Intel Corporation.
+
+#include <linux/device.h>
+#include <linux/debugfs.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_registers.h>
+#include "bus.h"
+
+#ifdef CONFIG_DEBUG_FS
+struct dentry *sdw_debugfs_root;
+#endif
+
+struct sdw_bus_debugfs {
+ struct sdw_bus *bus;
+
+ struct dentry *fs;
+};
+
+struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{
+ struct sdw_bus_debugfs *d;
+ char name[16];
+
+ if (!sdw_debugfs_root)
+ return NULL;
+
+ d = kzalloc(sizeof(*d), GFP_KERNEL);
+ if (!d)
+ return NULL;
+
+ /* create the debugfs master-N */
+ snprintf(name, sizeof(name), "master-%d", bus->link_id);
+ d->fs = debugfs_create_dir(name, sdw_debugfs_root);
+ if (IS_ERR_OR_NULL(d->fs)) {
+ dev_err(bus->dev, "debugfs root creation failed\n");
+ goto err;
+ }
+
+ d->bus = bus;
+
+ return d;
+
+err:
+ kfree(d);
+ return NULL;
+}
+
+void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d)
+{
+ debugfs_remove_recursive(d->fs);
+ kfree(d);
+}
+
+struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
+{
+ if (d)
+ return d->fs;
+ return NULL;
+}
+EXPORT_SYMBOL(sdw_bus_debugfs_get_root);
+
+struct sdw_slave_debugfs {
+ struct sdw_slave *slave;
+
+ struct dentry *fs;
+};
+
+#define RD_BUF (3 * PAGE_SIZE)
+
+static ssize_t sdw_sprintf(struct sdw_slave *slave,
+ char *buf, size_t pos, unsigned int reg)
+{
+ int value;
+
+ value = sdw_read(slave, reg);
+
+ if (value < 0)
+ return scnprintf(buf + pos, RD_BUF - pos, "%3x\tXX\n", reg);
+ else
+ return scnprintf(buf + pos, RD_BUF - pos,
+ "%3x\t%2x\n", reg, value);
+}
+
+static ssize_t sdw_slave_reg_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct sdw_slave *slave = file->private_data;
+ unsigned int reg;
+ char *buf;
+ ssize_t ret;
+ int i, j;
+
+ buf = kzalloc(RD_BUF, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = scnprintf(buf, RD_BUF, "Register Value\n");
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP0\n");
+
+ for (i = 0; i < 6; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+ ret += sdw_sprintf(slave, buf, ret, SDW_DP0_CHANNELEN);
+ for (i = SDW_DP0_SAMPLECTRL1; i <= SDW_DP0_LANECTRL; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+ ret += sdw_sprintf(slave, buf, ret,
+ SDW_DP0_CHANNELEN + SDW_BANK1_OFFSET);
+ for (i = SDW_DP0_SAMPLECTRL1 + SDW_BANK1_OFFSET;
+ i <= SDW_DP0_LANECTRL + SDW_BANK1_OFFSET; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nSCP\n");
+ for (i = SDW_SCP_INT1; i <= SDW_SCP_BANKDELAY; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
+ for (i = SDW_SCP_DEVID_0; i <= SDW_SCP_DEVID_5; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B0);
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B0);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B1);
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B1);
+
+ for (i = 1; i < 14; i++) {
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP%d\n", i);
+ reg = SDW_DPN_INT(i);
+ for (j = 0; j < 6; j++)
+ ret += sdw_sprintf(slave, buf, ret, reg + j);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+ reg = SDW_DPN_CHANNELEN_B0(i);
+ for (j = 0; j < 9; j++)
+ ret += sdw_sprintf(slave, buf, ret, reg + j);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+ reg = SDW_DPN_CHANNELEN_B1(i);
+ for (j = 0; j < 9; j++)
+ ret += sdw_sprintf(slave, buf, ret, reg + j);
+ }
+
+ ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+ kfree(buf);
+
+ return ret;
+}
+
+static const struct file_operations sdw_slave_reg_fops = {
+ .open = simple_open,
+ .read = sdw_slave_reg_read,
+ .llseek = default_llseek,
+};
+
+struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
+{
+ struct sdw_bus_debugfs *master;
+ struct sdw_slave_debugfs *d;
+ char name[32];
+
+ master = slave->bus->debugfs;
+ if (!master)
+ return NULL;
+
+ d = kzalloc(sizeof(*d), GFP_KERNEL);
+ if (!d)
+ return NULL;
+
+ /* create the debugfs slave-name */
+ snprintf(name, sizeof(name), "%s", dev_name(&slave->dev));
+ d->fs = debugfs_create_dir(name, master->fs);
+ if (IS_ERR_OR_NULL(d->fs)) {
+ dev_err(&slave->dev, "slave debugfs root creation failed\n");
+ goto err;
+ }
+
+ d->slave = slave;
+
+ debugfs_create_file("registers", 0400, d->fs,
+ slave, &sdw_slave_reg_fops);
+
+ return d;
+
+err:
+ kfree(d);
+ return NULL;
+}
+
+void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d)
+{
+ debugfs_remove_recursive(d->fs);
+ kfree(d);
+}
+
+void sdw_debugfs_init(void)
+{
+ sdw_debugfs_root = debugfs_create_dir("soundwire", NULL);
+ if (IS_ERR_OR_NULL(sdw_debugfs_root)) {
+ pr_warn("SoundWire: Failed to create debugfs directory\n");
+ sdw_debugfs_root = NULL;
+ return;
+ }
+}
+
+void sdw_debugfs_exit(void)
+{
+ debugfs_remove_recursive(sdw_debugfs_root);
+}
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index bad73a267fdd..e41332a7f340 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -57,6 +57,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
mutex_unlock(&bus->bus_lock);
put_device(&slave->dev);
}
+ slave->debugfs = sdw_slave_debugfs_init(slave);
return ret;
}
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index fae3a3ad6e68..3684ca106408 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -547,6 +547,8 @@ struct sdw_slave_ops {
enum sdw_port_prep_ops pre_ops);
};
+struct sdw_slave_debugfs;
+
/**
* struct sdw_slave - SoundWire Slave
* @id: MIPI device ID
@@ -556,6 +558,7 @@ struct sdw_slave_ops {
* @ops: Slave callback ops
* @prop: Slave properties
* @sysfs: Sysfs interface
+ * @debugfs: Slave debugfs
* @node: node for bus list
* @port_ready: Port ready completion flag for each Slave port
* @dev_num: Device Number assigned by Bus
@@ -568,6 +571,7 @@ struct sdw_slave {
const struct sdw_slave_ops *ops;
struct sdw_slave_prop prop;
struct sdw_slave_sysfs *sysfs;
+ struct sdw_slave_debugfs *debugfs;
struct list_head node;
struct completion *port_ready;
u16 dev_num;
@@ -728,6 +732,8 @@ struct sdw_master_ops {
};
+struct sdw_bus_debugfs;
+
/**
* struct sdw_bus - SoundWire bus
* @dev: Master linux device
@@ -742,8 +748,10 @@ struct sdw_master_ops {
* @params: Current bus parameters
* @prop: Master properties
* @m_rt_list: List of Master instance of all stream(s) running on Bus. This
+ * @rt_list: List of Master instance of all stream(s) running on Bus. This
* is used to compute and program bus bandwidth, clock, frame shape,
* transport and port parameters
+ * @debugfs: Bus debugfs
* @sysfs: Bus sysfs
* @defer_msg: Defer message
* @clk_stop_timeout: Clock stop timeout computed
@@ -765,6 +773,7 @@ struct sdw_bus {
struct sdw_master_prop prop;
struct list_head m_rt_list;
struct sdw_master_sysfs *sysfs;
+ struct sdw_bus_debugfs *debugfs;
struct sdw_defer defer_msg;
unsigned int clk_stop_timeout;
u32 bank_switch_timeout;
--
2.17.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 5/7] soundwire: add debugfs support
2019-05-04 1:00 ` [RFC PATCH 5/7] soundwire: add debugfs support Pierre-Louis Bossart
@ 2019-05-04 7:03 ` Greg KH
2019-05-06 14:48 ` [alsa-devel] " Pierre-Louis Bossart
0 siblings, 1 reply; 43+ messages in thread
From: Greg KH @ 2019-05-04 7:03 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, linux-kernel, tiwai, broonie, vkoul, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Sanyog Kale
On Fri, May 03, 2019 at 08:00:28PM -0500, Pierre-Louis Bossart wrote:
> Add base debugfs mechanism for SoundWire bus by creating soundwire
> root and master-N and slave-x hierarchy.
>
> Also add SDW Slave SCP, DP0 and DP-N register debug file.
>
> Registers not implemented will print as "XX"
>
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
> is the use of scnprintf to avoid known issues with snprintf.
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
> drivers/soundwire/Makefile | 3 +-
> drivers/soundwire/bus.c | 5 +
> drivers/soundwire/bus.h | 28 +++++
> drivers/soundwire/bus_type.c | 3 +
> drivers/soundwire/debugfs.c | 214 ++++++++++++++++++++++++++++++++++
> drivers/soundwire/slave.c | 1 +
> include/linux/soundwire/sdw.h | 9 ++
> 7 files changed, 262 insertions(+), 1 deletion(-)
> create mode 100644 drivers/soundwire/debugfs.c
>
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index a72a29731a28..f2c425fa15bd 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -4,7 +4,8 @@
>
> #Bus Objs
> soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
> - sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o
> + sysfs.o sysfs_slave_dp0.o sysfs_slave_dpn.o \
> + debugfs.o
> obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
>
> #Cadence Objs
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index dd9181693554..b79eba321b71 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -53,6 +53,8 @@ int sdw_add_bus_master(struct sdw_bus *bus)
> if (ret < 0)
> dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret);
>
> + bus->debugfs = sdw_bus_debugfs_init(bus);
> +
> /*
> * Device numbers in SoundWire are 0 through 15. Enumeration device
> * number (0), Broadcast device number (15), Group numbers (12 and
> @@ -114,6 +116,7 @@ static int sdw_delete_slave(struct device *dev, void *data)
> struct sdw_bus *bus = slave->bus;
>
> sdw_sysfs_slave_exit(slave);
> + sdw_slave_debugfs_exit(slave->debugfs);
>
> mutex_lock(&bus->bus_lock);
>
> @@ -136,6 +139,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
> void sdw_delete_bus_master(struct sdw_bus *bus)
> {
> sdw_sysfs_bus_exit(bus);
> + if (bus->debugfs)
> + sdw_bus_debugfs_exit(bus->debugfs);
No need to check, just call it.
>
> device_for_each_child(bus->dev, NULL, sdw_delete_slave);
> }
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 0707e68a8d21..f0b1f4f9d7b2 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -20,6 +20,34 @@ void sdw_extract_slave_id(struct sdw_bus *bus,
>
> extern const struct attribute_group *sdw_slave_dev_attr_groups[];
>
> +#ifdef CONFIG_DEBUG_FS
> +struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus);
> +void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d);
> +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d);
> +struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave);
> +void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d);
> +void sdw_debugfs_init(void);
> +void sdw_debugfs_exit(void);
> +#else
> +struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus)
> +{ return NULL; }
> +
> +void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d) {}
> +
> +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
> +{ return NULL; }
> +
> +struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
> +{ return NULL; }
> +
> +void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d) {}
> +
> +void sdw_debugfs_init(void) {}
> +
> +void sdw_debugfs_exit(void) {}
> +
> +#endif
> +
> enum {
> SDW_MSG_FLAG_READ = 0,
> SDW_MSG_FLAG_WRITE,
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index f68fe45c1037..f28c1a2446af 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -6,6 +6,7 @@
> #include <linux/pm_domain.h>
> #include <linux/soundwire/sdw.h>
> #include <linux/soundwire/sdw_type.h>
> +#include "bus.h"
>
> /**
> * sdw_get_device_id - find the matching SoundWire device id
> @@ -182,11 +183,13 @@ EXPORT_SYMBOL_GPL(sdw_unregister_driver);
>
> static int __init sdw_bus_init(void)
> {
> + sdw_debugfs_init();
> return bus_register(&sdw_bus_type);
> }
>
> static void __exit sdw_bus_exit(void)
> {
> + sdw_debugfs_exit();
> bus_unregister(&sdw_bus_type);
> }
>
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> new file mode 100644
> index 000000000000..8a8b4df95c46
> --- /dev/null
> +++ b/drivers/soundwire/debugfs.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2017-19 Intel Corporation.
> +
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/slab.h>
> +#include <linux/soundwire/sdw.h>
> +#include <linux/soundwire/sdw_registers.h>
> +#include "bus.h"
> +
> +#ifdef CONFIG_DEBUG_FS
> +struct dentry *sdw_debugfs_root;
> +#endif
> +
> +struct sdw_bus_debugfs {
> + struct sdw_bus *bus;
Why do you need to save this pointer?
> + struct dentry *fs;
This really is all you need to have around, right?
> +};
> +
> +struct sdw_bus_debugfs *sdw_bus_debugfs_init(struct sdw_bus *bus)
> +{
> + struct sdw_bus_debugfs *d;
> + char name[16];
> +
> + if (!sdw_debugfs_root)
> + return NULL;
> +
> + d = kzalloc(sizeof(*d), GFP_KERNEL);
> + if (!d)
> + return NULL;
> +
> + /* create the debugfs master-N */
> + snprintf(name, sizeof(name), "master-%d", bus->link_id);
> + d->fs = debugfs_create_dir(name, sdw_debugfs_root);
> + if (IS_ERR_OR_NULL(d->fs)) {
> + dev_err(bus->dev, "debugfs root creation failed\n");
> + goto err;
> + }
> +
> + d->bus = bus;
> +
> + return d;
> +
> +err:
> + kfree(d);
> + return NULL;
> +}
> +
> +void sdw_bus_debugfs_exit(struct sdw_bus_debugfs *d)
> +{
> + debugfs_remove_recursive(d->fs);
> + kfree(d);
> +}
> +
> +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
> +{
> + if (d)
> + return d->fs;
> + return NULL;
> +}
> +EXPORT_SYMBOL(sdw_bus_debugfs_get_root);
_GPL()?
But why is this exported at all? No one calls this function.
> +struct sdw_slave_debugfs {
> + struct sdw_slave *slave;
Same question as above, why do you need this pointer?
And meta-comment, if you _EVER_ save off a pointer to a reference
counted object (like this and the above one), you HAVE to grab a
reference to it, otherwise it can go away at any point in time as that
is the point of reference counted objects.
So even if you do need/want this, you have to properly handle the
reference count by incrementing/decrementing it as needed.
> +
> + struct dentry *fs;
> +};
> +
> +#define RD_BUF (3 * PAGE_SIZE)
> +
> +static ssize_t sdw_sprintf(struct sdw_slave *slave,
> + char *buf, size_t pos, unsigned int reg)
> +{
> + int value;
> +
> + value = sdw_read(slave, reg);
> +
> + if (value < 0)
> + return scnprintf(buf + pos, RD_BUF - pos, "%3x\tXX\n", reg);
> + else
> + return scnprintf(buf + pos, RD_BUF - pos,
> + "%3x\t%2x\n", reg, value);
> +}
> +
> +static ssize_t sdw_slave_reg_read(struct file *file, char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct sdw_slave *slave = file->private_data;
> + unsigned int reg;
> + char *buf;
> + ssize_t ret;
> + int i, j;
> +
> + buf = kzalloc(RD_BUF, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = scnprintf(buf, RD_BUF, "Register Value\n");
> + ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP0\n");
> +
> + for (i = 0; i < 6; i++)
> + ret += sdw_sprintf(slave, buf, ret, i);
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
> + ret += sdw_sprintf(slave, buf, ret, SDW_DP0_CHANNELEN);
> + for (i = SDW_DP0_SAMPLECTRL1; i <= SDW_DP0_LANECTRL; i++)
> + ret += sdw_sprintf(slave, buf, ret, i);
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
> + ret += sdw_sprintf(slave, buf, ret,
> + SDW_DP0_CHANNELEN + SDW_BANK1_OFFSET);
> + for (i = SDW_DP0_SAMPLECTRL1 + SDW_BANK1_OFFSET;
> + i <= SDW_DP0_LANECTRL + SDW_BANK1_OFFSET; i++)
> + ret += sdw_sprintf(slave, buf, ret, i);
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret, "\nSCP\n");
> + for (i = SDW_SCP_INT1; i <= SDW_SCP_BANKDELAY; i++)
> + ret += sdw_sprintf(slave, buf, ret, i);
> + for (i = SDW_SCP_DEVID_0; i <= SDW_SCP_DEVID_5; i++)
> + ret += sdw_sprintf(slave, buf, ret, i);
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
> + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B0);
> + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B0);
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
> + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B1);
> + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B1);
> +
> + for (i = 1; i < 14; i++) {
> + ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP%d\n", i);
> + reg = SDW_DPN_INT(i);
> + for (j = 0; j < 6; j++)
> + ret += sdw_sprintf(slave, buf, ret, reg + j);
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
> + reg = SDW_DPN_CHANNELEN_B0(i);
> + for (j = 0; j < 9; j++)
> + ret += sdw_sprintf(slave, buf, ret, reg + j);
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
> + reg = SDW_DPN_CHANNELEN_B1(i);
> + for (j = 0; j < 9; j++)
> + ret += sdw_sprintf(slave, buf, ret, reg + j);
> + }
> +
> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> + kfree(buf);
> +
> + return ret;
> +}
> +
> +static const struct file_operations sdw_slave_reg_fops = {
> + .open = simple_open,
> + .read = sdw_slave_reg_read,
> + .llseek = default_llseek,
> +};
> +
> +struct sdw_slave_debugfs *sdw_slave_debugfs_init(struct sdw_slave *slave)
> +{
> + struct sdw_bus_debugfs *master;
> + struct sdw_slave_debugfs *d;
> + char name[32];
> +
> + master = slave->bus->debugfs;
> + if (!master)
> + return NULL;
> +
> + d = kzalloc(sizeof(*d), GFP_KERNEL);
> + if (!d)
> + return NULL;
> +
> + /* create the debugfs slave-name */
> + snprintf(name, sizeof(name), "%s", dev_name(&slave->dev));
> + d->fs = debugfs_create_dir(name, master->fs);
> + if (IS_ERR_OR_NULL(d->fs)) {
> + dev_err(&slave->dev, "slave debugfs root creation failed\n");
> + goto err;
> + }
You never care about the return value of a debugfs call. I have a 100+
patch series stripping all of this out of the kernel, please don't force
me to add another one to it :)
Just call debugfs and move on, you can always put the return value of
one call into another one just fine, and your function logic should
never change if debugfs returns an error or not, you do not care.
> +
> + d->slave = slave;
> +
> + debugfs_create_file("registers", 0400, d->fs,
> + slave, &sdw_slave_reg_fops);
> +
> + return d;
> +
> +err:
> + kfree(d);
> + return NULL;
> +}
> +
> +void sdw_slave_debugfs_exit(struct sdw_slave_debugfs *d)
> +{
> + debugfs_remove_recursive(d->fs);
> + kfree(d);
> +}
> +
> +void sdw_debugfs_init(void)
> +{
> + sdw_debugfs_root = debugfs_create_dir("soundwire", NULL);
> + if (IS_ERR_OR_NULL(sdw_debugfs_root)) {
> + pr_warn("SoundWire: Failed to create debugfs directory\n");
> + sdw_debugfs_root = NULL;
> + return;
Same here, just call the function and return.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support
2019-05-04 7:03 ` Greg KH
@ 2019-05-06 14:48 ` Pierre-Louis Bossart
2019-05-06 16:38 ` Vinod Koul
0 siblings, 1 reply; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 14:48 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, vkoul, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
>> @@ -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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support
2019-05-06 14:48 ` [alsa-devel] " Pierre-Louis Bossart
@ 2019-05-06 16:38 ` Vinod Koul
2019-05-06 16:54 ` Pierre-Louis Bossart
2019-05-07 5:56 ` Greg KH
0 siblings, 2 replies; 43+ messages in thread
From: Vinod Koul @ 2019-05-06 16:38 UTC (permalink / raw)
To: Pierre-Louis Bossart, Greg KH
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
On 06-05-19, 09:48, Pierre-Louis Bossart wrote:
> > > +struct dentry *sdw_bus_debugfs_get_root(struct sdw_bus_debugfs *d)
> > > +{
> > > + if (d)
> > > + return d->fs;
> > > + return NULL;
> > > +}
> > > +EXPORT_SYMBOL(sdw_bus_debugfs_get_root);
> >
> > _GPL()?
>
> Oops, that's a big miss. will fix, thanks for spotting this.
Not really. The Soundwire code is dual licensed. Many of the soundwire
symbols are indeed exported as EXPORT_SYMBOL. But I agree this one is
'linux' specific so can be made _GPL.
Pierre, does Intel still care about this being dual licensed or not?
>
> >
> > But why is this exported at all? No one calls this function.
>
> I will have to check.
It is used by codec driver which are not upstream yet. So my suggestion
would be NOT to export this and only do so when we have users for it
That would be true for other APIs exported out as well.
> >
> > > +struct sdw_slave_debugfs {
> > > + struct sdw_slave *slave;
> >
> > Same question as above, why do you need this pointer?
>
> will check.
The deubugfs code does hold a ref to slave object to read the data and
dump, this particular instance might be able to get rid of (should be
doable)
> > And meta-comment, if you _EVER_ save off a pointer to a reference
> > counted object (like this and the above one), you HAVE to grab a
> > reference to it, otherwise it can go away at any point in time as that
> > is the point of reference counted objects.
> >
> > So even if you do need/want this, you have to properly handle the
> > reference count by incrementing/decrementing it as needed.
Yes, but then device exit routine is supposed to do debugfs cleanup as
well, so that would ensure these references are dropped at that point of
time. Greg should that not take care of it or we *should* always do
refcounting.
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support
2019-05-06 16:38 ` Vinod Koul
@ 2019-05-06 16:54 ` Pierre-Louis Bossart
2019-05-07 5:56 ` Greg KH
1 sibling, 0 replies; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 16:54 UTC (permalink / raw)
To: Vinod Koul, Greg KH
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 5/7] soundwire: add debugfs support
2019-05-06 16:38 ` Vinod Koul
2019-05-06 16:54 ` Pierre-Louis Bossart
@ 2019-05-07 5:56 ` Greg KH
1 sibling, 0 replies; 43+ messages in thread
From: Greg KH @ 2019-05-07 5:56 UTC (permalink / raw)
To: Vinod Koul
Cc: Pierre-Louis Bossart, alsa-devel, tiwai, linux-kernel,
liam.r.girdwood, broonie, srinivas.kandagatla, jank, joe,
Sanyog Kale
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?
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump
2019-05-04 1:00 [RFC PATCH 0/7] soundwire: add sysfs and debugfs support Pierre-Louis Bossart
` (4 preceding siblings ...)
2019-05-04 1:00 ` [RFC PATCH 5/7] soundwire: add debugfs support Pierre-Louis Bossart
@ 2019-05-04 1:00 ` Pierre-Louis Bossart
2019-05-04 7:03 ` Greg KH
2019-05-04 7:04 ` Greg KH
2019-05-04 1:00 ` [RFC PATCH 7/7] soundwire: intel: " Pierre-Louis Bossart
6 siblings, 2 replies; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-04 1:00 UTC (permalink / raw)
To: alsa-devel
Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Pierre-Louis Bossart,
Sanyog Kale
Add debugfs file to dump the Cadence master registers
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
is the use of scnprintf to avoid known issues with snprintf.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
drivers/soundwire/cadence_master.c | 98 ++++++++++++++++++++++++++++++
drivers/soundwire/cadence_master.h | 5 ++
2 files changed, 103 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 682789bb8ab3..e9c30f18ce25 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -8,6 +8,7 @@
#include <linux/delay.h>
#include <linux/device.h>
+#include <linux/debugfs.h>
#include <linux/interrupt.h>
#include <linux/module.h>
#include <linux/mod_devicetable.h>
@@ -222,6 +223,103 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value)
return -EAGAIN;
}
+/*
+ * debugfs
+ */
+
+#define RD_BUF (2 * PAGE_SIZE)
+
+static ssize_t cdns_sprintf(struct sdw_cdns *cdns,
+ char *buf, size_t pos, unsigned int reg)
+{
+ return scnprintf(buf + pos, RD_BUF - pos,
+ "%4x\t%4x\n", reg, cdns_readl(cdns, reg));
+}
+
+static ssize_t cdns_reg_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct sdw_cdns *cdns = file->private_data;
+ char *buf;
+ ssize_t ret;
+ int i, j;
+
+ buf = kzalloc(RD_BUF, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = scnprintf(buf, RD_BUF, "Register Value\n");
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nMCP Registers\n");
+ for (i = 0; i < 8; i++) /* 8 MCP registers */
+ ret += cdns_sprintf(cdns, buf, ret, i * 4);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+ "\nStatus & Intr Registers\n");
+ for (i = 0; i < 13; i++) /* 13 Status & Intr registers */
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_STAT + i * 4);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+ "\nSSP & Clk ctrl Registers\n");
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL0);
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL1);
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL0);
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL1);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+ "\nDPn B0 Registers\n");
+ for (i = 0; i < 7; i++) {
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+ "\nDP-%d\n", i);
+ for (j = 0; j < 6; j++)
+ ret += cdns_sprintf(cdns, buf, ret,
+ CDNS_DPN_B0_CONFIG(i) + j * 4);
+ }
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+ "\nDPn B1 Registers\n");
+ for (i = 0; i < 7; i++) {
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+ "\nDP-%d\n", i);
+
+ for (j = 0; j < 6; j++)
+ ret += cdns_sprintf(cdns, buf, ret,
+ CDNS_DPN_B1_CONFIG(i) + j * 4);
+ }
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+ "\nDPn Control Registers\n");
+ for (i = 0; i < 7; i++)
+ ret += cdns_sprintf(cdns, buf, ret,
+ CDNS_PORTCTRL + i * CDNS_PORT_OFFSET);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+ "\nPDIn Config Registers\n");
+ for (i = 0; i < 7; i++)
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_PDI_CONFIG(i));
+
+ ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+ kfree(buf);
+
+ return ret;
+}
+
+static const struct file_operations cdns_reg_fops = {
+ .open = simple_open,
+ .read = cdns_reg_read,
+ .llseek = default_llseek,
+};
+
+/**
+ * sdw_cdns_debugfs_init() - Cadence debugfs init
+ * @cdns: Cadence instance
+ * @root: debugfs root
+ */
+void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
+{
+ debugfs_create_file("cdns-registers", 0400, root, cdns, &cdns_reg_fops);
+}
+EXPORT_SYMBOL(sdw_cdns_debugfs_init);
+
/*
* IO Calls
*/
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
index fe2af62958b1..d375bbfead18 100644
--- a/drivers/soundwire/cadence_master.h
+++ b/drivers/soundwire/cadence_master.h
@@ -163,6 +163,11 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
struct sdw_cdns_stream_config config);
int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
+void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
+
+int sdw_cdns_suspend(struct sdw_cdns *cdns);
+bool sdw_cdns_check_resume_status(struct sdw_cdns *cdns);
+
int sdw_cdns_get_stream(struct sdw_cdns *cdns,
struct sdw_cdns_streams *stream,
u32 ch, u32 dir);
--
2.17.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump
2019-05-04 1:00 ` [RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump Pierre-Louis Bossart
@ 2019-05-04 7:03 ` Greg KH
2019-05-06 14:50 ` [alsa-devel] " Pierre-Louis Bossart
2019-05-04 7:04 ` Greg KH
1 sibling, 1 reply; 43+ messages in thread
From: Greg KH @ 2019-05-04 7:03 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, linux-kernel, tiwai, broonie, vkoul, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Sanyog Kale
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump
2019-05-04 7:03 ` Greg KH
@ 2019-05-06 14:50 ` Pierre-Louis Bossart
0 siblings, 0 replies; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 14:50 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, vkoul, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
>> +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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump
2019-05-04 1:00 ` [RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump Pierre-Louis Bossart
2019-05-04 7:03 ` Greg KH
@ 2019-05-04 7:04 ` Greg KH
1 sibling, 0 replies; 43+ messages in thread
From: Greg KH @ 2019-05-04 7:04 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, linux-kernel, tiwai, broonie, vkoul, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Sanyog Kale
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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC PATCH 7/7] soundwire: intel: add debugfs register dump
2019-05-04 1:00 [RFC PATCH 0/7] soundwire: add sysfs and debugfs support Pierre-Louis Bossart
` (5 preceding siblings ...)
2019-05-04 1:00 ` [RFC PATCH 6/7] soundwire: cadence_master: add debugfs register dump Pierre-Louis Bossart
@ 2019-05-04 1:00 ` Pierre-Louis Bossart
2019-05-04 7:04 ` Greg KH
6 siblings, 1 reply; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-04 1:00 UTC (permalink / raw)
To: alsa-devel
Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Pierre-Louis Bossart,
Sanyog Kale
Add debugfs file to dump the Intel SoundWire registers
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
is the use of scnprintf to avoid known issues with snprintf.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
drivers/soundwire/intel.c | 115 ++++++++++++++++++++++++++++++++++++++
1 file changed, 115 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 4ac141730b13..7fb2cd6d5bb5 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -6,6 +6,7 @@
*/
#include <linux/acpi.h>
+#include <linux/debugfs.h>
#include <linux/delay.h>
#include <linux/module.h>
#include <linux/interrupt.h>
@@ -16,6 +17,7 @@
#include <linux/soundwire/sdw.h>
#include <linux/soundwire/sdw_intel.h>
#include "cadence_master.h"
+#include "bus.h"
#include "intel.h"
/* Intel SHIM Registers Definition */
@@ -98,6 +100,7 @@ struct sdw_intel {
struct sdw_cdns cdns;
int instance;
struct sdw_intel_link_res *res;
+ struct dentry *fs;
};
#define cdns_to_intel(_cdns) container_of(_cdns, struct sdw_intel, cdns)
@@ -161,6 +164,115 @@ static int intel_set_bit(void __iomem *base, int offset, u32 value, u32 mask)
return -EAGAIN;
}
+/*
+ * debugfs
+ */
+
+#define RD_BUF (2 * PAGE_SIZE)
+
+static ssize_t intel_sprintf(void __iomem *mem, bool l,
+ char *buf, size_t pos, unsigned int reg)
+{
+ int value;
+
+ if (l)
+ value = intel_readl(mem, reg);
+ else
+ value = intel_readw(mem, reg);
+
+ return scnprintf(buf + pos, RD_BUF - pos, "%4x\t%4x\n", reg, value);
+}
+
+static ssize_t intel_reg_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct sdw_intel *sdw = file->private_data;
+ void __iomem *s = sdw->res->shim;
+ void __iomem *a = sdw->res->alh;
+ char *buf;
+ ssize_t ret;
+ int i, j;
+ unsigned int links, reg;
+
+ buf = kzalloc(RD_BUF, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ links = intel_readl(s, SDW_SHIM_LCAP) & GENMASK(2, 0);
+
+ ret = scnprintf(buf, RD_BUF, "Register Value\n");
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nShim\n");
+
+ for (i = 0; i < 4; i++) {
+ reg = SDW_SHIM_LCAP + i * 4;
+ ret += intel_sprintf(s, true, buf, ret, reg);
+ }
+
+ for (i = 0; i < links; i++) {
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nLink%d\n", i);
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLSCAP(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS0CM(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS1CM(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS2CM(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS3CM(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_PCMSCAP(i));
+
+ for (j = 0; j < 8; j++) {
+ ret += intel_sprintf(s, false, buf, ret,
+ SDW_SHIM_PCMSYCHM(i, j));
+ ret += intel_sprintf(s, false, buf, ret,
+ SDW_SHIM_PCMSYCHC(i, j));
+ }
+
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_PDMSCAP(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_IOCTL(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTMCTL(i));
+ }
+
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_WAKEEN);
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_WAKESTS);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nALH\n");
+ for (i = 0; i < 8; i++)
+ ret += intel_sprintf(a, true, buf, ret, SDW_ALH_STRMZCFG(i));
+
+ ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+ kfree(buf);
+
+ return ret;
+}
+
+static const struct file_operations intel_reg_fops = {
+ .open = simple_open,
+ .read = intel_reg_read,
+ .llseek = default_llseek,
+};
+
+static void intel_debugfs_init(struct sdw_intel *sdw)
+{
+ struct dentry *root = sdw_bus_debugfs_get_root(sdw->cdns.bus.debugfs);
+
+ if (!root)
+ return;
+
+ sdw->fs = debugfs_create_dir("intel-sdw", root);
+ if (IS_ERR_OR_NULL(sdw->fs)) {
+ dev_err(sdw->cdns.dev, "debugfs root creation failed\n");
+ sdw->fs = NULL;
+ return;
+ }
+
+ debugfs_create_file("intel-registers", 0400, sdw->fs, sdw,
+ &intel_reg_fops);
+
+ sdw_cdns_debugfs_init(&sdw->cdns, sdw->fs);
+}
+
+static void intel_debugfs_exit(struct sdw_intel *sdw)
+{
+ debugfs_remove_recursive(sdw->fs);
+}
+
/*
* shim ops
*/
@@ -890,6 +1002,8 @@ static int intel_probe(struct platform_device *pdev)
goto err_dai;
}
+ intel_debugfs_init(sdw);
+
return 0;
err_dai:
@@ -906,6 +1020,7 @@ static int intel_remove(struct platform_device *pdev)
sdw = platform_get_drvdata(pdev);
+ intel_debugfs_exit(sdw);
free_irq(sdw->res->irq, sdw);
snd_soc_unregister_component(sdw->cdns.dev);
sdw_delete_bus_master(&sdw->cdns.bus);
--
2.17.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 7/7] soundwire: intel: add debugfs register dump
2019-05-04 1:00 ` [RFC PATCH 7/7] soundwire: intel: " Pierre-Louis Bossart
@ 2019-05-04 7:04 ` Greg KH
2019-05-06 14:51 ` [alsa-devel] " Pierre-Louis Bossart
0 siblings, 1 reply; 43+ messages in thread
From: Greg KH @ 2019-05-04 7:04 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, linux-kernel, tiwai, broonie, vkoul, liam.r.girdwood,
jank, joe, srinivas.kandagatla, Sanyog Kale
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [alsa-devel] [RFC PATCH 7/7] soundwire: intel: add debugfs register dump
2019-05-04 7:04 ` Greg KH
@ 2019-05-06 14:51 ` Pierre-Louis Bossart
0 siblings, 0 replies; 43+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 14:51 UTC (permalink / raw)
To: Greg KH
Cc: alsa-devel, tiwai, linux-kernel, liam.r.girdwood, vkoul, broonie,
srinivas.kandagatla, jank, joe, Sanyog Kale
>> +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.
^ permalink raw reply [flat|nested] 43+ messages in thread