Re: [PATCH] regulator: ab8500: Fix get_mode for shared mode regulators

["Bengt Jönsson" <bengt.g.jonsson@stericsson.com>]

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 <axel.lin@ingics.com>
> ---
>   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,