[RFC,1/5] soundwire: bus_type: add sdw_master_device support

Commit Message

Bard Liao April 16, 2020, 8:55 p.m. UTC

From: Pierre-Louis Bossart <[e-mail]>

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.

In this patch, we convert the existing code to use an explicit
sdw_slave_type, then define a sdw_master_device structure.

A parent (such as the Intel audio controller or its equivalent on
Qualcomm devices) would use sdw_master_device_add() to create the
device, the master device would be released when
sdw_master_device_del() is invoked by the parent.

The 'Master Device' device can be configured with optional link ops,
in case an implementation needs link-specific resources allocated or
power-management capabilities. The .add/.del callbacks are handled
internally by the SoundWire core, while the optional
.startup/.process_wake need to be called by the parent directly (using
wrappers). The uevent handling is moved to the slave side since it's
not relevant for master devices.

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

Credits to Jaroslav Kysela <[e-mail]> for the idea of using link_ops.

Signed-off-by: Pierre-Louis Bossart <[e-mail]>
Signed-off-by: Bard Liao <[e-mail]>
---
 drivers/soundwire/Makefile         |   2 +-
 drivers/soundwire/bus.h            |   2 +
 drivers/soundwire/bus_type.c       |  19 ++--
 drivers/soundwire/master.c         | 167 +++++++++++++++++++++++++++++
 drivers/soundwire/slave.c          |   8 +-
 include/linux/soundwire/sdw.h      |  60 +++++++++++
 include/linux/soundwire/sdw_type.h |  10 +-
 7 files changed, 258 insertions(+), 10 deletions(-)
 create mode 100644 drivers/soundwire/master.c

Comments

Vinod Koul April 20, 2020, 7:26 a.m. UTC | #1

Hello Bard,

On 17-04-20, 04:55, Bard Liao wrote:
> From: Pierre-Louis Bossart <[e-mail]>
> 
> 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.

Greg KH April 23, 2020, 2:24 p.m. UTC | #2

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 <[e-mail]>
> > 
> > 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

Vinod Koul April 28, 2020, 4:31 a.m. UTC | #3

Hi Greg,

