RE: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L
> 
> On Mon, Nov 8, 2021 at 7:38 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > On 11/4/21 9:08 AM, Biju Das wrote:
> > > Add Watchdog Timer driver for RZ/G2L SoC.
> > >
> > > WDT IP block supports normal watchdog timer function and reset
> > > request function due to CPU parity error.
> > >
> > > This driver currently supports normal watchdog timer function and
> > > later will add support for reset request function due to CPU parity
> > > error.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> > > --- /dev/null
> > > +++ b/drivers/watchdog/rzg2l_wdt.c
> 
> > > +struct rzg2l_wdt_priv {
> > > +     void __iomem *base;
> > > +     struct watchdog_device wdev;
> > > +     struct reset_control *rstc;
> > > +     unsigned long osc_clk_rate;
> > > +     unsigned long pclk_rate;
> >
> > pclk_rate is only used in the probe function and thus not needed here.
> 
> Indeed...
OK.
> 
> > > +static int rzg2l_wdt_probe(struct platform_device *pdev) {
> > > +     struct device *dev = &pdev->dev;
> > > +     struct rzg2l_wdt_priv *priv;
> > > +     struct clk *wdt_clk;
> > > +     int ret;
> > > +
> > > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +     if (!priv)
> > > +             return -ENOMEM;
> > > +
> > > +     priv->base = devm_platform_ioremap_resource(pdev, 0);
> > > +     if (IS_ERR(priv->base))
> > > +             return PTR_ERR(priv->base);
> > > +
> > > +     /* Get watchdog main clock */
> > > +     wdt_clk = devm_clk_get(&pdev->dev, "oscclk");
> > > +     if (IS_ERR(wdt_clk))
> > > +             return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no
> > > + oscclk");
> > > +
> > > +     priv->osc_clk_rate = clk_get_rate(wdt_clk);
> > > +     if (!priv->osc_clk_rate)
> > > +             return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate
> > > + is 0");
> > > +
> > > +     /* Get Peripheral clock */
> > > +     wdt_clk = devm_clk_get(&pdev->dev, "pclk");
> > > +     if (IS_ERR(wdt_clk))
> > > +             return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no
> > > + pclk");
> > > +
> > > +     priv->pclk_rate = clk_get_rate(wdt_clk);
> > > +     if (!priv->pclk_rate)
> > > +             return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate
> > > + is 0");
> 
> ... and this can't really happen, can it?

But I need pclk frequency  for delay calculation. That is the reason I am doing a get. Probably after 
Getting the rate, I should do a "put". So that Run time PM will be in full control for the clocks.
Same for oscillator clk. Is it ok?

Regards,
Biju
> 
> So you don't need to get pclk at all.  It will be controlled through
> Runtime PM anyway.
> 
> 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