| Message ID | 20240529095009.1895618-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: int3472: make common part a separate module | expand |
On Wed, May 29, 2024 at 12:50 PM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > Linking an object file into multiple modules is not supported > and causes a W=1 warning: > > scripts/Makefile.build:236: drivers/platform/x86/intel/int3472/Makefile: common.o is added to multiple modules: intel_skl_int3472_discrete intel_skl_int3472_tps68470 > > Split out the common part here into a separate module to make it > more reliable. ... > obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472_discrete.o \ > - intel_skl_int3472_tps68470.o > + intel_skl_int3472_tps68470.o \ > + intel_skl_int3472_common.o A nit: Can this be put above instead? ... > +EXPORT_SYMBOL_GPL(skl_int3472_get_sensor_adev_and_name); Are these namespaced?
On Wed, May 29, 2024, at 15:41, Andy Shevchenko wrote: > On Wed, May 29, 2024 at 12:50 PM Arnd Bergmann <arnd@kernel.org> wrote: >> >> From: Arnd Bergmann <arnd@arndb.de> >> >> Linking an object file into multiple modules is not supported >> and causes a W=1 warning: >> >> scripts/Makefile.build:236: drivers/platform/x86/intel/int3472/Makefile: common.o is added to multiple modules: intel_skl_int3472_discrete intel_skl_int3472_tps68470 >> >> Split out the common part here into a separate module to make it >> more reliable. > > ... > >> obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472_discrete.o \ >> - intel_skl_int3472_tps68470.o > >> + intel_skl_int3472_tps68470.o \ >> + intel_skl_int3472_common.o > > A nit: Can this be put above instead? I've changed it like this now, is that what you meant? obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472_common.o \ intel_skl_int3472_discrete.o \ intel_skl_int3472_tps68470.o \ intel_skl_int3472_common-y += common.o intel_skl_int3472_discrete-y := discrete.o clk_and_regulator.o led.o intel_skl_int3472_tps68470-y := tps68470.o tps68470_board_data.o > ... > >> +EXPORT_SYMBOL_GPL(skl_int3472_get_sensor_adev_and_name); > > Are these namespaced? No, is there any advantage to making them namespaced? It's only a few symbols and they have proper prefixes. Arnd
On Wed, May 29, 2024 at 5:14 PM Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, May 29, 2024, at 15:41, Andy Shevchenko wrote: > > On Wed, May 29, 2024 at 12:50 PM Arnd Bergmann <arnd@kernel.org> wrote: ... > >> obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472_discrete.o \ > >> - intel_skl_int3472_tps68470.o > > > >> + intel_skl_int3472_tps68470.o \ > >> + intel_skl_int3472_common.o > > > > A nit: Can this be put above instead? > > I've changed it like this now, is that what you meant? > > > obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472_common.o \ > intel_skl_int3472_discrete.o \ > intel_skl_int3472_tps68470.o \ Without the last trailing \, but yes. > intel_skl_int3472_common-y += common.o > intel_skl_int3472_discrete-y := discrete.o clk_and_regulator.o led.o > intel_skl_int3472_tps68470-y := tps68470.o tps68470_board_data.o ... > >> +EXPORT_SYMBOL_GPL(skl_int3472_get_sensor_adev_and_name); > > > > Are these namespaced? > > No, is there any advantage to making them namespaced? Yes, to clean up the global namespace. > It's only a few symbols and they have proper prefixes. It's different from the exported namespace. The function prefixes are needed due to C language, as we can't have two functions named the same. The export OTOH is what used for linking modules and if there is no need to have it exported globally, if, for example, compiling in this one. So, can we move to the exported namespace at the same time?
On Wed, May 29, 2024, at 16:28, Andy Shevchenko wrote: > On Wed, May 29, 2024 at 5:14 PM Arnd Bergmann <arnd@arndb.de> wrote: >> On Wed, May 29, 2024, at 15:41, Andy Shevchenko wrote: >> > On Wed, May 29, 2024 at 12:50 PM Arnd Bergmann <arnd@kernel.org> > It's different from the exported namespace. > The function prefixes are needed due to C language, as we can't have > two functions named the same. The export OTOH is what used for linking > modules and if there is no need to have it exported globally, if, for > example, compiling in this one. > > So, can we move to the exported namespace at the same time? Maybe you can come up with a patch then? I have no idea which namespace to use here, seeing that there are already six differnet namespaces in use in drivers/platform/x86/intel/ but none of them seem to be a good fit for this one. Are you asking to just define another namespace here? How would I define what the rules about using this namespace are, and where are they documented? Arnd
On Wed, May 29, 2024 at 5:48 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Wed, May 29, 2024, at 16:28, Andy Shevchenko wrote: > > On Wed, May 29, 2024 at 5:14 PM Arnd Bergmann <arnd@arndb.de> wrote: > >> On Wed, May 29, 2024, at 15:41, Andy Shevchenko wrote: > >> > On Wed, May 29, 2024 at 12:50 PM Arnd Bergmann <arnd@kernel.org> > > It's different from the exported namespace. > > The function prefixes are needed due to C language, as we can't have > > two functions named the same. The export OTOH is what used for linking > > modules and if there is no need to have it exported globally, if, for > > example, compiling in this one. > > > > So, can we move to the exported namespace at the same time? > > Maybe you can come up with a patch then? Yes, why not. > I have no idea > which namespace to use here, seeing that there are already > six differnet namespaces in use in drivers/platform/x86/intel/ > but none of them seem to be a good fit for this one. > > Are you asking to just define another namespace here? Yes. > How would I define what the rules about using this namespace > are, and where are they documented? Currently we use a common sense, like a pattern: SUBSYSTEM_DRIVER or so. In this case INTEL_INT3472 is good enough as it's unique enough to not collide with anything else in Intel's world (okay, I hope that we learnt our mistakes in the past and won't issue same ACPI ID for different devices).
Hi, On 5/29/24 11:49 AM, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Linking an object file into multiple modules is not supported > and causes a W=1 warning: > > scripts/Makefile.build:236: drivers/platform/x86/intel/int3472/Makefile: common.o is added to multiple modules: intel_skl_int3472_discrete intel_skl_int3472_tps68470 > > Split out the common part here into a separate module to make it > more reliable. > > Fixes: a2f9fbc247ee ("platform/x86: int3472: Split into 2 drivers") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans I've fixed the following checkpatch warning while applying this: WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity") Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/intel/int3472/Makefile | 9 ++++++--- > drivers/platform/x86/intel/int3472/common.c | 7 +++++++ > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/platform/x86/intel/int3472/Makefile b/drivers/platform/x86/intel/int3472/Makefile > index 9f16cb514397..a8aba07bf1dc 100644 > --- a/drivers/platform/x86/intel/int3472/Makefile > +++ b/drivers/platform/x86/intel/int3472/Makefile > @@ -1,4 +1,7 @@ > obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472_discrete.o \ > - intel_skl_int3472_tps68470.o > -intel_skl_int3472_discrete-y := discrete.o clk_and_regulator.o led.o common.o > -intel_skl_int3472_tps68470-y := tps68470.o tps68470_board_data.o common.o > + intel_skl_int3472_tps68470.o \ > + intel_skl_int3472_common.o > +intel_skl_int3472_discrete-y := discrete.o clk_and_regulator.o led.o > +intel_skl_int3472_tps68470-y := tps68470.o tps68470_board_data.o > + > +intel_skl_int3472_common-y += common.o > diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c > index 9db2bb0bbba4..8e4a782b2c35 100644 > --- a/drivers/platform/x86/intel/int3472/common.c > +++ b/drivers/platform/x86/intel/int3472/common.c > @@ -29,6 +29,7 @@ union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev, char *i > > return obj; > } > +EXPORT_SYMBOL_GPL(skl_int3472_get_acpi_buffer); > > int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb) > { > @@ -52,6 +53,7 @@ int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb) > kfree(obj); > return ret; > } > +EXPORT_SYMBOL_GPL(skl_int3472_fill_cldb); > > /* sensor_adev_ret may be NULL, name_ret must not be NULL */ > int skl_int3472_get_sensor_adev_and_name(struct device *dev, > @@ -80,3 +82,8 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev, > > return ret; > } > +EXPORT_SYMBOL_GPL(skl_int3472_get_sensor_adev_and_name); > + > +MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Device Driver library"); > +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); > +MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/intel/int3472/Makefile b/drivers/platform/x86/intel/int3472/Makefile index 9f16cb514397..a8aba07bf1dc 100644 --- a/drivers/platform/x86/intel/int3472/Makefile +++ b/drivers/platform/x86/intel/int3472/Makefile @@ -1,4 +1,7 @@ obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472_discrete.o \ - intel_skl_int3472_tps68470.o -intel_skl_int3472_discrete-y := discrete.o clk_and_regulator.o led.o common.o -intel_skl_int3472_tps68470-y := tps68470.o tps68470_board_data.o common.o + intel_skl_int3472_tps68470.o \ + intel_skl_int3472_common.o +intel_skl_int3472_discrete-y := discrete.o clk_and_regulator.o led.o +intel_skl_int3472_tps68470-y := tps68470.o tps68470_board_data.o + +intel_skl_int3472_common-y += common.o diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c index 9db2bb0bbba4..8e4a782b2c35 100644 --- a/drivers/platform/x86/intel/int3472/common.c +++ b/drivers/platform/x86/intel/int3472/common.c @@ -29,6 +29,7 @@ union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev, char *i return obj; } +EXPORT_SYMBOL_GPL(skl_int3472_get_acpi_buffer); int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb) { @@ -52,6 +53,7 @@ int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb) kfree(obj); return ret; } +EXPORT_SYMBOL_GPL(skl_int3472_fill_cldb); /* sensor_adev_ret may be NULL, name_ret must not be NULL */ int skl_int3472_get_sensor_adev_and_name(struct device *dev, @@ -80,3 +82,8 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev, return ret; } +EXPORT_SYMBOL_GPL(skl_int3472_get_sensor_adev_and_name); + +MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Device Driver library"); +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); +MODULE_LICENSE("GPL v2");