Quoting Varadarajan Narayanan (2024-03-29 03:48:24) > On Thu, Mar 28, 2024 at 02:54:52PM -0700, Stephen Boyd wrote: > > Quoting Varadarajan Narayanan (2024-03-28 00:59:34) > > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c > > > index 75f09e6e057e..9fa271812373 100644 > > > --- a/drivers/clk/qcom/common.c > > > +++ b/drivers/clk/qcom/common.c > > > @@ -234,6 +236,41 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec, > > > return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL; > > > } > > > > > > +#if IS_ENABLED(CONFIG_INTERCONNECT_CLK) > > > +static int qcom_cc_icc_register(struct device *dev, > > > + const struct qcom_cc_desc *desc) > > > +{ > > > + struct icc_clk_data *icd; > > > + int i; > > > + > > > + if (!desc->icc_hws) > > > + return 0; > > > + > > > + icd = devm_kcalloc(dev, desc->num_icc_hws, sizeof(*icd), GFP_KERNEL); > > > + if (!icd) > > > + return -ENOMEM; > > > + > > > + for (i = 0; i < desc->num_icc_hws; i++) { > > > + icd[i].clk = devm_clk_hw_get_clk(dev, desc->icc_hws[i], "qcom"); > > > > Make the con_id "icc" instead please, so we know the consumer is > > icc_clk. > > Ok. > > > Even better would be for the icc_clk device itself to be the > > one requesting with devm_clk_hw_get_clk() so that we associate the clk > > handle with the consumer device. It would also help us make it so that > > drivers defer probe until their clk isn't an orphan. > > Not sure if I understand the comments correctly. > > In one of the previous patches, had > icd[i].clk = clks[noc_clks[i]]->hw.clk; > > This was said to be error prone since the clock would not be > ref counted. Hence used devm_clk_hw_get_clk before doing > icc_clk_register. > > Now, are you suggesting to use the direct clock pointer > and do a devm_clk_hw_get_clk from the consumer driver? > This will take care of the refcounting. However, we will > have to add these clock entries to the consumer DT node. > Is this ok? Why do they need to be added to the consumer DT node? Why can't the icc_clk device driver (icc_clk_driver?) use struct clk_hw instead of struct clk in struct icc_clk_data? The answer cannot be that the icc_clk driver cannot be changed. > > > diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h > > > index 9c8f7b798d9f..d8ac26d83f3c 100644 > > > --- a/drivers/clk/qcom/common.h > > > +++ b/drivers/clk/qcom/common.h > > > @@ -29,6 +29,9 @@ struct qcom_cc_desc { > > > size_t num_gdscs; > > > struct clk_hw **clk_hws; > > > size_t num_clk_hws; > > > + struct clk_hw **icc_hws; > > > + size_t num_icc_hws; > > > + unsigned int first_id; > > > > 'first_id' is gross. > > will change it to 'icc_id'. That's not what I meant :) The whole concept of having to pick some random number is bad. At the least, hide that in the icc_clk driver so that we don't have to put this in every clk provider that is also an interconnect provider.