On 23-04-20, 16:24, Greg KH wrote:
> 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 <[e-mail]>
> > > 
> > > 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 :(

Oops, sorry!

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

I agree it is *the* right this to do!

> > > 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?

Looking at it, the move of existing types first and then adding the new
type

> 
> > > +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?

The rest of the world would do using the real controller device. For
example the soundwire controller on Qualcomm devices is enumerated as a
DT device and is using these...

If Intel had a standalone controller or enumerated as individual
functions, it would have been a PCI device and would manage as such

Thanks

Greg KH April 28, 2020, 6:37 a.m. UTC | #4

On Tue, Apr 28, 2020 at 10:01:44AM +0530, Vinod Koul wrote:
> > > 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?
> 
> The rest of the world would do using the real controller device. For
> example the soundwire controller on Qualcomm devices is enumerated as a
> DT device and is using these...
> 
> If Intel had a standalone controller or enumerated as individual
> functions, it would have been a PCI device and would manage as such

If it is not a standalone controller, what exactly is it?  I thought it
was an acpi device, am I mistaken?

What is the device that the proper soundwire controller driver binds to
on an Intel-based system?

thanks,

greg k-h

Vinod Koul April 28, 2020, 6:49 a.m. UTC | #5

On 28-04-20, 08:37, Greg KH wrote:
> On Tue, Apr 28, 2020 at 10:01:44AM +0530, Vinod Koul wrote:
> > > > 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?
> > 
> > The rest of the world would do using the real controller device. For
> > example the soundwire controller on Qualcomm devices is enumerated as a
> > DT device and is using these...
> > 
> > If Intel had a standalone controller or enumerated as individual
> > functions, it would have been a PCI device and would manage as such
> 
> If it is not a standalone controller, what exactly is it?  I thought it
> was an acpi device, am I mistaken?
> 
> What is the device that the proper soundwire controller driver binds to
> on an Intel-based system?

The HDA controller which is a PCI device. The device represent HDA
function, DSP and Soundwire controller instances (yes it is typically
more than one instance)

Thanks

Greg KH April 28, 2020, 6:55 a.m. UTC | #6

On Tue, Apr 28, 2020 at 12:19:51PM +0530, Vinod Koul wrote:
> On 28-04-20, 08:37, Greg KH wrote:
> > On Tue, Apr 28, 2020 at 10:01:44AM +0530, Vinod Koul wrote:
> > > > > 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?
> > > 
> > > The rest of the world would do using the real controller device. For
> > > example the soundwire controller on Qualcomm devices is enumerated as a
> > > DT device and is using these...
> > > 
> > > If Intel had a standalone controller or enumerated as individual
> > > functions, it would have been a PCI device and would manage as such
> > 
> > If it is not a standalone controller, what exactly is it?  I thought it
> > was an acpi device, am I mistaken?
> > 
> > What is the device that the proper soundwire controller driver binds to
> > on an Intel-based system?
> 
> The HDA controller which is a PCI device. The device represent HDA
> function, DSP and Soundwire controller instances (yes it is typically
> more than one instance)

Then those "instances" should be split up into individual devices that a
driver can bind to.  See the work happening on the "virtual" bus for
examples of how that can be done.

A platform device better not be being used here, I'm afraid to look at
the code now...

greg k-h

Vinod Koul April 28, 2020, 7:51 a.m. UTC | #7

On 28-04-20, 08:55, Greg KH wrote:
> On Tue, Apr 28, 2020 at 12:19:51PM +0530, Vinod Koul wrote:
> > On 28-04-20, 08:37, Greg KH wrote:
> > > On Tue, Apr 28, 2020 at 10:01:44AM +0530, Vinod Koul wrote:
> > > > > > 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?
> > > > 
> > > > The rest of the world would do using the real controller device. For
> > > > example the soundwire controller on Qualcomm devices is enumerated as a
> > > > DT device and is using these...
> > > > 
> > > > If Intel had a standalone controller or enumerated as individual
> > > > functions, it would have been a PCI device and would manage as such
> > > 
> > > If it is not a standalone controller, what exactly is it?  I thought it
> > > was an acpi device, am I mistaken?
> > > 
> > > What is the device that the proper soundwire controller driver binds to
> > > on an Intel-based system?
> > 
> > The HDA controller which is a PCI device. The device represent HDA
> > function, DSP and Soundwire controller instances (yes it is typically
> > more than one instance)
> 
> Then those "instances" should be split up into individual devices that a
> driver can bind to.  See the work happening on the "virtual" bus for
> examples of how that can be done.

Yes removing platform devices is the goal for Intel now :) Pierre & Bard
have been diligently trying to solve this.

Only difference is the means to end goal. I am not convinced that this
should be in soundwire subsystem.

Looks like folks are trying to review and port to use this bus. Makes
sense to me..
https://lore.kernel.org/netdev/[e-mail]/

> A platform device better not be being used here, I'm afraid to look at
> the code now...

Well if the plan for 'virtual-bus' goes well, it should be  a simple
replacement of platform->virtual for Intel driver. Rest of the driver
should not be impacted :)

Thanks

Bard Liao April 30, 2020, 3:24 a.m. UTC | #8

On 4/28/2020 3:51 PM, Vinod Koul wrote:
> On 28-04-20, 08:55, Greg KH wrote:
>> On Tue, Apr 28, 2020 at 12:19:51PM +0530, Vinod Koul wrote:
>>> On 28-04-20, 08:37, Greg KH wrote:
>>>> On Tue, Apr 28, 2020 at 10:01:44AM +0530, Vinod Koul wrote:
>>>>>>> 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?
>>>>> The rest of the world would do using the real controller device. For
>>>>> example the soundwire controller on Qualcomm devices is enumerated as a
>>>>> DT device and is using these...
>>>>>
>>>>> If Intel had a standalone controller or enumerated as individual
>>>>> functions, it would have been a PCI device and would manage as such
>>>> If it is not a standalone controller, what exactly is it?  I thought it
>>>> was an acpi device, am I mistaken?
>>>>
>>>> What is the device that the proper soundwire controller driver binds to
>>>> on an Intel-based system?
>>> The HDA controller which is a PCI device. The device represent HDA
>>> function, DSP and Soundwire controller instances (yes it is typically
>>> more than one instance)
>> Then those "instances" should be split up into individual devices that a
>> driver can bind to.  See the work happening on the "virtual" bus for
>> examples of how that can be done.
> Yes removing platform devices is the goal for Intel now :) Pierre & Bard
> have been diligently trying to solve this.
>
> Only difference is the means to end goal. I am not convinced that this
> should be in soundwire subsystem.
>
> Looks like folks are trying to review and port to use this bus. Makes
> sense to me..
> https://lore.kernel.org/netdev/[e-mail]/
>
>> A platform device better not be being used here, I'm afraid to look at
>> the code now...
> Well if the plan for 'virtual-bus' goes well, it should be  a simple
> replacement of platform->virtual for Intel driver. Rest of the driver
> should not be impacted :)

