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
