linux-kernel - Re: [PATCH v5 1/5] regulator: Document binding for initial and suspend modes

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141107161029.GV8509@sirena.org.uk>
Date:	Fri, 7 Nov 2014 16:10:29 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
Cc:	Kukjin Kim <kgene.kim@...sung.com>,
	Chanwoo Choi <cw00.choi@...sung.com>,
	Olof Johansson <olof@...om.net>,
	Chris Zhong <zyw@...k-chips.com>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Abhilash Kesavan <kesavan.abhilash@...il.com>,
	linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v5 1/5] regulator: Document binding for initial and
 suspend modes

On Fri, Nov 07, 2014 at 04:38:04PM +0100, Javier Martinez Canillas wrote:
> On 11/07/2014 03:58 PM, Mark Brown wrote:
> > On Fri, Nov 07, 2014 at 02:00:01PM +0100, Javier Martinez Canillas wrote:

> >> +	  The "regulator-mode" property only takes effect if the regulator is
> >> +	  enabled for the given suspend state using "regulator-on-in-suspend".

> > Why?

> I saw that the regulator core only call the .set_suspend_mode callback if
> the regulator is either enabled or disabled explicitly...

That's the current implementation.  Why are you adding it to a
specification?

> static int suspend_set_state(struct regulator_dev *rdev,
> 	struct regulator_state *rstate)
> {
> 	....
> 	/* If we have no suspend mode configration don't set anything;
> 	 * only warn if the driver implements set_suspend_voltage or
> 	 * set_suspend_mode callback.
> 	 */
> 	if (!rstate->enabled && !rstate->disabled) {

For example why couldn't these get set by reading the current state?

> >> +	  If the regulator has not been explicitly disabled for the given state
> >> +	  with "regulator-off-in-suspend", then setting the operating mode
> >> +	  will also have no effect.

> > This seems surprising, I'd expect mode setting to be paid attention to
> > even if the regulator is off - we may add other ways to control the
> > enable state in suspend for example.

> ...and I thought that setting a mode if the regulator was disabled in suspend
> was not a possible configuration, that's why I documented that.

Like I say this is at the very least making it impossible for us to in
future add other ways of setting if the regulator is enabled or disabled
in suspend.

> I know that there is both a .set_suspend_disable and .set_suspend_mode but
> at least in the hardware that I'm interested in (max77802), the same hw
> register is used for setting a suspend mode and disable on suspend.

That's your device, other hardware exists which uses seprate bitfields.

> >> +If no mode is defined, then the OS will not manage the modes and the hardware
> >> +default values will be used instead.

> > Again that seems surprising, it precludes any future changes and isn't
> > going to be true for devices where we can't read the current state.

> I saw that you asked Chanwoo on an early version of his suspend states
> series, to point out that in the absence of a initial state, the state is
> the hardware default [0] so I thought that it should be the case for the
> regulator suspend mode too.

You're also saying that the OS won't manage the mode here, that's a step
further.

> So should I just explain what each property is about without trying to make
> assumptions about the limitations that different devices could have and let
> each device DT binding to specify those?

More towards that direction, yes.  Don't overspecify.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)