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

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi Krzysztof,

On 05/30/2014 05:23 PM, Krzysztof Kozlowski wrote:
> 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.

OK I'll modify it as your comment.

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


Thanks for your review.

Best Regards,
Chanwoo Choi