We can't expect when will 'virtual-bus' be upstream and it's not feasible
to wait forever. Can we move forward with current solution and switch to
'virtual-bus' whenever it is upstream?

> Thanks

Vinod Koul April 30, 2020, 4:57 a.m. UTC | #9

On 30-04-20, 11:24, Bard liao wrote:
> 
> On 4/28/2020 3:51 PM, Vinod Koul wrote:
> > On 28-04-20, 08:55, Greg KH wrote:
> > > On Tue, Apr 28, 2020 at 12:19:51PM +0530, Vinod Koul wrote:
> > > > On 28-04-20, 08:37, Greg KH wrote:
> > > > > On Tue, Apr 28, 2020 at 10:01:44AM +0530, Vinod Koul wrote:
> > > > > > > > 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?
> > > > > > The rest of the world would do using the real controller device. For
> > > > > > example the soundwire controller on Qualcomm devices is enumerated as a
> > > > > > DT device and is using these...
> > > > > > 
> > > > > > If Intel had a standalone controller or enumerated as individual
> > > > > > functions, it would have been a PCI device and would manage as such
> > > > > If it is not a standalone controller, what exactly is it?  I thought it
> > > > > was an acpi device, am I mistaken?
> > > > > 
> > > > > What is the device that the proper soundwire controller driver binds to
> > > > > on an Intel-based system?
> > > > The HDA controller which is a PCI device. The device represent HDA
> > > > function, DSP and Soundwire controller instances (yes it is typically
> > > > more than one instance)
> > > Then those "instances" should be split up into individual devices that a
> > > driver can bind to.  See the work happening on the "virtual" bus for
> > > examples of how that can be done.
> > Yes removing platform devices is the goal for Intel now :) Pierre & Bard
> > have been diligently trying to solve this.
> > 
> > Only difference is the means to end goal. I am not convinced that this
> > should be in soundwire subsystem.
> > 
> > Looks like folks are trying to review and port to use this bus. Makes
> > sense to me..
> > https://lore.kernel.org/netdev/[e-mail]/
> > 
> > > A platform device better not be being used here, I'm afraid to look at
> > > the code now...
> > Well if the plan for 'virtual-bus' goes well, it should be  a simple
> > replacement of platform->virtual for Intel driver. Rest of the driver
> > should not be impacted :)
> 
> We can't expect when will 'virtual-bus' be upstream and it's not feasible
> to wait forever. Can we move forward with current solution and switch to
> 'virtual-bus' whenever it is upstream?

the move from platform-device to virtual-device should happen once
the virtual-bus' is accepted upstream. till then imo you should continue
with current platform device and once you have virtual-bus upstream,
replace it with virtual-device. Note: I am going to hold you on that :)

Rest of the pieces like sdw_master_device and sysfs parts are not
dependent upon this and should be sent for review and we can merge when
ready, hopefully for 5.8.

Thanks

Patch

diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index e2cdff990e9f..7319918e0aec 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -4,7 +4,7 @@ 
 #
 
 #Bus Objs
-soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o
+soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o stream.o
 obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o
 
 ifdef CONFIG_DEBUG_FS
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 204204a26db8..99943d40222a 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -173,4 +173,6 @@  sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val)
 
 void sdw_clear_slave_status(struct sdw_bus *bus, u32 request);
 
