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

[Dmitry Baryshkov]

On Fri, 22 Mar 2024 at 22:59, Gabor Juhos <j4g8y7@xxxxxxxxx> wrote:
>
> 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?

This looks like a slight overkill, but yes, it is more acceptable. The
logic should be type => functions, not the other way around.


-- 
With best wishes
Dmitry