Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220122091731.283592-10-wenst@chromium.org> <20220126060449.24874-1-miles.chen@mediatek.com>
In-Reply-To: <20220126060449.24874-1-miles.chen@mediatek.com>
From:   Chen-Yu Tsai <wenst@chromium.org>
Date:   Wed, 26 Jan 2022 14:18:06 +0800
Message-ID: <CAGXv+5GKmOrT=yOkadsKVz38vvAj1-1sBN8i86zMLv3yusZeQw@mail.gmail.com>
Subject: Re: [PATCH 13/31] clk: mediatek: pll: Implement unregister API
To:     Miles Chen <miles.chen@mediatek.com>
Cc:     chun-jie.chen@mediatek.com, linux-arm-kernel@lists.infradead.org,
        linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mediatek@lists.infradead.org, matthias.bgg@gmail.com,
        mturquette@baylibre.com, sboyd@kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 26, 2022 at 2:04 PM Miles Chen <miles.chen@mediatek.com> wrote:
>
> > +static void mtk_clk_unregister_pll(struct clk *clk)
> > +{
> > +     struct clk_hw *hw = __clk_get_hw(clk);
> > +     struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> > +
> > +     clk_unregister(clk);
> > +     kfree(pll);
> > +}
> > +
>
> mtk_clk_unregister_pll() looks different.
> Do we need to check hw before passing it to to_mtk_clk_pll(hw), like
> mtk_clk_unregister_mux()?

Good catch.

In theory we should, since __get_clk_hw() would return NULL if clk is NULL.
However the code already does that check before it calls
mtk_clk_unregister_pll(), so the code is safe. We should make everything
consistent though.

I might have written the code on separate days and thus introduced some
discrepancies. I'll fix it in v2.


ChenYu

> >  void mtk_clk_register_plls(struct device_node *node,
> >               const struct mtk_pll_data *plls, int num_plls, struct clk_onecell_data *clk_data)
> >  {
> > @@ -388,4 +397,44 @@ void mtk_clk_register_plls(struct device_node *node,
> >  }
> >  EXPORT_SYMBOL_GPL(mtk_clk_register_plls);
> >
> > +static __iomem void *mtk_clk_pll_get_base(struct clk *clk,
> > +                                       const struct mtk_pll_data *data)
> > +{
> > +     struct clk_hw *hw = __clk_get_hw(clk);
> > +     struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> > +
> > +     return pll->base_addr - data->reg;
> > +}
> > +
> > +void mtk_clk_unregister_plls(const struct mtk_pll_data *plls, int num_plls,
> > +                          struct clk_onecell_data *clk_data)
> > +{
> > +     __iomem void *base = NULL;
> > +     int i;
> > +
> > +     if (!clk_data)
> > +             return;
> > +
> > +     for (i = num_plls; i > 0; i--) {
> > +             const struct mtk_pll_data *pll = &plls[i - 1];
> > +
> > +             if (IS_ERR_OR_NULL(clk_data->clks[pll->id]))
> > +                     continue;
> > +
> > +             /*
> > +              * This is quite ugly but unfortunately the clks don't have
> > +              * any device tied to them, so there's no place to store the
> > +              * pointer to the I/O region base address. We have to fetch
> > +              * it from one of the registered clks.
> > +              */
> > +             base = mtk_clk_pll_get_base(clk_data->clks[pll->id], pll);
> > +
> > +             mtk_clk_unregister_pll(clk_data->clks[pll->id]);
> > +             clk_data->clks[pll->id] = ERR_PTR(-ENOENT);
> > +     }
> > +
> > +     iounmap(base);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_clk_unregister_plls);
> > +
> >  MODULE_LICENSE("GPL");
> > diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
> > index d01b0c38311d..a889b1e472e7 100644
> > --- a/drivers/clk/mediatek/clk-pll.h
> > +++ b/drivers/clk/mediatek/clk-pll.h
> > @@ -51,5 +51,7 @@ struct mtk_pll_data {
> >  void mtk_clk_register_plls(struct device_node *node,
> >                          const struct mtk_pll_data *plls, int num_plls,
> >                          struct clk_onecell_data *clk_data);
> > +void mtk_clk_unregister_plls(const struct mtk_pll_data *plls, int num_plls,
> > +                          struct clk_onecell_data *clk_data);
> >
> >  #endif /* __DRV_CLK_MTK_PLL_H */
> > --
> > 2.35.0.rc0.227.g00780c9af4-goog
>
>