[PATCH] i2c: designware: Actually make use of the I2C_DW_COMMON and I2C_DW symbol namespaces

[PATCH] i2c: designware: Actually make use of the I2C_DW_COMMON and I2C_DW symbol namespaces

[Uwe Kleine-König]

DEFAULT_SYMBOL_NAMESPACE must already be defined when <linux/export.h>
is included. So move the define above the include block.

Fixes: fd57a3325a77 ("i2c: designware: Move exports to I2C_DW namespaces")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
Hello,

this is based on current Linus Torvalds's master branch and depends on
the topmost commit there.

Best regards
Uwe

 drivers/i2c/busses/i2c-designware-common.c | 5 +++--
 drivers/i2c/busses/i2c-designware-slave.c  | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 183a35038eef..8eb7bd640f8d 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -8,6 +8,9 @@
  * Copyright (C) 2007 MontaVista Software Inc.
  * Copyright (C) 2009 Provigent Ltd.
  */
+
+#define DEFAULT_SYMBOL_NAMESPACE	"I2C_DW_COMMON"
+
 #include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
@@ -29,8 +32,6 @@
 #include <linux/types.h>
 #include <linux/units.h>
 
-#define DEFAULT_SYMBOL_NAMESPACE	"I2C_DW_COMMON"
-
 #include "i2c-designware-core.h"
 
 static const char *const abort_sources[] = {
diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index dc2b788eac5b..5cd4a5f7a472 100644
--- a/drivers/i2c/busses/i2c-designware-slave.c
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -6,6 +6,9 @@
  *
  * Copyright (C) 2016 Synopsys Inc.
  */
+
+#define DEFAULT_SYMBOL_NAMESPACE	"I2C_DW"
+
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/errno.h>
@@ -16,8 +19,6 @@
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 
-#define DEFAULT_SYMBOL_NAMESPACE	"I2C_DW"
-
 #include "i2c-designware-core.h"
 
 static void i2c_dw_configure_fifo_slave(struct dw_i2c_dev *dev)

base-commit: ceb8bf2ceaa77fe222fe8fe32cb7789c9099ddf1
-- 
2.45.2

Re: [PATCH] i2c: designware: Actually make use of the I2C_DW_COMMON and I2C_DW symbol namespaces

[Andy Shevchenko]

On Tue, Dec 03, 2024 at 06:36:40PM +0100, Uwe Kleine-König wrote:
> DEFAULT_SYMBOL_NAMESPACE must already be defined when <linux/export.h>
> is included. So move the define above the include block.

As in the other email I pointed out the doc says that we need to undef the
symbol. No need to move it around.

The only requirement is to place that before any EXPORT_SYMBOL*() we want to
add it to.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: designware: Actually make use of the I2C_DW_COMMON and I2C_DW symbol namespaces

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]

Hello Andy,

On Tue, Dec 03, 2024 at 09:27:35PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 03, 2024 at 06:36:40PM +0100, Uwe Kleine-König wrote:
> > DEFAULT_SYMBOL_NAMESPACE must already be defined when <linux/export.h>
> > is included. So move the define above the include block.
> 
> As in the other email I pointed out the doc says that we need to undef the
> symbol. No need to move it around.
>
> The only requirement is to place that before any EXPORT_SYMBOL*() we want to
> add it to.

Did you test your statement? I did on top of v6.13-rc1:

 - Remove the MODULE_IMPORT_NS(I2C_DW_COMMON) statements from
   drivers/i2c/busses/i2c-designware-master.c,
   drivers/i2c/busses/i2c-designware-pcidrv.c,
   drivers/i2c/busses/i2c-designware-platdrv.c and
   drivers/i2c/busses/i2c-designware-slave.c
   => Built fine, ergo the symbols are not in the I2C_DW_COMMON
   namespace.

 - On top of the previous: Add an #undef for DEFAULT_SYMBOL_NAMESPACE
   directly before the #define
   => Built fine, ergo the #undef doesn't make the namespace define
   work.

 - On top of the previous: Move #undef + #define above the #includes
   => Several warnings like:

	WARNING: modpost: module i2c-designware-platform uses symbol i2c_dw_prepare_clk from namespace I2C_DW_COMMON, but does not import it.

   Ergo the position of the definition is relevant.

 - On top of the previous: Drop the #undef
   => same as before, ergo the #undef is not needed.

Independent of what is in the docs that matches my understandment of C.

I don't expect that DEFAULT_SYMBOL_NAMESPACE to be already defined. If
there was a definition already, a #define without #undef results (under
some likely conditions) in a compiler warning. With the #undef this
warning would be suppressed. So the #undef is something like a --force
switch that I prefer not to use without reason, because I want to get
the warning if my expectations are not met.

So I still think the patch is fine as is.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] i2c: designware: Actually make use of the I2C_DW_COMMON and I2C_DW symbol namespaces

[Andy Shevchenko]

On Tue, Dec 03, 2024 at 11:46:07PM +0100, Uwe Kleine-König wrote:
> On Tue, Dec 03, 2024 at 09:27:35PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 03, 2024 at 06:36:40PM +0100, Uwe Kleine-König wrote:
> > > DEFAULT_SYMBOL_NAMESPACE must already be defined when <linux/export.h>
> > > is included. So move the define above the include block.
> > 
> > As in the other email I pointed out the doc says that we need to undef the
> > symbol. No need to move it around.
> >
> > The only requirement is to place that before any EXPORT_SYMBOL*() we want to
> > add it to.
> 
> Did you test your statement?

I speak the documentation.

> I did on top of v6.13-rc1:
> 
>  - Remove the MODULE_IMPORT_NS(I2C_DW_COMMON) statements from
>    drivers/i2c/busses/i2c-designware-master.c,
>    drivers/i2c/busses/i2c-designware-pcidrv.c,
>    drivers/i2c/busses/i2c-designware-platdrv.c and
>    drivers/i2c/busses/i2c-designware-slave.c
>    => Built fine, ergo the symbols are not in the I2C_DW_COMMON
>    namespace.
> 
>  - On top of the previous: Add an #undef for DEFAULT_SYMBOL_NAMESPACE
>    directly before the #define
>    => Built fine, ergo the #undef doesn't make the namespace define
>    work.
> 
>  - On top of the previous: Move #undef + #define above the #includes
>    => Several warnings like:
> 
> 	WARNING: modpost: module i2c-designware-platform uses symbol i2c_dw_prepare_clk from namespace I2C_DW_COMMON, but does not import it.
> 
>    Ergo the position of the definition is relevant.
> 
>  - On top of the previous: Drop the #undef
>    => same as before, ergo the #undef is not needed.
> 
> Independent of what is in the docs that matches my understandment of C.
> 
> I don't expect that DEFAULT_SYMBOL_NAMESPACE to be already defined. If
> there was a definition already, a #define without #undef results (under
> some likely conditions) in a compiler warning. With the #undef this
> warning would be suppressed. So the #undef is something like a --force
> switch that I prefer not to use without reason, because I want to get
> the warning if my expectations are not met.
> 
> So I still think the patch is fine as is.

Perhaps we need to update the documentation first.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: designware: Actually make use of the I2C_DW_COMMON and I2C_DW symbol namespaces

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

Hello,

On Wed, Dec 04, 2024 at 03:23:52AM +0200, Andy Shevchenko wrote:
> On Tue, Dec 03, 2024 at 11:46:07PM +0100, Uwe Kleine-König wrote:
> > On Tue, Dec 03, 2024 at 09:27:35PM +0200, Andy Shevchenko wrote:
> > > On Tue, Dec 03, 2024 at 06:36:40PM +0100, Uwe Kleine-König wrote:
> > > > DEFAULT_SYMBOL_NAMESPACE must already be defined when <linux/export.h>
> > > > is included. So move the define above the include block.
> > > 
> > > As in the other email I pointed out the doc says that we need to undef the
> > > symbol. No need to move it around.
> > >
> > > The only requirement is to place that before any EXPORT_SYMBOL*() we want to
> > > add it to.
> > [...]
> 
> Perhaps we need to update the documentation first.

I addressed that in https://lore.kernel.org/all/3dd7ff6fa0a636de86e091286016be8c90e03631.1733305665.git.ukleinek@kernel.org/

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] i2c: designware: Actually make use of the I2C_DW_COMMON and I2C_DW symbol namespaces

[Andy Shevchenko]

