[v4,06/15] soundwire: add support for sdw_slave_type
diff mbox series

Message ID 20191213050409.12776-7-pierre-louis.bossart@linux.intel.com
State New
Headers show
Series
  • soundwire: intel: implement new ASoC interfaces
Related show

Commit Message

Pierre-Louis Bossart Dec. 13, 2019, 5:04 a.m. UTC
Currently the bus does not have any explicit support for master
devices.

First add explicit support for sdw_slave_type and error checks if this type
is not set.

In follow-up patches we can add support for the sdw_md_type (md==Master
Device), following the Grey Bus example.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus_type.c       | 16 ++++++++++++----
 drivers/soundwire/slave.c          |  7 ++++++-
 include/linux/soundwire/sdw_type.h |  6 ++++++
 3 files changed, 24 insertions(+), 5 deletions(-)

Comments

Greg KH Dec. 13, 2019, 7:21 a.m. UTC | #1
On Thu, Dec 12, 2019 at 11:04:00PM -0600, Pierre-Louis Bossart wrote:
> Currently the bus does not have any explicit support for master
> devices.
> 
> First add explicit support for sdw_slave_type and error checks if this type
> is not set.
> 
> In follow-up patches we can add support for the sdw_md_type (md==Master
> Device), following the Grey Bus example.

How are you using greybus as an example of "master devices"?  All you
are doing here is setting the type of the existing devices, right?


> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/bus_type.c       | 16 ++++++++++++----
>  drivers/soundwire/slave.c          |  7 ++++++-
>  include/linux/soundwire/sdw_type.h |  6 ++++++
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> index 9a0fd3ee1014..bbdedce5eb26 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -49,13 +49,21 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
>  
>  static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
> -	struct sdw_slave *slave = to_sdw_slave_device(dev);
> +	struct sdw_slave *slave;
>  	char modalias[32];
>  
> -	sdw_slave_modalias(slave, modalias, sizeof(modalias));
> +	if (is_sdw_slave(dev)) {
> +		slave = to_sdw_slave_device(dev);
> +
> +		sdw_slave_modalias(slave, modalias, sizeof(modalias));
>  
> -	if (add_uevent_var(env, "MODALIAS=%s", modalias))
> -		return -ENOMEM;
> +		if (add_uevent_var(env, "MODALIAS=%s", modalias))
> +			return -ENOMEM;
> +	} else {
> +		/* only Slave device type supported */
> +		dev_warn(dev, "uevent for unknown Soundwire type\n");
> +		return -EINVAL;

Right now, this can not happen, right?

Not a problem, just trying to understand the sequence of patches here...

thanks,

greg k-h
Pierre-Louis Bossart Dec. 13, 2019, 3:05 p.m. UTC | #2
On 12/13/19 1:21 AM, Greg KH wrote:
> On Thu, Dec 12, 2019 at 11:04:00PM -0600, Pierre-Louis Bossart wrote:
>> Currently the bus does not have any explicit support for master
>> devices.
>>
>> First add explicit support for sdw_slave_type and error checks if this type
>> is not set.
>>
>> In follow-up patches we can add support for the sdw_md_type (md==Master
>> Device), following the Grey Bus example.
> 
> How are you using greybus as an example of "master devices"?  All you
> are doing here is setting the type of the existing devices, right?

I took your advice to look at GreyBus and used the 'gb host device' as 
the model to implement the 'sdw master' add/startup/remove interfaces we 
needed.

so yes in this patch we just add a type for the slave, the interesting 
part is in the next patches.
>>   static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>>   {
>> -	struct sdw_slave *slave = to_sdw_slave_device(dev);
>> +	struct sdw_slave *slave;
>>   	char modalias[32];
>>   
>> -	sdw_slave_modalias(slave, modalias, sizeof(modalias));
>> +	if (is_sdw_slave(dev)) {
>> +		slave = to_sdw_slave_device(dev);
>> +
>> +		sdw_slave_modalias(slave, modalias, sizeof(modalias));
>>   
>> -	if (add_uevent_var(env, "MODALIAS=%s", modalias))
>> -		return -ENOMEM;
>> +		if (add_uevent_var(env, "MODALIAS=%s", modalias))
>> +			return -ENOMEM;
>> +	} else {
>> +		/* only Slave device type supported */
>> +		dev_warn(dev, "uevent for unknown Soundwire type\n");
>> +		return -EINVAL;
> 
> Right now, this can not happen, right?
> 
> Not a problem, just trying to understand the sequence of patches here...

yes this cannot happen at this point, it's more of a paranoid test. In 
theory a SoundWire solution could enable a 'monitor' device as defined 
in the standard.
Greg KH Dec. 13, 2019, 4:12 p.m. UTC | #3
On Fri, Dec 13, 2019 at 09:05:37AM -0600, Pierre-Louis Bossart wrote:
> On 12/13/19 1:21 AM, Greg KH wrote:
> > On Thu, Dec 12, 2019 at 11:04:00PM -0600, Pierre-Louis Bossart wrote:
> > > Currently the bus does not have any explicit support for master
> > > devices.
> > > 
> > > First add explicit support for sdw_slave_type and error checks if this type
> > > is not set.
> > > 
> > > In follow-up patches we can add support for the sdw_md_type (md==Master
> > > Device), following the Grey Bus example.
> > 
> > How are you using greybus as an example of "master devices"?  All you
> > are doing here is setting the type of the existing devices, right?
> 
> I took your advice to look at GreyBus and used the 'gb host device' as the
> model to implement the 'sdw master' add/startup/remove interfaces we needed.
> 
> so yes in this patch we just add a type for the slave, the interesting part
> is in the next patches.

Is that what a "master" device really is?  A host controller, like a USB
host controller?  Or something else?

I thought things were a bit more complex for this type of topology.

greg k-h
Pierre-Louis Bossart Dec. 13, 2019, 10:14 p.m. UTC | #4
On 12/13/19 10:12 AM, Greg KH wrote:
> On Fri, Dec 13, 2019 at 09:05:37AM -0600, Pierre-Louis Bossart wrote:
>> On 12/13/19 1:21 AM, Greg KH wrote:
>>> On Thu, Dec 12, 2019 at 11:04:00PM -0600, Pierre-Louis Bossart wrote:
>>>> Currently the bus does not have any explicit support for master
>>>> devices.
>>>>
>>>> First add explicit support for sdw_slave_type and error checks if this type
>>>> is not set.
>>>>
>>>> In follow-up patches we can add support for the sdw_md_type (md==Master
>>>> Device), following the Grey Bus example.
>>>
>>> How are you using greybus as an example of "master devices"?  All you
>>> are doing here is setting the type of the existing devices, right?
>>
>> I took your advice to look at GreyBus and used the 'gb host device' as the
>> model to implement the 'sdw master' add/startup/remove interfaces we needed.
>>
>> so yes in this patch we just add a type for the slave, the interesting part
>> is in the next patches.
> 
> Is that what a "master" device really is?  A host controller, like a USB
> host controller?  Or something else?
> 
> I thought things were a bit more complex for this type of topology.

The "Master Device" is similar to a USB host controller, but with a much 
lower complexity. It can also be viewed as similar to an 
HDaudio/AC97/SLIMbus  controller which handles a serial link with 
interleaved command/data, but with lower latency to e.g. support 1-bit 
oversampled PDM data typically used by digital microphones (or amplifiers).

The Master device provides the clock for the bus, handles clock 
stop/restart sequences in and out of idle state, and it issues commands 
which contain a sync pattern. The Master device will also typically have 
audio 'ports'.

The 'Slave Devices' are similar to USB/SLIMbus devices, they look for a 
sync pattern and when synchronized will respond to status/write/read 
commands. They cannot send commands on their own but can signal in-band 
interrupts. The bus is multi-drop and typically single-level (no 
hubs/bridges so far).

Unfortunately there is no host controller interface so we need a 
vendor-specific driver for each Master device implementation. The Master 
IP is typically part of the audio controller, so in the Intel 
implementation it's represented as an ACPI-enumerated child device of 
the PCI audio controller.

The patches in this series provide a means for the SOF/HDaudio driver to 
check the ACPI DSDT tables and detect if SoundWire links are enabled, 
allocate all necessary resources and start the hardware operation once 
all the power rail dependencies are handled.

Here are a couple of publicly-available pointers:

https://mipi.org/sites/default/files/Audio_Spec_Brief_20141007.pdf
https://mipi.org/sites/default/files/MIPI-SoundWire-webinar-20150121-final.pdf

Patch
diff mbox series

diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 9a0fd3ee1014..bbdedce5eb26 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -49,13 +49,21 @@  int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size)
 
 static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	struct sdw_slave *slave = to_sdw_slave_device(dev);
+	struct sdw_slave *slave;
 	char modalias[32];
 
-	sdw_slave_modalias(slave, modalias, sizeof(modalias));
+	if (is_sdw_slave(dev)) {
+		slave = to_sdw_slave_device(dev);
+
+		sdw_slave_modalias(slave, modalias, sizeof(modalias));
 
-	if (add_uevent_var(env, "MODALIAS=%s", modalias))
-		return -ENOMEM;
+		if (add_uevent_var(env, "MODALIAS=%s", modalias))
+			return -ENOMEM;
+	} else {
+		/* only Slave device type supported */
+		dev_warn(dev, "uevent for unknown Soundwire type\n");
+		return -EINVAL;
+	}
 
 	return 0;
 }
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 48a513680db6..c87267f12a3b 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -14,6 +14,11 @@  static void sdw_slave_release(struct device *dev)
 	kfree(slave);
 }
 
+struct device_type sdw_slave_type = {
+	.name =		"sdw_slave",
+	.release =	sdw_slave_release,
+};
+
 static int sdw_slave_add(struct sdw_bus *bus,
 			 struct sdw_slave_id *id, struct fwnode_handle *fwnode)
 {
@@ -41,9 +46,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;
 	slave->dev_num = 0;
diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h
index 7d4bc6a979bf..c681b3426478 100644
--- a/include/linux/soundwire/sdw_type.h
+++ b/include/linux/soundwire/sdw_type.h
@@ -5,6 +5,12 @@ 
 #define __SOUNDWIRE_TYPES_H
 
 extern struct bus_type sdw_bus_type;
+extern struct device_type sdw_slave_type;
+
+static inline int is_sdw_slave(const struct device *dev)
+{
+	return dev->type == &sdw_slave_type;
+}
 
 #define to_sdw_slave_driver(_drv) \
 	container_of(_drv, struct sdw_driver, driver)