Am Wed, 25 Sep 2024 10:07:29 +0300
schrieb Roger Quadros <rogerq@xxxxxxxxxx>:
[...]
> > +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
