Re: [PATCH 1/8] soundwire: bus_type: add master_device/driver support

On 27-02-20, 16:31, Pierre-Louis Bossart wrote:

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

I dont not see that as a soundwire issue. The external dependencies should be handled as any device would do in Linux kernel with subsystem specific things for soundwire mechanisms like wake-up

Intel has a big controller with HDA, DSP and Soundwire clubbed together, I dont think we should burden the susbstem due to hw design

also allow for better sysfs support without the reference count issues mentioned in the initial reviews.

In this patch, we convert the existing code to use an explicit sdw_slave_type, then define new objects (sdw_master_device and sdw_master_driver).

Thanks for sdw_master_device, that is required and fully agreed upon. What is not agreed is the sdw_master_driver. We do not need that.

As we have discussed your proposal with Greg and aligned (quoting that here) on following device model for Intel and ARM:

• For DT cases we will have: -> soundwire DT device (soundwire DT driver) -> soundwire master device -> soundwire slave device (slave drivers)
• For Intel case, you would have: -> HDA PCI device (SOF driver + soundwire module) -> soundwire master device -> soundwire slave device (slave drivers)

But you have gone ahead and kept the sdw_master_driver which does not fit into rest of the world except Intel.

I think I am okay with rest of proposal, except this one, so can you remove this and we can make progress. This issue is lingering since Oct!

A parent (such as the Intel audio controller or its equivalent on Qualcomm devices) would use sdw_master_device_add() to create the device, passing a driver name as a parameter. The master device would be released when device_unregister() is invoked by the parent.

We already have a DT driver for soundwire master! We dont need another layer which does not add value!

Note that since there is no standard for the Master host-facing interface, so the bus matching relies on a simple string matching (as previously done with platform devices).

The 'Master Device' driver exposes callbacks for probe/startup/shutdown/remove/process_wake. The startup and process wake need to be called by the parent directly (using wrappers), while the probe/shutdown/remove are handled by the SoundWire bus core upon device creation and release.

these are added to handle intel DSP and sequencing issue, rest of the world does not have these issues and does not needs them!

Additional callbacks will be added in the future for e.g. autonomous clock stop modes.

Yes these would be required, these can be added in sdw_master_device too, I dont see them requiring a dummy driver layer..

@@ -113,8 +152,6 @@ static int sdw_drv_probe(struct device *dev) slave->probed = true; complete(&slave->probe_complete);

• dev_dbg(dev, "probe complete\n");

This does not seem to belong to this patch.

+struct device_type sdw_master_type = {

• .name = "soundwire_master",
• .release = sdw_master_device_release,

+};

+struct sdw_master_device +*sdw_master_device_add(const char *master_name,

•        struct device *parent,
  

•        struct fwnode_handle *fwnode,
  

•        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->master_name = master_name;

should we not allocate the memory here for master_name?

• init_completion(&md->probe_complete);
• 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);

why do we need master_name if we are setting this here?

• 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);
  

• return ERR_PTR(-ENOMEM);
  

ENOMEM?

+int sdw_master_device_startup(struct sdw_master_device *md) +{

• struct sdw_master_driver *mdrv;
• struct device *dev;
• int ret = 0;
• if (IS_ERR_OR_NULL(md))

• return -EINVAL;
  

• dev = &md->dev;
• mdrv = drv_to_sdw_master_driver(dev->driver);
• if (mdrv && mdrv->startup)

• ret = mdrv->startup(md);
  

• return ret;

+} +EXPORT_SYMBOL_GPL(sdw_master_device_startup);

who invokes this and when, can you add kernel-doc style documentation to all APIs exported

+int sdw_master_device_process_wake_event(struct sdw_master_device *md) +{

• struct sdw_master_driver *mdrv;
• struct device *dev;
• int ret = 0;
• if (IS_ERR_OR_NULL(md))

• return -EINVAL;
  

• dev = &md->dev;
• mdrv = drv_to_sdw_master_driver(dev->driver);
• if (mdrv && mdrv->process_wake_event)

• ret = mdrv->process_wake_event(md);
  

• return ret;

+} +EXPORT_SYMBOL_GPL(sdw_master_device_process_wake_event);

Documentation required

+/**

• • struct sdw_master_device - SoundWire 'Master Device' representation
• • @dev: Linux device for this Master
• • @master_name: Linux driver name
• • @driver: Linux driver for this Master (set by SoundWire core during probe)
• • @probe_complete: used by parent if synchronous probe behavior is needed
• • @link_id: link index as defined by MIPI DisCo specification
• • @pm_runtime_suspended: flag to restore pm_runtime state after system resume
• • @pdata: private data typically provided with sdw_master_device_add()
• */

+struct sdw_master_device {

• struct device dev;
• const char *master_name;
• struct sdw_master_driver *driver;
• struct completion probe_complete;
• int link_id;
• bool pm_runtime_suspended;

why not use runtime_pm apis like pm_runtime_suspended()

+/**

• • sdw_master_device_add() - create a Linux Master Device representation.
• • @master_name: Linux driver name
• • @parent: the parent Linux device (e.g. a PCI device)
• • @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent)
• • @link_id: link index as defined by MIPI DisCo specification
• • @pdata: private data (e.g. register base, offsets, platform quirks, etc).
• */

+struct sdw_master_device +*sdw_master_device_add(const char *master_name,

•        struct device *parent,
  

•        struct fwnode_handle *fwnode,
  

•        int link_id,
  

•        void *pdata);
  

+/**

• • sdw_master_device_startup() - startup hardware
• • @md: Linux Soundwire master device

Please add more useful comments like when this API would be invoked and what shall be expected outcome

• */

+int sdw_master_device_startup(struct sdw_master_device *md);

+/**

• • sdw_master_device_process_wake_event() - handle external wake
• • event, e.g. handled at the PCI level
• • @md: Linux Soundwire master device
• */

+int sdw_master_device_process_wake_event(struct sdw_master_device *md);

If you look at existing headers the documentation is in C files for APIs, so can you move them over.

When adding stuff please look at the rest of the code as an example.