Re: [PATCH 2/3] clk: renesas: rcar-usb2-clock-sel: Add multiple clocks management

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Shimoda-san,

On Thu, Oct 24, 2019 at 1:17 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> This hardware needs to enable clocks of both host and peripheral.
> So, this patch adds multiple clocks management.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c
> +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c
> @@ -53,14 +60,32 @@ static void usb2_clock_sel_disable_extal_only(struct usb2_clock_sel_priv *priv)
>
>  static int usb2_clock_sel_enable(struct clk_hw *hw)
>  {
> -       usb2_clock_sel_enable_extal_only(to_priv(hw));
> +       struct usb2_clock_sel_priv *priv = to_priv(hw);
> +       int i, ret;
> +
> +       for (i = 0; i < CLK_NUM; i++) {
> +               ret = clk_prepare_enable(priv->clks[i]);
> +               if (ret) {
> +                       while (--i >= 0)
> +                               clk_disable_unprepare(priv->clks[i]);
> +                       return ret;
> +               }
> +       }

You can use the clk_bulk_* APIs, instead of open-coding the same
operation.

> @@ -131,6 +156,14 @@ static int rcar_usb2_clock_sel_probe(struct platform_device *pdev)
>         pm_runtime_enable(dev);
>         pm_runtime_get_sync(dev);

pm_runtime_get_sync() will have already enabled the first module clock listed in
the DT "clocks" property.

If you want the driver to manage all clocks itself, perhaps the PM Runtime
calls should be dropped?

> +       priv->clks[CLK_INDEX_EHCI_OHCI] = devm_clk_get(dev, "ehci_ohci");
> +       if (IS_ERR(priv->clks[CLK_INDEX_EHCI_OHCI]))
> +               return PTR_ERR(priv->clks[CLK_INDEX_EHCI_OHCI]);
> +
> +       priv->clks[CLK_INDEX_HS_USB] = devm_clk_get(dev, "hs-usb-if");
> +       if (IS_ERR(priv->clks[CLK_INDEX_HS_USB]))
> +               return PTR_ERR(priv->clks[CLK_INDEX_HS_USB]);
> +
>         clk = devm_clk_get(dev, "usb_extal");
>         if (!IS_ERR(clk) && !clk_prepare_enable(clk)) {
>                 priv->extal = !!clk_get_rate(clk);

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