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@xxxxxxxx>
>
> 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-parisc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
