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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sat, 23 Mar 2024 00:33:08 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Gabor Juhos <j4g8y7@...il.com>
Cc: Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>, 
	Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/6] clk: qcom: apss-ipq-pll: remove 'pll_type' field
 from struct 'apss_pll_data'

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


-- 
With best wishes
Dmitry