| Message ID | 20210606162423.22235-1-rdunlap@infradead.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: pcf8523: rename REG_OFFSET register | expand |
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
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?
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.
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.
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.
--- 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 = {
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(-)