Re: [PATCH v3 2/6] rtc: rzn1: Add new RTC driver

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Geert,

geert@linux-m68k.org wrote on Mon, 2 May 2022 16:41:20 +0200:

> Hi Miquel,
> 
> On Fri, Apr 29, 2022 at 12:46 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > From: Michel Pollet <michel.pollet@bp.renesas.com>
> >
> > Add a basic RTC driver for the RZ/N1.
> >
> > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> > Co-developed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/drivers/rtc/rtc-rzn1.c
> > @@ -0,0 +1,246 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Renesas RZN1 Real Time Clock interface for Linux  
> 
> RZ/N1

Done.

> 
> > +static int rzn1_rtc_probe(struct platform_device *pdev)
> > +{
> > +       struct rzn1_rtc *rtc;
> > +       int ret;
> > +
> > +       rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> > +       if (!rtc)
> > +               return -ENOMEM;
> > +
> > +       platform_set_drvdata(pdev, rtc);
> > +
> > +       rtc->base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (IS_ERR(rtc->base))
> > +               return dev_err_probe(&pdev->dev, PTR_ERR(rtc->base), "Missing reg\n");
> > +
> > +       rtc->rtcdev = devm_rtc_allocate_device(&pdev->dev);
> > +       if (IS_ERR(rtc->rtcdev))
> > +               return PTR_ERR(rtc);
> > +
> > +       rtc->rtcdev->range_min = RTC_TIMESTAMP_BEGIN_2000;
> > +       rtc->rtcdev->range_max = RTC_TIMESTAMP_END_2099;
> > +       rtc->rtcdev->ops = &rzn1_rtc_ops;
> > +       clear_bit(RTC_FEATURE_ALARM, rtc->rtcdev->features);
> > +       clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtcdev->features);
> > +
> > +       pm_runtime_enable(&pdev->dev);
> > +       pm_runtime_get_sync(&pdev->dev);  
> 
> While this call cannot really fail on this platform, you may want to
> call pm_runtime_resume_and_get() instead, and check its return
> value (else the janitors will make that change later ;-)

Right, I misunderstood the doc, I'll change it.

> 
> > +
> > +       /*
> > +        * Ensure the clock counter is enabled.
> > +        * Set 24-hour mode and possible oscillator offset compensation in SUBU mode.
> > +        */
> > +       writel(RZN1_RTC_CTL0_CE | RZN1_RTC_CTL0_AMPM | RZN1_RTC_CTL0_SLSB_SUBU,
> > +              rtc->base + RZN1_RTC_CTL0);
> > +
> > +       /* Disable all interrupts */
> > +       writel(0, rtc->base + RZN1_RTC_CTL1);
> > +
> > +       ret = devm_rtc_register_device(rtc->rtcdev);
> > +       if (ret)
> > +               goto dis_runtime_pm;
> > +
> > +       return 0;
> > +
> > +dis_runtime_pm:
> > +       pm_runtime_put_sync(&pdev->dev);  
> 
> pm_runtime_put() should be fine, no need for the synchronous version.

Right, there is no need for the _sync() version here and below I
believe.

> 
> > +       pm_runtime_disable(&pdev->dev);
> > +
> > +       return ret;
> > +}
> > +
> > +static int rzn1_rtc_remove(struct platform_device *pdev)
> > +{
> > +       pm_runtime_put_sync(&pdev->dev);  
> 
> pm_runtime_put().
> 
> > +       pm_runtime_disable(&pdev->dev);
> > +
> > +       return 0;
> > +}  
> 
> 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


Thanks,
Miquèl