| Message ID | 20240321-apss-ipq-pll-cleanup-v2-3-201f3cf79fd4@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clk: qcom: apss-ipq-pll: various cleanups | expand |
On Thu, 21 Mar 2024 at 09:50, Gabor Juhos <j4g8y7@gmail.com> wrote: > > The value of the 'pll_type' field of 'struct apps_pll_data' > is used only by the probe function to decide which config > function should be called for the actual PLL. However this > can be derived also from the 'pll' field which makes the > 'pll_type' field redundant. > > Additionally, the CLK_ALPHA_PLL_TYPE_* enumeration values > are meant to be used as indices to the 'clk_alpha_pll_regs' > array so using those to define the pll type in this driver > is misleading anyway. > > Change the probe function to use the 'pll' field to determine > the configuration function to be used, and remove the > 'pll_type' field to simplify the code. I can't fully appreciate this idea. There can be cases when different PLL types share the set of ops. I think having a type is more versatile and makes the code more obvious. > > No functional changes. > > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> > --- > Changes in v2: > - no changes > - Link to v1: https://lore.kernel.org/r/20240318-apss-ipq-pll-cleanup-v1-3-52f795429d5d@gmail.com > --- > drivers/clk/qcom/apss-ipq-pll.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c > index 8cf17374a2e2a..816b0d1f8d8c8 100644 > --- a/drivers/clk/qcom/apss-ipq-pll.c > +++ b/drivers/clk/qcom/apss-ipq-pll.c > @@ -131,37 +131,31 @@ static const struct alpha_pll_config ipq9574_pll_config = { > }; > > struct apss_pll_data { > - int pll_type; > struct clk_alpha_pll *pll; > const struct alpha_pll_config *pll_config; > }; > > static const struct apss_pll_data ipq5018_pll_data = { > - .pll_type = CLK_ALPHA_PLL_TYPE_STROMER, > .pll = &ipq_pll_stromer, > .pll_config = &ipq5018_pll_config, > }; > > static struct apss_pll_data ipq5332_pll_data = { > - .pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS, > .pll = &ipq_pll_stromer_plus, > .pll_config = &ipq5332_pll_config, > }; > > static struct apss_pll_data ipq8074_pll_data = { > - .pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA, > .pll = &ipq_pll_huayra, > .pll_config = &ipq8074_pll_config, > }; > > static struct apss_pll_data ipq6018_pll_data = { > - .pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA, > .pll = &ipq_pll_huayra, > .pll_config = &ipq6018_pll_config, > }; > > static struct apss_pll_data ipq9574_pll_data = { > - .pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA, > .pll = &ipq_pll_huayra, > .pll_config = &ipq9574_pll_config, > }; > @@ -194,10 +188,10 @@ static int apss_ipq_pll_probe(struct platform_device *pdev) > if (!data) > return -ENODEV; > > - if (data->pll_type == CLK_ALPHA_PLL_TYPE_HUAYRA) > + if (data->pll == &ipq_pll_huayra) > clk_alpha_pll_configure(data->pll, regmap, data->pll_config); > - else if (data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER || > - data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER_PLUS) > + else if (data->pll == &ipq_pll_stromer || > + data->pll == &ipq_pll_stromer_plus) > clk_stromer_pll_configure(data->pll, regmap, data->pll_config); > > ret = devm_clk_register_regmap(dev, &data->pll->clkr); > > -- > 2.44.0 >
2024. 03. 21. 11:37 keltezéssel, Dmitry Baryshkov írta: > On Thu, 21 Mar 2024 at 09:50, Gabor Juhos <j4g8y7@gmail.com> wrote: >> >> The value of the 'pll_type' field of 'struct apps_pll_data' >> is used only by the probe function to decide which config >> function should be called for the actual PLL. However this >> can be derived also from the 'pll' field which makes the >> 'pll_type' field redundant. >> >> Additionally, the CLK_ALPHA_PLL_TYPE_* enumeration values >> are meant to be used as indices to the 'clk_alpha_pll_regs' >> array so using those to define the pll type in this driver >> is misleading anyway. >> >> Change the probe function to use the 'pll' field to determine >> the configuration function to be used, and remove the >> 'pll_type' field to simplify the code. > > I can't fully appreciate this idea. There can be cases when different > PLL types share the set of ops. I think having a type is more > versatile and makes the code more obvious. I understand your concerns, but let me explain the reasons about why I have choosed this implementation in more detail. The driver declares three distinct clocks for the three different PLL types it supports. Each one of these clocks are using different register maps and clock operations which in a whole uniquely identifies the type of the PLL. In contrary to this, the CLK_ALPHA_PLL_TYPE_* values assigned to 'pll_type' are only indicating that which register map should be used for the given PLL. However this is also specified indirectly through the 'regs' member of the clocks so this is a redundant information. Additionally, using the CLK_ALPHA_PLL_TYPE_* for anything other than for specifying the register map is misleading. For example, here are some snippets from the driver before the patch: static struct clk_alpha_pll ipq_pll_stromer_plus = { ... .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_STROMER], ... static struct apss_pll_data ipq5332_pll_data = { .pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS, .pll = &ipq_pll_stromer_plus, ... Since it is not obvious at a first glance, one could ask that why the two CLK_ALPHA_PLL_TYPE_* values are different? Although my opinion that it is redundant still stand, but I'm not against keeping the pll_type. However if we keep that, then at least we should use private enums (IPQ_APSS_PLL_TYPE_* or similar) for that in order to make it more obvious that it means a different thing than the CLK_ALPHA_PLL_TYPE_* values. This solution would be more acceptable? Regards, Gabor
On Fri, 22 Mar 2024 at 22:59, Gabor Juhos <j4g8y7@gmail.com> wrote: > > 2024. 03. 21. 11:37 keltezéssel, Dmitry Baryshkov írta: > > On Thu, 21 Mar 2024 at 09:50, Gabor Juhos <j4g8y7@gmail.com> wrote: > >> > >> The value of the 'pll_type' field of 'struct apps_pll_data' > >> is used only by the probe function to decide which config > >> function should be called for the actual PLL. However this > >> can be derived also from the 'pll' field which makes the > >> 'pll_type' field redundant. > >> > >> Additionally, the CLK_ALPHA_PLL_TYPE_* enumeration values > >> are meant to be used as indices to the 'clk_alpha_pll_regs' > >> array so using those to define the pll type in this driver > >> is misleading anyway. > >> > >> Change the probe function to use the 'pll' field to determine > >> the configuration function to be used, and remove the > >> 'pll_type' field to simplify the code. > > > > I can't fully appreciate this idea. There can be cases when different > > PLL types share the set of ops. I think having a type is more > > versatile and makes the code more obvious. > > I understand your concerns, but let me explain the reasons about why I have > choosed this implementation in more detail. > > The driver declares three distinct clocks for the three different PLL types it > supports. Each one of these clocks are using different register maps and clock > operations which in a whole uniquely identifies the type of the PLL. In contrary > to this, the CLK_ALPHA_PLL_TYPE_* values assigned to 'pll_type' are only > indicating that which register map should be used for the given PLL. However > this is also specified indirectly through the 'regs' member of the clocks so > this is a redundant information. > > Additionally, using the CLK_ALPHA_PLL_TYPE_* for anything other than for > specifying the register map is misleading. For example, here are some snippets > from the driver before the patch: > > static struct clk_alpha_pll ipq_pll_stromer_plus = { > ... > .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_STROMER], > ... > > static struct apss_pll_data ipq5332_pll_data = { > .pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS, > .pll = &ipq_pll_stromer_plus, > ... > > Since it is not obvious at a first glance, one could ask that why the two > CLK_ALPHA_PLL_TYPE_* values are different? > > Although my opinion that it is redundant still stand, but I'm not against > keeping the pll_type. However if we keep that, then at least we should use > private enums (IPQ_APSS_PLL_TYPE_* or similar) for that in order to make it more > obvious that it means a different thing than the CLK_ALPHA_PLL_TYPE_* values. > > This solution would be more acceptable? This looks like a slight overkill, but yes, it is more acceptable. The logic should be type => functions, not the other way around.
2024. 03. 22. 23:33 keltezéssel, Dmitry Baryshkov írta: ... >> Although my opinion that it is redundant still stand, but I'm not against >> keeping the pll_type. However if we keep that, then at least we should use >> private enums (IPQ_APSS_PLL_TYPE_* or similar) for that in order to make it more >> obvious that it means a different thing than the CLK_ALPHA_PLL_TYPE_* values. >> >> This solution would be more acceptable? > > This looks like a slight overkill, but yes, it is more acceptable. The > logic should be type => functions, not the other way around. > > If that is overkill, it does not worth the change. I will drop the patch and send an updated series. Regards, Gabor
On Sun, 24 Mar 2024 at 09:29, Gabor Juhos <j4g8y7@gmail.com> wrote: > > 2024. 03. 22. 23:33 keltezéssel, Dmitry Baryshkov írta: > > ... > > >> Although my opinion that it is redundant still stand, but I'm not against > >> keeping the pll_type. However if we keep that, then at least we should use > >> private enums (IPQ_APSS_PLL_TYPE_* or similar) for that in order to make it more > >> obvious that it means a different thing than the CLK_ALPHA_PLL_TYPE_* values. > >> > >> This solution would be more acceptable? > > > > This looks like a slight overkill, but yes, it is more acceptable. The > > logic should be type => functions, not the other way around. > > > > > > If that is overkill, it does not worth the change. I will drop the patch and > send an updated series. Either way is fine to me.
diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c index 8cf17374a2e2a..816b0d1f8d8c8 100644 --- a/drivers/clk/qcom/apss-ipq-pll.c +++ b/drivers/clk/qcom/apss-ipq-pll.c @@ -131,37 +131,31 @@ static const struct alpha_pll_config ipq9574_pll_config = { }; struct apss_pll_data { - int pll_type; struct clk_alpha_pll *pll; const struct alpha_pll_config *pll_config; }; static const struct apss_pll_data ipq5018_pll_data = { - .pll_type = CLK_ALPHA_PLL_TYPE_STROMER, .pll = &ipq_pll_stromer, .pll_config = &ipq5018_pll_config, }; static struct apss_pll_data ipq5332_pll_data = { - .pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS, .pll = &ipq_pll_stromer_plus, .pll_config = &ipq5332_pll_config, }; static struct apss_pll_data ipq8074_pll_data = { - .pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA, .pll = &ipq_pll_huayra, .pll_config = &ipq8074_pll_config, }; static struct apss_pll_data ipq6018_pll_data = { - .pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA, .pll = &ipq_pll_huayra, .pll_config = &ipq6018_pll_config, }; static struct apss_pll_data ipq9574_pll_data = { - .pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA, .pll = &ipq_pll_huayra, .pll_config = &ipq9574_pll_config, }; @@ -194,10 +188,10 @@ static int apss_ipq_pll_probe(struct platform_device *pdev) if (!data) return -ENODEV; - if (data->pll_type == CLK_ALPHA_PLL_TYPE_HUAYRA) + if (data->pll == &ipq_pll_huayra) clk_alpha_pll_configure(data->pll, regmap, data->pll_config); - else if (data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER || - data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER_PLUS) + else if (data->pll == &ipq_pll_stromer || + data->pll == &ipq_pll_stromer_plus) clk_stromer_pll_configure(data->pll, regmap, data->pll_config); ret = devm_clk_register_regmap(dev, &data->pll->clkr);
The value of the 'pll_type' field of 'struct apps_pll_data' is used only by the probe function to decide which config function should be called for the actual PLL. However this can be derived also from the 'pll' field which makes the 'pll_type' field redundant. Additionally, the CLK_ALPHA_PLL_TYPE_* enumeration values are meant to be used as indices to the 'clk_alpha_pll_regs' array so using those to define the pll type in this driver is misleading anyway. Change the probe function to use the 'pll' field to determine the configuration function to be used, and remove the 'pll_type' field to simplify the code. No functional changes. Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> --- Changes in v2: - no changes - Link to v1: https://lore.kernel.org/r/20240318-apss-ipq-pll-cleanup-v1-3-52f795429d5d@gmail.com --- drivers/clk/qcom/apss-ipq-pll.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)