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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240925121217.0aa54808@akair>
Date: Wed, 25 Sep 2024 12:12:17 +0200
From: Andreas Kemnade <andreas@...nade.info>
To: Roger Quadros <rogerq@...nel.org>
Cc: linux-omap@...r.kernel.org, Stephen Boyd <sboyd@...nel.org>, Kevin
 Hilman <khilman@...libre.com>, Michael Turquette <mturquette@...libre.com>,
 Aaro Koskinen <aaro.koskinen@....fi>, linux-kernel@...r.kernel.org, Lee
 Jones <lee@...nel.org>, Tony Lindgren <tony@...mide.com>,
 linux-clk@...r.kernel.org
Subject: Re: [PATCH 2/2] clk: twl: add TWL6030 support

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