Re: [PATCHv2 2/3] regulator: s2mps11: Add support S2MPU02 regulator device

[Krzysztof Kozlowski <k.kozlowski@samsung.com>]

On pią, 2014-05-30 at 09:25 +0900, Chanwoo Choi wrote:
> This patch add S2MPU02 regulator device to existing S2MPS11 device driver
> because of little difference between S2MPS1x and S2MPU02. The S2MPU02
> regulator device includes LDO[1-28] and BUCK[1-7].
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> [Add missing linear_min_sel of S2MPU02 LDO regulators by Jonghwa Lee]
> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> ---
>  drivers/mfd/sec-core.c              |  26 +++
>  drivers/regulator/s2mps11.c         | 319 +++++++++++++++++++++++++++++++++---
>  include/linux/mfd/samsung/s2mpu02.h | 200 ++++++++++++++++++++++
>  3 files changed, 525 insertions(+), 20 deletions(-)
>  create mode 100644 include/linux/mfd/samsung/s2mpu02.h
> 

(...)

> diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
> index 02e2fb2..f7e5dab3d 100644
> --- a/drivers/regulator/s2mps11.c
> +++ b/drivers/regulator/s2mps11.c
> @@ -31,6 +31,7 @@
>  #include <linux/mfd/samsung/core.h>
>  #include <linux/mfd/samsung/s2mps11.h>
>  #include <linux/mfd/samsung/s2mps14.h>
> +#include <linux/mfd/samsung/s2mpu02.h>
>  
>  struct s2mps11_info {
>  	unsigned int rdev_num;
> @@ -40,11 +41,15 @@ struct s2mps11_info {
>  	int ramp_delay16;
>  	int ramp_delay7810;
>  	int ramp_delay9;
> +
> +	enum sec_device_type dev_type;
> +
>  	/*
> -	 * One bit for each S2MPS14 regulator whether the suspend mode
> +	 * One bit for each S2MPS14/S2MPU02 regulator whether the suspend mode
>  	 * was enabled.
>  	 */
> -	unsigned int s2mps14_suspend_state:30;
> +	unsigned long long s2mps14_suspend_state:35;
> +
>  	/* Array of size rdev_num with GPIO-s for external sleep control */
>  	int *ext_control_gpio;
>  };
> @@ -415,12 +420,24 @@ static int s2mps14_regulator_enable(struct regulator_dev *rdev)
>  	struct s2mps11_info *s2mps11 = rdev_get_drvdata(rdev);
>  	unsigned int val;
>  
> -	if (s2mps11->s2mps14_suspend_state & (1 << rdev_get_id(rdev)))
> -		val = S2MPS14_ENABLE_SUSPEND;
> -	else if (gpio_is_valid(s2mps11->ext_control_gpio[rdev_get_id(rdev)]))
> -		val = S2MPS14_ENABLE_EXT_CONTROL;
> -	else
> -		val = rdev->desc->enable_mask;
> +	switch (s2mps11->dev_type) {
> +	case S2MPS14X:
> +		if (s2mps11->s2mps14_suspend_state & (1 << rdev_get_id(rdev)))
> +			val = S2MPS14_ENABLE_SUSPEND;
> +		else if (gpio_is_valid(s2mps11->ext_control_gpio[rdev_get_id(rdev)]))
> +			val = S2MPS14_ENABLE_EXT_CONTROL;
> +		else
> +			val = rdev->desc->enable_mask;
> +		break;
> +	case S2MPU02:
> +		if (s2mps11->s2mps14_suspend_state & (1 << rdev_get_id(rdev)))
> +			val = S2MPU02_ENABLE_SUSPEND;
> +		else
> +			val = rdev->desc->enable_mask;
> +		break;
> +	default:
> +		return -EINVAL;
> +	};
>  
>  	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
>  			rdev->desc->enable_mask, val);
> @@ -431,16 +448,42 @@ static int s2mps14_regulator_set_suspend_disable(struct regulator_dev *rdev)
>  	int ret;
>  	unsigned int val;
>  	struct s2mps11_info *s2mps11 = rdev_get_drvdata(rdev);
> +	int rdev_id = rdev_get_id(rdev);
>  
> -	/* LDO3 should be always on and does not support suspend mode */
> -	if (rdev_get_id(rdev) == S2MPS14_LDO3)
> -		return 0;
> +	/* Below LDO should be always on or does not support suspend mode. */
> +	switch (s2mps11->dev_type) {
> +	case S2MPS14X:
> +		switch (rdev_id) {
> +		case S2MPS14_LDO3:
> +			return 0;
> +		};
> +	case S2MPU02:
> +		switch (rdev_id) {
> +		case S2MPU02_LDO13:
> +		case S2MPU02_LDO14:
> +		case S2MPU02_LDO15:
> +		case S2MPU02_LDO16:
> +		case S2MPU02_LDO17:
> +		case S2MPU02_BUCK7:
> +			return 0;
> +		};
> +	default:
> +		return -EINVAL;
> +	};
>  
>  	ret = regmap_read(rdev->regmap, rdev->desc->enable_reg, &val);
>  	if (ret < 0)
>  		return ret;
>  
> -	s2mps11->s2mps14_suspend_state |= (1 << rdev_get_id(rdev));
> +	switch (s2mps11->dev_type) {
> +	case S2MPS14X:
> +	case S2MPU02:
> +		s2mps11->s2mps14_suspend_state |= (1 << rdev_get_id(rdev));
> +		break;
> +	default:
> +		return -EINVAL;
> +	};
> +

I think this change is not needed. You are actually returning EINVAL on
wrong devices in switch above so:
	s2mps11->s2mps14_suspend_state |= (1 << rdev_get_id(rdev));
would be safe and sufficient.

Anyway:
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof