Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1388140 > unrolled thread

[PATCH v2 1/6] rtc: m68k: provide rtc_class_ops directly

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 1/6] rtc: m68k: provide rtc_class_ops directly Arnd Bergmann <arnd@arndb.de> - 2016-04-27 00:00 +0200
    Re: [PATCH v2 1/6] rtc: m68k: provide rtc_class_ops directly Geert Uytterhoeven <geert@linux-m68k.org> - 2016-04-27 09:50 +0200
      Re: [PATCH v2 1/6] rtc: m68k: provide rtc_class_ops directly Arnd Bergmann <arnd@arndb.de> - 2016-04-27 12:40 +0200

#1388140 — [PATCH v2 1/6] rtc: m68k: provide rtc_class_ops directly

The rtc-generic driver provides an architecture specific
wrapper on top of the generic rtc_class_ops abstraction,
and m68k has another abstraction on top, which is a bit
silly.

This changes the m68k rtc-generic device to provide its
rtc_class_ops directly, to reduce the number of layers
by one.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/m68k/kernel/time.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/arch/m68k/kernel/time.c b/arch/m68k/kernel/time.c
index 3857737e3958..fe35890feede 100644
--- a/arch/m68k/kernel/time.c
+++ b/arch/m68k/kernel/time.c
@@ -86,7 +86,24 @@ void read_persistent_clock(struct timespec *ts)
 	}
 }
 
-#ifdef CONFIG_ARCH_USES_GETTIMEOFFSET
+#if defined(CONFIG_ARCH_USES_GETTIMEOFFSET) && defined(CONFIG_RTC_DRV_GENERIC)
+static int rtc_generic_get_time(struct device *dev, struct rtc_time *tm)
+{
+	mach_hwclk(0, tm);
+	return rtc_valid_tm(tm);
+}
+
+static int rtc_generic_set_time(struct device *dev, struct rtc_time *tm)
+{
+	if (mach_hwclk(1, tm) < 0)
+		return -EOPNOTSUPP;
+	return 0;
+}
+
+static const struct rtc_class_ops generic_rtc_ops = {
+	.read_time = rtc_generic_get_time,
+	.set_time = rtc_generic_set_time,
+};
 
 static int __init rtc_init(void)
 {
@@ -95,7 +112,10 @@ static int __init rtc_init(void)
 	if (!mach_hwclk)
 		return -ENODEV;
 
-	pdev = platform_device_register_simple("rtc-generic", -1, NULL, 0);
+	/* or just call devm_rtc_device_register instead? */
+	pdev = platform_device_register_data(NULL, "rtc-generic", -1,
+					     &generic_rtc_ops,
+					     sizeof(generic_rtc_ops));
 	return PTR_ERR_OR_ZERO(pdev);
 }
 
-- 
2.7.0

[toc] | [next] | [standalone]

#1388733

Hi Arnd,

On Tue, Apr 26, 2016 at 11:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> The rtc-generic driver provides an architecture specific
> wrapper on top of the generic rtc_class_ops abstraction,
> and m68k has another abstraction on top, which is a bit
> silly.
>
> This changes the m68k rtc-generic device to provide its
> rtc_class_ops directly, to reduce the number of layers
> by one.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/m68k/kernel/time.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/arch/m68k/kernel/time.c b/arch/m68k/kernel/time.c
> index 3857737e3958..fe35890feede 100644
> --- a/arch/m68k/kernel/time.c
> +++ b/arch/m68k/kernel/time.c
> @@ -86,7 +86,24 @@ void read_persistent_clock(struct timespec *ts)
>         }
>  }
>
> -#ifdef CONFIG_ARCH_USES_GETTIMEOFFSET
> +#if defined(CONFIG_ARCH_USES_GETTIMEOFFSET) && defined(CONFIG_RTC_DRV_GENERIC)

s/defined/IS_ENABLED/ for the modular case.

> @@ -95,7 +112,10 @@ static int __init rtc_init(void)
>         if (!mach_hwclk)
>                 return -ENODEV;
>
> -       pdev = platform_device_register_simple("rtc-generic", -1, NULL, 0);
> +       /* or just call devm_rtc_device_register instead? */

I guess this comment is a bogus leftover? There's no "dev" parameter to
pass to devm_rtc_device_register() here.

> +       pdev = platform_device_register_data(NULL, "rtc-generic", -1,
> +                                            &generic_rtc_ops,
> +                                            sizeof(generic_rtc_ops));
>         return PTR_ERR_OR_ZERO(pdev);
>  }

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

[toc] | [prev] | [next] | [standalone]

#1388886

On Wednesday 27 April 2016 09:47:56 Geert Uytterhoeven wrote:
> > --- a/arch/m68k/kernel/time.c
> > +++ b/arch/m68k/kernel/time.c
> > @@ -86,7 +86,24 @@ void read_persistent_clock(struct timespec *ts)
> >         }
> >  }
> >
> > -#ifdef CONFIG_ARCH_USES_GETTIMEOFFSET
> > +#if defined(CONFIG_ARCH_USES_GETTIMEOFFSET) && defined(CONFIG_RTC_DRV_GENERIC)
> 
> s/defined/IS_ENABLED/ for the modular case.

Thanks, fixed in all three architectures/

> > @@ -95,7 +112,10 @@ static int __init rtc_init(void)
> >         if (!mach_hwclk)
> >                 return -ENODEV;
> >
> > -       pdev = platform_device_register_simple("rtc-generic", -1, NULL, 0);
> > +       /* or just call devm_rtc_device_register instead? */
> 
> I guess this comment is a bogus leftover? There's no "dev" parameter to
> pass to devm_rtc_device_register() here.

Sort of. When I wrote it, I thought that a NULL argument would work,
and I later found out myself that it doesn't.

I'll drop the comment there, but there are still a few ways we (probably
not me, but whoever is interested) could take this further:

- register both the device and the driver here, and call
  devm_rtc_device_register from the probe function so we can move away
  from drivers/rtc/rtc-generic.c

- do this separately for mac, mvme147, mvme16x, sun3, q40 and sun3x

- move the six implementations into drivers/rtc as standalone drivers.

One (AFAIK) unsolved problem here is the question of how to handle
read_boot_clock/read_persistent_clock/update_persistent_clock in this
case. This is a really odd API that is implemented in various ways
on a few major architectures, and not at all on others, so it's all
highly inconsistent.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web