Re: [PATCH v2 3/6] clk: qcom: apss-ipq-pll: remove 'pll_type' field from struct 'apss_pll_data'

[Gabor Juhos]

2024. 03. 21. 11:37 keltezéssel, Dmitry Baryshkov írta:
> On Thu, 21 Mar 2024 at 09:50, Gabor Juhos <j4g8y7@xxxxxxxxx> 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