On Wed, Dec 04, 2024 at 11:25:40AM +0100, Uwe Kleine-König wrote:
> On Wed, Dec 04, 2024 at 03:23:52AM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 03, 2024 at 11:46:07PM +0100, Uwe Kleine-König wrote:
> > > On Tue, Dec 03, 2024 at 09:27:35PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Dec 03, 2024 at 06:36:40PM +0100, Uwe Kleine-König wrote:
> > > > > DEFAULT_SYMBOL_NAMESPACE must already be defined when <linux/export.h>
> > > > > is included. So move the define above the include block.
> > > > 
> > > > As in the other email I pointed out the doc says that we need to undef the
> > > > symbol. No need to move it around.
> > > >
> > > > The only requirement is to place that before any EXPORT_SYMBOL*() we want to
> > > > add it to.
> > > [...]
> > 
> > Perhaps we need to update the documentation first.
> 
> I addressed that in https://lore.kernel.org/all/3dd7ff6fa0a636de86e091286016be8c90e03631.1733305665.git.ukleinek@kernel.org/

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: designware: Actually make use of the I2C_DW_COMMON and I2C_DW symbol namespaces

[Jarkko Nikula]

On 12/5/24 9:53 AM, Andy Shevchenko wrote:
> On Wed, Dec 04, 2024 at 11:25:40AM +0100, Uwe Kleine-König wrote:
>> On Wed, Dec 04, 2024 at 03:23:52AM +0200, Andy Shevchenko wrote:
>>> On Tue, Dec 03, 2024 at 11:46:07PM +0100, Uwe Kleine-König wrote:
>>>> On Tue, Dec 03, 2024 at 09:27:35PM +0200, Andy Shevchenko wrote:
>>>>> On Tue, Dec 03, 2024 at 06:36:40PM +0100, Uwe Kleine-König wrote:
>>>>>> DEFAULT_SYMBOL_NAMESPACE must already be defined when <linux/export.h>
>>>>>> is included. So move the define above the include block.
>>>>>
>>>>> As in the other email I pointed out the doc says that we need to undef the
>>>>> symbol. No need to move it around.
>>>>>
>>>>> The only requirement is to place that before any EXPORT_SYMBOL*() we want to
>>>>> add it to.
>>>> [...]
>>>
>>> Perhaps we need to update the documentation first.
>>
>> I addressed that in https://lore.kernel.org/all/3dd7ff6fa0a636de86e091286016be8c90e03631.1733305665.git.ukleinek@kernel.org/
> 
> Thank you!
> 
Andy: is this your reviewed by? If so then

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH] i2c: designware: Actually make use of the I2C_DW_COMMON and I2C_DW symbol namespaces

[Andy Shevchenko]

On Thu, Dec 05, 2024 at 10:12:43AM +0200, Jarkko Nikula wrote:
> On 12/5/24 9:53 AM, Andy Shevchenko wrote:
> > On Wed, Dec 04, 2024 at 11:25:40AM +0100, Uwe Kleine-König wrote:
> > > On Wed, Dec 04, 2024 at 03:23:52AM +0200, Andy Shevchenko wrote:
> > > > On Tue, Dec 03, 2024 at 11:46:07PM +0100, Uwe Kleine-König wrote:
> > > > > On Tue, Dec 03, 2024 at 09:27:35PM +0200, Andy Shevchenko wrote:
> > > > > > On Tue, Dec 03, 2024 at 06:36:40PM +0100, Uwe Kleine-König wrote:
> > > > > > > DEFAULT_SYMBOL_NAMESPACE must already be defined when <linux/export.h>
> > > > > > > is included. So move the define above the include block.
> > > > > > 
> > > > > > As in the other email I pointed out the doc says that we need to undef the
> > > > > > symbol. No need to move it around.
> > > > > > 
> > > > > > The only requirement is to place that before any EXPORT_SYMBOL*() we want to
> > > > > > add it to.

[...]

> > > > Perhaps we need to update the documentation first.
> > > 
> > > I addressed that in https://lore.kernel.org/all/3dd7ff6fa0a636de86e091286016be8c90e03631.1733305665.git.ukleinek@kernel.org/
> > 
> > Thank you!

> Andy: is this your reviewed by? If so then
> 
> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

I would like to have a clarifications from Documentation to be settled down
first. When it's done, depending on the outcome it may or may not be my Rb tag.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: designware: Actually make use of the I2C_DW_COMMON and I2C_DW symbol namespaces

[Andi Shyti]

Hi,

