[v4,08/15] soundwire: add initial definitions for sdw_master_device
diff mbox series

Message ID 20191213050409.12776-9-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
Since we want an explicit support for the SoundWire Master device, add
the definitions, following the Grey Bus example.

Unlike for the Slave device, we do not set a variable when dealing
with the master uevent.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/Makefile         |  2 +-
 drivers/soundwire/bus_type.c       |  7 +++-
 drivers/soundwire/master.c         | 62 ++++++++++++++++++++++++++++++
 include/linux/soundwire/sdw.h      | 35 +++++++++++++++++
 include/linux/soundwire/sdw_type.h |  9 +++++
 5 files changed, 112 insertions(+), 3 deletions(-)
 create mode 100644 drivers/soundwire/master.c

Comments

Greg KH Dec. 13, 2019, 7:28 a.m. UTC | #1
On Thu, Dec 12, 2019 at 11:04:02PM -0600, Pierre-Louis Bossart wrote:
> Since we want an explicit support for the SoundWire Master device, add
> the definitions, following the Grey Bus example.

"Greybus"  All one word please.

> 
> Unlike for the Slave device, we do not set a variable when dealing
> with the master uevent.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/Makefile         |  2 +-
>  drivers/soundwire/bus_type.c       |  7 +++-
>  drivers/soundwire/master.c         | 62 ++++++++++++++++++++++++++++++
>  include/linux/soundwire/sdw.h      | 35 +++++++++++++++++
>  include/linux/soundwire/sdw_type.h |  9 +++++
>  5 files changed, 112 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/soundwire/master.c
> 
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index 563894e5ecaf..89b29819dd3a 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_type.c b/drivers/soundwire/bus_type.c
> index 5c18c21545b5..df1271f6db61 100644
> --- a/drivers/soundwire/bus_type.c
> +++ b/drivers/soundwire/bus_type.c
> @@ -59,9 +59,12 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>  
>  		if (add_uevent_var(env, "MODALIAS=%s", modalias))
>  			return -ENOMEM;
> +	} else if (is_sdw_md(dev)) {

Ok, "is_sdw_md()" is a horrid function name.  Spell it out please, this
ends up in the global namespace.

Actually, why are you not using module namespaces here for this new
code?  That would help you out a lot.


> +		/* this should not happen but throw an error */
> +		dev_warn(dev, "uevent for Master device, unsupported\n");

Um, what?  This is supported as it will happen when you create such a
device.  It's an issue of "I didn't write the code yet", not that it is
not "supported".

> +		return -EINVAL;
>  	} else {
> -		/* only Slave device type supported */
> -		dev_warn(dev, "uevent for unknown Soundwire type\n");
> +		dev_warn(dev, "uevent for unknown device\n");
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
> new file mode 100644
> index 000000000000..6210098c892b
> --- /dev/null
> +++ b/drivers/soundwire/master.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)

Still with the crazy dual license?  I thought we went over this all
before.

You can not do this for code that touches driver core stuff, like this.
Please stop and just make all of this GPLv2 like we discussed months
ago.

> +// Copyright(c) 2019 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_md_release(struct device *dev)
> +{
> +	struct sdw_master_device *md = to_sdw_master_device(dev);
> +
> +	kfree(md);
> +}
> +
> +struct device_type sdw_md_type = {
> +	.name =		"soundwire_master",
> +	.release =	sdw_md_release,
> +};
> +
> +struct sdw_master_device *sdw_md_add(struct sdw_md_driver *driver,

Bad function names, please spell things out, you have plenty of
characters to go around.


> +				     struct device *parent,
> +				     struct fwnode_handle *fwnode,
> +				     int link_id)
> +{
> +	struct sdw_master_device *md;
> +	int ret;
> +
> +	if (!driver->probe) {
> +		dev_err(parent, "mandatory probe callback missing\n");

The callback is missing for the driver you passed in, not for the
parent, right?

> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	md = kzalloc(sizeof(*md), GFP_KERNEL);
> +	if (!md)
> +		return ERR_PTR(-ENOMEM);
> +
> +	md->link_id = link_id;
> +
> +	md->driver = driver;
> +
> +	md->dev.parent = parent;
> +	md->dev.fwnode = fwnode;
> +	md->dev.bus = &sdw_bus_type;
> +	md->dev.type = &sdw_md_type;
> +	md->dev.dma_mask = md->dev.parent->dma_mask;
> +	dev_set_name(&md->dev, "sdw-master-%d", md->link_id);
> +
> +	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);

But you still return a valid pointer?  Why????

> +	}
> +
> +	return md;
> +}
> +EXPORT_SYMBOL(sdw_md_add);

