Re: [PATCH 5/9] drm/msm/dsi_phy_28nm_8960: Replace parent names with clk_hw pointers

On Tue, 24 May 2022 at 00:38, Marijn Suijten marijn.suijten@somainline.org wrote:

parent_hw pointers are easier to manage and cheaper to use than repeatedly formatting the parent name and subsequently leaving the clk framework to perform lookups based on that name.

Can you please add a followup patch (or a preface one) removing the rest of devm_kzalloc()'ed clock names.

Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

Minor nit below.

Signed-off-by: Marijn Suijten marijn.suijten@somainline.org

.../gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c index fc56cdcc9ad6..943a7e847c90 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c @@ -383,7 +383,7 @@ static int dsi_28nm_pll_restore_state(struct msm_dsi_phy *phy)

static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm, struct clk_hw **provided_clocks) {

•   char *clk_name, *parent_name, *vco_name;
  

•   char *clk_name, *vco_name;
    struct clk_init_data vco_init = {
            .parent_data = &(const struct clk_parent_data) {
                    .fw_name = "ref",
  

@@ -408,10 +408,6 @@ static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm, struct clk_hw **prov if (!vco_name) return -ENOMEM;

•   parent_name = devm_kzalloc(dev, 32, GFP_KERNEL);
  

•   if (!parent_name)
  

•           return -ENOMEM;
  

•   clk_name = devm_kzalloc(dev, 32, GFP_KERNEL);
    if (!clk_name)
            return -ENOMEM;
  

@@ -429,13 +425,14 @@ static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm, struct clk_hw **prov bytediv->hw.init = &bytediv_init; bytediv->reg = pll_28nm->phy->pll_base + REG_DSI_28nm_8960_PHY_PLL_CTRL_9;

•   snprintf(parent_name, 32, "dsi%dvco_clk", pll_28nm->phy->id);
    snprintf(clk_name, 32, "dsi%dpllbyte", pll_28nm->phy->id + 1);
  
    bytediv_init.name = clk_name;
    bytediv_init.ops = &clk_bytediv_ops;
    bytediv_init.flags = CLK_SET_RATE_PARENT;
  

•   bytediv_init.parent_names = (const char * const *) &parent_name;
  

•   bytediv_init.parent_hws = (const struct clk_hw*[]){
  

•           &pll_28nm->clk_hw,
  

•   };
    bytediv_init.num_parents = 1;
  

I wonder if we can express the bytediv clock with the standard ops. However it's definitely a separate topic.

    /* DIV2 */

@@ -446,10 +443,9 @@ static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm, struct clk_hw **prov

    snprintf(clk_name, 32, "dsi%dpll", pll_28nm->phy->id + 1);
    /* DIV3 */

•   hw = devm_clk_hw_register_divider(dev, clk_name,
  

•                           parent_name, 0, pll_28nm->phy->pll_base +
  

•                           REG_DSI_28nm_8960_PHY_PLL_CTRL_10,
  

•                           0, 8, 0, NULL);
  

•   hw = devm_clk_hw_register_divider_parent_hw(dev, clk_name,
  

•                   &pll_28nm->clk_hw, 0, pll_28nm->phy->pll_base +
  

•                   REG_DSI_28nm_8960_PHY_PLL_CTRL_10, 0, 8, 0, NULL);
  

Again, could you please keep the linebreak in place?

    if (IS_ERR(hw))
            return PTR_ERR(hw);
    provided_clocks[DSI_PIXEL_PLL_CLK] = hw;

-- 2.36.1