Re: [PATCH 2/2] clk: twl: add TWL6030 support

[Andreas Kemnade <andreas@kemnade.info>]

Am Wed, 25 Sep 2024 10:07:29 +0300
schrieb Roger Quadros <rogerq@kernel.org>:

[...]
> > +static void twl6030_clks_unprepare(struct clk_hw *hw)
> > +{
> > +	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> > +
> > +	twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
> > +		     ALL_GRP << TWL6030_CFG_STATE_GRP_SHIFT |  
> 
> Why are you unpreparing ALL_GRP? In prepare you only used VREG_GRP.
>
well, if we want control, then I think using every group to turn it off
into a defined state is a good idea.


> > +		     TWL6030_CFG_STATE_OFF);
> > +}
> > +
> > +static int twl6030_clks_is_prepared(struct clk_hw *hw)
> > +{
> > +	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> > +	int val;
> > +
> > +	val = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER, VREG_GRP);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	if (!(val & P1_GRP))
> > +		return 0;
> > +
> > +	val = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER,
> > VREG_STATE);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	val = TWL6030_CFG_STATE_APP(val);
> > +	return val == TWL6030_CFG_STATE_ON  
> 
> Is there a possibility that after calling twl6030_clks_prepare()
> the clock can still remain OFF?

I do not see a reason. 

> If not then we could just use a private flag to indicate clock
> prepared status and return that instead of reading the registers
> again.
>
The clock core already uses prepare_count if no is_prepared() is
defined.
So this prepare functions can just be dropped.

> 
> > +}
> > +
> >  static int twl6032_clks_prepare(struct clk_hw *hw)
> >  {
> >  	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> > @@ -93,6 +148,13 @@ static int twl6032_clks_is_prepared(struct
> > clk_hw *hw) return val == TWL6030_CFG_STATE_ON;
> >  }
> >  
> > +static const struct clk_ops twl6030_clks_ops = {
> > +	.prepare	= twl6030_clks_prepare,
> > +	.unprepare	= twl6030_clks_unprepare,
> > +	.is_prepared	= twl6030_clks_is_prepared,
> > +	.recalc_rate	= twl_clks_recalc_rate,
> > +};  
> 
> Instead of re-defining all the clock ops can't we just reuse the
> existing twl6032 clock ops?
> 
> We just need to tackle the twl6030 specific stuff inside the ops
> based on some platform driver data flag.
> 
a big if (driver_data == TWL6032) in each of the ops might be ok, since
we have an int and not a pointer there anyways might be the easiest way
to go.

Regards,
Andreas