Hello Bard,
On 17-04-20, 04:55, Bard Liao wrote:
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 will allow for configuration functions to account for external dependencies such as power rails, clock sources or wake-up mechanisms. This transition will also allow for better sysfs support without the reference count issues mentioned in the initial reviews.
Well the primary reason for doing sdw_master_device for creating a adding sysfs representation. It *also* helps some vendors due to inherent model should not be constructed as the primary approach for the sdw_master_device.
In this patch, we convert the existing code to use an explicit sdw_slave_type, then define a sdw_master_device structure.
Please split that up, we should do the conversions required first and then do addition of new things.
+struct device_type sdw_master_type = {
- .name = "soundwire_master",
- .release = sdw_master_device_release,
+};
+/**
- sdw_master_device_add() - create a Linux Master Device representation.
- @parent: the parent Linux device (e.g. a PCI device)
- @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent)
- @link_ops: link-specific ops (optional)
- @link_id: link index as defined by MIPI DisCo specification
- @pdata: private data (e.g. register base, offsets, platform quirks, etc).
- The link_ops argument can be NULL, it is only used when link-specific
- initializations and power-management are required.
- */
+struct sdw_master_device +*sdw_master_device_add(struct device *parent,
struct fwnode_handle *fwnode,struct sdw_link_ops *link_ops,int link_id,void *pdata)+{
- struct sdw_master_device *md;
- int ret;
- md = kzalloc(sizeof(*md), GFP_KERNEL);
- if (!md)
return ERR_PTR(-ENOMEM);- md->link_id = link_id;
- md->pdata = pdata;
- md->link_ops = link_ops;
- md->dev.parent = parent;
- md->dev.fwnode = fwnode;
- md->dev.bus = &sdw_bus_type;
- md->dev.type = &sdw_master_type;
- md->dev.dma_mask = md->dev.parent->dma_mask;
- dev_set_name(&md->dev, "sdw-master-%d", md->link_id);
- if (link_ops && link_ops->driver) {
/** A driver is only needed for ASoC integration (need* driver->name) and for link-specific power management* w/ a pm_dev_ops structure.
That is not true for everyone, it is only true for Intel, pls call that out as well...
** The driver needs to be registered by the parent*/md->dev.driver = link_ops->driver;- }
- 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;- }
- if (link_ops && link_ops->add) {
ret = link_ops->add(md, pdata);if (ret < 0) {dev_err(&md->dev, "link_ops add callback failed: %d\n",ret);goto link_add_err;}- }
- return md;
+link_add_err:
- device_unregister(&md->dev);
+device_register_err:
- return ERR_PTR(ret);
+} +EXPORT_SYMBOL_GPL(sdw_master_device_add);
This looks better than last version for sure. But I would like this to be split into two parts, a generic sdw_master_device addition without the link_ops parts. And then the link_ops parts..
As discussed earlier with you, I see no reason why users should have two APIs. We should fold the sdw_master_device_add() within the sdw_add_bus_master() afterall as part of adding bus, we should be creating the sdw_master_dev as well as sdw_slave.
Since you have additional link_ops, we can pass that to sdw_add_bus_master() (set to NULL for rest) and then call sdw_master_device_add() internally..
As requested above, please split this to separate patches, first generic sdw_master_device addition and calling from sdw_add_bus_master() and then adding link_ops parts for Intel.
Ofcourse any preparatory patches should come before that.