[TRIVIAL PATCH 1/2] regulator: s5m8767: Define symbol for buck control mask

[TRIVIAL PATCH 1/2] regulator: s5m8767: Define symbol for buck control mask

[Krzysztof Kozlowski]

Replace hard-coded value for mask used in BUCKX_CTRL registers (for
BUCKX_EN field) with a symbol. This also removes two local variables.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/s5m8767.c         |   11 ++++++-----
 include/linux/mfd/samsung/s5m8767.h |    3 ++-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index aeb40aad0ae7..58eb7a6c45e6 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -217,7 +217,7 @@ static int s5m8767_reg_is_enabled(struct regulator_dev *rdev)
 {
 	struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
 	int ret, reg;
-	int mask = 0xc0, enable_ctrl;
+	int enable_ctrl;
 	unsigned int val;
 
 	ret = s5m8767_get_register(rdev, &reg, &enable_ctrl);
@@ -230,27 +230,28 @@ static int s5m8767_reg_is_enabled(struct regulator_dev *rdev)
 	if (ret)
 		return ret;
 
-	return (val & mask) == enable_ctrl;
+	return (val & S5M8767_ENCTRL_MASK) == enable_ctrl;
 }
 
 static int s5m8767_reg_enable(struct regulator_dev *rdev)
 {
 	struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
 	int ret, reg;
-	int mask = 0xc0, enable_ctrl;
+	int enable_ctrl;
 
 	ret = s5m8767_get_register(rdev, &reg, &enable_ctrl);
 	if (ret)
 		return ret;
 
-	return sec_reg_update(s5m8767->iodev, reg, enable_ctrl, mask);
+	return sec_reg_update(s5m8767->iodev, reg, enable_ctrl,
+			S5M8767_ENCTRL_MASK);
 }
 
 static int s5m8767_reg_disable(struct regulator_dev *rdev)
 {
 	struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
 	int ret, reg;
-	int  mask = 0xc0, enable_ctrl;
+	int mask = S5M8767_ENCTRL_MASK, enable_ctrl;
 
 	ret = s5m8767_get_register(rdev, &reg, &enable_ctrl);
 	if (ret)
diff --git a/include/linux/mfd/samsung/s5m8767.h b/include/linux/mfd/samsung/s5m8767.h
index 306a95fc558c..9198377ee859 100644
--- a/include/linux/mfd/samsung/s5m8767.h
+++ b/include/linux/mfd/samsung/s5m8767.h
@@ -183,6 +183,7 @@ enum s5m8767_regulators {
 	S5M8767_REG_MAX,
 };
 
-#define S5M8767_ENCTRL_SHIFT  6
+#define S5M8767_ENCTRL_SHIFT		6
+#define S5M8767_ENCTRL_MASK		(0x3 << S5M8767_ENCTRL_SHIFT)
 
 #endif /* __LINUX_MFD_S5M8767_H */
-- 
1.7.9.5

[PATCH 2/2] regulator: s5m8767: Fix driver probe fail on BUCK7/8 regulators

[Krzysztof Kozlowski]

The BUCK7 and BUCK8 were not supported by s5m8767 driver. If they were
configured in DTS then the driver probing would fail with EINVAL:
[    0.288474] VCC_SUB_1.35V: failed to apply 1350000uV constraint
[    0.288671] s5m8767-pmic s5m8767-pmic: regulator init failed for 35
[    0.294931] s5m8767-pmic: probe of s5m8767-pmic failed with error -22

This patch adds support for BUCK7 and BUCK8 regulators using standard
regmap get/set voltage sel functions.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/s5m8767.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index 58eb7a6c45e6..397917611ecd 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -120,8 +120,8 @@ static const struct sec_voltage_desc *reg_voltage_map[] = {
 	[S5M8767_BUCK4] = &buck_voltage_val2,
 	[S5M8767_BUCK5] = &buck_voltage_val1,
 	[S5M8767_BUCK6] = &buck_voltage_val1,
-	[S5M8767_BUCK7] = NULL,
-	[S5M8767_BUCK8] = NULL,
+	[S5M8767_BUCK7] = &buck_voltage_val3,
+	[S5M8767_BUCK8] = &buck_voltage_val3,
 	[S5M8767_BUCK9] = &buck_voltage_val3,
 };
 
@@ -418,9 +418,12 @@ static struct regulator_ops s5m8767_ops = {
 };
 
 static struct regulator_ops s5m8767_buck78_ops = {
+	.list_voltage		= regulator_list_voltage_linear,
 	.is_enabled		= s5m8767_reg_is_enabled,
 	.enable			= s5m8767_reg_enable,
 	.disable		= s5m8767_reg_disable,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
 };
 
 #define s5m8767_regulator_desc(_name) {		\
-- 
1.7.9.5

[PATCH 2/2] regulator: s5m8767: Fix driver probe fail on BUCK7/8 regulators

[Mark Brown]

On Tue, Dec 10, 2013 at 10:09:43AM +0100, Krzysztof Kozlowski wrote:
> The BUCK7 and BUCK8 were not supported by s5m8767 driver. If they were
> configured in DTS then the driver probing would fail with EINVAL:
> [    0.288474] VCC_SUB_1.35V: failed to apply 1350000uV constraint
> [    0.288671] s5m8767-pmic s5m8767-pmic: regulator init failed for 35
> [    0.294931] s5m8767-pmic: probe of s5m8767-pmic failed with error -22

> This patch adds support for BUCK7 and BUCK8 regulators using standard
> regmap get/set voltage sel functions.

I've applied these but this changelog doesn't entirely add up so I
rewrote it a bit.  The patch is implementing the get and set voltage
operations, the regulators were actually supported.  The errors were
being caused by the DT attempting to set the voltage for a fixed voltage
regulator which obviously won't work - I suspect that the DTs might not
be ideal, it seems common to overspecify voltages in DTs for some
reason.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131211/858fb28c/attachment.sig>

[TRIVIAL PATCH 1/2] regulator: s5m8767: Define symbol for buck control mask

[Sachin Kamat]

Hi Krzysztof,

On 10 December 2013 14:39, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:

>  static int s5m8767_reg_disable(struct regulator_dev *rdev)
>  {
>         struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
>         int ret, reg;
> -       int  mask = 0xc0, enable_ctrl;
> +       int mask = S5M8767_ENCTRL_MASK, enable_ctrl;

Couldn't we get rid of this mask variable here too?

Otherwise the patch looks good to me.

-- 
With warm regards,
Sachin

[TRIVIAL PATCH 1/2] regulator: s5m8767: Define symbol for buck control mask

[Krzysztof Kozlowski]

On Tue, 2013-12-10 at 14:48 +0530, Sachin Kamat wrote:
> Hi Krzysztof,
> 
> On 10 December 2013 14:39, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> 
> >  static int s5m8767_reg_disable(struct regulator_dev *rdev)
> >  {
> >         struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
> >         int ret, reg;
> > -       int  mask = 0xc0, enable_ctrl;
> > +       int mask = S5M8767_ENCTRL_MASK, enable_ctrl;
> 
> Couldn't we get rid of this mask variable here too?
> 
> Otherwise the patch looks good to me.

Not in this patch because it leads to warning:

drivers/regulator/s5m8767.c: In function ?s5m8767_reg_disable?:
drivers/regulator/s5m8767.c:259:2: warning: large integer implicitly truncated to unsigned type [-Woverflow]
drivers/regulator/s5m8767.c:253:6: warning: unused variable ?mask? [-Wunused-variable]

I plan to remove the sec_reg_*() helpers because they are confusing
and error-prone (mixing u8 and unsigned int). After using standard regmap
API this "mask" variable could be removed.

Best regards,
Krzysztof

[TRIVIAL PATCH 1/2] regulator: s5m8767: Define symbol for buck control mask

[Sachin Kamat]

On 10 December 2013 15:00, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> On Tue, 2013-12-10 at 14:48 +0530, Sachin Kamat wrote:
>> Hi Krzysztof,
>>
>> On 10 December 2013 14:39, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
>>
>> >  static int s5m8767_reg_disable(struct regulator_dev *rdev)
>> >  {
>> >         struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
>> >         int ret, reg;
>> > -       int  mask = 0xc0, enable_ctrl;
>> > +       int mask = S5M8767_ENCTRL_MASK, enable_ctrl;
>>
>> Couldn't we get rid of this mask variable here too?
>>
>> Otherwise the patch looks good to me.
>
> Not in this patch because it leads to warning:
>
> drivers/regulator/s5m8767.c: In function ?s5m8767_reg_disable?:
> drivers/regulator/s5m8767.c:259:2: warning: large integer implicitly truncated to unsigned type [-Woverflow]
> drivers/regulator/s5m8767.c:253:6: warning: unused variable ?mask? [-Wunused-variable]

I see.

>
> I plan to remove the sec_reg_*() helpers because they are confusing
> and error-prone (mixing u8 and unsigned int). After using standard regmap
> API this "mask" variable could be removed.

Sounds good.

For the subject patch,
Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>

-- 
With warm regards,
Sachin

[TRIVIAL PATCH 1/2] regulator: s5m8767: Define symbol for buck control mask

[Lee Jones]

On Tue, 10 Dec 2013, Krzysztof Kozlowski wrote:

> Replace hard-coded value for mask used in BUCKX_CTRL registers (for
> BUCKX_EN field) with a symbol. This also removes two local variables.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/regulator/s5m8767.c         |   11 ++++++-----
>  include/linux/mfd/samsung/s5m8767.h |    3 ++-
>  2 files changed, 8 insertions(+), 6 deletions(-)

For the MFD part:
  Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog