Date:	Fri, 07 Nov 2014 16:38:04 +0100
From:	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
To:	Mark Brown <broonie@...nel.org>
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

Hello Mark,

Thanks a lot for your feedback.

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...

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) {
		if (rdev->desc->ops->set_suspend_voltage ||
		    rdev->desc->ops->set_suspend_mode)
			rdev_warn(rdev, "No configuration\n");
		return 0;
	}
	...
	
};

>> +	  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.

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.

So if this is allowed and a user specifies both a disable on suspend and a
suspend mode, the later will overwrite the configuration of the former.

But now that I think about it, this is only a constraint of the max77802
and probably needs to be specified in the max77802 DT binding and not in
the regulator generic one since otherwise limits what other devices can
do.

>> +- regulator-initial-mode: initial operating mode. The set of possible operating
>> +  modes is the same used for the regulator-mode property and the device binding
>> +  documentation explains which property each regulator supports.
>> +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.

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?

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/9/4/652 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists