Re: [PATCH] regulator: anatop: add is_enabled interface

Re: [PATCH] regulator: anatop: add is_enabled interface

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

On Mon, Dec 16, 2013 at 04:20:27PM -0500, Anson Huang wrote:

> +{
> +	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
> +	u32 val;
> +
> +	if (!anatop_reg->control_reg)
> +		return -ENOTSUPP;

In what situation would this happen and why would the operation be
provided in those situations?

> +	regmap_read(anatop_reg->anatop, anatop_reg->control_reg, &val);
> +
> +	return (val >> anatop_reg->vol_bit_shift) &
> +		((1 << anatop_reg->vol_bit_width) - 1) ? 1 : 0;
> +}

This isn't terribly legible but it looks like it's trying to read to see
if a non-zero voltage has been written.  If that's the case it looks
like enable and disable operations should be supported since presumably
the register can be written as well.

Please try to avoid the ternery operator, especially in cases like this
where it's not adding anything.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] regulator: anatop: add is_enabled interface

[Anson Huang]

Add is_enabled interface for anatop regulator in case
some drivers need to know regulator's status.

Signed-off-by: Anson Huang <b20788@freescale.com>
---
 drivers/regulator/anatop-regulator.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index c734d09..770d836 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -97,10 +97,25 @@ static int anatop_regmap_get_voltage_sel(struct regulator_dev *reg)
 	return regulator_get_voltage_sel_regmap(reg);
 }
 
+static int anatop_regmap_is_enabled(struct regulator_dev *reg)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	u32 val;
+
+	if (!anatop_reg->control_reg)
+		return -ENOTSUPP;
+
+	regmap_read(anatop_reg->anatop, anatop_reg->control_reg, &val);
+
+	return (val >> anatop_reg->vol_bit_shift) &
+		((1 << anatop_reg->vol_bit_width) - 1) ? 1 : 0;
+}
+
 static struct regulator_ops anatop_rops = {
 	.set_voltage_sel = anatop_regmap_set_voltage_sel,
 	.set_voltage_time_sel = anatop_regmap_set_voltage_time_sel,
 	.get_voltage_sel = anatop_regmap_get_voltage_sel,
+	.is_enabled = anatop_regmap_is_enabled,
 	.list_voltage = regulator_list_voltage_linear,
 	.map_voltage = regulator_map_voltage_linear,
 };
-- 
1.7.9.5

RE: [PATCH] regulator: anatop: add is_enabled interface

[Anson.Huang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2872 bytes --]

Hi, Mark
	Thanks for your quick reply, see below inline:

Best Regards.
Anson huang 黄勇才
 
Freescale Semiconductor Shanghai
上海浦东新区亮景路192号A座2楼
201203
Tel:021-28937058


>-----Original Message-----
>From: Mark Brown [mailto:broonie@kernel.org]
>Sent: Tuesday, December 17, 2013 2:58 AM
>To: Huang Yongcai-B20788
>Cc: lgirdwood@gmail.com; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH] regulator: anatop: add is_enabled interface
>
>On Mon, Dec 16, 2013 at 04:20:27PM -0500, Anson Huang wrote:
>
>> +{
>> +	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
>> +	u32 val;
>> +
>> +	if (!anatop_reg->control_reg)
>> +		return -ENOTSUPP;
>
>In what situation would this happen and why would the operation be provided in
>those situations?
[Anson] This condition check is to avoid the case that control_reg is not
Initialized correctly in the probe function.
>
>> +	regmap_read(anatop_reg->anatop, anatop_reg->control_reg, &val);
>> +
>> +	return (val >> anatop_reg->vol_bit_shift) &
>> +		((1 << anatop_reg->vol_bit_width) - 1) ? 1 : 0; }
>
>This isn't terribly legible but it looks like it's trying to read to see if a
>non-zero voltage has been written.  If that's the case it looks like enable
>and disable operations should be supported since presumably the register can
>be written as well.

[Anson]Yes, from the register description, we can see that if the bit field of
REGx_TARG is not 0, then the LDO is enabled and there is output available. Adding this
Interface is because some specific requirement of our i.MX6's freq/volt scaling, and
We will add the enable/disable interface later when we add the GPU/VPU driver, as only
VDDPU can be on/off according to GPU/VPU's behavior, and there is a complicated flow
We need to follow, current is_enabled interface is for the case of VDDPU is turned off
In uboot.

