| Message ID | 20230815085205.9868-4-quic_luoj@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | add clock controller of qca8386/qca8084 | expand |
On Tue, Aug 15, 2023 at 04:52:04PM +0800, Luo Jie wrote: > Add the common function _qcom_cc_really_probe, which takes > struct device as parameter. This commit message completely fails to describe the problem/issue the change is solving. So when we look back in the git history, there will be no indication of why things looks like they do. > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com> > --- > drivers/clk/qcom/common.c | 10 ++++++++-- > drivers/clk/qcom/common.h | 2 ++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c > index 75f09e6e057e..4cbdbfb65606 100644 > --- a/drivers/clk/qcom/common.c > +++ b/drivers/clk/qcom/common.c > @@ -234,11 +234,10 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec, > return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL; > } > > -int qcom_cc_really_probe(struct platform_device *pdev, > +int _qcom_cc_really_probe(struct device *dev, > const struct qcom_cc_desc *desc, struct regmap *regmap) > { > int i, ret; > - struct device *dev = &pdev->dev; > struct qcom_reset_controller *reset; > struct qcom_cc *cc; > struct gdsc_desc *scd; > @@ -305,6 +304,13 @@ int qcom_cc_really_probe(struct platform_device *pdev, > > return 0; > } > +EXPORT_SYMBOL_GPL(_qcom_cc_really_probe); > + > +int qcom_cc_really_probe(struct platform_device *pdev, > + const struct qcom_cc_desc *desc, struct regmap *regmap) Why do we want to keep this wrapper around? PS. Please give some time before posting v5, I would like to understand the MDIO regmap operations in patch 4 better before commenting on it. Regards, Bjorn > +{ > + return _qcom_cc_really_probe(&pdev->dev, desc, regmap); > +} > EXPORT_SYMBOL_GPL(qcom_cc_really_probe); > > int qcom_cc_probe(struct platform_device *pdev, const struct qcom_cc_desc *desc) > diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h > index 9c8f7b798d9f..9710ade9bf15 100644 > --- a/drivers/clk/qcom/common.h > +++ b/drivers/clk/qcom/common.h > @@ -58,6 +58,8 @@ extern int qcom_cc_register_sleep_clk(struct device *dev); > > extern struct regmap *qcom_cc_map(struct platform_device *pdev, > const struct qcom_cc_desc *desc); > +extern int _qcom_cc_really_probe(struct device *dev, > + const struct qcom_cc_desc *desc, struct regmap *regmap); > extern int qcom_cc_really_probe(struct platform_device *pdev, > const struct qcom_cc_desc *desc, > struct regmap *regmap); > -- > 2.17.1 >
On 8/18/2023 11:14 AM, Bjorn Andersson wrote: > On Tue, Aug 15, 2023 at 04:52:04PM +0800, Luo Jie wrote: >> Add the common function _qcom_cc_really_probe, which takes >> struct device as parameter. > > This commit message completely fails to describe the problem/issue the > change is solving. So when we look back in the git history, there will > be no indication of why things looks like they do. > Thanks for the comments, i will update the commit message in detail on the issue resolved in the next patch set. >> >> Signed-off-by: Luo Jie <quic_luoj@quicinc.com> >> --- >> drivers/clk/qcom/common.c | 10 ++++++++-- >> drivers/clk/qcom/common.h | 2 ++ >> 2 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c >> index 75f09e6e057e..4cbdbfb65606 100644 >> --- a/drivers/clk/qcom/common.c >> +++ b/drivers/clk/qcom/common.c >> @@ -234,11 +234,10 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec, >> return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL; >> } >> >> -int qcom_cc_really_probe(struct platform_device *pdev, >> +int _qcom_cc_really_probe(struct device *dev, >> const struct qcom_cc_desc *desc, struct regmap *regmap) >> { >> int i, ret; >> - struct device *dev = &pdev->dev; >> struct qcom_reset_controller *reset; >> struct qcom_cc *cc; >> struct gdsc_desc *scd; >> @@ -305,6 +304,13 @@ int qcom_cc_really_probe(struct platform_device *pdev, >> >> return 0; >> } >> +EXPORT_SYMBOL_GPL(_qcom_cc_really_probe); >> + >> +int qcom_cc_really_probe(struct platform_device *pdev, >> + const struct qcom_cc_desc *desc, struct regmap *regmap) > > Why do we want to keep this wrapper around? > There are many existed clock controller drivers using this wrapper qcom_cc_really_probe, so i still keep this wrapper. do we need to remove this wrapper and update the existed drivers to use _qcom_cc_really_probe? > > PS. Please give some time before posting v5, I would like to understand > the MDIO regmap operations in patch 4 better before commenting on it. > > Regards, > Bjorn > Sure, the MDIO read/write operations need to check whether the MDIO bus is in busy state, the poll and sleep are taken to check. Thanks Bjorn for your time and review comments. >> +{ >> + return _qcom_cc_really_probe(&pdev->dev, desc, regmap); >> +} >> EXPORT_SYMBOL_GPL(qcom_cc_really_probe); >> >> int qcom_cc_probe(struct platform_device *pdev, const struct qcom_cc_desc *desc) >> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h >> index 9c8f7b798d9f..9710ade9bf15 100644 >> --- a/drivers/clk/qcom/common.h >> +++ b/drivers/clk/qcom/common.h >> @@ -58,6 +58,8 @@ extern int qcom_cc_register_sleep_clk(struct device *dev); >> >> extern struct regmap *qcom_cc_map(struct platform_device *pdev, >> const struct qcom_cc_desc *desc); >> +extern int _qcom_cc_really_probe(struct device *dev, >> + const struct qcom_cc_desc *desc, struct regmap *regmap); >> extern int qcom_cc_really_probe(struct platform_device *pdev, >> const struct qcom_cc_desc *desc, >> struct regmap *regmap); >> -- >> 2.17.1 >>
On Fri, Aug 18, 2023 at 04:35:52PM +0800, Jie Luo wrote: > On 8/18/2023 11:14 AM, Bjorn Andersson wrote: > > On Tue, Aug 15, 2023 at 04:52:04PM +0800, Luo Jie wrote: [..] > > > +int qcom_cc_really_probe(struct platform_device *pdev, > > > + const struct qcom_cc_desc *desc, struct regmap *regmap) > > > > Why do we want to keep this wrapper around? > > > There are many existed clock controller drivers using this wrapper > qcom_cc_really_probe, so i still keep this wrapper. > > do we need to remove this wrapper and update the existed drivers to use > _qcom_cc_really_probe? Yes please. The additional API does not add value, but can be confusing, so let's invest the extra time in fixing up all the drivers to keep the interface clean. Regards, Bjorn
On 8/19/2023 10:54 AM, Bjorn Andersson wrote: > On Fri, Aug 18, 2023 at 04:35:52PM +0800, Jie Luo wrote: >> On 8/18/2023 11:14 AM, Bjorn Andersson wrote: >>> On Tue, Aug 15, 2023 at 04:52:04PM +0800, Luo Jie wrote: > [..] >>>> +int qcom_cc_really_probe(struct platform_device *pdev, >>>> + const struct qcom_cc_desc *desc, struct regmap *regmap) >>> >>> Why do we want to keep this wrapper around? >>> >> There are many existed clock controller drivers using this wrapper >> qcom_cc_really_probe, so i still keep this wrapper. >> >> do we need to remove this wrapper and update the existed drivers to use >> _qcom_cc_really_probe? > > Yes please. The additional API does not add value, but can be confusing, > so let's invest the extra time in fixing up all the drivers to keep the > interface clean. > > Regards, > Bjorn Thanks Bjorn for the suggestion, will update this patch in the next version.
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c index 75f09e6e057e..4cbdbfb65606 100644 --- a/drivers/clk/qcom/common.c +++ b/drivers/clk/qcom/common.c @@ -234,11 +234,10 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec, return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL; } -int qcom_cc_really_probe(struct platform_device *pdev, +int _qcom_cc_really_probe(struct device *dev, const struct qcom_cc_desc *desc, struct regmap *regmap) { int i, ret; - struct device *dev = &pdev->dev; struct qcom_reset_controller *reset; struct qcom_cc *cc; struct gdsc_desc *scd; @@ -305,6 +304,13 @@ int qcom_cc_really_probe(struct platform_device *pdev, return 0; } +EXPORT_SYMBOL_GPL(_qcom_cc_really_probe); + +int qcom_cc_really_probe(struct platform_device *pdev, + const struct qcom_cc_desc *desc, struct regmap *regmap) +{ + return _qcom_cc_really_probe(&pdev->dev, desc, regmap); +} EXPORT_SYMBOL_GPL(qcom_cc_really_probe); int qcom_cc_probe(struct platform_device *pdev, const struct qcom_cc_desc *desc) diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h index 9c8f7b798d9f..9710ade9bf15 100644 --- a/drivers/clk/qcom/common.h +++ b/drivers/clk/qcom/common.h @@ -58,6 +58,8 @@ extern int qcom_cc_register_sleep_clk(struct device *dev); extern struct regmap *qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc); +extern int _qcom_cc_really_probe(struct device *dev, + const struct qcom_cc_desc *desc, struct regmap *regmap); extern int qcom_cc_really_probe(struct platform_device *pdev, const struct qcom_cc_desc *desc, struct regmap *regmap);
Add the common function _qcom_cc_really_probe, which takes struct device as parameter. Signed-off-by: Luo Jie <quic_luoj@quicinc.com> --- drivers/clk/qcom/common.c | 10 ++++++++-- drivers/clk/qcom/common.h | 2 ++ 2 files changed, 10 insertions(+), 2 deletions(-)