Re: [PATCH 2/2] m68k: rtc: dp8570a: make it a proper RTC class driver

[Thadeu Lima de Souza Cascardo <cascardo@igalia.com>]

On Thu, Feb 27, 2025 at 12:05:26PM +0100, Geert Uytterhoeven wrote:
> Hi Thadeu,
> 
> On Wed, 26 Feb 2025 at 13:27, Thadeu Lima de Souza Cascardo
> <cascardo@igalia.com> wrote:
> > In the past, each rtc implementation had to rewrite the same ioctls in
> > order to be compatible. But since 2006, a common RTC interface has been
> > introduced. Use it for the last user of RTC_MINOR.
> >
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> 
> Thanks for your patch!
> 
> > --- a/arch/m68k/bvme6000/rtc.c
> > +++ b/arch/m68k/bvme6000/rtc.c
> 
> >
> >  static int __init rtc_DP8570A_init(void)
> >  {
> > +       struct platform_device *pdev;
> > +
> >         if (!MACH_IS_BVME6000)
> >                 return -ENODEV;
> >
> >         pr_info("DP8570A Real Time Clock Driver v%s\n", RTC_VERSION);
> > -       return misc_register(&rtc_dev);
> > +
> > +       pdev = platform_device_register_data(NULL, "rtc-generic", -1,
> > +                                            &dp8570a_rtc_ops,
> > +                                            sizeof(dp8570a_rtc_ops));
> 
> Doesn't this conflict with the creation of the same device in rtc_init()[1]?
> 

I didn't look too deep and just assumed that this would be one of those
cases where this was set for machines other than BVME6000.

Ah, right, that is because of this check around it.

#if defined(CONFIG_M68KCLASSIC) || defined(CONFIG_SUN3).

But CONFIG_M68KCLASSIC is set for BVME6000.

On the other hand, mach_hwclk is accessing the same registers as the rtc.c
code, so they are redundant. One thing that is missing is
local_irq_save/local_irq_restore around it. I can add that and remove
rtc.c, which would make more sense here.

Thanks.
Cascardo.

> On BVME6000, mach_hwclk is set:
> 
>     arch/m68k/bvme6000/config.c:    mach_hwclk           = bvme6000_hwclk;
> 
> > +
> > +       return PTR_ERR_OR_ZERO(pdev);
> >  }
> >  module_init(rtc_DP8570A_init);
> 
> [1] https://elixir.bootlin.com/linux/v6.13.4/source/arch/m68k/kernel/time.c#L144
> 
> 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