On Wednesday 27 April 2016 19:21:22 Rich Felker wrote:

On Thu, Apr 28, 2016 at 12:34:18AM +0200, Arnd Bergmann wrote:

quoted

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.

quoted

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.

quoted

-		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);
}

quoted

 }
-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.

quoted

+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