diff mbox

[v3,04/16] rtc: sh: provide rtc_class_ops directly

Message ID 1461796470-1291527-5-git-send-email-arnd@arndb.de (mailing list archive)
State New, archived
Headers show

Commit Message

Arnd Bergmann April 27, 2016, 10:34 p.m. UTC
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(-)

Comments

Rich Felker April 27, 2016, 11:21 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven April 28, 2016, 7:21 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann April 28, 2016, 9:08 a.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann April 28, 2016, 9:38 a.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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)