1. For cpufreq change, we need to scale VDDARM as well as VDDSOC/PU's voltage, each
setpoint has different VDDARM, VDDSOC/PU voltage;
2. VDDARM and VDDSOC will be always on but VDDPU can be off in uboot due to the big leakage
it has(as high as ~43mA on i.MX6Q), as it is only for GPU/VPU module, and we will have
dynamic VDDPU power management available, so VDDPU LDO's status is changing according to GPU/VPU's
behavior;
3. If VDDPU is off during cpufreq change, then there is no need to scaling VDDPU's voltage,
so we will check whether VDDPU is enabled before scaling it in cpufreq driver, this is done
in my other patch thread;

>
>Please try to avoid the ternery operator, especially in cases like this where
>it's not adding anything.
[Anson] Accepted, will send out V2 patch if you think adding this interface for
our cpufreq driver is acceptable.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] regulator: anatop: add is_enabled interface

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

On Tue, Dec 17, 2013 at 02:38:56AM +0000, Anson.Huang@freescale.com wrote:

Please fix the line length you're using to word wrap, it should be less
than 80 columns to make your mails readable.

> >> +	if (!anatop_reg->control_reg)
> >> +		return -ENOTSUPP;

> >In what situation would this happen and why would the operation be provided in
> >those situations?

> [Anson] This condition check is to avoid the case that control_reg is not
> Initialized correctly in the probe function.

Why would that happen without the probe function failing - it would be
better to add the error checking there wouldn't it?

> 1. For cpufreq change, we need to scale VDDARM as well as VDDSOC/PU's voltage, each
> setpoint has different VDDARM, VDDSOC/PU voltage;
> 2. VDDARM and VDDSOC will be always on but VDDPU can be off in uboot due to the big leakage
> it has(as high as ~43mA on i.MX6Q), as it is only for GPU/VPU module, and we will have
> dynamic VDDPU power management available, so VDDPU LDO's status is changing according to GPU/VPU's
> behavior;
> 3. If VDDPU is off during cpufreq change, then there is no need to scaling VDDPU's voltage,
> so we will check whether VDDPU is enabled before scaling it in cpufreq driver, this is done
> in my other patch thread;

This sounds like you need to have some higher level synchronisation
between whatever is managing this supply and your cpufreq driver - if
you rely on reading back the current status from the hardware there will
always be races between reading the state and the other thing doing the
enable or disable.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: anatop: add is_enabled interface

[Anson.Huang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2347 bytes --]



Sent from Anson's iPhone

> ÔÚ 2013Äê12ÔÂ17ÈÕ£¬20:29£¬"Mark Brown" <broonie@kernel.org> дµÀ£º
> 
> On Tue, Dec 17, 2013 at 02:38:56AM +0000, Anson.Huang@freescale.com wrote:
> 
> Please fix the line length you're using to word wrap, it should be less
> than 80 columns to make your mails readable.
> 
>>>> +    if (!anatop_reg->control_reg)
>>>> +        return -ENOTSUPP;
> 
>>> In what situation would this happen and why would the operation be provided in
>>> those situations?
> 
>> [Anson] This condition check is to avoid the case that control_reg is not
>> Initialized correctly in the probe function.
> 
> Why would that happen without the probe function failing - it would be
> better to add the error checking there wouldn't it?

Okay, than what about other functions? there is such condition check there is other functions too, that is why I add it here. if you think it is no necessary, I will remove this check in my patch?

> 
>> 1. For cpufreq change, we need to scale VDDARM as well as VDDSOC/PU's voltage, each
>> setpoint has different VDDARM, VDDSOC/PU voltage;
>> 2. VDDARM and VDDSOC will be always on but VDDPU can be off in uboot due to the big leakage
>> it has(as high as ~43mA on i.MX6Q), as it is only for GPU/VPU module, and we will have
>> dynamic VDDPU power management available, so VDDPU LDO's status is changing according to GPU/VPU's
>> behavior;
>> 3. If VDDPU is off during cpufreq change, then there is no need to scaling VDDPU's voltage,
>> so we will check whether VDDPU is enabled before scaling it in cpufreq driver, this is done
>> in my other patch thread;
> 
> This sounds like you need to have some higher level synchronisation
> between whatever is managing this supply and your cpufreq driver - if
> you rely on reading back the current status from the hardware there will
> always be races between reading the state and the other thing doing the
> enable or disable.