+int sdw_slave_uevent(struct device *dev, struct kobj_uevent_env *env);
+
 #endif /* __SDW_BUS_H */
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 17f096dd6806..2c1a19caba51 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -33,13 +33,21 @@  sdw_get_device_id(struct sdw_slave *slave, struct sdw_driver *drv)
 
 static int sdw_bus_match(struct device *dev, struct device_driver *ddrv)
 {
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
-	struct sdw_driver *drv = drv_to_sdw_driver(ddrv);
+	struct sdw_slave *slave;
+	struct sdw_driver *drv;
+	int ret = 0;
+
+	if (is_sdw_slave(dev)) {
+		slave = dev_to_sdw_dev(dev);
+		drv = drv_to_sdw_driver(ddrv);
 
-	return !!sdw_get_device_id(slave, drv);
+		ret = !!sdw_get_device_id(slave, drv);
+	}
+	return ret;
 }
 
-int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
+static int sdw_slave_modalias(const struct sdw_slave *slave, char *buf,
+			      size_t size)
 {
 	/* modalias is sdw:m<mfg_id>p<part_id> */
 
@@ -47,7 +55,7 @@  int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
 			slave->id.mfg_id, slave->id.part_id);
 }
 
-static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
+int sdw_slave_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct sdw_slave *slave = dev_to_sdw_dev(dev);
 	char modalias[32];
@@ -63,7 +71,6 @@  static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
 struct bus_type sdw_bus_type = {
 	.name = "soundwire",
 	.match = sdw_bus_match,
-	.uevent = sdw_uevent,
 };
 EXPORT_SYMBOL_GPL(sdw_bus_type);
 
diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
new file mode 100644
index 000000000000..094518817601
--- /dev/null
+++ b/drivers/soundwire/master.c
@@ -0,0 +1,167 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright(c) 2019-2020 Intel Corporation.
+
+#include <linux/device.h>
+#include <linux/acpi.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_type.h>
+#include "bus.h"
+
+static void sdw_master_device_release(struct device *dev)
+{
+	struct sdw_master_device *md = dev_to_sdw_master_device(dev);
+
+	kfree(md);
+}
+
+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.
+		 *
+		 * 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);
+
+/**
+ * sdw_master_device_del() - delete a Linux Master Device representation.
+ * @md: the master device
+ *
+ * This function is the dual of sdw_master_device_add(), itreleases
+ * all link-specific resources and unregisters the device.
+ */
+int sdw_master_device_del(struct sdw_master_device *md)
+{
+	int ret = 0;
+
+	if (md && md->link_ops && md->link_ops->del) {
+		ret = md->link_ops->del(md);
+		if (ret < 0) {
+			dev_err(&md->dev, "link_ops del callback failed: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	device_unregister(&md->dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sdw_master_device_del);
+
+/**
+ * sdw_master_device_startup() - startup hardware
+ *
+ * @md: Linux Soundwire master device
+ */
+int sdw_master_device_startup(struct sdw_master_device *md)
+{
+	struct sdw_link_ops *link_ops;
+	int ret = 0;
+
+	if (IS_ERR_OR_NULL(md))
+		return -EINVAL;
+
+	link_ops = md->link_ops;
+
+	if (link_ops && link_ops->startup)
+		ret = link_ops->startup(md);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sdw_master_device_startup);
+
+/**
+ * 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)
+{
+	struct sdw_link_ops *link_ops;
+	int ret = 0;
+
+	if (IS_ERR_OR_NULL(md))
+		return -EINVAL;
+
+	link_ops = md->link_ops;
+
+	if (link_ops && link_ops->process_wake_event)
+		ret = link_ops->process_wake_event(md);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sdw_master_device_process_wake_event);
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index aace57fae7f8..ed068a004bd9 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -14,6 +14,12 @@  static void sdw_slave_release(struct device *dev)
 	kfree(slave);
 }
 
+struct device_type sdw_slave_type = {
+	.name =		"sdw_slave",
+	.release =	sdw_slave_release,
+	.uevent =	sdw_slave_uevent,
+};
+
 static int sdw_slave_add(struct sdw_bus *bus,
 			 struct sdw_slave_id *id, struct fwnode_handle *fwnode)
 {
@@ -41,9 +47,9 @@  static int sdw_slave_add(struct sdw_bus *bus,
 			     id->class_id, id->unique_id);
 	}
 
-	slave->dev.release = sdw_slave_release;
 	slave->dev.bus = &sdw_bus_type;
 	slave->dev.of_node = of_node_get(to_of_node(fwnode));
+	slave->dev.type = &sdw_slave_type;
 	slave->bus = bus;
 	slave->status = SDW_SLAVE_UNATTACHED;
 	init_completion(&slave->enumeration_complete);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 00f5826092e3..ecd070df4ae9 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -632,6 +632,53 @@  struct sdw_slave {
 
 #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
 
+struct sdw_link_ops;
+
+/**
+ * struct sdw_master_device - SoundWire 'Master Device' representation
+ *
+ * @dev: Linux device for this Master
+ * @bus: Bus handle
+ * @link_ops: link-specific ops, initialized with sdw_master_device_add()
+ * @link_id: link index as defined by MIPI DisCo specification
+ * @pdata: private data typically provided with sdw_master_device_add()
+ *
+ * link_ops can be NULL when link-level initializations and power-management
+ * are not desired.
+ */
+struct sdw_master_device {
+	struct device dev;
+	struct sdw_bus *bus;
+	struct sdw_link_ops *link_ops;
+	int link_id;
+	void *pdata;
+};
+
+/**
+ * struct sdw_link_ops - SoundWire link-specific ops
+ * @add: initializations and allocation (hardware may not be enabled yet)
+ * @startup: initialization handled after the hardware is enabled, all
+ * clock/power dependencies are available
+ * @del: free all remaining resources
+ * @process_wake_event: handle external wake
+ * @driver: raw structure used for name/PM hooks.
+ *
+ * This optional structure is provided for link specific
+ * operations. All members are optional, but if .add() is supported the
+ * dual .del() function shall be used to release all resources allocated
+ * in .add().
+ */
+struct sdw_link_ops {
+	int (*add)(struct sdw_master_device *md, void *link_ctx);
+	int (*startup)(struct sdw_master_device *md);
+	int (*del)(struct sdw_master_device *md);
+	int (*process_wake_event)(struct sdw_master_device *md);
+	struct device_driver *driver;
+};
+
+#define dev_to_sdw_master_device(d)	\
+	container_of(d, struct sdw_master_device, dev)
+
 struct sdw_driver {
 	const char *name;
 
@@ -835,6 +882,19 @@  struct sdw_bus {
 int sdw_add_bus_master(struct sdw_bus *bus);
 void sdw_delete_bus_master(struct sdw_bus *bus);
 
+struct sdw_master_device
+*sdw_master_device_add(struct device *parent,
+		       struct fwnode_handle *fwnode,
+		       struct sdw_link_ops *master_ops,
+		       int link_id,
+		       void *pdata);
+
+int sdw_master_device_del(struct sdw_master_device *md);
+
+int sdw_master_device_startup(struct sdw_master_device *md);
+
+int sdw_master_device_process_wake_event(struct sdw_master_device *md);
+
 /**
  * sdw_port_config: Master or Slave Port configuration
  *
diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h
index aaa7f4267c14..990857eddff8 100644
--- a/include/linux/soundwire/sdw_type.h
+++ b/include/linux/soundwire/sdw_type.h
@@ -5,6 +5,13 @@ 
 #define __SOUNDWIRE_TYPES_H
 
 extern struct bus_type sdw_bus_type;
+extern struct device_type sdw_slave_type;
+extern struct device_type sdw_master_type;
+
+static inline int is_sdw_slave(const struct device *dev)
+{
+	return dev->type == &sdw_slave_type;
+}
 
 #define drv_to_sdw_driver(_drv) container_of(_drv, struct sdw_driver, driver)
 
@@ -14,8 +21,6 @@  extern struct bus_type sdw_bus_type;
 int __sdw_register_driver(struct sdw_driver *drv, struct module *owner);
 void sdw_unregister_driver(struct sdw_driver *drv);
 
-int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size);
-
 /**
  * module_sdw_driver() - Helper macro for registering a Soundwire driver
  * @__sdw_driver: soundwire slave driver struct
@@ -27,4 +32,5 @@  int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size);
 #define module_sdw_driver(__sdw_driver) \
 	module_driver(__sdw_driver, sdw_register_driver, \
 			sdw_unregister_driver)
+
 #endif /* __SOUNDWIRE_TYPES_H */