On Thu, Dec 05, 2024 at 10:26:34AM +0200, Andy Shevchenko wrote:
> On Thu, Dec 05, 2024 at 10:12:43AM +0200, Jarkko Nikula wrote:
> > On 12/5/24 9:53 AM, Andy Shevchenko wrote:
> > > On Wed, Dec 04, 2024 at 11:25:40AM +0100, Uwe Kleine-König wrote:
> > > > On Wed, Dec 04, 2024 at 03:23:52AM +0200, Andy Shevchenko wrote:
> > > > > On Tue, Dec 03, 2024 at 11:46:07PM +0100, Uwe Kleine-König wrote:
> > > > > > On Tue, Dec 03, 2024 at 09:27:35PM +0200, Andy Shevchenko wrote:
> > > > > > > On Tue, Dec 03, 2024 at 06:36:40PM +0100, Uwe Kleine-König wrote:
> > > > > > > > DEFAULT_SYMBOL_NAMESPACE must already be defined when <linux/export.h>
> > > > > > > > is included. So move the define above the include block.
> > > > > > > 
> > > > > > > As in the other email I pointed out the doc says that we need to undef the
> > > > > > > symbol. No need to move it around.
> > > > > > > 
> > > > > > > The only requirement is to place that before any EXPORT_SYMBOL*() we want to
> > > > > > > add it to.
> 
> [...]
> 
> > > > > Perhaps we need to update the documentation first.
> > > > 
> > > > I addressed that in https://lore.kernel.org/all/3dd7ff6fa0a636de86e091286016be8c90e03631.1733305665.git.ukleinek@kernel.org/
> > > 
> > > Thank you!
> 
> > Andy: is this your reviewed by? If so then
> > 
> > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> 
> I would like to have a clarifications from Documentation to be settled down
> first. When it's done, depending on the outcome it may or may not be my Rb tag.

ping! Andy, I don't feel like merging this patch without your ack
as you had quite many comments here.

Can you please check here again?

Thanks,
Andi

Re: [PATCH] i2c: designware: Actually make use of the I2C_DW_COMMON and I2C_DW symbol namespaces

[Andy Shevchenko]

On Fri, Dec 27, 2024 at 01:07:11AM +0100, Andi Shyti wrote:
> On Thu, Dec 05, 2024 at 10:26:34AM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 05, 2024 at 10:12:43AM +0200, Jarkko Nikula wrote:
> > > On 12/5/24 9:53 AM, Andy Shevchenko wrote:
> > > > On Wed, Dec 04, 2024 at 11:25:40AM +0100, Uwe Kleine-König wrote:
> > > > > On Wed, Dec 04, 2024 at 03:23:52AM +0200, Andy Shevchenko wrote:
> > > > > > On Tue, Dec 03, 2024 at 11:46:07PM +0100, Uwe Kleine-König wrote:
> > > > > > > On Tue, Dec 03, 2024 at 09:27:35PM +0200, Andy Shevchenko wrote:
> > > > > > > > On Tue, Dec 03, 2024 at 06:36:40PM +0100, Uwe Kleine-König wrote:
> > > > > > > > > DEFAULT_SYMBOL_NAMESPACE must already be defined when <linux/export.h>
> > > > > > > > > is included. So move the define above the include block.
> > > > > > > > 
> > > > > > > > As in the other email I pointed out the doc says that we need to undef the
> > > > > > > > symbol. No need to move it around.
> > > > > > > > 
> > > > > > > > The only requirement is to place that before any EXPORT_SYMBOL*() we want to
> > > > > > > > add it to.

[...]

> > > > > > Perhaps we need to update the documentation first.
> > > > > 
> > > > > I addressed that in https://lore.kernel.org/all/3dd7ff6fa0a636de86e091286016be8c90e03631.1733305665.git.ukleinek@kernel.org/
> > > > 
> > > > Thank you!
> > 
> > > Andy: is this your reviewed by? If so then
> > > 
> > > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > 
> > I would like to have a clarifications from Documentation to be settled down
> > first. When it's done, depending on the outcome it may or may not be my Rb tag.
> 
> ping! Andy,

Sorry, I'm on vacation till mid-January.

> I don't feel like merging this patch without your ack
> as you had quite many comments here.

Yeah, it seems now we got the second approach, we need to choose one and
document it. This just makes my point: documentation needs to have
clarification to make sure everybody got it right and provide a unified
solution.

> Can you please check here again?

FWIW, technically the patch is correct and needed, but see above.

-- 
With Best Regards,
Andy Shevchenko