| Message ID | 20231231-hfpll-yaml-v1-2-359d44a4e194@z3ntu.xyz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert qcom,hfpll documentation to yaml + related changes | expand |
On 31/12/2023 15:48, Luca Weiss wrote: > It doesn't appear that the configuration is for the HFPLL is generic, so That's ok... > add a qcs404-specific compatible and rename the existing struct to but why this is the solution? If the qcom,hfpll compatible was deprecated, but it is not. This commit is contradictory to the bindings. > qcs404. > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz> > --- > drivers/clk/qcom/hfpll.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c > index dac27e31ef60..5b12982519be 100644 > --- a/drivers/clk/qcom/hfpll.c > +++ b/drivers/clk/qcom/hfpll.c > @@ -14,7 +14,7 @@ > #include "clk-regmap.h" > #include "clk-hfpll.h" > > -static const struct hfpll_data hdata = { > +static const struct hfpll_data qcs404 = { > .mode_reg = 0x00, > .l_reg = 0x04, > .m_reg = 0x08, > @@ -84,10 +84,12 @@ static const struct hfpll_data msm8976_cci = { > }; > > static const struct of_device_id qcom_hfpll_match_table[] = { > - { .compatible = "qcom,hfpll", .data = &hdata }, > { .compatible = "qcom,msm8976-hfpll-a53", .data = &msm8976_a53 }, > { .compatible = "qcom,msm8976-hfpll-a72", .data = &msm8976_a72 }, > { .compatible = "qcom,msm8976-hfpll-cci", .data = &msm8976_cci }, > + { .compatible = "qcom,qcs404-hfpll", .data = &qcs404 }, > + /* deprecated, use SoC-specific compatible */ Why? That's not a deprecated compatible. You now expect to create many unnecessary entries, which is not really needed. This is opposite of what we try to achieve with compatibility lists. Best regards, Krzysztof
On Dienstag, 2. Jänner 2024 11:41:26 CET Krzysztof Kozlowski wrote: > On 31/12/2023 15:48, Luca Weiss wrote: > > It doesn't appear that the configuration is for the HFPLL is generic, so > > That's ok... > > > add a qcs404-specific compatible and rename the existing struct to > > but why this is the solution? If the qcom,hfpll compatible was > deprecated, but it is not. This commit is contradictory to the bindings. > > > qcs404. > > > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz> > > --- > > > > drivers/clk/qcom/hfpll.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c > > index dac27e31ef60..5b12982519be 100644 > > --- a/drivers/clk/qcom/hfpll.c > > +++ b/drivers/clk/qcom/hfpll.c > > @@ -14,7 +14,7 @@ > > > > #include "clk-regmap.h" > > #include "clk-hfpll.h" > > > > -static const struct hfpll_data hdata = { > > +static const struct hfpll_data qcs404 = { > > > > .mode_reg = 0x00, > > .l_reg = 0x04, > > .m_reg = 0x08, > > > > @@ -84,10 +84,12 @@ static const struct hfpll_data msm8976_cci = { > > > > }; > > > > static const struct of_device_id qcom_hfpll_match_table[] = { > > > > - { .compatible = "qcom,hfpll", .data = &hdata }, > > > > { .compatible = "qcom,msm8976-hfpll-a53", .data = &msm8976_a53 }, > > { .compatible = "qcom,msm8976-hfpll-a72", .data = &msm8976_a72 }, > > { .compatible = "qcom,msm8976-hfpll-cci", .data = &msm8976_cci }, > > > > + { .compatible = "qcom,qcs404-hfpll", .data = &qcs404 }, > > + /* deprecated, use SoC-specific compatible */ > > Why? That's not a deprecated compatible. You now expect to create many > unnecessary entries, which is not really needed. This is opposite of > what we try to achieve with compatibility lists. Just "qcom,hfpll" is not allowed by the bindings. The problem is that it's actually unclear to me what "qcom,hfpll" was supposed to be currently. It was added originally for MSM8974 and friends (see git log) but then is currently only used by QCS404 while in QCS404 downstream msm-4.4 (I think it was 4.4) I see different driver data than what's here. So I wanted to just move what's used here to be qcs404-specific and then in an upcoming patch add a msm8974-specific compatible with different driver data. Also wouldn't the "don't extend driver lists when not neccessary" mean using something like "qcom,msm1234-hfpll", "qcom,qcs404-hfpll", "qcom,hfpll" then? That was kind of my idea if some other SoC can reuse e.g. qcs404 data? Regards Luca > > Best regards, > Krzysztof
On 06/01/2024 11:19, Luca Weiss wrote: > On Dienstag, 2. Jänner 2024 11:41:26 CET Krzysztof Kozlowski wrote: >> On 31/12/2023 15:48, Luca Weiss wrote: >>> It doesn't appear that the configuration is for the HFPLL is generic, so >> >> That's ok... >> >>> add a qcs404-specific compatible and rename the existing struct to >> >> but why this is the solution? If the qcom,hfpll compatible was >> deprecated, but it is not. This commit is contradictory to the bindings. >> >>> qcs404. >>> >>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz> >>> --- >>> >>> drivers/clk/qcom/hfpll.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c >>> index dac27e31ef60..5b12982519be 100644 >>> --- a/drivers/clk/qcom/hfpll.c >>> +++ b/drivers/clk/qcom/hfpll.c >>> @@ -14,7 +14,7 @@ >>> >>> #include "clk-regmap.h" >>> #include "clk-hfpll.h" >>> >>> -static const struct hfpll_data hdata = { >>> +static const struct hfpll_data qcs404 = { >>> >>> .mode_reg = 0x00, >>> .l_reg = 0x04, >>> .m_reg = 0x08, >>> >>> @@ -84,10 +84,12 @@ static const struct hfpll_data msm8976_cci = { >>> >>> }; >>> >>> static const struct of_device_id qcom_hfpll_match_table[] = { >>> >>> - { .compatible = "qcom,hfpll", .data = &hdata }, >>> >>> { .compatible = "qcom,msm8976-hfpll-a53", .data = &msm8976_a53 }, >>> { .compatible = "qcom,msm8976-hfpll-a72", .data = &msm8976_a72 }, >>> { .compatible = "qcom,msm8976-hfpll-cci", .data = &msm8976_cci }, >>> >>> + { .compatible = "qcom,qcs404-hfpll", .data = &qcs404 }, >>> + /* deprecated, use SoC-specific compatible */ >> >> Why? That's not a deprecated compatible. You now expect to create many >> unnecessary entries, which is not really needed. This is opposite of >> what we try to achieve with compatibility lists. > > Just "qcom,hfpll" is not allowed by the bindings. Okay... sentence is correct but how is it related to the driver? > The problem is that it's actually unclear to me what "qcom,hfpll" was supposed > to be currently. It was added originally for MSM8974 and friends (see git log) > but then is currently only used by QCS404 while in QCS404 downstream msm-4.4 > (I think it was 4.4) I see different driver data than what's here. I discourage from using generic compatibles, because their meaning is too often fluid, but if we already have it then: it is supposed to be whatever driver and bindings defined it when they were added. > > So I wanted to just move what's used here to be qcs404-specific and then in an > upcoming patch add a msm8974-specific compatible with different driver data. > > Also wouldn't the "don't extend driver lists when not neccessary" mean using > something like "qcom,msm1234-hfpll", "qcom,qcs404-hfpll", "qcom,hfpll" then? qcs404 and hfpll are the same aren't they? Then why would third compatible appear? > That was kind of my idea if some other SoC can reuse e.g. qcs404 data? If any other SoC wants to reuse qcs404, why that SoC cannot use hfpll? If hfpll compatible is not correct, it should be deprecated, which is not happening in this patchset. Best regards, Krzysztof
diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c index dac27e31ef60..5b12982519be 100644 --- a/drivers/clk/qcom/hfpll.c +++ b/drivers/clk/qcom/hfpll.c @@ -14,7 +14,7 @@ #include "clk-regmap.h" #include "clk-hfpll.h" -static const struct hfpll_data hdata = { +static const struct hfpll_data qcs404 = { .mode_reg = 0x00, .l_reg = 0x04, .m_reg = 0x08, @@ -84,10 +84,12 @@ static const struct hfpll_data msm8976_cci = { }; static const struct of_device_id qcom_hfpll_match_table[] = { - { .compatible = "qcom,hfpll", .data = &hdata }, { .compatible = "qcom,msm8976-hfpll-a53", .data = &msm8976_a53 }, { .compatible = "qcom,msm8976-hfpll-a72", .data = &msm8976_a72 }, { .compatible = "qcom,msm8976-hfpll-cci", .data = &msm8976_cci }, + { .compatible = "qcom,qcs404-hfpll", .data = &qcs404 }, + /* deprecated, use SoC-specific compatible */ + { .compatible = "qcom,hfpll", .data = &qcs404 }, { } }; MODULE_DEVICE_TABLE(of, qcom_hfpll_match_table);
It doesn't appear that the configuration is for the HFPLL is generic, so add a qcs404-specific compatible and rename the existing struct to qcs404. Signed-off-by: Luca Weiss <luca@z3ntu.xyz> --- drivers/clk/qcom/hfpll.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)