Hello Bard,
On 17-04-20, 04:55, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
>
> 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.
--
~Vinod
