[PATCH] rtc: pcf8523: rename REG_OFFSET register

[PATCH] rtc: pcf8523: rename REG_OFFSET register

[Randy Dunlap]

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 <rdunlap@infradead.org>
Reported-by: kernel test robot <lkp@intel.com>
Cc: Russell King <rmk+kernel@armlinux.org.uk>
Cc: Alessandro Zummo <a.zummo@towertech.it>
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 */
 #define REG_OFFSET_MODE BIT(7)
 
 #define REG_TMR_CLKOUT_CTRL 0x0f
@@ -442,7 +442,7 @@ static int pcf8523_rtc_read_offset(struc
 	u8 value;
 	s8 val;
 
-	err = pcf8523_read(client, REG_OFFSET, &value);
+	err = pcf8523_read(client, REG_OFFSET_TUNE, &value);
 	if (err < 0)
 		return err;
 
@@ -467,7 +467,7 @@ static int pcf8523_rtc_set_offset(struct
 	else
 		value = (reg_m1 & 0x7f) | REG_OFFSET_MODE;
 
-	return pcf8523_write(client, REG_OFFSET, value);
+	return pcf8523_write(client, REG_OFFSET_TUNE, value);
 }
 
 static const struct rtc_class_ops pcf8523_rtc_ops = {

Re: [PATCH] rtc: pcf8523: rename REG_OFFSET register

[Nobuhiro Iwamatsu]

Hi,

2021年6月7日(月) 1:24 Randy Dunlap <rdunlap@infradead.org>:
>
> 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 <rdunlap@infradead.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> 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 */
>  #define REG_OFFSET_MODE BIT(7)
>
>  #define REG_TMR_CLKOUT_CTRL 0x0f
> @@ -442,7 +442,7 @@ static int pcf8523_rtc_read_offset(struc
>         u8 value;
>         s8 val;
>
> -       err = pcf8523_read(client, REG_OFFSET, &value);
> +       err = pcf8523_read(client, REG_OFFSET_TUNE, &value);
>         if (err < 0)
>                 return err;
>
> @@ -467,7 +467,7 @@ static int pcf8523_rtc_set_offset(struct
>         else
>                 value = (reg_m1 & 0x7f) | REG_OFFSET_MODE;
>
> -       return pcf8523_write(client, REG_OFFSET, value);
> +       return pcf8523_write(client, REG_OFFSET_TUNE, value);
>  }
>
>  static const struct rtc_class_ops pcf8523_rtc_ops = {

Looks good to me.
Reviewed-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

Best regards,
  Nobuhiro
-- 
Nobuhiro Iwamatsu
   iwamatsu at {nigauri.org / debian.org / kernel.org}
   GPG ID: 40AD1FA6

Re: [PATCH] rtc: pcf8523: rename REG_OFFSET register

[Alexandre Belloni]

On 06/06/2021 09:24:23-0700, Randy Dunlap wrote:
> 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 <rdunlap@infradead.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> 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?


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

Re: [PATCH] rtc: pcf8523: rename REG_OFFSET register

[Randy Dunlap]

On 6/8/21 5:24 AM, Alexandre Belloni wrote:
> On 06/06/2021 09:24:23-0700, Randy Dunlap wrote:
>> 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 <rdunlap@infradead.org>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Cc: Russell King <rmk+kernel@armlinux.org.uk>
>> Cc: Alessandro Zummo <a.zummo@towertech.it>
>> 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 for both locations, names like REG_OFFSET are just too generic.
They should be more specific so that name clashes won't happen.

-- 
~Randy

Re: [PATCH] rtc: pcf8523: rename REG_OFFSET register

[Alexandre Belloni]

On 08/06/2021 10:07:47-0700, Randy Dunlap wrote:
> On 6/8/21 5:24 AM, Alexandre Belloni wrote:
> > On 06/06/2021 09:24:23-0700, Randy Dunlap wrote:
> >> 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 <rdunlap@infradead.org>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> >> Cc: Alessandro Zummo <a.zummo@towertech.it>
> >> 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

Re: [PATCH] rtc: pcf8523: rename REG_OFFSET register

[Randy Dunlap]

On 6/8/21 3:36 PM, Alexandre Belloni wrote:
> On 08/06/2021 10:07:47-0700, Randy Dunlap wrote:
>> On 6/8/21 5:24 AM, Alexandre Belloni wrote:
>>> On 06/06/2021 09:24:23-0700, Randy Dunlap wrote:
>>>> 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 <rdunlap@infradead.org>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Cc: Russell King <rmk+kernel@armlinux.org.uk>
>>>> Cc: Alessandro Zummo <a.zummo@towertech.it>
>>>> 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.

OK, our criteria for what or where to patch differ. That's OK.

pfc8523 is self-contained (so simpler to patch or muck up)
and newer than ixp4xx. Those are my top criteria.

G'day.

-- 
~Randy