| Message ID | 1461796470-1291527-5-git-send-email-arnd@arndb.de |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Apr 28, 2016 at 12:34:18AM +0200, Arnd Bergmann wrote: > The rtc-generic driver provides an architecture specific > wrapper on top of the generic rtc_class_ops abstraction, > and on sh, that goes through another indirection using > the rtc_sh_get_time/rtc_sh_set_time functions. > > This changes the sh rtc-generic device to provide its > rtc_class_ops directly, skipping one of the abstraction > levels. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Looks ok in principle. Have you tested that it builds? Some questions inline: > --- > arch/sh/include/asm/rtc.h | 11 ----------- > arch/sh/kernel/time.c | 32 +++++++++++++++++++------------- > drivers/rtc/rtc-generic.c | 2 +- > 3 files changed, 20 insertions(+), 25 deletions(-) > > diff --git a/arch/sh/include/asm/rtc.h b/arch/sh/include/asm/rtc.h > index 52b0c2dba979..f7b010d48af7 100644 > --- a/arch/sh/include/asm/rtc.h > +++ b/arch/sh/include/asm/rtc.h > @@ -6,17 +6,6 @@ extern void (*board_time_init)(void); > extern void (*rtc_sh_get_time)(struct timespec *); > extern int (*rtc_sh_set_time)(const time_t); > > -/* some dummy definitions */ > -#define RTC_BATT_BAD 0x100 /* battery bad */ > -#define RTC_SQWE 0x08 /* enable square-wave output */ > -#define RTC_DM_BINARY 0x04 /* all time/date values are BCD if clear */ > -#define RTC_24H 0x02 /* 24 hour mode - else hours bit 7 means pm */ > -#define RTC_DST_EN 0x01 /* auto switch DST - works f. USA only */ > - > -struct rtc_time; > -unsigned int get_rtc_time(struct rtc_time *); > -int set_rtc_time(struct rtc_time *); > - > #define RTC_CAP_4_DIGIT_YEAR (1 << 0) > > struct sh_rtc_platform_info { > diff --git a/arch/sh/kernel/time.c b/arch/sh/kernel/time.c > index d6d0a986c6e9..92cd676970d9 100644 > --- a/arch/sh/kernel/time.c > +++ b/arch/sh/kernel/time.c > @@ -50,27 +50,30 @@ int update_persistent_clock(struct timespec now) > } > #endif > > -unsigned int get_rtc_time(struct rtc_time *tm) > +static int rtc_generic_get_time(struct device *dev, struct rtc_time *tm) > { > - if (rtc_sh_get_time != null_rtc_get_time) { This seems like a functional change -- whereas previously the null_rtc_get_time case left *tm unchanged, now the function gets called and junk gets filled in. Is that desired? > - struct timespec tv; > + struct timespec tv; > > - rtc_sh_get_time(&tv); > - rtc_time_to_tm(tv.tv_sec, tm); > - } > - > - return RTC_24H; > + rtc_sh_get_time(&tv); > + rtc_time_to_tm(tv.tv_sec, tm); > + return 0; Also the return value is changed. Is this correct? > } > -EXPORT_SYMBOL(get_rtc_time); > > -int set_rtc_time(struct rtc_time *tm) > +static int rtc_generic_set_time(struct device *dev, struct rtc_time *tm) > { > unsigned long secs; > > rtc_tm_to_time(tm, &secs); > - return rtc_sh_set_time(secs); > + if (!rtc_sh_set_time || rtc_sh_set_time(secs) < 0) > + return -EOPNOTSUPP; > + > + return 0; > } > -EXPORT_SYMBOL(set_rtc_time); Why checking rtc_sh_set_time for a null pointer? null_rtc_set_time is not a null pointer but a dummy function that's always safe to call, I think. > + > +static const struct rtc_class_ops rtc_generic_ops = { > + .read_time = rtc_generic_get_time, > + .set_time = rtc_generic_set_time, > +}; > > static int __init rtc_generic_init(void) > { > @@ -79,7 +82,10 @@ static int __init rtc_generic_init(void) > if (rtc_sh_get_time == null_rtc_get_time) > return -ENODEV; > > - pdev = platform_device_register_simple("rtc-generic", -1, NULL, 0); > + pdev = platform_device_register_data(NULL, "rtc-generic", -1, > + &rtc_generic_ops, > + sizeof(rtc_generic_ops)); > + Not a complaint about your patch, but I'd like to get rid of this platform device and abstraction layer completely since it doesn't seem like something that can be modeled correctly in device tree. When you're done cleaning this up, will it be possible to just have rtc drivers that use whatever generic framework is left, where the right driver is automatically attached to compatible DT nodes? I'm trying to move all of arch/sh over to device tree and remove hard-coded platform devices. Rich
Hi Rich, On Thu, Apr 28, 2016 at 1:21 AM, Rich Felker <dalias@libc.org> wrote: > Not a complaint about your patch, but I'd like to get rid of this > platform device and abstraction layer completely since it doesn't seem > like something that can be modeled correctly in device tree. When > you're done cleaning this up, will it be possible to just have rtc > drivers that use whatever generic framework is left, where the right > driver is automatically attached to compatible DT nodes? I'm trying to > move all of arch/sh over to device tree and remove hard-coded platform > devices. If you describe the RTC in DT, it can bound to a hardware-specific driver in drivers/rtc/rtc-*.c. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wednesday 27 April 2016 19:21:22 Rich Felker wrote: > On Thu, Apr 28, 2016 at 12:34:18AM +0200, Arnd Bergmann wrote: > > The rtc-generic driver provides an architecture specific > > wrapper on top of the generic rtc_class_ops abstraction, > > and on sh, that goes through another indirection using > > the rtc_sh_get_time/rtc_sh_set_time functions. > > > > This changes the sh rtc-generic device to provide its > > rtc_class_ops directly, skipping one of the abstraction > > levels. > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Looks ok in principle. Have you tested that it builds? I think I build tested version 1, but not the current version. > > diff --git a/arch/sh/kernel/time.c b/arch/sh/kernel/time.c > > index d6d0a986c6e9..92cd676970d9 100644 > > --- a/arch/sh/kernel/time.c > > +++ b/arch/sh/kernel/time.c > > @@ -50,27 +50,30 @@ int update_persistent_clock(struct timespec now) > > } > > #endif > > > > -unsigned int get_rtc_time(struct rtc_time *tm) > > +static int rtc_generic_get_time(struct device *dev, struct rtc_time *tm) > > { > > - if (rtc_sh_get_time != null_rtc_get_time) { > > This seems like a functional change -- whereas previously the > null_rtc_get_time case left *tm unchanged, now the function gets > called and junk gets filled in. Is that desired? I dropped the check because it duplicates the check in rtc_generic_init() below it: the old genrtc driver needed the check in get_rtc_time() because it would call that function unconditionally, but with the rtc-generic driver, we know that we only ever call this after registering the device successfully. > > - struct timespec tv; > > + struct timespec tv; > > > > - rtc_sh_get_time(&tv); > > - rtc_time_to_tm(tv.tv_sec, tm); > > - } > > - > > - return RTC_24H; > > + rtc_sh_get_time(&tv); > > + rtc_time_to_tm(tv.tv_sec, tm); > > + return 0; > > Also the return value is changed. Is this correct? Yes: again, the genrtc driver had obscure calling conventions requiring RTC_24H to be returned on success, while the rtc-generic driver uses the normal kernel coding style of using 0 for success. Previously, this function was used to convert from get_rtc_time() calling conventions (without a device) to the normal rtc_class_ops: static int generic_get_time(struct device *dev, struct rtc_time *tm) { unsigned int ret = get_rtc_time(tm); if (ret & RTC_BATT_BAD) return -EOPNOTSUPP; return rtc_valid_tm(tm); } > > } > > -EXPORT_SYMBOL(get_rtc_time); > > > > -int set_rtc_time(struct rtc_time *tm) > > +static int rtc_generic_set_time(struct device *dev, struct rtc_time *tm) > > { > > unsigned long secs; > > > > rtc_tm_to_time(tm, &secs); > > - return rtc_sh_set_time(secs); > > + if (!rtc_sh_set_time || rtc_sh_set_time(secs) < 0) > > + return -EOPNOTSUPP; > > + > > + return 0; > > } > > -EXPORT_SYMBOL(set_rtc_time); > > Why checking rtc_sh_set_time for a null pointer? null_rtc_set_time is > not a null pointer but a dummy function that's always safe to call, I > think. You are right, it should check for null_rtc_set_time instead, I probably copied it from powerpc, which does this a bit differently. Actually calling null_rtc_set_time however would be (slightly) wrong here, because we want to return an error to user space if we try to set a read-only rtc. > > +static const struct rtc_class_ops rtc_generic_ops = { > > + .read_time = rtc_generic_get_time, > > + .set_time = rtc_generic_set_time, > > +}; > > > > static int __init rtc_generic_init(void) > > { > > @@ -79,7 +82,10 @@ static int __init rtc_generic_init(void) > > if (rtc_sh_get_time == null_rtc_get_time) > > return -ENODEV; > > > > - pdev = platform_device_register_simple("rtc-generic", -1, NULL, 0); > > + pdev = platform_device_register_data(NULL, "rtc-generic", -1, > > + &rtc_generic_ops, > > + sizeof(rtc_generic_ops)); > > + > > Not a complaint about your patch, but I'd like to get rid of this > platform device and abstraction layer completely since it doesn't seem > like something that can be modeled correctly in device tree. When > you're done cleaning this up, will it be possible to just have rtc > drivers that use whatever generic framework is left, where the right > driver is automatically attached to compatible DT nodes? I'm trying to > move all of arch/sh over to device tree and remove hard-coded platform > devices. Yes, I think that would be great. When an rtc driver is registered, you don't actually need the read_persistent_clock/update_persistent_clock functions (there are __weak versions of them that do nothing and cause a fallback to calling into the rtc subsystem), so you can replace the rtc_sh_get_time/rtc_sh_set_time with proper drivers one at a time. I only see two of them anyway (dreamcast and sh03), so that should be easy enough to do. For instance in arch/sh/boards/mach-sh03/rtc.c, the sh03_time_init() function should register a platform driver, whose probe function calls devm_rtc_device_register() to register with the rtc subsystem. Then you move the entire file to drivers/rtc/ and change the callers of sh03_time_init() to create the device manually (for the classic board file) or drop it (for DT). After you have done that, all rtc related code can be removed from arch/sh/kernel/time.c. Arnd
On Thursday 28 April 2016 11:08:41 Arnd Bergmann wrote: > I only see two of them anyway (dreamcast and sh03), so that should > be easy enough to do. For instance in arch/sh/boards/mach-sh03/rtc.c, > the sh03_time_init() function should register a platform driver, > whose probe function calls devm_rtc_device_register() to register > with the rtc subsystem. Then you move the entire file to drivers/rtc/ > and change the callers of sh03_time_init() to create the device > manually (for the classic board file) or drop it (for DT). Just FYI: Another look at the sh03_rtc_settimeofday function shows that it's always been wrong: unlike the set_mmss() function it calls, it should set all the time fields, not just minutes and seconds. Arnd
diff --git a/arch/sh/include/asm/rtc.h b/arch/sh/include/asm/rtc.h index 52b0c2dba979..f7b010d48af7 100644 --- a/arch/sh/include/asm/rtc.h +++ b/arch/sh/include/asm/rtc.h @@ -6,17 +6,6 @@ extern void (*board_time_init)(void); extern void (*rtc_sh_get_time)(struct timespec *); extern int (*rtc_sh_set_time)(const time_t); -/* some dummy definitions */ -#define RTC_BATT_BAD 0x100 /* battery bad */ -#define RTC_SQWE 0x08 /* enable square-wave output */ -#define RTC_DM_BINARY 0x04 /* all time/date values are BCD if clear */ -#define RTC_24H 0x02 /* 24 hour mode - else hours bit 7 means pm */ -#define RTC_DST_EN 0x01 /* auto switch DST - works f. USA only */ - -struct rtc_time; -unsigned int get_rtc_time(struct rtc_time *); -int set_rtc_time(struct rtc_time *); - #define RTC_CAP_4_DIGIT_YEAR (1 << 0) struct sh_rtc_platform_info { diff --git a/arch/sh/kernel/time.c b/arch/sh/kernel/time.c index d6d0a986c6e9..92cd676970d9 100644 --- a/arch/sh/kernel/time.c +++ b/arch/sh/kernel/time.c @@ -50,27 +50,30 @@ int update_persistent_clock(struct timespec now) } #endif -unsigned int get_rtc_time(struct rtc_time *tm) +static int rtc_generic_get_time(struct device *dev, struct rtc_time *tm) { - if (rtc_sh_get_time != null_rtc_get_time) { - struct timespec tv; + struct timespec tv; - rtc_sh_get_time(&tv); - rtc_time_to_tm(tv.tv_sec, tm); - } - - return RTC_24H; + rtc_sh_get_time(&tv); + rtc_time_to_tm(tv.tv_sec, tm); + return 0; } -EXPORT_SYMBOL(get_rtc_time); -int set_rtc_time(struct rtc_time *tm) +static int rtc_generic_set_time(struct device *dev, struct rtc_time *tm) { unsigned long secs; rtc_tm_to_time(tm, &secs); - return rtc_sh_set_time(secs); + if (!rtc_sh_set_time || rtc_sh_set_time(secs) < 0) + return -EOPNOTSUPP; + + return 0; } -EXPORT_SYMBOL(set_rtc_time); + +static const struct rtc_class_ops rtc_generic_ops = { + .read_time = rtc_generic_get_time, + .set_time = rtc_generic_set_time, +}; static int __init rtc_generic_init(void) { @@ -79,7 +82,10 @@ static int __init rtc_generic_init(void) if (rtc_sh_get_time == null_rtc_get_time) return -ENODEV; - pdev = platform_device_register_simple("rtc-generic", -1, NULL, 0); + pdev = platform_device_register_data(NULL, "rtc-generic", -1, + &rtc_generic_ops, + sizeof(rtc_generic_ops)); + return PTR_ERR_OR_ZERO(pdev); } diff --git a/drivers/rtc/rtc-generic.c b/drivers/rtc/rtc-generic.c index d726c6aa96a8..3958e87a05fa 100644 --- a/drivers/rtc/rtc-generic.c +++ b/drivers/rtc/rtc-generic.c @@ -10,7 +10,7 @@ #include <linux/rtc.h> #if defined(CONFIG_M68K) || defined(CONFIG_PARISC) || \ - defined(CONFIG_PPC) || defined(CONFIG_SUPERH32) + defined(CONFIG_PPC) #include <asm/rtc.h> static int generic_get_time(struct device *dev, struct rtc_time *tm)
The rtc-generic driver provides an architecture specific wrapper on top of the generic rtc_class_ops abstraction, and on sh, that goes through another indirection using the rtc_sh_get_time/rtc_sh_set_time functions. This changes the sh rtc-generic device to provide its rtc_class_ops directly, skipping one of the abstraction levels. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- arch/sh/include/asm/rtc.h | 11 ----------- arch/sh/kernel/time.c | 32 +++++++++++++++++++------------- drivers/rtc/rtc-generic.c | 2 +- 3 files changed, 20 insertions(+), 25 deletions(-)