The special handling code for getting shared mode status is wrong
because it needs to check info->shared_mode->lp_mode_req for
both regulators that shared the same mode register.
In set_mode(), current code ensures we won't set mode to REGULATOR_MODE_IDLE
if only one of the regulator requests to set idle.
In get_mode(), we can just remove the special handling code for shared mode.
Read the register value always returns correct status no matter the regulator
has shared mode register or not.
Signed-off-by: Axel Lin <[email protected]>
---
drivers/regulator/ab8500.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
index 9ebd131..acdffc5 100644
--- a/drivers/regulator/ab8500.c
+++ b/drivers/regulator/ab8500.c
@@ -456,14 +456,6 @@ static unsigned int ab8500_regulator_get_mode(struct regulator_dev *rdev)
return -EINVAL;
}
- /* Need special handling for shared mode */
- if (info->shared_mode) {
- if (info->shared_mode->lp_mode_req)
- return REGULATOR_MODE_IDLE;
- else
- return REGULATOR_MODE_NORMAL;
- }
-
if (info->mode_mask) {
/* Dedicated register for handling mode */
ret = abx500_get_register_interruptible(info->dev,
--
1.7.10.4
Ping.
Hi Lee,
Can you review this patch.
I think this one is a bug fix.
Regards,
Axel
2013/4/8 Axel Lin <[email protected]>:
> The special handling code for getting shared mode status is wrong
> because it needs to check info->shared_mode->lp_mode_req for
> both regulators that shared the same mode register.
>
> In set_mode(), current code ensures we won't set mode to REGULATOR_MODE_IDLE
> if only one of the regulator requests to set idle.
>
> In get_mode(), we can just remove the special handling code for shared mode.
> Read the register value always returns correct status no matter the regulator
> has shared mode register or not.
>
> Signed-off-by: Axel Lin <[email protected]>
> ---
> drivers/regulator/ab8500.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
> index 9ebd131..acdffc5 100644
> --- a/drivers/regulator/ab8500.c
> +++ b/drivers/regulator/ab8500.c
> @@ -456,14 +456,6 @@ static unsigned int ab8500_regulator_get_mode(struct regulator_dev *rdev)
> return -EINVAL;
> }
>
> - /* Need special handling for shared mode */
> - if (info->shared_mode) {
> - if (info->shared_mode->lp_mode_req)
> - return REGULATOR_MODE_IDLE;
> - else
> - return REGULATOR_MODE_NORMAL;
> - }
> -
> if (info->mode_mask) {
> /* Dedicated register for handling mode */
> ret = abx500_get_register_interruptible(info->dev,
> --
> 1.7.10.4
>
>
>
On 04/08/2013 02:31 PM, Axel Lin wrote:
> The special handling code for getting shared mode status is wrong
> because it needs to check info->shared_mode->lp_mode_req for
> both regulators that shared the same mode register.
>
> In set_mode(), current code ensures we won't set mode to REGULATOR_MODE_IDLE
> if only one of the regulator requests to set idle.
>
> In get_mode(), we can just remove the special handling code for shared mode.
> Read the register value always returns correct status no matter the regulator
> has shared mode register or not.
I am not convinced about this patch. The purpose of the original code is
that the regulator framework should be unaware that the mode register is
shared with another regulator. If we apply this patch, get_mode may
return different results depending on the other regulators mode settings.
Let me take an example: The other shared regulator is already set to LP
mode and the current regulator is requested to low power mode. Then the
framework first checks current mode and compares to requested mode. If
equal, it returns. With this patch applied, it will see that the
regulator is already set to LP mode and return without calling set_mode
in the driver. However, the state information in the driver will be
wrong so when the other regulator is requested to HP mode and back to LP
mode it will not actually set LP mode again to HW.
Regards,
Bengt
>
> Signed-off-by: Axel Lin <[email protected]>
> ---
> drivers/regulator/ab8500.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
> index 9ebd131..acdffc5 100644
> --- a/drivers/regulator/ab8500.c
> +++ b/drivers/regulator/ab8500.c
> @@ -456,14 +456,6 @@ static unsigned int ab8500_regulator_get_mode(struct regulator_dev *rdev)
> return -EINVAL;
> }
>
> - /* Need special handling for shared mode */
> - if (info->shared_mode) {
> - if (info->shared_mode->lp_mode_req)
> - return REGULATOR_MODE_IDLE;
> - else
> - return REGULATOR_MODE_NORMAL;
> - }
> -
> if (info->mode_mask) {
> /* Dedicated register for handling mode */
> ret = abx500_get_register_interruptible(info->dev,
Hi Axel,
> Ping.
>
> Hi Lee,
> Can you review this patch.
> I think this one is a bug fix.
Bengt is the SME for all things ab*-regulator related.
No one knows the driver better than him.
Bengt, would you mind reviewing?
> 2013/4/8 Axel Lin <[email protected]>:
> > The special handling code for getting shared mode status is wrong
> > because it needs to check info->shared_mode->lp_mode_req for
> > both regulators that shared the same mode register.
> >
> > In set_mode(), current code ensures we won't set mode to REGULATOR_MODE_IDLE
> > if only one of the regulator requests to set idle.
> >
> > In get_mode(), we can just remove the special handling code for shared mode.
> > Read the register value always returns correct status no matter the regulator
> > has shared mode register or not.
> >
> > Signed-off-by: Axel Lin <[email protected]>
> > ---
> > drivers/regulator/ab8500.c | 8 --------
> > 1 file changed, 8 deletions(-)
> >
> > diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
> > index 9ebd131..acdffc5 100644
> > --- a/drivers/regulator/ab8500.c
> > +++ b/drivers/regulator/ab8500.c
> > @@ -456,14 +456,6 @@ static unsigned int ab8500_regulator_get_mode(struct regulator_dev *rdev)
> > return -EINVAL;
> > }
> >
> > - /* Need special handling for shared mode */
> > - if (info->shared_mode) {
> > - if (info->shared_mode->lp_mode_req)
> > - return REGULATOR_MODE_IDLE;
> > - else
> > - return REGULATOR_MODE_NORMAL;
> > - }
> > -
> > if (info->mode_mask) {
> > /* Dedicated register for handling mode */
> > ret = abx500_get_register_interruptible(info->dev,
> >
> >
> >
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
2013/4/15 Bengt J?nsson <[email protected]>:
> On 04/08/2013 02:31 PM, Axel Lin wrote:
>>
>> The special handling code for getting shared mode status is wrong
>> because it needs to check info->shared_mode->lp_mode_req for
>> both regulators that shared the same mode register.
>>
>> In set_mode(), current code ensures we won't set mode to
>> REGULATOR_MODE_IDLE
>> if only one of the regulator requests to set idle.
>>
>> In get_mode(), we can just remove the special handling code for shared
>> mode.
>> Read the register value always returns correct status no matter the
>> regulator
>> has shared mode register or not.
>
> I am not convinced about this patch. The purpose of the original code is
> that the regulator framework should be unaware that the mode register is
> shared with another regulator. If we apply this patch, get_mode may return
> different results depending on the other regulators mode settings.
No. Original code is just wrong.
First, take a look at ab8500_regulator_set_mode().
When setting REGULATOR_MODE_IDLE mode, current code will only write
the register to idle mode only when *both* shared regulator have set idle mode.
Which means if only one of the shared mode has set regulator to idle,
both should be still in NORMAL mode.
In ab8500_regulator_get_mode(), the special handling code only check it's own
lp_mode_req flag which is wrong. it needs to check both it's own lp_mode_req
and the other one shared mode regulator.
However, this patch makes things simpler by just remove these check.
Because set_mode ensures the register is set to IDLE only when *both*
shared regulator are in idle.
>
> Let me take an example: The other shared regulator is already set to LP mode
> and the current regulator is requested to low power mode. Then the framework
> first checks current mode and compares to requested mode. If equal, it
> returns. With this patch applied, it will see that the regulator is already
> set to LP mode and return without calling set_mode in the driver. However,
> the state information in the driver will be wrong so when the other
> regulator is requested to HP mode and back to LP mode it will not actually
> set LP mode again to HW.
I'm not sure if I understand this part.
My understanding is for shared mode regulators:
It can be in LP mode only when *BOTH* are in LP mode.
If only one of the regulator in HP mode, then *BOTH* should be in HP mode.
Did I misunderstand something?
Axel
> My understanding is for shared mode regulators:
> It can be in LP mode only when *BOTH* are in LP mode.
> If only one of the regulator in HP mode, then *BOTH* should be in HP mode.
> Did I misunderstand something?
Let me put this issue this way:
Current code behavior:
get_mode() returns IDLE if only one lp_mode_req flag is true, but mode
register value is HP.
AB8540_LDO_ANAMIC1 AB8540_LDO_ANAMIC2 mode register
get_mode() returns
lp_mode_req=true lp_mode_req=true HP
REGULATOR_MODE_NORMAL
lp_mode_req=true lp_mode_req=false HP
REGULATOR_MODE_IDLE
lp_mode_req=false lp_mode_req=true HP
REGULATOR_MODE_IDLE
lp_mode_req=false lp_mode_req=false LP
REGULATOR_MODE_IDLE
with this path:
mode register value is consistent with get_mode().
AB8540_LDO_ANAMIC1 AB8540_LDO_ANAMIC2 mode register
get_mode() returns
lp_mode_req=true lp_mode_req=true HP
REGULATOR_MODE_NORMAL
lp_mode_req=true lp_mode_req=false HP
REGULATOR_MODE_NORMAL
lp_mode_req=false lp_mode_req=true HP
REGULATOR_MODE_NORMAL
lp_mode_req=false lp_mode_req=false LP
REGULATOR_MODE_IDLE
On 04/15/2013 10:50 AM, Axel Lin wrote:
>> My understanding is for shared mode regulators:
>> It can be in LP mode only when *BOTH* are in LP mode.
>> If only one of the regulator in HP mode, then *BOTH* should be in HP mode.
>> Did I misunderstand something?
Your understanding is correct.
> Let me put this issue this way:
>
> Current code behavior:
> get_mode() returns IDLE if only one lp_mode_req flag is true, but mode
> register value is HP.
>
> AB8540_LDO_ANAMIC1 AB8540_LDO_ANAMIC2 mode register
> get_mode() returns
> lp_mode_req=true lp_mode_req=true HP
> REGULATOR_MODE_NORMAL
> lp_mode_req=true lp_mode_req=false HP
> REGULATOR_MODE_IDLE
> lp_mode_req=false lp_mode_req=true HP
> REGULATOR_MODE_IDLE
> lp_mode_req=false lp_mode_req=false LP
> REGULATOR_MODE_IDLE
I think it looks like this:
AB8540_LDO_ANAMIC1 AB8540_LDO_ANAMIC2 mode register
get_mode() returns
lp_mode_req=true lp_mode_req=true LP
REGULATOR_MODE_IDLE REGULATOR_MODE_IDLE
lp_mode_req=true lp_mode_req=false HP
REGULATOR_MODE_IDLE REGULATOR_MODE_NORMAL
lp_mode_req=false lp_mode_req=true HP
REGULATOR_MODE_NORMAL REGULATOR_MODE_IDLE
lp_mode_req=false lp_mode_req=false HP
REGULATOR_MODE_NORMAL REGULATOR_MODE_NORMAL
> with this path:
> mode register value is consistent with get_mode().
>
> AB8540_LDO_ANAMIC1 AB8540_LDO_ANAMIC2 mode register
> get_mode() returns
> lp_mode_req=true lp_mode_req=true HP
> REGULATOR_MODE_NORMAL
> lp_mode_req=true lp_mode_req=false HP
> REGULATOR_MODE_NORMAL
> lp_mode_req=false lp_mode_req=true HP
> REGULATOR_MODE_NORMAL
> lp_mode_req=false lp_mode_req=false LP
> REGULATOR_MODE_IDLE
And like this:
AB8540_LDO_ANAMIC1 AB8540_LDO_ANAMIC2 mode register
get_mode() returns
lp_mode_req=true lp_mode_req=true LP
REGULATOR_MODE_IDLE REGULATOR_MODE_IDLE
lp_mode_req=true lp_mode_req=false HP
REGULATOR_MODE_NORMAL REGULATOR_MODE_NORMAL
lp_mode_req=false lp_mode_req=true HP
REGULATOR_MODE_NORMAL REGULATOR_MODE_NORMAL
lp_mode_req=false lp_mode_req=false HP
REGULATOR_MODE_NORMAL REGULATOR_MODE_NORMAL
I guess what you don't like with the current approach is that the driver returns REGULATOR_MODE_IDLE in some cases where the mode register is set to LP. But I think, with patch applied, the control may be wrong in some cases because the regulator framework will call get_mode and see that the mode is already correct and not call set_mode so lp_mode_req will not get updated. I realised my previous example was incorrect so here I describe another example:
0. Start condition:
ANAMIC1 is requested in LP mode (lp_mode_req=true) and ANAMIC2 is requested in HP mode (lp_mode_req=false).
So the mode register is set to HP.
1. Now ANAMIC1 is requested to HP mode from consumer side. The regulator framework checks the current mode with get_mode which returns HP. So the regulator framework returns without calling set_mode because the mode is already correct.
For ANAMIC1 lp_mode_req=true and for ANAMIC2 lp_mode_req=false (still).
The mode register will be correct at HP.
2. If ANAMIC2 is now requested to LP mode from consumer side, the regulator framework calls get_mode which returns HP, so the framework calls set_mode.
In set_mode, the function checks the other regulator's status which is lp_mode_req=true. So the function will continue and set the regulator in LP mode even if it should not be.
Bengt
> I guess what you don't like with the current approach is that the driver
> returns REGULATOR_MODE_IDLE in some cases where the mode register is set to
> LP. But I think, with patch applied, the control may be wrong in some cases
> because the regulator framework will call get_mode and see that the mode is
> already correct and not call set_mode so lp_mode_req will not get updated. I
I got your point now.
My point is get_mode() should always return "correct" status by
reading register value.
And as you mentioned, regulator_set_mode() did check current mode and won't call
set_mode callback if current mode is the same as the target mode.
And that is why this patch won't work.
However, Make get_mode() return "incorrect" status to avoid above
issue looks wrong to me.
Regards,
Axel
On 04/15/2013 02:13 PM, Axel Lin wrote:
>> I guess what you don't like with the current approach is that the driver
>> returns REGULATOR_MODE_IDLE in some cases where the mode register is set to
>> LP. But I think, with patch applied, the control may be wrong in some cases
>> because the regulator framework will call get_mode and see that the mode is
>> already correct and not call set_mode so lp_mode_req will not get updated. I
> I got your point now.
>
> My point is get_mode() should always return "correct" status by
> reading register value.
> And as you mentioned, regulator_set_mode() did check current mode and won't call
> set_mode callback if current mode is the same as the target mode.
> And that is why this patch won't work.
>
> However, Make get_mode() return "incorrect" status to avoid above
> issue looks wrong to me.
>
> Regards,
> Axel
I understand your point of view, but I think that the framework (as it
is currently implemented) expects to get the requested mode of the
regulator in this case, not the actual mode (in the shared mode
register).The alternative could be to change the framework in some way.
Any ideas? Otherwise I propose to keep the code and maybe add a comment.
Regards,
Bengt
2013/4/15 Bengt J?nsson <[email protected]>:
> On 04/15/2013 02:13 PM, Axel Lin wrote:
>>>
>>> I guess what you don't like with the current approach is that the driver
>>> returns REGULATOR_MODE_IDLE in some cases where the mode register is set
>>> to
>>> LP. But I think, with patch applied, the control may be wrong in some
>>> cases
>>> because the regulator framework will call get_mode and see that the mode
>>> is
>>> already correct and not call set_mode so lp_mode_req will not get
>>> updated. I
>>
>> I got your point now.
>>
>> My point is get_mode() should always return "correct" status by
>> reading register value.
>> And as you mentioned, regulator_set_mode() did check current mode and
>> won't call
>> set_mode callback if current mode is the same as the target mode.
>> And that is why this patch won't work.
>>
>> However, Make get_mode() return "incorrect" status to avoid above
>> issue looks wrong to me.
>>
>> Regards,
>> Axel
>
> I understand your point of view, but I think that the framework (as it is
> currently implemented) expects to get the requested mode of the regulator in
> this case, not the actual mode (in the shared mode register).The alternative
> could be to change the framework in some way.
>
> Any ideas? Otherwise I propose to keep the code and maybe add a comment.
It looks to me a simple fix is to just get rid of the check of old mode with
new mode setting.
Something like reverse of commit 500b4ac90d1103
"regulator: return set_mode is same mode is requested" would work.
Regards,
Axel
On 04/15/2013 04:11 PM, Axel Lin wrote:
> 2013/4/15 Bengt J?nsson <[email protected]>:
>> On 04/15/2013 02:13 PM, Axel Lin wrote:
>>>> I guess what you don't like with the current approach is that the driver
>>>> returns REGULATOR_MODE_IDLE in some cases where the mode register is set
>>>> to
>>>> LP. But I think, with patch applied, the control may be wrong in some
>>>> cases
>>>> because the regulator framework will call get_mode and see that the mode
>>>> is
>>>> already correct and not call set_mode so lp_mode_req will not get
>>>> updated. I
>>> I got your point now.
>>>
>>> My point is get_mode() should always return "correct" status by
>>> reading register value.
>>> And as you mentioned, regulator_set_mode() did check current mode and
>>> won't call
>>> set_mode callback if current mode is the same as the target mode.
>>> And that is why this patch won't work.
>>>
>>> However, Make get_mode() return "incorrect" status to avoid above
>>> issue looks wrong to me.
>>>
>>> Regards,
>>> Axel
>> I understand your point of view, but I think that the framework (as it is
>> currently implemented) expects to get the requested mode of the regulator in
>> this case, not the actual mode (in the shared mode register).The alternative
>> could be to change the framework in some way.
>>
>> Any ideas? Otherwise I propose to keep the code and maybe add a comment.
> It looks to me a simple fix is to just get rid of the check of old mode with
> new mode setting.
>
> Something like reverse of commit 500b4ac90d1103
> "regulator: return set_mode is same mode is requested" would work.
>
> Regards,
> Axel
Reverting 500b4ac90d1103 makes sense, but first I want to mention two
things:
1. In some cases it is not even possible to know the actual current
state of a regulator because it is controlled by HW as well as SW. We
have several examples of this.
2. regulator_enable/disable also checks the current status before
setting the regulator. Should these checks be removed as well?
Regards,
Bengt
On Mon, Apr 15, 2013 at 04:48:03PM +0200, Bengt J?nsson wrote:
> Reverting 500b4ac90d1103 makes sense, but first I want to mention
> two things:
> 1. In some cases it is not even possible to know the actual current
> state of a regulator because it is controlled by HW as well as SW.
> We have several examples of this.
If we're getting diverging statuses here then we need to introduce a
separate function to report the actual hardware state. The get/set
should be the request.
> 2. regulator_enable/disable also checks the current status before
> setting the regulator. Should these checks be removed as well?
No, the enable state should reflect the state of the request from the
AP. regulator_get_status() should return the actual physical state.