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.c...@samsung.com>
>> [Add missing linear_min_sel of S2MPU02 LDO regulators by Jonghwa Lee]
>> Signed-off-by: Jonghwa Lee <jonghwa3....@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.kozlow...@samsung.com>
Thanks for your review.
Best Regards,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
