Message ID | 20220906195735.87361-2-andriy.shevchenko@linux.intel.com |
---|---|
State | Superseded |
Headers | show |
Series | [v1,1/9] pwm: lpss: Deduplicate board info data structures | expand |
Hello Andy, On Tue, Sep 06, 2022 at 10:57:28PM +0300, Andy Shevchenko wrote: > Avoid unnecessary pollution of the global symbol namespace by > moving library functions in to a specific namespace and import > that into the drivers that make use of the functions. > > For more info: https://lwn.net/Articles/760045/ > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pwm/pwm-lpss-pci.c | 1 + > drivers/pwm/pwm-lpss-platform.c | 1 + > drivers/pwm/pwm-lpss.c | 2 +- > 3 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c > index 75b778e839b3..9f2c666b95ec 100644 > --- a/drivers/pwm/pwm-lpss-pci.c > +++ b/drivers/pwm/pwm-lpss-pci.c > @@ -92,3 +92,4 @@ module_pci_driver(pwm_lpss_driver_pci); > > MODULE_DESCRIPTION("PWM PCI driver for Intel LPSS"); > MODULE_LICENSE("GPL v2"); > +MODULE_IMPORT_NS(PWM_LPSS); > diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c > index fcd80cca2f6d..96fde1b2b967 100644 > --- a/drivers/pwm/pwm-lpss-platform.c > +++ b/drivers/pwm/pwm-lpss-platform.c > @@ -87,4 +87,5 @@ module_platform_driver(pwm_lpss_driver_platform); > > MODULE_DESCRIPTION("PWM platform driver for Intel LPSS"); > MODULE_LICENSE("GPL v2"); > +MODULE_IMPORT_NS(PWM_LPSS); > MODULE_ALIAS("platform:pwm-lpss"); While it's not wrong to add the IMPORT_NS statement to each file, I'd had added it to pwm-lpss.h. IMHO that makes sense as every includer of that header needs that IMPORT_NS to actually use the symbols declared there. > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c > index 36d4e83e6b79..a82a57eb2482 100644 > --- a/drivers/pwm/pwm-lpss.c > +++ b/drivers/pwm/pwm-lpss.c > @@ -250,7 +250,7 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r, > > return lpwm; > } > -EXPORT_SYMBOL_GPL(pwm_lpss_probe); > +EXPORT_SYMBOL_NS_GPL(pwm_lpss_probe, PWM_LPSS); There is something possible with more magic: #define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS which you only need once in pwm-lpss.c and then all exports use that namespace. (And if you pick up my suggestion for patch 1 you also benefit from that.) Best regards Uwe
On Wed, Sep 07, 2022 at 11:11:44AM +0200, Uwe Kleine-König wrote: > On Tue, Sep 06, 2022 at 10:57:28PM +0300, Andy Shevchenko wrote: > > MODULE_DESCRIPTION("PWM platform driver for Intel LPSS"); > > MODULE_LICENSE("GPL v2"); > > +MODULE_IMPORT_NS(PWM_LPSS); > > MODULE_ALIAS("platform:pwm-lpss"); > > While it's not wrong to add the IMPORT_NS statement to each file, I'd > had added it to pwm-lpss.h. IMHO that makes sense as every includer of > that header needs that IMPORT_NS to actually use the symbols declared > there. If you have an optional dependency you may not need to include namespace to avoid dragging it for peanuts. ... > > -EXPORT_SYMBOL_GPL(pwm_lpss_probe); > > +EXPORT_SYMBOL_NS_GPL(pwm_lpss_probe, PWM_LPSS); > > There is something possible with more magic: I know. > #define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS > > which you only need once in pwm-lpss.c and then all exports use that > namespace. (And if you pick up my suggestion for patch 1 you also > benefit from that.) For a single export (even for a few of them) it's an overkill. Taking above into consideration I don't think we need to alter a proposed change. Thanks for review!
On Wed, Sep 07, 2022 at 05:21:53PM +0300, Andy Shevchenko wrote: > On Wed, Sep 07, 2022 at 11:11:44AM +0200, Uwe Kleine-König wrote: > > On Tue, Sep 06, 2022 at 10:57:28PM +0300, Andy Shevchenko wrote: ... > > > -EXPORT_SYMBOL_GPL(pwm_lpss_probe); > > > +EXPORT_SYMBOL_NS_GPL(pwm_lpss_probe, PWM_LPSS); > > > > There is something possible with more magic: > > #define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS > > > > which you only need once in pwm-lpss.c and then all exports use that > > namespace. (And if you pick up my suggestion for patch 1 you also > > benefit from that.) > > For a single export (even for a few of them) it's an overkill. Ah, you adding there 4 more. But still I think it's an overkill. It's so small driver that duplicating namespace in each of the exported symbols is not an issue, is it?
On Wed, Sep 07, 2022 at 08:00:42PM +0300, Andy Shevchenko wrote: > On Wed, Sep 07, 2022 at 05:21:53PM +0300, Andy Shevchenko wrote: > > On Wed, Sep 07, 2022 at 11:11:44AM +0200, Uwe Kleine-König wrote: > > > On Tue, Sep 06, 2022 at 10:57:28PM +0300, Andy Shevchenko wrote: ... > > > > -EXPORT_SYMBOL_GPL(pwm_lpss_probe); > > > > +EXPORT_SYMBOL_NS_GPL(pwm_lpss_probe, PWM_LPSS); > > > > > > There is something possible with more magic: > > > #define DEFAULT_SYMBOL_NAMESPACE PWM_LPSS > > > > > > which you only need once in pwm-lpss.c and then all exports use that > > > namespace. (And if you pick up my suggestion for patch 1 you also > > > benefit from that.) > > > > For a single export (even for a few of them) it's an overkill. > > Ah, you adding there 4 more. But still I think it's an overkill. It's so small > driver that duplicating namespace in each of the exported symbols is not an > issue, is it? Okay, now looking at the patch organization (I forgot that I moved NS one to be not the first one) your suggestion makes a point. We won't change the code we just introduced. That said, I would like to get your SoB or what you agree with to the patch 1 and I make this one as you suggested.
diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c index 75b778e839b3..9f2c666b95ec 100644 --- a/drivers/pwm/pwm-lpss-pci.c +++ b/drivers/pwm/pwm-lpss-pci.c @@ -92,3 +92,4 @@ module_pci_driver(pwm_lpss_driver_pci); MODULE_DESCRIPTION("PWM PCI driver for Intel LPSS"); MODULE_LICENSE("GPL v2"); +MODULE_IMPORT_NS(PWM_LPSS); diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c index fcd80cca2f6d..96fde1b2b967 100644 --- a/drivers/pwm/pwm-lpss-platform.c +++ b/drivers/pwm/pwm-lpss-platform.c @@ -87,4 +87,5 @@ module_platform_driver(pwm_lpss_driver_platform); MODULE_DESCRIPTION("PWM platform driver for Intel LPSS"); MODULE_LICENSE("GPL v2"); +MODULE_IMPORT_NS(PWM_LPSS); MODULE_ALIAS("platform:pwm-lpss"); diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 36d4e83e6b79..a82a57eb2482 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -250,7 +250,7 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r, return lpwm; } -EXPORT_SYMBOL_GPL(pwm_lpss_probe); +EXPORT_SYMBOL_NS_GPL(pwm_lpss_probe, PWM_LPSS); MODULE_DESCRIPTION("PWM driver for Intel LPSS"); MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
Avoid unnecessary pollution of the global symbol namespace by moving library functions in to a specific namespace and import that into the drivers that make use of the functions. For more info: https://lwn.net/Articles/760045/ Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/pwm/pwm-lpss-pci.c | 1 + drivers/pwm/pwm-lpss-platform.c | 1 + drivers/pwm/pwm-lpss.c | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-)