EXPORT_SYMBOL_GPL()?


> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 5b1180f1e6b5..af0a72e7afdf 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -585,6 +585,16 @@ struct sdw_slave {
>  #define to_sdw_slave_device(d) \
>  	container_of(d, struct sdw_slave, dev)
>  
> +struct sdw_master_device {
> +	struct device dev;
> +	int link_id;
> +	struct sdw_md_driver *driver;
> +	void *pdata; /* core does not touch */

Core of what?

> +};
> +
> +#define to_sdw_master_device(d)	\
> +	container_of(d, struct sdw_master_device, dev)
> +
>  struct sdw_driver {
>  	const char *name;
>  
> @@ -599,6 +609,26 @@ struct sdw_driver {
>  	struct device_driver driver;
>  };
>  
> +struct sdw_md_driver {
> +	/* initializations and allocations */
> +	int (*probe)(struct sdw_master_device *md, void *link_ctx);
> +	/* hardware enablement, all clock/power dependencies are available */
> +	int (*startup)(struct sdw_master_device *md);
> +	/* hardware disabled */
> +	int (*shutdown)(struct sdw_master_device *md);
> +	/* free all resources */
> +	int (*remove)(struct sdw_master_device *md);
> +	/*
> +	 * enable/disable driver control while in clock-stop mode,
> +	 * typically in always-on/D0ix modes. When the driver yields
> +	 * control, another entity in the system (typically firmware
> +	 * running on an always-on microprocessor) is responsible to
> +	 * tracking Slave-initiated wakes
> +	 */
> +	int (*autonomous_clock_stop_enable)(struct sdw_master_device *md,
> +					    bool state);
> +};

Use kerneldoc comments for this to make it easier to understand and for
others to read?

thanks,

greg k-h
Pierre-Louis Bossart Dec. 13, 2019, 3:49 p.m. UTC | #2
On 12/13/19 1:28 AM, Greg KH wrote:
> On Thu, Dec 12, 2019 at 11:04:02PM -0600, Pierre-Louis Bossart wrote:
>> Since we want an explicit support for the SoundWire Master device, add
>> the definitions, following the Grey Bus example.
> 
> "Greybus"  All one word please.

Ack, will fix.

>> @@ -59,9 +59,12 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
>>   
>>   		if (add_uevent_var(env, "MODALIAS=%s", modalias))
>>   			return -ENOMEM;
>> +	} else if (is_sdw_md(dev)) {
> 
> Ok, "is_sdw_md()" is a horrid function name.  Spell it out please, this
> ends up in the global namespace.

ok, will use is_sdw_master_device.

> 
> Actually, why are you not using module namespaces here for this new
> code?  That would help you out a lot.

I must admit I don't understand the question. This is literally modeled 
after is_gb_host_device(), did I miss something in the Greybus 
implementation?

> 
>> +		/* this should not happen but throw an error */
>> +		dev_warn(dev, "uevent for Master device, unsupported\n");
> 
> Um, what?  This is supported as it will happen when you create such a
> device.  It's an issue of "I didn't write the code yet", not that it is
> not "supported".

I will remove, it cannot happen.

>> diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
>> new file mode 100644
>> index 000000000000..6210098c892b
>> --- /dev/null
>> +++ b/drivers/soundwire/master.c
>> @@ -0,0 +1,62 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> 
> Still with the crazy dual license?  I thought we went over this all
> before.
> 
> You can not do this for code that touches driver core stuff, like this.
> Please stop and just make all of this GPLv2 like we discussed months
> ago.

I don't recall this was the guidance but fine.

> 
>> +// Copyright(c) 2019 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_md_release(struct device *dev)
>> +{
>> +	struct sdw_master_device *md = to_sdw_master_device(dev);
>> +
>> +	kfree(md);
>> +}
>> +
>> +struct device_type sdw_md_type = {
>> +	.name =		"soundwire_master",
>> +	.release =	sdw_md_release,
>> +};
>> +
>> +struct sdw_master_device *sdw_md_add(struct sdw_md_driver *driver,
> 
> Bad function names, please spell things out, you have plenty of
> characters to go around.

This was modeled after gb_hd_create ;-)

sdw_master_device_add starts to be on the long side, no?


>> +				     struct device *parent,
>> +				     struct fwnode_handle *fwnode,
>> +				     int link_id)
>> +{
>> +	struct sdw_master_device *md;
>> +	int ret;
>> +
>> +	if (!driver->probe) {
>> +		dev_err(parent, "mandatory probe callback missing\n");
> 
> The callback is missing for the driver you passed in, not for the
> parent, right?

yes, this function is called as part of the parent probe.

>> +	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);
> 
> But you still return a valid pointer?  Why????

Ah, yes, this is clearly wrong, thanks for pointing this out.

What's the recommended error code for this? Greybus uses:

return ERR_PTR(-ENOMEM);

>> +EXPORT_SYMBOL(sdw_md_add);
> 
> EXPORT_SYMBOL_GPL()?

yes, will fix

> 
> 
>> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
>> index 5b1180f1e6b5..af0a72e7afdf 100644
>> --- a/include/linux/soundwire/sdw.h
>> +++ b/include/linux/soundwire/sdw.h
>> @@ -585,6 +585,16 @@ struct sdw_slave {
>>   #define to_sdw_slave_device(d) \
>>   	container_of(d, struct sdw_slave, dev)
>>   
>> +struct sdw_master_device {
>> +	struct device dev;
>> +	int link_id;
>> +	struct sdw_md_driver *driver;
>> +	void *pdata; /* core does not touch */
> 
> Core of what?

SoundWire bus driver. This is a copy/paste from the SOF code I am 
afraid, will fix.

> 
>> +};
>> +
>> +#define to_sdw_master_device(d)	\
>> +	container_of(d, struct sdw_master_device, dev)
>> +
>>   struct sdw_driver {
>>   	const char *name;
>>   
>> @@ -599,6 +609,26 @@ struct sdw_driver {
>>   	struct device_driver driver;
>>   };
>>   
>> +struct sdw_md_driver {
>> +	/* initializations and allocations */
>> +	int (*probe)(struct sdw_master_device *md, void *link_ctx);
>> +	/* hardware enablement, all clock/power dependencies are available */
>> +	int (*startup)(struct sdw_master_device *md);
>> +	/* hardware disabled */
>> +	int (*shutdown)(struct sdw_master_device *md);
>> +	/* free all resources */
>> +	int (*remove)(struct sdw_master_device *md);
>> +	/*
>> +	 * enable/disable driver control while in clock-stop mode,
>> +	 * typically in always-on/D0ix modes. When the driver yields
>> +	 * control, another entity in the system (typically firmware
>> +	 * running on an always-on microprocessor) is responsible to
>> +	 * tracking Slave-initiated wakes
>> +	 */
>> +	int (*autonomous_clock_stop_enable)(struct sdw_master_device *md,
>> +					    bool state);
>> +};
> 
> Use kerneldoc comments for this to make it easier to understand and for
> others to read?

yes, I used kerneldoc everywhere except here, will fix.
Greg KH Dec. 13, 2019, 4:10 p.m. UTC | #3
On Fri, Dec 13, 2019 at 09:49:57AM -0600, Pierre-Louis Bossart wrote:
> On 12/13/19 1:28 AM, Greg KH wrote:
> > On Thu, Dec 12, 2019 at 11:04:02PM -0600, Pierre-Louis Bossart wrote:
> > > Since we want an explicit support for the SoundWire Master device, add
> > > the definitions, following the Grey Bus example.
> > 
> > "Greybus"  All one word please.
> 
> Ack, will fix.
> 
> > > @@ -59,9 +59,12 @@ int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
> > >   		if (add_uevent_var(env, "MODALIAS=%s", modalias))
> > >   			return -ENOMEM;
> > > +	} else if (is_sdw_md(dev)) {
> > 
> > Ok, "is_sdw_md()" is a horrid function name.  Spell it out please, this
> > ends up in the global namespace.
> 
> ok, will use is_sdw_master_device.
> 
> > 
> > Actually, why are you not using module namespaces here for this new
> > code?  That would help you out a lot.
> 
> I must admit I don't understand the question. This is literally modeled
> after is_gb_host_device(), did I miss something in the Greybus
> implementation?

No, I mean the new MODULE_NAMESPACE() support that is in the kernel.
I'll move the greybus code to use it too, but when you are adding new
apis, it just makes sense to use it then as well.

thanks,

greg k-h
Pierre-Louis Bossart Dec. 13, 2019, 10:15 p.m. UTC | #4
>>> Actually, why are you not using module namespaces here for this new
>>> code?  That would help you out a lot.
>>
>> I must admit I don't understand the question. This is literally modeled
>> after is_gb_host_device(), did I miss something in the Greybus
>> implementation?
> 
> No, I mean the new MODULE_NAMESPACE() support that is in the kernel.
> I'll move the greybus code to use it too, but when you are adding new
> apis, it just makes sense to use it then as well.

ok, thanks for the pointer, will look into this.
Pierre-Louis Bossart Dec. 13, 2019, 11:25 p.m. UTC | #5
> No, I mean the new MODULE_NAMESPACE() support that is in the kernel.
> I'll move the greybus code to use it too, but when you are adding new
> apis, it just makes sense to use it then as well.

Greg, would the patch below be what you had in mind?
Thanks
-Pierre


diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index 76a5c52b12b4..5bad8422887e 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -7,9 +7,11 @@ ccflags-y += -DDEBUG
  #Bus Objs
  soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o 
stream.o
  obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o
+ccflags-$(CONFIG_SOUNDWIRE) += -DDEFAULT_SYMBOL_NAMESPACE=SDW_CORE

  soundwire-generic-allocation-objs := generic_bandwidth_allocation.o
  obj-$(CONFIG_SOUNDWIRE_GENERIC_ALLOCATION) += 
soundwire-generic-allocation.o
+ccflags-$(CONFIG_SOUNDWIRE_GENERIC_ALLOCATION) += 
-DDEFAULT_SYMBOL_NAMESPACE=SDW_CORE

  ifdef CONFIG_DEBUG_FS
  soundwire-bus-objs += debugfs.o
@@ -18,6 +20,7 @@ endif
  #Cadence Objs
  soundwire-cadence-objs := cadence_master.o
  obj-$(CONFIG_SOUNDWIRE_CADENCE) += soundwire-cadence.o
+ccflags-$(CONFIG_SOUNDWIRE_CADENCE) += 
-DDEFAULT_SYMBOL_NAMESPACE=SDW_CADENCE

  #Intel driver
  soundwire-intel-objs :=        intel.o
@@ -25,3 +28,4 @@ obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel.o

  soundwire-intel-init-objs := intel_init.o
  obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel-init.o
+ccflags-$(CONFIG_SOUNDWIRE_INTEL) += -DDEFAULT_SYMBOL_NAMESPACE=SDW_INTEL
diff --git a/drivers/soundwire/cadence_master.c 
b/drivers/soundwire/cadence_master.c
index cf96532b0a8e..89ed97e303b9 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -1517,3 +1517,4 @@ EXPORT_SYMBOL(sdw_cdns_alloc_pdi);

  MODULE_LICENSE("Dual BSD/GPL");
  MODULE_DESCRIPTION("Cadence Soundwire Library");
+MODULE_IMPORT_NS(SDW_CORE);
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 0b510e198928..2be9c365d342 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1823,3 +1823,5 @@ EXPORT_SYMBOL(intel_sdw_driver);
  MODULE_LICENSE("Dual BSD/GPL");
  MODULE_ALIAS("platform:int-sdw");
  MODULE_DESCRIPTION("Intel Soundwire Master Driver");
+MODULE_IMPORT_NS(SDW_CADENCE);
+MODULE_IMPORT_NS(SDW_CORE);
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 834a09bafdcc..0685be32012d 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -432,3 +432,5 @@ EXPORT_SYMBOL(sdw_intel_exit);

  MODULE_LICENSE("Dual BSD/GPL");
  MODULE_DESCRIPTION("Intel Soundwire Init Library");
+MODULE_IMPORT_NS(SDW_CADENCE);
+MODULE_IMPORT_NS(SDW_CORE);
Greg KH Dec. 14, 2019, 8:27 a.m. UTC | #6
On Fri, Dec 13, 2019 at 05:25:23PM -0600, Pierre-Louis Bossart wrote:
> 
> > No, I mean the new MODULE_NAMESPACE() support that is in the kernel.
> > I'll move the greybus code to use it too, but when you are adding new
> > apis, it just makes sense to use it then as well.
> 
> Greg, would the patch below be what you had in mind?
> Thanks
> -Pierre
> 
> 
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index 76a5c52b12b4..5bad8422887e 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -7,9 +7,11 @@ ccflags-y += -DDEBUG
>  #Bus Objs
>  soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o
> stream.o
>  obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o
> +ccflags-$(CONFIG_SOUNDWIRE) += -DDEFAULT_SYMBOL_NAMESPACE=SDW_CORE
> 
>  soundwire-generic-allocation-objs := generic_bandwidth_allocation.o
>  obj-$(CONFIG_SOUNDWIRE_GENERIC_ALLOCATION) +=
> soundwire-generic-allocation.o
> +ccflags-$(CONFIG_SOUNDWIRE_GENERIC_ALLOCATION) +=
> -DDEFAULT_SYMBOL_NAMESPACE=SDW_CORE

Don't use ccflags, just use the correct MODULE_EXPORT_NS() tag instead.

And "SDW_CORE" is odd, "SOUNDWIRE" instead?

thanks,

greg k-h
Pierre-Louis Bossart Dec. 16, 2019, 3:02 p.m. UTC | #7
On 12/14/19 2:27 AM, Greg KH wrote:
> On Fri, Dec 13, 2019 at 05:25:23PM -0600, Pierre-Louis Bossart wrote:
>>
>>> No, I mean the new MODULE_NAMESPACE() support that is in the kernel.
>>> I'll move the greybus code to use it too, but when you are adding new
>>> apis, it just makes sense to use it then as well.
>>
>> Greg, would the patch below be what you had in mind?
>> Thanks
>> -Pierre
>>
>>
>> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
>> index 76a5c52b12b4..5bad8422887e 100644
>> --- a/drivers/soundwire/Makefile
>> +++ b/drivers/soundwire/Makefile
>> @@ -7,9 +7,11 @@ ccflags-y += -DDEBUG
>>   #Bus Objs
>>   soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o
>> stream.o
>>   obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o
>> +ccflags-$(CONFIG_SOUNDWIRE) += -DDEFAULT_SYMBOL_NAMESPACE=SDW_CORE
>>
>>   soundwire-generic-allocation-objs := generic_bandwidth_allocation.o
>>   obj-$(CONFIG_SOUNDWIRE_GENERIC_ALLOCATION) +=
>> soundwire-generic-allocation.o
>> +ccflags-$(CONFIG_SOUNDWIRE_GENERIC_ALLOCATION) +=
>> -DDEFAULT_SYMBOL_NAMESPACE=SDW_CORE
> 
> Don't use ccflags, just use the correct MODULE_EXPORT_NS() tag instead.

The documentation [1] states

"
Defining namespaces for all symbols of a subsystem can be very verbose 
and may become hard to maintain. Therefore a default define 
(DEFAULT_SYMBOL_NAMESPACE) is been provided, that, if set, will become 
the default for all EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL() macro 
expansions that do not specify a namespace.
"

If the ccflags option is not supported or no longer desired, it'd be 
worth updating the documentation for dummies like me. I took the wording 
as a hint to avoid using MODULE_EXPORT_NS.

> And "SDW_CORE" is odd, "SOUNDWIRE" instead?

'sdw' is the prefix used everywhere for SoundWire symbols.

[1] https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
Greg KH Dec. 16, 2019, 4:25 p.m. UTC | #8
On Mon, Dec 16, 2019 at 09:02:01AM -0600, Pierre-Louis Bossart wrote:
> 
> 
> On 12/14/19 2:27 AM, Greg KH wrote:
> > On Fri, Dec 13, 2019 at 05:25:23PM -0600, Pierre-Louis Bossart wrote:
> > > 
> > > > No, I mean the new MODULE_NAMESPACE() support that is in the kernel.
> > > > I'll move the greybus code to use it too, but when you are adding new
> > > > apis, it just makes sense to use it then as well.
> > > 
> > > Greg, would the patch below be what you had in mind?
> > > Thanks
> > > -Pierre
> > > 
> > > 
> > > diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> > > index 76a5c52b12b4..5bad8422887e 100644
> > > --- a/drivers/soundwire/Makefile
> > > +++ b/drivers/soundwire/Makefile
> > > @@ -7,9 +7,11 @@ ccflags-y += -DDEBUG
> > >   #Bus Objs
> > >   soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o
> > > stream.o
> > >   obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o
> > > +ccflags-$(CONFIG_SOUNDWIRE) += -DDEFAULT_SYMBOL_NAMESPACE=SDW_CORE
> > > 
> > >   soundwire-generic-allocation-objs := generic_bandwidth_allocation.o
> > >   obj-$(CONFIG_SOUNDWIRE_GENERIC_ALLOCATION) +=
> > > soundwire-generic-allocation.o
> > > +ccflags-$(CONFIG_SOUNDWIRE_GENERIC_ALLOCATION) +=
> > > -DDEFAULT_SYMBOL_NAMESPACE=SDW_CORE
> > 
> > Don't use ccflags, just use the correct MODULE_EXPORT_NS() tag instead.
> 
> The documentation [1] states
> 
> "
> Defining namespaces for all symbols of a subsystem can be very verbose and
> may become hard to maintain. Therefore a default define
> (DEFAULT_SYMBOL_NAMESPACE) is been provided, that, if set, will become the
> default for all EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL() macro expansions
> that do not specify a namespace.
> "
> 
> If the ccflags option is not supported or no longer desired, it'd be worth
> updating the documentation for dummies like me. I took the wording as a hint
> to avoid using MODULE_EXPORT_NS.

It's supported, and works just fine.  It's just that you really don't
have a ton of exports, right?  What's wrong with manually marking them?

> > And "SDW_CORE" is odd, "SOUNDWIRE" instead?
> 
> 'sdw' is the prefix used everywhere for SoundWire symbols.

Ok, I guess that ship has sailed :(

greg k-h
Pierre-Louis Bossart Dec. 16, 2019, 5:07 p.m. UTC | #9
>>>> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
>>>> index 76a5c52b12b4..5bad8422887e 100644
>>>> --- a/drivers/soundwire/Makefile
>>>> +++ b/drivers/soundwire/Makefile
>>>> @@ -7,9 +7,11 @@ ccflags-y += -DDEBUG
>>>>    #Bus Objs
>>>>    soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o
>>>> stream.o
>>>>    obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o
>>>> +ccflags-$(CONFIG_SOUNDWIRE) += -DDEFAULT_SYMBOL_NAMESPACE=SDW_CORE
>>>>
>>>>    soundwire-generic-allocation-objs := generic_bandwidth_allocation.o
>>>>    obj-$(CONFIG_SOUNDWIRE_GENERIC_ALLOCATION) +=
>>>> soundwire-generic-allocation.o
>>>> +ccflags-$(CONFIG_SOUNDWIRE_GENERIC_ALLOCATION) +=
>>>> -DDEFAULT_SYMBOL_NAMESPACE=SDW_CORE
>>>
>>> Don't use ccflags, just use the correct MODULE_EXPORT_NS() tag instead.
>>
>> The documentation [1] states
>>
>> "
>> Defining namespaces for all symbols of a subsystem can be very verbose and
>> may become hard to maintain. Therefore a default define
>> (DEFAULT_SYMBOL_NAMESPACE) is been provided, that, if set, will become the
>> default for all EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL() macro expansions
>> that do not specify a namespace.
>> "
>>
>> If the ccflags option is not supported or no longer desired, it'd be worth
>> updating the documentation for dummies like me. I took the wording as a hint
>> to avoid using MODULE_EXPORT_NS.
> 
> It's supported, and works just fine.  It's just that you really don't
> have a ton of exports, right?  What's wrong with manually marking them?

I don't see a MODULE_EXPORT_NS so we'd have to change every single 
EXPORT_SYMBOL to EXPORT_SYMBOL_NS.

If we are talking about adding the namespaces just about the top-level 
functions used by Intel, yes we have less than 10 and since they were 
renamed it's no big deal.

But if we want to use this namespace for lower-level components of the 
SoundWire code, we have 17 exports in cadence_master.c and more than 27 
for the core parts. With the Makefile changes shared last week you'd 
have 3 changes, I find it more manageable but it's true that the 
information would be split with the IMPORT_NS in the code and the 
namespace definition in the Makefile.


>>> And "SDW_CORE" is odd, "SOUNDWIRE" instead?
>>
>> 'sdw' is the prefix used everywhere for SoundWire symbols.
> 
> Ok, I guess that ship has sailed :(

we can still use SOUNDWIRE for the namespaces, that'd be fine. you're 
right to call us on acronyms, it's a bad habit.
Pierre-Louis Bossart Dec. 16, 2019, 10:34 p.m. UTC | #10
>> No, I mean the new MODULE_NAMESPACE() support that is in the kernel.
>> I'll move the greybus code to use it too, but when you are adding new
>> apis, it just makes sense to use it then as well.
> 
> ok, thanks for the pointer, will look into this.

This namespace check already saved me a lot of time. I found an issue in 
our latest code (not submitted for review yet) with a new functionality 
added to the wrong module and without the proper abstraction required...
Really nice tool, I can already see how useful it will be for on-going 
code partitioning (SOF multi-client work and virtualization support).

Patch
diff mbox series

diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index 563894e5ecaf..89b29819dd3a 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_type.c b/drivers/soundwire/bus_type.c
index 5c18c21545b5..df1271f6db61 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -59,9 +59,12 @@  int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
 
 		if (add_uevent_var(env, "MODALIAS=%s", modalias))
 			return -ENOMEM;
+	} else if (is_sdw_md(dev)) {
+		/* this should not happen but throw an error */
+		dev_warn(dev, "uevent for Master device, unsupported\n");
+		return -EINVAL;
 	} else {
-		/* only Slave device type supported */
-		dev_warn(dev, "uevent for unknown Soundwire type\n");
+		dev_warn(dev, "uevent for unknown device\n");
 		return -EINVAL;
 	}
 
diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c
new file mode 100644
index 000000000000..6210098c892b
--- /dev/null
+++ b/drivers/soundwire/master.c
@@ -0,0 +1,62 @@ 
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2019 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_md_release(struct device *dev)
+{
+	struct sdw_master_device *md = to_sdw_master_device(dev);
+
+	kfree(md);
+}
+
+struct device_type sdw_md_type = {
+	.name =		"soundwire_master",
+	.release =	sdw_md_release,
+};
+
+struct sdw_master_device *sdw_md_add(struct sdw_md_driver *driver,
+				     struct device *parent,
+				     struct fwnode_handle *fwnode,
+				     int link_id)
+{
+	struct sdw_master_device *md;
+	int ret;
+
+	if (!driver->probe) {
+		dev_err(parent, "mandatory probe callback missing\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	md = kzalloc(sizeof(*md), GFP_KERNEL);
+	if (!md)
+		return ERR_PTR(-ENOMEM);
+
+	md->link_id = link_id;
+
+	md->driver = driver;
+
+	md->dev.parent = parent;
+	md->dev.fwnode = fwnode;
+	md->dev.bus = &sdw_bus_type;
+	md->dev.type = &sdw_md_type;
+	md->dev.dma_mask = md->dev.parent->dma_mask;
+	dev_set_name(&md->dev, "sdw-master-%d", md->link_id);
+
+	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 md;
+}
+EXPORT_SYMBOL(sdw_md_add);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 5b1180f1e6b5..af0a72e7afdf 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -585,6 +585,16 @@  struct sdw_slave {
 #define to_sdw_slave_device(d) \
 	container_of(d, struct sdw_slave, dev)
 
+struct sdw_master_device {
+	struct device dev;
+	int link_id;
+	struct sdw_md_driver *driver;
+	void *pdata; /* core does not touch */
+};
+
+#define to_sdw_master_device(d)	\
+	container_of(d, struct sdw_master_device, dev)
+
 struct sdw_driver {
 	const char *name;
 
@@ -599,6 +609,26 @@  struct sdw_driver {
 	struct device_driver driver;
 };
 
+struct sdw_md_driver {
+	/* initializations and allocations */
+	int (*probe)(struct sdw_master_device *md, void *link_ctx);
+	/* hardware enablement, all clock/power dependencies are available */
+	int (*startup)(struct sdw_master_device *md);
+	/* hardware disabled */
+	int (*shutdown)(struct sdw_master_device *md);
+	/* free all resources */
+	int (*remove)(struct sdw_master_device *md);
+	/*
+	 * enable/disable driver control while in clock-stop mode,
+	 * typically in always-on/D0ix modes. When the driver yields
+	 * control, another entity in the system (typically firmware
+	 * running on an always-on microprocessor) is responsible to
+	 * tracking Slave-initiated wakes
+	 */
+	int (*autonomous_clock_stop_enable)(struct sdw_master_device *md,
+					    bool state);
+};
+
 #define SDW_SLAVE_ENTRY(_mfg_id, _part_id, _drv_data) \
 	{ .mfg_id = (_mfg_id), .part_id = (_part_id), \
 	  .driver_data = (unsigned long)(_drv_data) }
@@ -788,6 +818,11 @@  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_md_add(struct sdw_md_driver *driver,
+				     struct device *parent,
+				     struct fwnode_handle *fwnode,
+				     int link_id);
+
 /**
  * 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 c681b3426478..463d6d018d56 100644
--- a/include/linux/soundwire/sdw_type.h
+++ b/include/linux/soundwire/sdw_type.h
@@ -6,15 +6,24 @@ 
 
 extern struct bus_type sdw_bus_type;
 extern struct device_type sdw_slave_type;
+extern struct device_type sdw_md_type;
 
 static inline int is_sdw_slave(const struct device *dev)
 {
 	return dev->type == &sdw_slave_type;
 }
 
+static inline int is_sdw_md(const struct device *dev)
+{
+	return dev->type == &sdw_md_type;
+}
+
 #define to_sdw_slave_driver(_drv) \
 	container_of(_drv, struct sdw_driver, driver)
 
+#define to_sdw_md_driver(_drv) \
+	container_of(_drv, struct sdw_md_driver, driver)
+
 #define sdw_register_slave_driver(drv) \
 	__sdw_register_slave_driver(drv, THIS_MODULE)