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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>
> > > --- /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
