Re: [PATCH 2/7] phy: qcom: qmp-pcie: refactor clock register code

[Neil Armstrong <neil.armstrong@xxxxxxxxxx>]

On 19/03/2024 11:50, Dmitry Baryshkov wrote:
> On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@xxxxxxxxxx> wrote:
> > The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock,
> > in order to expose it, split the current clock registering in two parts:
> > - CCF clock registering
> > - DT clock registering
> >
> > Also switch to devm_of_clk_add_hw_provider().
> >
> > Signed-off-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> > ---
> >   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 27 +++++++++++----------------
> >   1 file changed, 11 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > index 8836bb1ff0cc..079b3e306489 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > @@ -3635,11 +3635,6 @@ static int qmp_pcie_clk_init(struct qmp_pcie *qmp)
> >          return devm_clk_bulk_get_optional(dev, num, qmp->clks);
> >   }
> >
> > -static void phy_clk_release_provider(void *res)
> > -{
> > -       of_clk_del_provider(res);
> > -}
> > -
> >   /*
> >    * Register a fixed rate pipe clock.
> >    *
> > @@ -3664,7 +3659,7 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
> >          struct clk_init_data init = { };
> >          int ret;
> >
> > -       ret = of_property_read_string(np, "clock-output-names", &init.name);
> > +       ret = of_property_read_string_index(np, "clock-output-names", 0, &init.name);
> >          if (ret) {
> >                  dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np);
> >                  return ret;
> > @@ -3683,19 +3678,19 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np)
> >
> >          fixed->hw.init = &init;
> >
> > -       ret = devm_clk_hw_register(qmp->dev, &fixed->hw);
> > -       if (ret)
> > -               return ret;
> > +       return devm_clk_hw_register(qmp->dev, &fixed->hw);
> > +}
> >
> > -       ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
> > +static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np)
> > +{
> > +       int ret;
> > +
> > +       ret = phy_pipe_clk_register(qmp, np);
> >          if (ret)
> >                  return ret;
> >
> > -       /*
> > -        * Roll a devm action because the clock provider is the child node, but
> > -        * the child node is not actually a device.
> > -        */
> > -       return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, np);
> > +       return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get,
> > +                                          &qmp->pipe_clk_fixed.hw);
>
> No. The driver has to register a clock provider at the np rather than
> at dev->of_node. Otherwise legacy DT will be broken.

Indeed

Thx
Neil

> >   }
> >
> >   static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np)
> > @@ -3899,7 +3894,7 @@ static int qmp_pcie_probe(struct platform_device *pdev)
> >          if (ret)
> >                  goto err_node_put;
> >
> > -       ret = phy_pipe_clk_register(qmp, np);
> > +       ret = qmp_pcie_register_clocks(qmp, np);
> >          if (ret)
> >                  goto err_node_put;
> >
> >
> > --
> > 2.34.1