| Message ID | 20191213050409.12776-9-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: intel: implement new ASoC interfaces | expand |
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
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.
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
>>> 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.
> 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);
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
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
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
>>>> 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.
>> 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).
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)
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