Re: [PATCH 07/14] clk: qcom: clk-branch: Add support for SREG branch ops

[Stephen Boyd <sboyd@kernel.org>]

Quoting Dmitry Baryshkov (2024-10-17 09:56:57)
> From: Kalpak Kawadkar <quic_kkawadka@quicinc.com>
> 
> Add support for SREG branch ops. This is for the clocks which require

What is SREG? Can you spell it out?

> additional register operations with the SREG register as a part of
> enable / disable operations.
> 
> Signed-off-by: Kalpak Kawadkar <quic_kkawadka@quicinc.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
[...]
> diff --git a/drivers/clk/qcom/clk-branch.h b/drivers/clk/qcom/clk-branch.h
> index 47bf59a671c3c8516a57c283fce548a6e5f16619..149d04bae25d1a54999e0f938c4fce175a7c3e42 100644
> --- a/drivers/clk/qcom/clk-branch.h
> +++ b/drivers/clk/qcom/clk-branch.h
> @@ -24,8 +24,11 @@
>  struct clk_branch {
>         u32     hwcg_reg;
>         u32     halt_reg;
> +       u32     sreg_enable_reg;
>         u8      hwcg_bit;
>         u8      halt_bit;
> +       u32     sreg_core_ack_bit;
> +       u32     sreg_periph_ack_bit;

Are these bits? Should be u8 then. Or are they a mask?

>         u8      halt_check;

Instead of adding these new members can you wrap the struct in another
struct? There are usually a lot of branches in the system and this
bloats those structures when the members are never used.

	struct clk_sreg_branch {
		u32 sreg_enable_reg;
		u32 sreg_core_ack_bit;
		u32 sreg_periph_ack_bit;
		struct clk_branch branch;
	};

But I'm not even sure that is needed vs. just putting a clk_regmap
inside because the clk_ops don't seem to use any of these other members?