On Thu, Oct 17, 2024 at 11:10:20AM -0700, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2024-10-17 09:56:57)
> > From: Kalpak Kawadkar <quic_kkawadka@xxxxxxxxxxx>
> >
> > Add support for SREG branch ops. This is for the clocks which require
>
> What is SREG? Can you spell it out?
Unfortunately, no idea. This is the only register name I know.
>
> > additional register operations with the SREG register as a part of
> > enable / disable operations.
> >
> > Signed-off-by: Kalpak Kawadkar <quic_kkawadka@xxxxxxxxxxx>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> [...]
> > 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?
masks, will rename.
>
> > 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?
Yes, nice idea. Is it ok to keep the _branch suffix or we'd better
rename it dropping the _branch (and move to another source file while we
are at it)?
--
With best wishes
Dmitry
