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

[Vinod Koul <vkoul@kernel.org>]

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.
-- 
~Vinod