Messages in this thread | | | Subject | Re: [PATCHv2 2/3] regulator: s2mps11: Add support S2MPU02 regulator device | From | Krzysztof Kozlowski <> | Date | Fri, 30 May 2014 10:23:25 +0200 |
| |
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
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |