Re: [PATCH v2 5/8] clk: qcom: rpmh: rename ARC clock data

From: Alex Elder <elder@linaro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Taniya Das <quic_tdas@quicinc.com>
Cc: linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org
Subject: Re: [PATCH v2 5/8] clk: qcom: rpmh: rename ARC clock data
Date: Thu, 1 Dec 2022 13:21:07 -0600	[thread overview]
Message-ID: <b9ecc4bf-67c6-b7bf-6a5b-5cce6a86c3da@linaro.org> (raw)
In-Reply-To: <20221130131001.20912-6-dmitry.baryshkov@linaro.org>

On 11/30/22 7:09 AM, Dmitry Baryshkov wrote:
> RPMH ARC clocks are frequently shared between several platfoms. It makes
> little sense to encode the SoC name into the clock name, if the same
> clock is used for other SoCs.
> 
> Rework the ARC clocks defintions to remove the SoC name. Keep the

s/clocks/clock/

> userspace-visible clock name, but encode the divider into the variable
> name. This also make it obvious which divider is used by the platform,

s/make/makes/

> making the code less error-prone.

You could do this as multiple patches:  one to distinguish the
clock symbol name from the user space name; one for formulating
the divider symbol name; and one for eliminating the platform.

I think if you do the first one, you could make that change to
all clock types at once, cleanly.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org > ---
>   drivers/clk/qcom/clk-rpmh.c | 86 ++++++++++++++++++-------------------
>   1 file changed, 43 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index 5044f9fa156d..bd6cb07b6154 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -70,15 +70,15 @@ struct clk_rpmh_desc {
>   
>   static DEFINE_MUTEX(rpmh_clk_lock);
>   
> -#define __DEFINE_CLK_RPMH(_platform, _name, _res_name,			\
> +#define __DEFINE_CLK_RPMH(_platform, _name, _cname, _res_name,		\
>   			  _res_en_offset, _res_on, _div)		\

Maybe _clk_name instead of _cname?

					-Alex

> -	static struct clk_rpmh _platform##_##_name##_ao;		\
> -	static struct clk_rpmh _platform##_##_name = {			\
> +	static struct clk_rpmh _platform##_##_cname##_ao;		\
> +	static struct clk_rpmh _platform##_##_cname = {			\
>   		.res_name = _res_name,					\
>   		.res_addr = _res_en_offset,				\
>   		.res_on_val = _res_on,					\
>   		.div = _div,						\
> -		.peer = &_platform##_##_name##_ao,			\
> +		.peer = &_platform##_##_cname##_ao,			\
>   		.valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) |	\
>   				      BIT(RPMH_ACTIVE_ONLY_STATE) |	\
>   				      BIT(RPMH_SLEEP_STATE)),		\

. . .