[PATCH] platform/x86: int3472: make common part a separate module

[PATCH] platform/x86: int3472: make common part a separate module

[Arnd Bergmann]

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>
---
 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");
-- 
2.39.2

Re: [PATCH] platform/x86: int3472: make common part a separate module

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: int3472: make common part a separate module

[Arnd Bergmann]

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

Re: [PATCH] platform/x86: int3472: make common part a separate module

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: int3472: make common part a separate module

[Arnd Bergmann]

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

Re: [PATCH] platform/x86: int3472: make common part a separate module

[Andy Shevchenko]

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).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: int3472: make common part a separate module

[Hans de Goede]

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");