[PATCH 0/3] soundwire: bus_type: add sdw_master_device support
This series adds sdw master devices support.
Pierre-Louis Bossart (3): soundwire: bus: rename sdw_bus_master_add/delete, add arguments soundwire: bus_type: introduce sdw_slave_type and sdw_master_type soundwire: bus_type: add sdw_master_device support
.../driver-api/soundwire/summary.rst | 7 +- drivers/soundwire/Makefile | 2 +- drivers/soundwire/bus.c | 27 ++++--- drivers/soundwire/bus.h | 3 + drivers/soundwire/bus_type.c | 19 +++-- drivers/soundwire/intel.c | 9 ++- drivers/soundwire/master.c | 79 +++++++++++++++++++ drivers/soundwire/qcom.c | 7 +- drivers/soundwire/slave.c | 8 +- include/linux/soundwire/sdw.h | 22 +++++- include/linux/soundwire/sdw_type.h | 9 ++- 11 files changed, 160 insertions(+), 32 deletions(-) create mode 100644 drivers/soundwire/master.c
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
In preparation for future extensions, rename functions to use sdw_bus_master prefix and add a parent and fwnode argument to sdw_bus_master_add to help with device registration in follow-up patches.
No functionality change, just renames and additional arguments.
The Intel code is currently unused, the two additional arguments are only needed for compilation.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- Documentation/driver-api/soundwire/summary.rst | 7 ++++--- drivers/soundwire/bus.c | 15 +++++++++------ drivers/soundwire/intel.c | 9 +++++---- drivers/soundwire/qcom.c | 6 +++--- include/linux/soundwire/sdw.h | 5 +++-- 5 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/Documentation/driver-api/soundwire/summary.rst b/Documentation/driver-api/soundwire/summary.rst index 8193125a2bfb..01dcb954f6d7 100644 --- a/Documentation/driver-api/soundwire/summary.rst +++ b/Documentation/driver-api/soundwire/summary.rst @@ -101,10 +101,11 @@ Following is the Bus API to register the SoundWire Bus:
.. code-block:: c
- int sdw_add_bus_master(struct sdw_bus *bus) + int sdw_bus_master_add(struct sdw_bus *bus, + struct device *parent, + struct fwnode_handle) { - if (!bus->dev) - return -ENODEV; + sdw_master_device_add(bus, parent, fwnode);
mutex_init(&bus->lock); INIT_LIST_HEAD(&bus->slaves); diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 488c3c9e4947..18024ff770f8 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -10,13 +10,16 @@ #include "bus.h"
/** - * sdw_add_bus_master() - add a bus Master instance + * sdw_bus_master_add() - add a bus Master instance * @bus: bus instance + * @parent: parent device + * @fwnode: firmware node handle * * Initializes the bus instance, read properties and create child * devices. */ -int sdw_add_bus_master(struct sdw_bus *bus) +int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent, + struct fwnode_handle *fwnode) { struct sdw_master_prop *prop = NULL; int ret; @@ -107,7 +110,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
return 0; } -EXPORT_SYMBOL(sdw_add_bus_master); +EXPORT_SYMBOL(sdw_bus_master_add);
static int sdw_delete_slave(struct device *dev, void *data) { @@ -131,18 +134,18 @@ static int sdw_delete_slave(struct device *dev, void *data) }
/** - * sdw_delete_bus_master() - delete the bus master instance + * sdw_bus_master_delete() - delete the bus master instance * @bus: bus to be deleted * * Remove the instance, delete the child devices. */ -void sdw_delete_bus_master(struct sdw_bus *bus) +void sdw_bus_master_delete(struct sdw_bus *bus) { device_for_each_child(bus->dev, NULL, sdw_delete_slave);
sdw_bus_debugfs_exit(bus); } -EXPORT_SYMBOL(sdw_delete_bus_master); +EXPORT_SYMBOL(sdw_bus_master_delete);
/* * SDW IO Calls diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index ed8d576bf5dc..1b600f423d8b 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1110,9 +1110,10 @@ static int intel_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, sdw);
- ret = sdw_add_bus_master(&sdw->cdns.bus); + /* 2nd and 3rd arguments are just added for compilation */ + ret = sdw_bus_master_add(&sdw->cdns.bus, NULL, NULL); if (ret) { - dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret); + dev_err(&pdev->dev, "sdw_bus_master_add fail: %d\n", ret); return ret; }
@@ -1173,7 +1174,7 @@ static int intel_probe(struct platform_device *pdev) sdw_cdns_enable_interrupt(&sdw->cdns, false); free_irq(sdw->link_res->irq, sdw); err_init: - sdw_delete_bus_master(&sdw->cdns.bus); + sdw_bus_master_delete(&sdw->cdns.bus); return ret; }
@@ -1189,7 +1190,7 @@ static int intel_remove(struct platform_device *pdev) free_irq(sdw->link_res->irq, sdw); snd_soc_unregister_component(sdw->cdns.dev); } - sdw_delete_bus_master(&sdw->cdns.bus); + sdw_bus_master_delete(&sdw->cdns.bus);
return 0; } diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index e08a17c13f92..401811d6627e 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -821,7 +821,7 @@ static int qcom_swrm_probe(struct platform_device *pdev) goto err_clk; }
- ret = sdw_add_bus_master(&ctrl->bus); + ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode); if (ret) { dev_err(dev, "Failed to register Soundwire controller (%d)\n", ret); @@ -840,7 +840,7 @@ static int qcom_swrm_probe(struct platform_device *pdev) return 0;
err_master_add: - sdw_delete_bus_master(&ctrl->bus); + sdw_bus_master_delete(&ctrl->bus); err_clk: clk_disable_unprepare(ctrl->hclk); err_init: @@ -851,7 +851,7 @@ static int qcom_swrm_remove(struct platform_device *pdev) { struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
- sdw_delete_bus_master(&ctrl->bus); + sdw_bus_master_delete(&ctrl->bus); clk_disable_unprepare(ctrl->hclk);
return 0; diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 00f5826092e3..2003e8c55538 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -832,8 +832,9 @@ struct sdw_bus { bool multi_link; };
-int sdw_add_bus_master(struct sdw_bus *bus); -void sdw_delete_bus_master(struct sdw_bus *bus); +int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent, + struct fwnode_handle *fwnode); +void sdw_bus_master_delete(struct sdw_bus *bus);
/** * sdw_port_config: Master or Slave Port configuration
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
this is a preparatory patch before the introduction of the sdw_master_type. The SoundWire slave support is slightly modified with the use of a sdw_slave_type, and the uevent handling move to slave.c (since it's not necessary for the master).
No functionality change other than moving code around.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/bus_type.c | 19 +++++++++++++------ drivers/soundwire/slave.c | 8 +++++++- include/linux/soundwire/sdw_type.h | 9 ++++++++- 3 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index 17f096dd6806..2c1a19caba51 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -33,13 +33,21 @@ sdw_get_device_id(struct sdw_slave *slave, struct sdw_driver *drv)
static int sdw_bus_match(struct device *dev, struct device_driver *ddrv) { - struct sdw_slave *slave = dev_to_sdw_dev(dev); - struct sdw_driver *drv = drv_to_sdw_driver(ddrv); + struct sdw_slave *slave; + struct sdw_driver *drv; + int ret = 0; + + if (is_sdw_slave(dev)) { + slave = dev_to_sdw_dev(dev); + drv = drv_to_sdw_driver(ddrv);
- return !!sdw_get_device_id(slave, drv); + ret = !!sdw_get_device_id(slave, drv); + } + return ret; }
-int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size) +static int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, + size_t size) { /* modalias is sdw:m<mfg_id>p<part_id> */
@@ -47,7 +55,7 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size) slave->id.mfg_id, slave->id.part_id); }
-static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) +int sdw_slave_uevent(struct device *dev, struct kobj_uevent_env *env) { struct sdw_slave *slave = dev_to_sdw_dev(dev); char modalias[32]; @@ -63,7 +71,6 @@ static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) struct bus_type sdw_bus_type = { .name = "soundwire", .match = sdw_bus_match, - .uevent = sdw_uevent, }; EXPORT_SYMBOL_GPL(sdw_bus_type);
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index aace57fae7f8..ed068a004bd9 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -14,6 +14,12 @@ static void sdw_slave_release(struct device *dev) kfree(slave); }
+struct device_type sdw_slave_type = { + .name = "sdw_slave", + .release = sdw_slave_release, + .uevent = sdw_slave_uevent, +}; + static int sdw_slave_add(struct sdw_bus *bus, struct sdw_slave_id *id, struct fwnode_handle *fwnode) { @@ -41,9 +47,9 @@ static int sdw_slave_add(struct sdw_bus *bus, id->class_id, id->unique_id); }
- slave->dev.release = sdw_slave_release; slave->dev.bus = &sdw_bus_type; slave->dev.of_node = of_node_get(to_of_node(fwnode)); + slave->dev.type = &sdw_slave_type; slave->bus = bus; slave->status = SDW_SLAVE_UNATTACHED; init_completion(&slave->enumeration_complete); diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h index aaa7f4267c14..52eb66cd11bc 100644 --- a/include/linux/soundwire/sdw_type.h +++ b/include/linux/soundwire/sdw_type.h @@ -5,6 +5,13 @@ #define __SOUNDWIRE_TYPES_H
extern struct bus_type sdw_bus_type; +extern struct device_type sdw_slave_type; +extern struct device_type sdw_master_type; + +static inline int is_sdw_slave(const struct device *dev) +{ + return dev->type == &sdw_slave_type; +}
#define drv_to_sdw_driver(_drv) container_of(_drv, struct sdw_driver, driver)
@@ -14,7 +21,7 @@ extern struct bus_type sdw_bus_type; int __sdw_register_driver(struct sdw_driver *drv, struct module *owner); void sdw_unregister_driver(struct sdw_driver *drv);
-int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size); +int sdw_slave_uevent(struct device *dev, struct kobj_uevent_env *env);
/** * module_sdw_driver() - Helper macro for registering a Soundwire driver
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
In the existing SoundWire code, Master Devices are not explicitly represented - only SoundWire Slave Devices are exposed (the use of capital letters follows the SoundWire specification conventions).
The SoundWire Master Device provides the clock, synchronization information and command/control channels. When multiple links are supported, a Controller may expose more than one Master Device; they are typically embedded inside a larger audio cluster (be it in an SOC/chipset or an external audio codec), and we need to describe it using the Linux device and driver model.
This transition will avoid abusing platform devices and allow for better sysfs support without the reference count issues mentioned in the initial reviews.
The sdw_master_device addition is done with minimal internal plumbing and not exposed externally. The existing API based on sdw_bus_master_add() and sdw_bus_master_delete() will deal with the sdw_master_device life cycle, which minimizes changes to existing drivers.
Note that the Intel code will be modified in follow-up patches (no impact on any platform since the connection with ASoC is not supported upstream so far).
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/Makefile | 2 +- drivers/soundwire/bus.c | 12 ++++-- drivers/soundwire/bus.h | 3 ++ drivers/soundwire/master.c | 79 +++++++++++++++++++++++++++++++++++ drivers/soundwire/qcom.c | 1 - include/linux/soundwire/sdw.h | 17 +++++++- 6 files changed, 108 insertions(+), 6 deletions(-) create mode 100644 drivers/soundwire/master.c
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile index e2cdff990e9f..7319918e0aec 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 +soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o stream.o obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o
ifdef CONFIG_DEBUG_FS diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 18024ff770f8..7eb1e6efd567 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent, struct sdw_master_prop *prop = NULL; int ret;
- if (!bus->dev) { - pr_err("SoundWire bus has no device\n"); - return -ENODEV; + if (!bus) + return -EINVAL; + + ret = sdw_master_device_add(bus, parent, fwnode); + if (ret) { + dev_err(parent, "Failed to add master device at link %d\n", + bus->link_id); + return ret; }
if (!bus->ops) { @@ -142,6 +147,7 @@ static int sdw_delete_slave(struct device *dev, void *data) void sdw_bus_master_delete(struct sdw_bus *bus) { device_for_each_child(bus->dev, NULL, sdw_delete_slave); + sdw_master_device_del(bus);
sdw_bus_debugfs_exit(bus); } diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h index 204204a26db8..93ab0234a491 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -19,6 +19,9 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus) int sdw_of_find_slaves(struct sdw_bus *bus); void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id); +int sdw_master_device_add(struct sdw_bus *bus, struct device *parent, + struct fwnode_handle *fwnode); +int sdw_master_device_del(struct sdw_bus *bus);
#ifdef CONFIG_DEBUG_FS void sdw_bus_debugfs_init(struct sdw_bus *bus); diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c new file mode 100644 index 000000000000..2eeb2d7f56e0 --- /dev/null +++ b/drivers/soundwire/master.c @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: GPL-2.0-only +// Copyright(c) 2019-2020 Intel Corporation. + +#include <linux/device.h> +#include <linux/acpi.h> +#include <linux/soundwire/sdw.h> +#include <linux/soundwire/sdw_type.h> +#include "bus.h" + +/* nothing to free but this function is mandatory */ +static void sdw_master_device_release(struct device *dev) +{ +} + +struct device_type sdw_master_type = { + .name = "soundwire_master", + .release = sdw_master_device_release, +}; + +/** + * sdw_master_device_add() - create a Linux Master Device representation. + * @bus: SDW bus instance + * @parent: parent device + * @fwnode: firmware node handle + */ +int sdw_master_device_add(struct sdw_bus *bus, struct device *parent, + struct fwnode_handle *fwnode) +{ + struct sdw_master_device *md; + int ret; + + if (!bus) + return -EINVAL; + + /* + * Unlike traditional devices, there's no allocation here since the + * sdw_master_device is embedded in the bus structure. + */ + md = &bus->md; + md->dev.bus = &sdw_bus_type; + md->dev.type = &sdw_master_type; + md->dev.parent = parent; + md->dev.of_node = parent->of_node; + md->dev.fwnode = fwnode; + md->dev.dma_mask = parent->dma_mask; + + dev_set_name(&md->dev, "sdw-master-%d", bus->link_id); + + ret = device_register(&md->dev); + if (ret) { + dev_err(parent, "Failed to add master: ret %d\n", ret); + /* + * On err, don't free but drop ref as this will be freed + * when release method is invoked. + */ + put_device(&md->dev); + goto device_register_err; + } + + /* add shortcuts to improve code readability/compactness */ + md->bus = bus; + bus->dev = &md->dev; + +device_register_err: + return ret; +} + +/** + * sdw_master_device_del() - delete a Linux Master Device representation. + * @bus: bus handle + * + * This function is the dual of sdw_master_device_add() + */ +int sdw_master_device_del(struct sdw_bus *bus) +{ + device_unregister(bus->dev); + + return 0; +} diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index 401811d6627e..1c335ab1cd3f 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -784,7 +784,6 @@ static int qcom_swrm_probe(struct platform_device *pdev) mutex_init(&ctrl->port_lock); INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
- ctrl->bus.dev = dev; ctrl->bus.ops = &qcom_swrm_ops; ctrl->bus.port_ops = &qcom_swrm_port_ops; ctrl->bus.compute_params = &qcom_swrm_compute_params; diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 2003e8c55538..071adf2b463f 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -632,6 +632,19 @@ struct sdw_slave {
#define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
+/** + * struct sdw_master_device - SoundWire 'Master Device' representation + * @dev: Linux device for this Master + * @bus: Bus handle shortcut to improve readability (same as container_of) + */ +struct sdw_master_device { + struct device dev; + struct sdw_bus *bus; +}; + +#define dev_to_sdw_master_device(d) \ + container_of(d, struct sdw_master_device, dev) + struct sdw_driver { const char *name;
@@ -787,7 +800,8 @@ struct sdw_master_ops {
/** * struct sdw_bus - SoundWire bus - * @dev: Master linux device + * @dev: shortcut to &md->dev to improve readability + * @md: Master device * @link_id: Link id number, can be 0 to N, unique for each Master * @slaves: list of Slaves on this bus * @assigned: Bitmap for Slave device numbers. @@ -812,6 +826,7 @@ struct sdw_master_ops { */ struct sdw_bus { struct device *dev; + struct sdw_master_device md; unsigned int link_id; struct list_head slaves; DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
On 30-04-20, 02:51, Bard Liao wrote:
@@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent, struct sdw_master_prop *prop = NULL; int ret;
- if (!bus->dev) {
pr_err("SoundWire bus has no device\n");return -ENODEV;
This check is removed and not moved into sdw_master_device_add() either, can you add here or in patch 1 and keep checking the parent device please
+int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
struct fwnode_handle *fwnode)+{
- struct sdw_master_device *md;
- int ret;
- if (!bus)
return -EINVAL;- /*
* Unlike traditional devices, there's no allocation here since the* sdw_master_device is embedded in the bus structure.*/
Looking at this and empty sdw_master_device_release() above, makes me wonder if it is a wise move? Should we rather allocate the sdw_master_device() and then free that up in sdw_master_device_release() or it is really overkill given that this is called when we remove the sdw_bus instance as well...
- md = &bus->md;
- md->dev.bus = &sdw_bus_type;
- md->dev.type = &sdw_master_type;
- md->dev.parent = parent;
- md->dev.of_node = parent->of_node;
- md->dev.fwnode = fwnode;
- md->dev.dma_mask = parent->dma_mask;
- dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
This give nice sdw-master-0. In DT this comes from reg property. I dont seem to recall if the ACPI/Disco spec treats link_id as unique across the system, can you check that please, if not we would need to update this.
-----Original Message----- From: Vinod Koul vkoul@kernel.org Sent: Monday, May 11, 2020 2:32 PM To: Bard Liao yung-chuan.liao@linux.intel.com Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de; broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com; srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com; ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre- louis.bossart@linux.intel.com; Kale, Sanyog R sanyog.r.kale@intel.com; Blauciak, Slawomir slawomir.blauciak@intel.com; Lin, Mengdong mengdong.lin@intel.com; Liao, Bard bard.liao@intel.com Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
On 30-04-20, 02:51, Bard Liao wrote:
@@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct
device *parent,
struct sdw_master_prop *prop = NULL; int ret;
- if (!bus->dev) {
pr_err("SoundWire bus has no device\n");return -ENODEV;This check is removed and not moved into sdw_master_device_add() either, can you add here or in patch 1 and keep checking the parent device please
We will set bus->dev = &md->dev in the end of sdw_master_device_add(). That's why we remove the test. But now I am wandering does it make sense to set bus->dev = &md->dev? Maybe it makes more sense to set bus->dev = sdw control device. A follow up question is that should slave device a child of bus device or master device? I would prefer bus device if it makes sense. I will check bus->dev and parent and remove bus->dev = &md->dev in the next version.
+int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
struct fwnode_handle *fwnode)+{
- struct sdw_master_device *md;
- int ret;
- if (!bus)
return -EINVAL;- /*
* Unlike traditional devices, there's no allocation here since the* sdw_master_device is embedded in the bus structure.*/Looking at this and empty sdw_master_device_release() above, makes me wonder if it is a wise move? Should we rather allocate the sdw_master_device() and then free that up in sdw_master_device_release() or it is really overkill given that this is called when we remove the sdw_bus instance as well...
Yes, I will allocate sdw_master_device here and free it in sdw_master_device_release().
- md = &bus->md;
- md->dev.bus = &sdw_bus_type;
- md->dev.type = &sdw_master_type;
- md->dev.parent = parent;
- md->dev.of_node = parent->of_node;
- md->dev.fwnode = fwnode;
- md->dev.dma_mask = parent->dma_mask;
- dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
This give nice sdw-master-0. In DT this comes from reg property. I dont seem to recall if the ACPI/Disco spec treats link_id as unique across the system, can you check that please, if not we would need to update this.
Sure, I will check it.
-- ~Vinod
On 11-05-20, 08:04, Liao, Bard wrote:
-----Original Message----- From: Vinod Koul vkoul@kernel.org Sent: Monday, May 11, 2020 2:32 PM To: Bard Liao yung-chuan.liao@linux.intel.com Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de; broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com; srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com; ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre- louis.bossart@linux.intel.com; Kale, Sanyog R sanyog.r.kale@intel.com; Blauciak, Slawomir slawomir.blauciak@intel.com; Lin, Mengdong mengdong.lin@intel.com; Liao, Bard bard.liao@intel.com Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
On 30-04-20, 02:51, Bard Liao wrote:
@@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct
device *parent,
struct sdw_master_prop *prop = NULL; int ret;
- if (!bus->dev) {
pr_err("SoundWire bus has no device\n");return -ENODEV;This check is removed and not moved into sdw_master_device_add() either, can you add here or in patch 1 and keep checking the parent device please
We will set bus->dev = &md->dev in the end of sdw_master_device_add().
We need to test if this is valid or not :)
That's why we remove the test. But now I am wandering does it make sense to set bus->dev = &md->dev? Maybe it makes more sense to set bus->dev = sdw control device. A follow up question is that should slave device a child of bus device or master device? I would prefer bus device if it makes sense. I will check bus->dev and parent and remove bus->dev = &md->dev in the next version.
the parent is bus->dev and sdw_master_device created would be child of this and should be set as such. You can remove it from bus object and keep in sdw_master_device object, that is fine by me.
The sdw_slave is child of sdw_master_device now and looks to be set correct.
-----Original Message----- From: Vinod Koul vkoul@kernel.org Sent: Monday, May 11, 2020 5:00 PM To: Liao, Bard bard.liao@intel.com Cc: Bard Liao yung-chuan.liao@linux.intel.com; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de; broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com; srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com; ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre- louis.bossart@linux.intel.com; Kale, Sanyog R sanyog.r.kale@intel.com; Blauciak, Slawomir slawomir.blauciak@intel.com; Lin, Mengdong mengdong.lin@intel.com Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
On 11-05-20, 08:04, Liao, Bard wrote:
-----Original Message----- From: Vinod Koul vkoul@kernel.org Sent: Monday, May 11, 2020 2:32 PM To: Bard Liao yung-chuan.liao@linux.intel.com Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de; broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com; srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com; ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre- louis.bossart@linux.intel.com; Kale, Sanyog R sanyog.r.kale@intel.com; Blauciak, Slawomir slawomir.blauciak@intel.com; Lin, Mengdong mengdong.lin@intel.com; Liao, Bard bard.liao@intel.com Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
On 30-04-20, 02:51, Bard Liao wrote:
@@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct
device *parent,
struct sdw_master_prop *prop = NULL; int ret;
- if (!bus->dev) {
pr_err("SoundWire bus has no device\n");return -ENODEV;This check is removed and not moved into sdw_master_device_add() either, can you add here or in patch 1 and keep checking the parent device please
We will set bus->dev = &md->dev in the end of sdw_master_device_add().
We need to test if this is valid or not :)
That's why we remove the test. But now I am wandering does it make sense to set bus->dev = &md->dev? Maybe it makes more sense to set bus->dev = sdw control device. A follow up question is that should slave device a child of bus device or master device? I would prefer bus device if it makes sense. I will check bus->dev and parent and remove bus->dev = &md->dev in the next version.
the parent is bus->dev and sdw_master_device created would be child of this and should be set as such. You can remove it from bus object and keep in sdw_master_device object, that is fine by me.
Looks like we don't need the parent and fwnode parameter since we can get them from bus->dev 😊
The sdw_slave is child of sdw_master_device now and looks to be set correct.
So, it will be bus device -> master device -> slave device right?
I have a question here. We have pm supported on bus and slave devices, but not master device. Will pm work with this architecture? Can it be bus device -> master device & slave device?
-- ~Vinod
On 11-05-20, 11:34, Liao, Bard wrote:
-----Original Message----- From: Vinod Koul vkoul@kernel.org Sent: Monday, May 11, 2020 5:00 PM To: Liao, Bard bard.liao@intel.com Cc: Bard Liao yung-chuan.liao@linux.intel.com; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de; broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com; srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com; ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre- louis.bossart@linux.intel.com; Kale, Sanyog R sanyog.r.kale@intel.com; Blauciak, Slawomir slawomir.blauciak@intel.com; Lin, Mengdong mengdong.lin@intel.com Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
On 11-05-20, 08:04, Liao, Bard wrote:
-----Original Message----- From: Vinod Koul vkoul@kernel.org Sent: Monday, May 11, 2020 2:32 PM To: Bard Liao yung-chuan.liao@linux.intel.com Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de; broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com; srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com; ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre- louis.bossart@linux.intel.com; Kale, Sanyog R sanyog.r.kale@intel.com; Blauciak, Slawomir slawomir.blauciak@intel.com; Lin, Mengdong mengdong.lin@intel.com; Liao, Bard bard.liao@intel.com Subject: Re: [PATCH 3/3] soundwire: bus_type: add sdw_master_device support
On 30-04-20, 02:51, Bard Liao wrote:
@@ -24,9 +24,14 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct
device *parent,
struct sdw_master_prop *prop = NULL; int ret;
- if (!bus->dev) {
pr_err("SoundWire bus has no device\n");return -ENODEV;This check is removed and not moved into sdw_master_device_add() either, can you add here or in patch 1 and keep checking the parent device please
We will set bus->dev = &md->dev in the end of sdw_master_device_add().
We need to test if this is valid or not :)
That's why we remove the test. But now I am wandering does it make sense to set bus->dev = &md->dev? Maybe it makes more sense to set bus->dev = sdw control device. A follow up question is that should slave device a child of bus device or master device? I would prefer bus device if it makes sense. I will check bus->dev and parent and remove bus->dev = &md->dev in the next version.
the parent is bus->dev and sdw_master_device created would be child of this and should be set as such. You can remove it from bus object and keep in sdw_master_device object, that is fine by me.
Looks like we don't need the parent and fwnode parameter since we can get them from bus->dev 😊
Quite right
The sdw_slave is child of sdw_master_device now and looks to be set correct.
So, it will be bus device -> master device -> slave device right?
yes
I have a question here. We have pm supported on bus and slave devices, but not master device. Will pm work with this architecture? Can it be bus device -> master device & slave device?
yes it would and you should check it out. The pm (runtime_pm) works well with child devices and parents, so we need to ensure that parents are set properly.
Thanks
- md = &bus->md;
- md->dev.bus = &sdw_bus_type;
- md->dev.type = &sdw_master_type;
- md->dev.parent = parent;
- md->dev.of_node = parent->of_node;
- md->dev.fwnode = fwnode;
- md->dev.dma_mask = parent->dma_mask;
- dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
This give nice sdw-master-0. In DT this comes from reg property. I dont seem to recall if the ACPI/Disco spec treats link_id as unique across the system, can you check that please, if not we would need to update this.
Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are relative to the immediate parent Device."
There isn't any known implementation with more than one controller.
On 11-05-20, 14:00, Pierre-Louis Bossart wrote:
- md = &bus->md;
- md->dev.bus = &sdw_bus_type;
- md->dev.type = &sdw_master_type;
- md->dev.parent = parent;
- md->dev.of_node = parent->of_node;
- md->dev.fwnode = fwnode;
- md->dev.dma_mask = parent->dma_mask;
- dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
This give nice sdw-master-0. In DT this comes from reg property. I dont seem to recall if the ACPI/Disco spec treats link_id as unique across the system, can you check that please, if not we would need to update this.
Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are relative to the immediate parent Device."
There isn't any known implementation with more than one controller.
But then it can come in "future" right. So lets try to make it future proof by not using the link_id (we can expose that as a sysfs if people want to know). So a global unique id needs to allocated (hint: idr or equivalent) and used as master_id
Thanks
On 5/11/20 10:30 PM, Vinod Koul wrote:
On 11-05-20, 14:00, Pierre-Louis Bossart wrote:
- md = &bus->md;
- md->dev.bus = &sdw_bus_type;
- md->dev.type = &sdw_master_type;
- md->dev.parent = parent;
- md->dev.of_node = parent->of_node;
- md->dev.fwnode = fwnode;
- md->dev.dma_mask = parent->dma_mask;
- dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
This give nice sdw-master-0. In DT this comes from reg property. I dont seem to recall if the ACPI/Disco spec treats link_id as unique across the system, can you check that please, if not we would need to update this.
Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are relative to the immediate parent Device."
There isn't any known implementation with more than one controller.
But then it can come in "future" right. So lets try to make it future proof by not using the link_id (we can expose that as a sysfs if people want to know). So a global unique id needs to allocated (hint: idr or equivalent) and used as master_id
Can you clarify if you are asking for a global ID for Intel/ACPI platforms, or for DT as well? I can't figure out from the soundwire-controller.yaml definitions if there is already a notion of unique ID.
properties: $nodename: pattern: "^soundwire(@.*)?$"
soundwire@c2d0000 { #address-cells = <2>; #size-cells = <0>; reg = <0x0c2d0000 0x2000>;
On 12-05-20, 09:36, Pierre-Louis Bossart wrote:
On 5/11/20 10:30 PM, Vinod Koul wrote:
On 11-05-20, 14:00, Pierre-Louis Bossart wrote:
- md = &bus->md;
- md->dev.bus = &sdw_bus_type;
- md->dev.type = &sdw_master_type;
- md->dev.parent = parent;
- md->dev.of_node = parent->of_node;
- md->dev.fwnode = fwnode;
- md->dev.dma_mask = parent->dma_mask;
- dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
This give nice sdw-master-0. In DT this comes from reg property. I dont seem to recall if the ACPI/Disco spec treats link_id as unique across the system, can you check that please, if not we would need to update this.
Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are relative to the immediate parent Device."
There isn't any known implementation with more than one controller.
But then it can come in "future" right. So lets try to make it future proof by not using the link_id (we can expose that as a sysfs if people want to know). So a global unique id needs to allocated (hint: idr or equivalent) and used as master_id
Can you clarify if you are asking for a global ID for Intel/ACPI platforms, or for DT as well? I can't figure out from the soundwire-controller.yaml definitions if there is already a notion of unique ID.
If ACPI was unique, then I was planning to update the definition below to include that. Given that it is not the case, let's make it agnostic to underlying firmware.
properties: $nodename: pattern: "^soundwire(@.*)?$"
soundwire@c2d0000 { #address-cells = <2>; #size-cells = <0>; reg = <0x0c2d0000 0x2000>;
On 5/12/20 10:59 AM, Vinod Koul wrote:
On 12-05-20, 09:36, Pierre-Louis Bossart wrote:
On 5/11/20 10:30 PM, Vinod Koul wrote:
On 11-05-20, 14:00, Pierre-Louis Bossart wrote:
- md = &bus->md;
- md->dev.bus = &sdw_bus_type;
- md->dev.type = &sdw_master_type;
- md->dev.parent = parent;
- md->dev.of_node = parent->of_node;
- md->dev.fwnode = fwnode;
- md->dev.dma_mask = parent->dma_mask;
- dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
This give nice sdw-master-0. In DT this comes from reg property. I dont seem to recall if the ACPI/Disco spec treats link_id as unique across the system, can you check that please, if not we would need to update this.
Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are relative to the immediate parent Device."
There isn't any known implementation with more than one controller.
But then it can come in "future" right. So lets try to make it future proof by not using the link_id (we can expose that as a sysfs if people want to know). So a global unique id needs to allocated (hint: idr or equivalent) and used as master_id
Can you clarify if you are asking for a global ID for Intel/ACPI platforms, or for DT as well? I can't figure out from the soundwire-controller.yaml definitions if there is already a notion of unique ID.
If ACPI was unique, then I was planning to update the definition below to include that. Given that it is not the case, let's make it agnostic to underlying firmware.
I am not sure I understand how this would be done.
The call sequence is
sdw_bus_master_add(bus) sdw_master_device_add(bus, parent, fw_node)
At the bus level, we don't have any information on which controller the bus is related to.
We'd need to add an argument to sdw_bus_master_add() and have the controller unique ID be allocated outside of the SoundWire core, hence my question on whether the DT definition should not be extended.
properties: $nodename: pattern: "^soundwire(@.*)?$"
soundwire@c2d0000 { #address-cells = <2>; #size-cells = <0>; reg = <0x0c2d0000 0x2000>;
On 5/12/20 11:08 AM, Pierre-Louis Bossart wrote:
On 5/12/20 10:59 AM, Vinod Koul wrote:
On 12-05-20, 09:36, Pierre-Louis Bossart wrote:
On 5/11/20 10:30 PM, Vinod Koul wrote:
On 11-05-20, 14:00, Pierre-Louis Bossart wrote:
> +Â Â Â md = &bus->md; > +Â Â Â md->dev.bus = &sdw_bus_type; > +Â Â Â md->dev.type = &sdw_master_type; > +Â Â Â md->dev.parent = parent; > +Â Â Â md->dev.of_node = parent->of_node; > +Â Â Â md->dev.fwnode = fwnode; > +Â Â Â md->dev.dma_mask = parent->dma_mask; > + > +Â Â Â dev_set_name(&md->dev, "sdw-master-%d", bus->link_id);
This give nice sdw-master-0. In DT this comes from reg property. I dont seem to recall if the ACPI/Disco spec treats link_id as unique across the system, can you check that please, if not we would need to update this.
Table 3 in the Disco for Soundwire 1.0 spec: "all LinkID values are relative to the immediate parent Device."
There isn't any known implementation with more than one controller.
But then it can come in "future" right. So lets try to make it future proof by not using the link_id (we can expose that as a sysfs if people want to know). So a global unique id needs to allocated (hint: idr or equivalent) and used as master_id
Can you clarify if you are asking for a global ID for Intel/ACPI platforms, or for DT as well? I can't figure out from the soundwire-controller.yaml definitions if there is already a notion of unique ID.
If ACPI was unique, then I was planning to update the definition below to include that. Given that it is not the case, let's make it agnostic to underlying firmware.
I am not sure I understand how this would be done.
The call sequence is
sdw_bus_master_add(bus) Â Â Â sdw_master_device_add(bus, parent, fw_node)
At the bus level, we don't have any information on which controller the bus is related to.
We'd need to add an argument to sdw_bus_master_add() and have the controller unique ID be allocated outside of the SoundWire core, hence my question on whether the DT definition should not be extended.
And btw I don't think it makes sense to add a new definition for Intel. We already have a notion of HDaudio bus->idx that's set to zero since we don't have a case for multiple HDaudio controllers.
if we ever do have more than once controller, then we should rely on HDaudio bus->idx as the identifier and not create one specifically for SoundWire - which means as I mentioned above passing an argument and not defining a controller ID in the SoundWire core.
On 12-05-20, 12:01, Pierre-Louis Bossart wrote:
There isn't any known implementation with more than one controller.
But then it can come in "future" right. So lets try to make it future proof by not using the link_id (we can expose that as a sysfs if people want to know). So a global unique id needs to allocated (hint: idr or equivalent) and used as master_id
Can you clarify if you are asking for a global ID for Intel/ACPI platforms, or for DT as well? I can't figure out from the soundwire-controller.yaml definitions if there is already a notion of unique ID.
If ACPI was unique, then I was planning to update the definition below to include that. Given that it is not the case, let's make it agnostic to underlying firmware.
I am not sure I understand how this would be done.
The call sequence is
sdw_bus_master_add(bus) Â Â Â sdw_master_device_add(bus, parent, fw_node)
At the bus level, we don't have any information on which controller the bus is related to.
This should be done inside the sdw_bus. controller should not care about this IMO.
We'd need to add an argument to sdw_bus_master_add() and have the controller unique ID be allocated outside of the SoundWire core, hence my question on whether the DT definition should not be extended.
And btw I don't think it makes sense to add a new definition for Intel. We already have a notion of HDaudio bus->idx that's set to zero since we don't have a case for multiple HDaudio controllers.
if we ever do have more than once controller, then we should rely on HDaudio bus->idx as the identifier and not create one specifically for SoundWire - which means as I mentioned above passing an argument and not defining a controller ID in the SoundWire core.
I was thinking of following code in bus.c
static DEFINE_IDA(sdw_ida);
static sdw_get_id(struct sdw_bus *bus) { int rc = ida_alloc(&sdw_ida, GFP_KERNEL);
if (rc < 0) return rc;
bus->id = rc; return 0; }
int sdw_add_bus_master(struct sdw_bus *bus) { ...
ret = sdw_get_id(bus);
... }
void sdw_delete_bus_master(struct sdw_bus *bus) { da_free(&sdw_ida, bus->id); }
This way you get a unique master number across all devices and this has nothing to do with link/of ids and is used for numbering masters in sysfs uniquely.
HTH
participants (4)
-
Bard Liao -
Liao, Bard -
Pierre-Louis Bossart -
Vinod Koul