On Mon, Apr 20, 2020 at 12:56:31PM +0530, Vinod Koul wrote:
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.
-ENOPARSE :(
It *also* helps some vendors due to inherent model should not be constructed as the primary approach for the sdw_master_device.
No, the PRIMARY reason is "it is the correct thing to do". It's how to tie into the driver model correctly, without it, crazy things happen as we have seen.
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.
Can you really do that in two different steps?
+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...
Why is it not true for everyone? How else do you get the pm stuff back to your hardware?
thanks,
greg k-h