On 08/06/2021 10:07:47-0700, Randy Dunlap wrote:

On 6/8/21 5:24 AM, Alexandre Belloni wrote:

quoted

On 06/06/2021 09:24:23-0700, Randy Dunlap wrote:

quoted

REG_OFFSET is defined both here and in arch/arm/mach-ixp4xx/, which
causes a build warning, so rename this macro in the RTC driver.

../drivers/rtc/rtc-pcf8523.c:44: warning: "REG_OFFSET" redefined
   44 | #define REG_OFFSET   0x0e

../arch/arm/mach-ixp4xx/include/mach/platform.h:25: note: this is the location of the previous definition
   25 | #define REG_OFFSET 3

Fixes: bc3bee025272 ("rtc: pcf8523: add support for trimming the RTC oscillator")
Signed-off-by: Randy Dunlap <redacted>
Reported-by: kernel test robot <redacted>
Cc: Russell King <redacted>
Cc: Alessandro Zummo <redacted>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: linux-rtc@vger.kernel.org
---
 drivers/rtc/rtc-pcf8523.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- linux-next-20210604.orig/drivers/rtc/rtc-pcf8523.c
+++ linux-next-20210604/drivers/rtc/rtc-pcf8523.c

@@ -41,7 +41,7 @@
 #define REG_WEEKDAY_ALARM	0x0d
 #define ALARM_DIS BIT(7)
 
-#define REG_OFFSET   0x0e
+#define REG_OFFSET_TUNE   0x0e /* offset register is used for tuning */

All the other defines are using the names from the datasheet, probably
ixp4xx should be fixed instead?

That would be something like 25 changes in 14 files instead
of 3 changes in one file.

But REG_OFFSET from mach-ixp4xx/include/mach/platform.h is the one
leaking out. I think you can agree that rtc-pcf8523.c is self contained.

But for both locations, names like REG_OFFSET are just too generic.
They should be more specific so that name clashes won't happen.

This name clash wouldn't happen if ixp4xx was converted to multiplatform
as this would prevent it from including random headers globally but
there hasn't been any significant move in that direction since February
2019. I know nslu2 is a popular platform but maybe at some point we should
consider simply dropping ixp4 support if it doesn't improve.

I remember that at the time I took the patch I though the REG_ prefix
was a bit generic but again, this is pretty self-contained in the
driver.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com