yes, you are right. but I think we have handled that, all the operations of this LDO will via regulator interface, and regulator framework already has mutex lock. so there should be no such race in kernel as long as we all use regulator interface to access anatop LDO.ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] regulator: anatop: add is_enabled interface

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]

On Tue, Dec 17, 2013 at 12:38:33PM +0000, Anson.Huang@freescale.com wrote:

> > better to add the error checking there wouldn't it?

> Okay, than what about other functions? there is such condition check
> there is other functions too, that is why I add it here. if you think
> it is no necessary, I will remove this check in my patch?

It shouldn't be in the other functions either.

> > This sounds like you need to have some higher level synchronisation
> > between whatever is managing this supply and your cpufreq driver - if
> > you rely on reading back the current status from the hardware there will
> > always be races between reading the state and the other thing doing the
> > enable or disable.

> yes, you are right. but I think we have handled that, all the
> operations of this LDO will via regulator interface, and regulator
> framework already has mutex lock. so there should be no such race in
> kernel as long as we all use regulator interface to access anatop LDO.

No, that's not going to work.  Consider this sequence:

	cpufreq		other driver
			disable()
	is_enabled()
			enable()

The locking the regulator core does won't help you here, nothing stops
the state of the regulator changing after it's read.  Your cpufreq
driver should just change the voltage and not worry if the regulator is
enabled, the voltage change won't have any effect while the regulator is
off and presumably if it does get enabled then it needs to be enabled at
whatever voltage cpufreq set anyway.

In any case I'd be much happier with this patch if it implemented the
enable and disable operations as well.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: anatop: add is_enabled interface

[Anson.Huang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2234 bytes --]



Sent from Anson's iPhone

> ÔÚ 2013Äê12ÔÂ17ÈÕ£¬21:00£¬"Mark Brown" <broonie@kernel.org> дµÀ£º
> 
> On Tue, Dec 17, 2013 at 12:38:33PM +0000, Anson.Huang@freescale.com wrote:
> 
>>> better to add the error checking there wouldn't it?
> 
>> Okay, than what about other functions? there is such condition check
>> there is other functions too, that is why I add it here. if you think
>> it is no necessary, I will remove this check in my patch?
> 
> It shouldn't be in the other functions either.

got it, I will remove it later.


> 
>>> This sounds like you need to have some higher level synchronisation
>>> between whatever is managing this supply and your cpufreq driver - if
>>> you rely on reading back the current status from the hardware there will
>>> always be races between reading the state and the other thing doing the
>>> enable or disable.
> 
>> yes, you are right. but I think we have handled that, all the
>> operations of this LDO will via regulator interface, and regulator
>> framework already has mutex lock. so there should be no such race in
>> kernel as long as we all use regulator interface to access anatop LDO.
> 
> No, that's not going to work.  Consider this sequence:
> 
>    cpufreq        other driver
>            disable()
>    is_enabled()
>            enable()
> 
> The locking the regulator core does won't help you here, nothing stops
> the state of the regulator changing after it's read.  Your cpufreq
> driver should just change the voltage and not worry if the regulator is
> enabled, the voltage change won't have any effect while the regulator is
> off and presumably if it does get enabled then it needs to be enabled at
> whatever voltage cpufreq set anyway.
> 
> In any case I'd be much happier with this patch if it implemented the
> enable and disable operations as well.

understand now. then maybe I should remove the PU check in cpufreq, although setting PU LDO if it is off before would bring unnessary power leak. I will add all these changes together with the dynamic PU LDO management feature, thanks for your time!ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] regulator: anatop: add is_enabled interface

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 561 bytes --]

On Tue, Dec 17, 2013 at 01:10:12PM +0000, Anson.Huang@freescale.com wrote:

> > In any case I'd be much happier with this patch if it implemented the
> > enable and disable operations as well.

> understand now. then maybe I should remove the PU check in cpufreq,
> although setting PU LDO if it is off before would bring unnessary
> power leak. I will add all these changes together with the dynamic PU
> LDO management feature, thanks for your time!

If you've implemented the enable support correctly then setting the
voltage shouldn't enable the regulator.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]