[PATCH v4 0/7] regulator: Parse ena_gpio in core, add GPIO to max77686

[PATCH v4 0/7] regulator: Parse ena_gpio in core, add GPIO to max77686

[Krzysztof Kozlowski]

Hi,


The patchset generalizes the ena_gpio bindings and finally adds GPIO
support to the max77686 driver. Adding GPIO to max77686 allows removal of
fixed regulators from DTS which duplicate the description of hardware.

Rationale behind adding ena-gpios to regulator core
===================================================
Drivers often add custom DTS properties for parsing the GPIO for
regulator enable control. Some of them don't have to do this in a
special custom way and would work with a generic approach (e.g. S5M8767,
S2MPS1x, MAX77686). As such drivers have to do this on their own,
multiple different bindings are added and each driver duplicates similar
code.

Add a generic binding so the regulator core will do this work for
drivers. This should offload some work from drivers and also limit
creation of new custom properties for GPIO control.

Changes since v3
================
1. Regulator cleanup patches were applied by Mark, drop them.
2. Re-work idea by adding generic ena-gpios binding.

Changes since v2
================
Re-work the board file support removal after Javier's comments: use
new DT style parsing. This imposes a lot of changes.
1. Add "of_compatible" for regulator drivers.
2. Provide backward compatibility if such "of_compatible" is not present.
   The driver will search for regulators node and use it as dev->of_node.
   Everything should be bisect-friendly.
3. New patches: 1, 2, 3, 5, 13 and 14.
4. Because of new style DT parsing it is much easier to put "gpio"
   properties in regulators top node (not in each regulator).
   This will be also new-gpio-lib friendly.

Changes since v1
================
1. Add patch: 1/8 "regulator: max77686: Consistently index opmode
   array by rdev id".
2. Remove patch "regulator: max77686: Make regulator_desc array
   const" (applied).
3. Re-work patches removing from regulators board file support (2/8
   and 3/8). Parse regulators with of_regulator_match() at once, remove
   num_regulators.
4. Patch 4/8: Add depends on OF to mfd/Kconfig. Add Javier's
   reviewed-by.
5. Patch 5/8: Add Javier's reviewed-by.
6. Patch 6/8: Add depends on GPIOLIB to regulator/Kconfig. Rename
   "external control" to "GPIO control" and "ext_control_gpios" to
   simpler "gpios".
   I tried to use new GPIO API but it ended with more problems
   https://lkml.org/lkml/2014/10/29/239


Patchset is rebased on next-20141114.

Best regards,
Krzysztof

Krzysztof Kozlowski (7):
  mfd: max77686/802: Remove support for board files
  regulator: dt-bindings: Document the ena-gpios property
  regulator: of: Parse ena-gpios property from DTS
  regulator: Use ena_gpio supplied with generic regulator bindings
  regulator: max77686: Add GPIO control
  mfd/regulator: dt-bindings: max77686: Document gpio properties
  ARM: dts: exynos4412-trats: Switch max77686 regulators to GPIO control

 Documentation/devicetree/bindings/mfd/max77686.txt | 14 +++-
 .../devicetree/bindings/regulator/regulator.txt    |  4 +
 arch/arm/boot/dts/exynos4412-trats2.dts            | 25 ++----
 drivers/mfd/Kconfig                                |  1 +
 drivers/mfd/max77686.c                             | 23 ------
 drivers/regulator/core.c                           | 96 ++++++++++++++++------
 drivers/regulator/max77686.c                       | 58 +++++++++++--
 drivers/regulator/of_regulator.c                   | 11 +++
 include/linux/mfd/max77686-private.h               |  1 -
 include/linux/mfd/max77686.h                       | 28 -------
 include/linux/regulator/driver.h                   |  5 ++
 include/linux/regulator/machine.h                  | 13 +++
 12 files changed, 178 insertions(+), 101 deletions(-)

-- 
1.9.1

[PATCH v4 0/7] regulator: Parse ena_gpio in core, add GPIO to max77686

[Krzysztof Kozlowski]

Hi,


The patchset generalizes the ena_gpio bindings and finally adds GPIO
support to the max77686 driver. Adding GPIO to max77686 allows removal of
fixed regulators from DTS which duplicate the description of hardware.

Rationale behind adding ena-gpios to regulator core
===================================================
Drivers often add custom DTS properties for parsing the GPIO for
regulator enable control. Some of them don't have to do this in a
special custom way and would work with a generic approach (e.g. S5M8767,
S2MPS1x, MAX77686). As such drivers have to do this on their own,
multiple different bindings are added and each driver duplicates similar
code.

Add a generic binding so the regulator core will do this work for
drivers. This should offload some work from drivers and also limit
creation of new custom properties for GPIO control.

Changes since v3
================
1. Regulator cleanup patches were applied by Mark, drop them.
2. Re-work idea by adding generic ena-gpios binding.

Changes since v2
================
Re-work the board file support removal after Javier's comments: use
new DT style parsing. This imposes a lot of changes.
1. Add "of_compatible" for regulator drivers.
2. Provide backward compatibility if such "of_compatible" is not present.
   The driver will search for regulators node and use it as dev->of_node.
   Everything should be bisect-friendly.
3. New patches: 1, 2, 3, 5, 13 and 14.
4. Because of new style DT parsing it is much easier to put "gpio"
   properties in regulators top node (not in each regulator).
   This will be also new-gpio-lib friendly.

Changes since v1
================
1. Add patch: 1/8 "regulator: max77686: Consistently index opmode
   array by rdev id".
2. Remove patch "regulator: max77686: Make regulator_desc array
   const" (applied).
3. Re-work patches removing from regulators board file support (2/8
   and 3/8). Parse regulators with of_regulator_match() at once, remove
   num_regulators.
4. Patch 4/8: Add depends on OF to mfd/Kconfig. Add Javier's
   reviewed-by.
5. Patch 5/8: Add Javier's reviewed-by.
6. Patch 6/8: Add depends on GPIOLIB to regulator/Kconfig. Rename
   "external control" to "GPIO control" and "ext_control_gpios" to
   simpler "gpios".
   I tried to use new GPIO API but it ended with more problems
   https://lkml.org/lkml/2014/10/29/239


Patchset is rebased on next-20141114.

Best regards,
Krzysztof

Krzysztof Kozlowski (7):
  mfd: max77686/802: Remove support for board files
  regulator: dt-bindings: Document the ena-gpios property
  regulator: of: Parse ena-gpios property from DTS
  regulator: Use ena_gpio supplied with generic regulator bindings
  regulator: max77686: Add GPIO control
  mfd/regulator: dt-bindings: max77686: Document gpio properties
  ARM: dts: exynos4412-trats: Switch max77686 regulators to GPIO control

 Documentation/devicetree/bindings/mfd/max77686.txt | 14 +++-
 .../devicetree/bindings/regulator/regulator.txt    |  4 +
 arch/arm/boot/dts/exynos4412-trats2.dts            | 25 ++----
 drivers/mfd/Kconfig                                |  1 +
 drivers/mfd/max77686.c                             | 23 ------
 drivers/regulator/core.c                           | 96 ++++++++++++++++------
 drivers/regulator/max77686.c                       | 58 +++++++++++--
 drivers/regulator/of_regulator.c                   | 11 +++
 include/linux/mfd/max77686-private.h               |  1 -
 include/linux/mfd/max77686.h                       | 28 -------
 include/linux/regulator/driver.h                   |  5 ++
 include/linux/regulator/machine.h                  | 13 +++
 12 files changed, 178 insertions(+), 101 deletions(-)

-- 
1.9.1

[PATCH v4 1/7] mfd: max77686/802: Remove support for board files

[Krzysztof Kozlowski]

The driver is used only on Exynos based boards with DTS support.
After removal of board file support from max77686 and max77802 regulator
drivers, the MFD driver can be converted to DTS-only version. This
simplifies a little the code:
1. No dead (unused) entries in platform_data structure.
2. More code removed.
3. Regulator driver does not depend on allocated memory
   from MFD driver.
4. It makes also easier extending the regulator driver.

Add to the max77686 MFD driver dependency on CONFIG_OF because without
DTS the regulator drivers (max77686 and max77802) won't bind.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/mfd/Kconfig                  |  1 +
 drivers/mfd/max77686.c               | 23 -----------------------
 include/linux/mfd/max77686-private.h |  1 -
 include/linux/mfd/max77686.h         | 28 ----------------------------
 4 files changed, 1 insertion(+), 52 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 72d38081f779..2bb0789a828b 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -412,6 +412,7 @@ config MFD_MAX14577
 config MFD_MAX77686
 	bool "Maxim Semiconductor MAX77686/802 PMIC Support"
 	depends on I2C=y
+	depends on OF
 	select MFD_CORE
 	select REGMAP_I2C
 	select REGMAP_IRQ
diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index 929795eae9fc..3da237afacde 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -205,24 +205,10 @@ static const struct of_device_id max77686_pmic_dt_match[] = {
 	{ },
 };
 
-static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device
-								  *dev)
-{
-	struct max77686_platform_data *pd;
-
-	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
-	if (!pd)
-		return NULL;
-
-	dev->platform_data = pd;
-	return pd;
-}
-
 static int max77686_i2c_probe(struct i2c_client *i2c,
 			      const struct i2c_device_id *id)
 {
 	struct max77686_dev *max77686 = NULL;
-	struct max77686_platform_data *pdata = dev_get_platdata(&i2c->dev);
 	const struct of_device_id *match;
 	unsigned int data;
 	int ret = 0;
@@ -233,14 +219,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 	const struct mfd_cell *cells;
 	int n_devs;
 
-	if (IS_ENABLED(CONFIG_OF) && i2c->dev.of_node && !pdata)
-		pdata = max77686_i2c_parse_dt_pdata(&i2c->dev);
-
-	if (!pdata) {
-		dev_err(&i2c->dev, "No platform data found.\n");
-		return -EINVAL;
-	}
-
 	max77686 = devm_kzalloc(&i2c->dev,
 				sizeof(struct max77686_dev), GFP_KERNEL);
 	if (!max77686)
@@ -259,7 +237,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 	max77686->dev = &i2c->dev;
 	max77686->i2c = i2c;
 
-	max77686->wakeup = pdata->wakeup;
 	max77686->irq = i2c->irq;
 
 	if (max77686->type == TYPE_MAX77686) {
diff --git a/include/linux/mfd/max77686-private.h b/include/linux/mfd/max77686-private.h
index 960b92ad450d..f5043490d67c 100644
--- a/include/linux/mfd/max77686-private.h
+++ b/include/linux/mfd/max77686-private.h
@@ -447,7 +447,6 @@ struct max77686_dev {
 	struct regmap_irq_chip_data *rtc_irq_data;
 
 	int irq;
-	bool wakeup;
 	struct mutex irqlock;
 	int irq_masks_cur[MAX77686_IRQ_GROUP_NR];
 	int irq_masks_cache[MAX77686_IRQ_GROUP_NR];
diff --git a/include/linux/mfd/max77686.h b/include/linux/mfd/max77686.h
index 553f7d09258a..bb995ab9a575 100644
--- a/include/linux/mfd/max77686.h
+++ b/include/linux/mfd/max77686.h
@@ -119,12 +119,6 @@ enum max77802_regulators {
 	MAX77802_REG_MAX,
 };
 
-struct max77686_regulator_data {
-	int id;
-	struct regulator_init_data *initdata;
-	struct device_node *of_node;
-};
-
 enum max77686_opmode {
 	MAX77686_OPMODE_NORMAL,
 	MAX77686_OPMODE_LP,
@@ -136,26 +130,4 @@ struct max77686_opmode_data {
 	int mode;
 };
 
-struct max77686_platform_data {
-	int ono;
-	int wakeup;
-
-	/* ---- PMIC ---- */
-	struct max77686_regulator_data *regulators;
-	int num_regulators;
-
-	struct max77686_opmode_data *opmode_data;
-
-	/*
-	 * GPIO-DVS feature is not enabled with the current version of
-	 * MAX77686 driver. Buck2/3/4_voltages[0] is used as the default
-	 * voltage at probe. DVS/SELB gpios are set as OUTPUT-LOW.
-	 */
-	int buck234_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */
-	int buck234_gpio_selb[3]; /* [0]SELB2, [1]SELB3, [2]SELB4 */
-	unsigned int buck2_voltage[8]; /* buckx_voltage in uV */
-	unsigned int buck3_voltage[8];
-	unsigned int buck4_voltage[8];
-};
-
 #endif /* __LINUX_MFD_MAX77686_H */
-- 
1.9.1

[PATCH v4 1/7] mfd: max77686/802: Remove support for board files

[Krzysztof Kozlowski]

The driver is used only on Exynos based boards with DTS support.
After removal of board file support from max77686 and max77802 regulator
drivers, the MFD driver can be converted to DTS-only version. This
simplifies a little the code:
1. No dead (unused) entries in platform_data structure.
2. More code removed.
3. Regulator driver does not depend on allocated memory
   from MFD driver.
4. It makes also easier extending the regulator driver.

Add to the max77686 MFD driver dependency on CONFIG_OF because without
DTS the regulator drivers (max77686 and max77802) won't bind.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/mfd/Kconfig                  |  1 +
 drivers/mfd/max77686.c               | 23 -----------------------
 include/linux/mfd/max77686-private.h |  1 -
 include/linux/mfd/max77686.h         | 28 ----------------------------
 4 files changed, 1 insertion(+), 52 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 72d38081f779..2bb0789a828b 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -412,6 +412,7 @@ config MFD_MAX14577
 config MFD_MAX77686
 	bool "Maxim Semiconductor MAX77686/802 PMIC Support"
 	depends on I2C=y
+	depends on OF
 	select MFD_CORE
 	select REGMAP_I2C
 	select REGMAP_IRQ
diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index 929795eae9fc..3da237afacde 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -205,24 +205,10 @@ static const struct of_device_id max77686_pmic_dt_match[] = {
 	{ },
 };
 
-static struct max77686_platform_data *max77686_i2c_parse_dt_pdata(struct device
-								  *dev)
-{
-	struct max77686_platform_data *pd;
-
-	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
-	if (!pd)
-		return NULL;
-
-	dev->platform_data = pd;
-	return pd;
-}
-
 static int max77686_i2c_probe(struct i2c_client *i2c,
 			      const struct i2c_device_id *id)
 {
 	struct max77686_dev *max77686 = NULL;
-	struct max77686_platform_data *pdata = dev_get_platdata(&i2c->dev);
 	const struct of_device_id *match;
 	unsigned int data;
 	int ret = 0;
@@ -233,14 +219,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 	const struct mfd_cell *cells;
 	int n_devs;
 
-	if (IS_ENABLED(CONFIG_OF) && i2c->dev.of_node && !pdata)
-		pdata = max77686_i2c_parse_dt_pdata(&i2c->dev);
-
-	if (!pdata) {
-		dev_err(&i2c->dev, "No platform data found.\n");
-		return -EINVAL;
-	}
-
 	max77686 = devm_kzalloc(&i2c->dev,
 				sizeof(struct max77686_dev), GFP_KERNEL);
 	if (!max77686)
@@ -259,7 +237,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 	max77686->dev = &i2c->dev;
 	max77686->i2c = i2c;
 
-	max77686->wakeup = pdata->wakeup;
 	max77686->irq = i2c->irq;
 
 	if (max77686->type == TYPE_MAX77686) {
diff --git a/include/linux/mfd/max77686-private.h b/include/linux/mfd/max77686-private.h
index 960b92ad450d..f5043490d67c 100644
--- a/include/linux/mfd/max77686-private.h
+++ b/include/linux/mfd/max77686-private.h
@@ -447,7 +447,6 @@ struct max77686_dev {
 	struct regmap_irq_chip_data *rtc_irq_data;
 
 	int irq;
-	bool wakeup;
 	struct mutex irqlock;
 	int irq_masks_cur[MAX77686_IRQ_GROUP_NR];
 	int irq_masks_cache[MAX77686_IRQ_GROUP_NR];
diff --git a/include/linux/mfd/max77686.h b/include/linux/mfd/max77686.h
index 553f7d09258a..bb995ab9a575 100644
--- a/include/linux/mfd/max77686.h
+++ b/include/linux/mfd/max77686.h
@@ -119,12 +119,6 @@ enum max77802_regulators {
 	MAX77802_REG_MAX,
 };
 
-struct max77686_regulator_data {
-	int id;
-	struct regulator_init_data *initdata;
-	struct device_node *of_node;
-};
-
 enum max77686_opmode {
 	MAX77686_OPMODE_NORMAL,
 	MAX77686_OPMODE_LP,
@@ -136,26 +130,4 @@ struct max77686_opmode_data {
 	int mode;
 };
 
-struct max77686_platform_data {
-	int ono;
-	int wakeup;
-
-	/* ---- PMIC ---- */
-	struct max77686_regulator_data *regulators;
-	int num_regulators;
-
-	struct max77686_opmode_data *opmode_data;
-
-	/*
-	 * GPIO-DVS feature is not enabled with the current version of
-	 * MAX77686 driver. Buck2/3/4_voltages[0] is used as the default
-	 * voltage at probe. DVS/SELB gpios are set as OUTPUT-LOW.
-	 */
-	int buck234_gpio_dvs[3]; /* GPIO of [0]DVS1, [1]DVS2, [2]DVS3 */
-	int buck234_gpio_selb[3]; /* [0]SELB2, [1]SELB3, [2]SELB4 */
-	unsigned int buck2_voltage[8]; /* buckx_voltage in uV */
-	unsigned int buck3_voltage[8];
-	unsigned int buck4_voltage[8];
-};
-
 #endif /* __LINUX_MFD_MAX77686_H */
-- 
1.9.1

Re: [PATCH v4 1/7] mfd: max77686/802: Remove support for board files

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]

On Thu, Nov 27, 2014 at 12:20:47PM +0100, Krzysztof Kozlowski wrote:
> The driver is used only on Exynos based boards with DTS support.
> After removal of board file support from max77686 and max77802 regulator
> drivers, the MFD driver can be converted to DTS-only version. This
> simplifies a little the code:

Why is this a dependency for new features in the regulator core?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v4 1/7] mfd: max77686/802: Remove support for board files

[Mark Brown]

On Thu, Nov 27, 2014 at 12:20:47PM +0100, Krzysztof Kozlowski wrote:
> The driver is used only on Exynos based boards with DTS support.
> After removal of board file support from max77686 and max77802 regulator
> drivers, the MFD driver can be converted to DTS-only version. This
> simplifies a little the code:

Why is this a dependency for new features in the regulator core?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/3d80f30f/attachment.sig>

Re: [PATCH v4 1/7] mfd: max77686/802: Remove support for board files

[Krzysztof Kozlowski]

On czw, 2014-11-27 at 13:03 +0000, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 12:20:47PM +0100, Krzysztof Kozlowski wrote:
> > The driver is used only on Exynos based boards with DTS support.
> > After removal of board file support from max77686 and max77802 regulator
> > drivers, the MFD driver can be converted to DTS-only version. This
> > simplifies a little the code:
> 
> Why is this a dependency for new features in the regulator core?
My mistake, I based this patchset on regulator cleanup but this MFD
change is not needed for it. It can be skipped here. Sorry for the
noise.

Best regards,
Krzysztof

[PATCH v4 1/7] mfd: max77686/802: Remove support for board files

[Krzysztof Kozlowski]

On czw, 2014-11-27 at 13:03 +0000, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 12:20:47PM +0100, Krzysztof Kozlowski wrote:
> > The driver is used only on Exynos based boards with DTS support.
> > After removal of board file support from max77686 and max77802 regulator
> > drivers, the MFD driver can be converted to DTS-only version. This
> > simplifies a little the code:
> 
> Why is this a dependency for new features in the regulator core?
My mistake, I based this patchset on regulator cleanup but this MFD
change is not needed for it. It can be skipped here. Sorry for the
noise.

Best regards,
Krzysztof

[PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Krzysztof Kozlowski]

Document new properties for regulators (ena-gpios and
ena-gpio-open-drain) for enabling control over GPIO.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 Documentation/devicetree/bindings/regulator/regulator.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index 4e7ed762b3bb..dad57b3badc7 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -30,6 +30,10 @@ Optional properties:
 	- regulator-off-in-suspend: regulator should be off in suspend state.
 	- regulator-suspend-microvolt: regulator should be set to this voltage
 	  in suspend.
+- ena-gpios: GPIO to use for enable control. Actual implementation depends
+  on regulator driver. The bindings documentation for given driver describes
+  which regulator actually supports it.
+- ena-gpio-open-drain: GPIO is open drain type.
 
 Deprecated properties:
 - regulator-compatible: If a regulator chip contains multiple
-- 
1.9.1

[PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Krzysztof Kozlowski]

Document new properties for regulators (ena-gpios and
ena-gpio-open-drain) for enabling control over GPIO.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 Documentation/devicetree/bindings/regulator/regulator.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index 4e7ed762b3bb..dad57b3badc7 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -30,6 +30,10 @@ Optional properties:
 	- regulator-off-in-suspend: regulator should be off in suspend state.
 	- regulator-suspend-microvolt: regulator should be set to this voltage
 	  in suspend.
+- ena-gpios: GPIO to use for enable control. Actual implementation depends
+  on regulator driver. The bindings documentation for given driver describes
+  which regulator actually supports it.
+- ena-gpio-open-drain: GPIO is open drain type.
 
 Deprecated properties:
 - regulator-compatible: If a regulator chip contains multiple
-- 
1.9.1

Re: [PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

On Thu, Nov 27, 2014 at 12:20:48PM +0100, Krzysztof Kozlowski wrote:

> +- ena-gpios: GPIO to use for enable control. Actual implementation depends
> +  on regulator driver. The bindings documentation for given driver describes
> +  which regulator actually supports it.
> +- ena-gpio-open-drain: GPIO is open drain type.

I'm relly not a big fan of adding a fixed name property here with no
override capability, it means that the naming won't reflect the specific
regulator design so closely and in practice for many of the PMICs the
GPIO control can do rather more than just control enables and supports
reprogramming.  The latter case where we've got a signal which can
sometimes be simply and enable but sometimes more makes it especially
worrying to have the property always be there, it's something that might
work in some configurations but could easily be broken if we try to
exploit more advanced functionality (things also triggering other
configuration changes at the same time).

Factoring out the code is good but it seems better to have it be
something which drivers can control, for example by having them
explicitly specify a property name to use or perhaps a flag to enable
the default name.

We also need an invert option.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Mark Brown]

On Thu, Nov 27, 2014 at 12:20:48PM +0100, Krzysztof Kozlowski wrote:

> +- ena-gpios: GPIO to use for enable control. Actual implementation depends
> +  on regulator driver. The bindings documentation for given driver describes
> +  which regulator actually supports it.
> +- ena-gpio-open-drain: GPIO is open drain type.

I'm relly not a big fan of adding a fixed name property here with no
override capability, it means that the naming won't reflect the specific
regulator design so closely and in practice for many of the PMICs the
GPIO control can do rather more than just control enables and supports
reprogramming.  The latter case where we've got a signal which can
sometimes be simply and enable but sometimes more makes it especially
worrying to have the property always be there, it's something that might
work in some configurations but could easily be broken if we try to
exploit more advanced functionality (things also triggering other
configuration changes at the same time).

Factoring out the code is good but it seems better to have it be
something which drivers can control, for example by having them
explicitly specify a property name to use or perhaps a flag to enable
the default name.

We also need an invert option.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/310d8ea9/attachment.sig>

Re: [PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Krzysztof Kozlowski]

On czw, 2014-11-27 at 18:30 +0000, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 12:20:48PM +0100, Krzysztof Kozlowski wrote:
> 
> > +- ena-gpios: GPIO to use for enable control. Actual implementation depends
> > +  on regulator driver. The bindings documentation for given driver describes
> > +  which regulator actually supports it.
> > +- ena-gpio-open-drain: GPIO is open drain type.
> 
> I'm relly not a big fan of adding a fixed name property here with no
> override capability, it means that the naming won't reflect the specific
> regulator design so closely and in practice for many of the PMICs the
> GPIO control can do rather more than just control enables and supports
> reprogramming.  The latter case where we've got a signal which can
> sometimes be simply and enable but sometimes more makes it especially
> worrying to have the property always be there, it's something that might
> work in some configurations but could easily be broken if we try to
> exploit more advanced functionality (things also triggering other
> configuration changes at the same time).

I understand your concerns here however I didn't want to overengineer
this. Is the same GPIO (on more complex PMICs) used in different
contexts? Like enable control and something more in the same time?

For example the S5M8767 uses different GPIOs for:
1. enable control - one GPIO per regulator,
2. voltage selection (DVS) - 3 GPIOs total,
so there is no benefit in merging this into one common regulator code.

> Factoring out the code is good but it seems better to have it be
> something which drivers can control, for example by having them
> explicitly specify a property name to use or perhaps a flag to enable
> the default name.

Something like:
 struct regulator_desc desc {
 	.name		= "LDO1
 	.of_match	= of_match_ptr("LDO1"),
 	.regulators_node = of_match_ptr("voltage-regulators"),
 	.ops		= &max77686_ldo_ops,
+	.of_ena_gpio	= of_match_ptr("ena-gpios"),
 ...
 }
?


> We also need an invert option.

This is parsed from gpio specifier in DTS: the flags from
include/dt-bindings/gpio/gpio.h.

Thank you for feedback,
Krzysztof

[PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Krzysztof Kozlowski]

On czw, 2014-11-27 at 18:30 +0000, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 12:20:48PM +0100, Krzysztof Kozlowski wrote:
> 
> > +- ena-gpios: GPIO to use for enable control. Actual implementation depends
> > +  on regulator driver. The bindings documentation for given driver describes
> > +  which regulator actually supports it.
> > +- ena-gpio-open-drain: GPIO is open drain type.
> 
> I'm relly not a big fan of adding a fixed name property here with no
> override capability, it means that the naming won't reflect the specific
> regulator design so closely and in practice for many of the PMICs the
> GPIO control can do rather more than just control enables and supports
> reprogramming.  The latter case where we've got a signal which can
> sometimes be simply and enable but sometimes more makes it especially
> worrying to have the property always be there, it's something that might
> work in some configurations but could easily be broken if we try to
> exploit more advanced functionality (things also triggering other
> configuration changes at the same time).

I understand your concerns here however I didn't want to overengineer
this. Is the same GPIO (on more complex PMICs) used in different
contexts? Like enable control and something more in the same time?

For example the S5M8767 uses different GPIOs for:
1. enable control - one GPIO per regulator,
2. voltage selection (DVS) - 3 GPIOs total,
so there is no benefit in merging this into one common regulator code.

> Factoring out the code is good but it seems better to have it be
> something which drivers can control, for example by having them
> explicitly specify a property name to use or perhaps a flag to enable
> the default name.

Something like:
 struct regulator_desc desc {
 	.name		= "LDO1
 	.of_match	= of_match_ptr("LDO1"),
 	.regulators_node = of_match_ptr("voltage-regulators"),
 	.ops		= &max77686_ldo_ops,
+	.of_ena_gpio	= of_match_ptr("ena-gpios"),
 ...
 }
?


> We also need an invert option.

This is parsed from gpio specifier in DTS: the flags from
include/dt-bindings/gpio/gpio.h.

Thank you for feedback,
Krzysztof

Re: [PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Krzysztof Kozlowski]

On czw, 2014-11-27 at 18:30 +0000, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 12:20:48PM +0100, Krzysztof Kozlowski wrote:
> 
> > +- ena-gpios: GPIO to use for enable control. Actual implementation depends
> > +  on regulator driver. The bindings documentation for given driver describes
> > +  which regulator actually supports it.
> > +- ena-gpio-open-drain: GPIO is open drain type.
> 
> I'm relly not a big fan of adding a fixed name property here with no
> override capability, it means that the naming won't reflect the specific
> regulator design so closely and in practice for many of the PMICs the
> GPIO control can do rather more than just control enables and supports
> reprogramming.  The latter case where we've got a signal which can
> sometimes be simply and enable but sometimes more makes it especially
> worrying to have the property always be there, it's something that might
> work in some configurations but could easily be broken if we try to
> exploit more advanced functionality (things also triggering other
> configuration changes at the same time).

I understand your concerns here however I didn't want to overengineer
this. Is the same GPIO (on more complex PMICs) used in different
contexts? Like enable control and something more in the same time?

For example the S5M8767 uses different GPIOs for:
1. enable control - one GPIO per regulator,
2. voltage selection (DVS) - 3 GPIOs total,
so there is no benefit in merging this into one common regulator code.

> Factoring out the code is good but it seems better to have it be
> something which drivers can control, for example by having them
> explicitly specify a property name to use or perhaps a flag to enable
> the default name.

Something like:
 struct regulator_desc desc {
 	.name		= "LDO1
 	.of_match	= of_match_ptr("LDO1"),
 	.regulators_node = of_match_ptr("voltage-regulators"),
 	.ops		= &max77686_ldo_ops,
+	.of_ena_gpio	= of_match_ptr("ena-gpios"),
 ...
 }
?


> We also need an invert option.

This is parsed from gpio specifier in DTS: the flags from
include/dt-bindings/gpio/gpio.h.

Thank you for feedback,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:

> I understand your concerns here however I didn't want to overengineer
> this. Is the same GPIO (on more complex PMICs) used in different
> contexts? Like enable control and something more in the same time?

Yes, and it's often reprogrammable at runtime.

> Something like:
>  struct regulator_desc desc {
>  	.name		= "LDO1
>  	.of_match	= of_match_ptr("LDO1"),
>  	.regulators_node = of_match_ptr("voltage-regulators"),
>  	.ops		= &max77686_ldo_ops,
> +	.of_ena_gpio	= of_match_ptr("ena-gpios"),
>  ...
>  }

Yes, and note that this also means existing bindings can use the core
code.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Mark Brown]

On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:

> I understand your concerns here however I didn't want to overengineer
> this. Is the same GPIO (on more complex PMICs) used in different
> contexts? Like enable control and something more in the same time?

Yes, and it's often reprogrammable at runtime.

> Something like:
>  struct regulator_desc desc {
>  	.name		= "LDO1
>  	.of_match	= of_match_ptr("LDO1"),
>  	.regulators_node = of_match_ptr("voltage-regulators"),
>  	.ops		= &max77686_ldo_ops,
> +	.of_ena_gpio	= of_match_ptr("ena-gpios"),
>  ...
>  }

Yes, and note that this also means existing bindings can use the core
code.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141128/beefc92c/attachment.sig>

Re: [PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Krzysztof Kozlowski]

On pią, 2014-11-28 at 11:21 +0000, Mark Brown wrote:
> On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:
> 
> > I understand your concerns here however I didn't want to overengineer
> > this. Is the same GPIO (on more complex PMICs) used in different
> > contexts? Like enable control and something more in the same time?
> 
> Yes, and it's often reprogrammable at runtime.

I have doubts if generalized code could support such configuration...

> 
> > Something like:
> >  struct regulator_desc desc {
> >  	.name		= "LDO1
> >  	.of_match	= of_match_ptr("LDO1"),
> >  	.regulators_node = of_match_ptr("voltage-regulators"),
> >  	.ops		= &max77686_ldo_ops,
> > +	.of_ena_gpio	= of_match_ptr("ena-gpios"),
> >  ...
> >  }
> 
> Yes, and note that this also means existing bindings can use the core
> code.

Thanks for idea,
Krzysztof

[PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Krzysztof Kozlowski]

On pi?, 2014-11-28 at 11:21 +0000, Mark Brown wrote:
> On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:
> 
> > I understand your concerns here however I didn't want to overengineer
> > this. Is the same GPIO (on more complex PMICs) used in different
> > contexts? Like enable control and something more in the same time?
> 
> Yes, and it's often reprogrammable at runtime.

I have doubts if generalized code could support such configuration...

> 
> > Something like:
> >  struct regulator_desc desc {
> >  	.name		= "LDO1
> >  	.of_match	= of_match_ptr("LDO1"),
> >  	.regulators_node = of_match_ptr("voltage-regulators"),
> >  	.ops		= &max77686_ldo_ops,
> > +	.of_ena_gpio	= of_match_ptr("ena-gpios"),
> >  ...
> >  }
> 
> Yes, and note that this also means existing bindings can use the core
> code.

Thanks for idea,
Krzysztof

Re: [PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Krzysztof Kozlowski]

On pią, 2014-11-28 at 11:21 +0000, Mark Brown wrote:
> On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:
> 
> > I understand your concerns here however I didn't want to overengineer
> > this. Is the same GPIO (on more complex PMICs) used in different
> > contexts? Like enable control and something more in the same time?
> 
> Yes, and it's often reprogrammable at runtime.

I have doubts if generalized code could support such configuration...

> 
> > Something like:
> >  struct regulator_desc desc {
> >  	.name		= "LDO1
> >  	.of_match	= of_match_ptr("LDO1"),
> >  	.regulators_node = of_match_ptr("voltage-regulators"),
> >  	.ops		= &max77686_ldo_ops,
> > +	.of_ena_gpio	= of_match_ptr("ena-gpios"),
> >  ...
> >  }
> 
> Yes, and note that this also means existing bindings can use the core
> code.

Thanks for idea,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

On Fri, Nov 28, 2014 at 12:54:27PM +0100, Krzysztof Kozlowski wrote:
> On pią, 2014-11-28 at 11:21 +0000, Mark Brown wrote:
> > On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:

> > > I understand your concerns here however I didn't want to overengineer
> > > this. Is the same GPIO (on more complex PMICs) used in different
> > > contexts? Like enable control and something more in the same time?

> > Yes, and it's often reprogrammable at runtime.

> I have doubts if generalized code could support such configuration...

That's pretty much my point.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

[Mark Brown]

On Fri, Nov 28, 2014 at 12:54:27PM +0100, Krzysztof Kozlowski wrote:
> On pi?, 2014-11-28 at 11:21 +0000, Mark Brown wrote:
> > On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:

> > > I understand your concerns here however I didn't want to overengineer
> > > this. Is the same GPIO (on more complex PMICs) used in different
> > > contexts? Like enable control and something more in the same time?

> > Yes, and it's often reprogrammable at runtime.

> I have doubts if generalized code could support such configuration...

That's pretty much my point.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141128/79e28c69/attachment.sig>

[PATCH v4 3/7] regulator: of: Parse ena-gpios property from DTS

[Krzysztof Kozlowski]

Drivers often add custom DTS properties for parsing the GPIO for
regulator enable control. Some of them don't have to do this in a
special custom way and would work with a generic approach (e.g. S5M8767,
S2MPS1x, MAX77686). As such drivers have to do this on their own,
multiple different bindings are added and each driver duplicates similar
code.

Add a generic binding so the regulator core will do this work for
drivers. This should offload some work from drivers and also limit
creation of new custom properties for GPIO control.

The patch only fills regulator constraints with data from DTS. Data will
be used by regulator core in consecutive patch.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/of_regulator.c  | 11 +++++++++++
 include/linux/regulator/machine.h | 13 +++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 03edb175f3ae..f64739a97296 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/regulator/machine.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/of_regulator.h>
@@ -31,6 +32,7 @@ static void of_get_regulation_constraints(struct device_node *np,
 	struct regulation_constraints *constraints = &(*init_data)->constraints;
 	struct regulator_state *suspend_state;
 	struct device_node *suspend_np;
+	enum of_gpio_flags gpio_flags;
 	int ret, i;
 	u32 pval;
 
@@ -81,6 +83,15 @@ static void of_get_regulation_constraints(struct device_node *np,
 	if (!ret)
 		constraints->enable_time = pval;
 
+	constraints->ena_gpio = of_get_named_gpio_flags(np, "ena-gpios", 0,
+							&gpio_flags);
+	if (gpio_is_valid(constraints->ena_gpio)) {
+		constraints->use_ena_gpio = true;
+		constraints->ena_gpio_open_drain = of_property_read_bool(np,
+							   "ena-gpio-open-drain");
+		constraints->ena_gpio_flags = gpio_flags;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(regulator_states); i++) {
 		switch (i) {
 		case PM_SUSPEND_MEM:
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 0b08d05d470b..2faf2b3b71e7 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -95,6 +95,13 @@ struct regulator_state {
  *                 mode.
  * @initial_state: Suspend state to set by default.
  * @initial_mode: Mode to set at startup.
+ *
+ * @use_ena_gpio: True if ena_gpio is a valid GPIO to use for enable control.
+ *                If false, all other ena_gpio* fields are ignored.
+ * @ena_gpio: GPIO to use for enable control
+ * @ena_gpio_open_drain: Is GPIO open drain
+ * @ena_gpio_flags: Flags for GPIO request, see enum of_gpio_flags
+ *
  * @ramp_delay: Time to settle down after voltage change (unit: uV/us)
  * @enable_time: Turn-on time of the rails (unit: microseconds)
  */
@@ -130,6 +137,12 @@ struct regulation_constraints {
 	/* mode to set on startup */
 	unsigned int initial_mode;
 
+	/* enable control over GPIO */
+	bool use_ena_gpio;
+	int ena_gpio;
+	bool ena_gpio_open_drain;
+	unsigned int ena_gpio_flags;
+
 	unsigned int ramp_delay;
 	unsigned int enable_time;
 
-- 
1.9.1

[PATCH v4 3/7] regulator: of: Parse ena-gpios property from DTS

[Krzysztof Kozlowski]

Drivers often add custom DTS properties for parsing the GPIO for
regulator enable control. Some of them don't have to do this in a
special custom way and would work with a generic approach (e.g. S5M8767,
S2MPS1x, MAX77686). As such drivers have to do this on their own,
multiple different bindings are added and each driver duplicates similar
code.

Add a generic binding so the regulator core will do this work for
drivers. This should offload some work from drivers and also limit
creation of new custom properties for GPIO control.

The patch only fills regulator constraints with data from DTS. Data will
be used by regulator core in consecutive patch.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/of_regulator.c  | 11 +++++++++++
 include/linux/regulator/machine.h | 13 +++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 03edb175f3ae..f64739a97296 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/regulator/machine.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/of_regulator.h>
@@ -31,6 +32,7 @@ static void of_get_regulation_constraints(struct device_node *np,
 	struct regulation_constraints *constraints = &(*init_data)->constraints;
 	struct regulator_state *suspend_state;
 	struct device_node *suspend_np;
+	enum of_gpio_flags gpio_flags;
 	int ret, i;
 	u32 pval;
 
@@ -81,6 +83,15 @@ static void of_get_regulation_constraints(struct device_node *np,
 	if (!ret)
 		constraints->enable_time = pval;
 
+	constraints->ena_gpio = of_get_named_gpio_flags(np, "ena-gpios", 0,
+							&gpio_flags);
+	if (gpio_is_valid(constraints->ena_gpio)) {
+		constraints->use_ena_gpio = true;
+		constraints->ena_gpio_open_drain = of_property_read_bool(np,
+							   "ena-gpio-open-drain");
+		constraints->ena_gpio_flags = gpio_flags;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(regulator_states); i++) {
 		switch (i) {
 		case PM_SUSPEND_MEM:
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 0b08d05d470b..2faf2b3b71e7 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -95,6 +95,13 @@ struct regulator_state {
  *                 mode.
  * @initial_state: Suspend state to set by default.
  * @initial_mode: Mode to set@startup.
+ *
+ * @use_ena_gpio: True if ena_gpio is a valid GPIO to use for enable control.
+ *                If false, all other ena_gpio* fields are ignored.
+ * @ena_gpio: GPIO to use for enable control
+ * @ena_gpio_open_drain: Is GPIO open drain
+ * @ena_gpio_flags: Flags for GPIO request, see enum of_gpio_flags
+ *
  * @ramp_delay: Time to settle down after voltage change (unit: uV/us)
  * @enable_time: Turn-on time of the rails (unit: microseconds)
  */
@@ -130,6 +137,12 @@ struct regulation_constraints {
 	/* mode to set on startup */
 	unsigned int initial_mode;
 
+	/* enable control over GPIO */
+	bool use_ena_gpio;
+	int ena_gpio;
+	bool ena_gpio_open_drain;
+	unsigned int ena_gpio_flags;
+
 	unsigned int ramp_delay;
 	unsigned int enable_time;
 
-- 
1.9.1

[PATCH v4 3/7] regulator: of: Parse ena-gpios property from DTS

[Krzysztof Kozlowski]

Drivers often add custom DTS properties for parsing the GPIO for
regulator enable control. Some of them don't have to do this in a
special custom way and would work with a generic approach (e.g. S5M8767,
S2MPS1x, MAX77686). As such drivers have to do this on their own,
multiple different bindings are added and each driver duplicates similar
code.

Add a generic binding so the regulator core will do this work for
drivers. This should offload some work from drivers and also limit
creation of new custom properties for GPIO control.

The patch only fills regulator constraints with data from DTS. Data will
be used by regulator core in consecutive patch.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/of_regulator.c  | 11 +++++++++++
 include/linux/regulator/machine.h | 13 +++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 03edb175f3ae..f64739a97296 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/regulator/machine.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/of_regulator.h>
@@ -31,6 +32,7 @@ static void of_get_regulation_constraints(struct device_node *np,
 	struct regulation_constraints *constraints = &(*init_data)->constraints;
 	struct regulator_state *suspend_state;
 	struct device_node *suspend_np;
+	enum of_gpio_flags gpio_flags;
 	int ret, i;
 	u32 pval;
 
@@ -81,6 +83,15 @@ static void of_get_regulation_constraints(struct device_node *np,
 	if (!ret)
 		constraints->enable_time = pval;
 
+	constraints->ena_gpio = of_get_named_gpio_flags(np, "ena-gpios", 0,
+							&gpio_flags);
+	if (gpio_is_valid(constraints->ena_gpio)) {
+		constraints->use_ena_gpio = true;
+		constraints->ena_gpio_open_drain = of_property_read_bool(np,
+							   "ena-gpio-open-drain");
+		constraints->ena_gpio_flags = gpio_flags;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(regulator_states); i++) {
 		switch (i) {
 		case PM_SUSPEND_MEM:
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 0b08d05d470b..2faf2b3b71e7 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -95,6 +95,13 @@ struct regulator_state {
  *                 mode.
  * @initial_state: Suspend state to set by default.
  * @initial_mode: Mode to set at startup.
+ *
+ * @use_ena_gpio: True if ena_gpio is a valid GPIO to use for enable control.
+ *                If false, all other ena_gpio* fields are ignored.
+ * @ena_gpio: GPIO to use for enable control
+ * @ena_gpio_open_drain: Is GPIO open drain
+ * @ena_gpio_flags: Flags for GPIO request, see enum of_gpio_flags
+ *
  * @ramp_delay: Time to settle down after voltage change (unit: uV/us)
  * @enable_time: Turn-on time of the rails (unit: microseconds)
  */
@@ -130,6 +137,12 @@ struct regulation_constraints {
 	/* mode to set on startup */
 	unsigned int initial_mode;
 
+	/* enable control over GPIO */
+	bool use_ena_gpio;
+	int ena_gpio;
+	bool ena_gpio_open_drain;
+	unsigned int ena_gpio_flags;
+
 	unsigned int ramp_delay;
 	unsigned int enable_time;
 
-- 
1.9.1

Re: [PATCH v4 3/7] regulator: of: Parse ena-gpios property from DTS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 446 bytes --]

On Thu, Nov 27, 2014 at 12:20:49PM +0100, Krzysztof Kozlowski wrote:

> +	constraints->ena_gpio = of_get_named_gpio_flags(np, "ena-gpios", 0,
> +							&gpio_flags);
> +	if (gpio_is_valid(constraints->ena_gpio)) {

No, this isn't sensible - in what way would an enable control GPIO be a
constraint?  The whole reason we have separate constraint and config
structures is that these are different things.  Keep the GPIO setup in
the configuration.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v4 3/7] regulator: of: Parse ena-gpios property from DTS

[Mark Brown]

On Thu, Nov 27, 2014 at 12:20:49PM +0100, Krzysztof Kozlowski wrote:

> +	constraints->ena_gpio = of_get_named_gpio_flags(np, "ena-gpios", 0,
> +							&gpio_flags);
> +	if (gpio_is_valid(constraints->ena_gpio)) {

No, this isn't sensible - in what way would an enable control GPIO be a
constraint?  The whole reason we have separate constraint and config
structures is that these are different things.  Keep the GPIO setup in
the configuration.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/779f525e/attachment.sig>

Re: [PATCH v4 3/7] regulator: of: Parse ena-gpios property from DTS

[Krzysztof Kozlowski]

On czw, 2014-11-27 at 18:45 +0000, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 12:20:49PM +0100, Krzysztof Kozlowski wrote:
> 
> > +	constraints->ena_gpio = of_get_named_gpio_flags(np, "ena-gpios", 0,
> > +							&gpio_flags);
> > +	if (gpio_is_valid(constraints->ena_gpio)) {
> 
> No, this isn't sensible - in what way would an enable control GPIO be a
> constraint?  The whole reason we have separate constraint and config
> structures is that these are different things.  Keep the GPIO setup in
> the configuration.

OK, I'll change it to config.

Best regards,
Krzysztof

[PATCH v4 3/7] regulator: of: Parse ena-gpios property from DTS

[Krzysztof Kozlowski]

On czw, 2014-11-27 at 18:45 +0000, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 12:20:49PM +0100, Krzysztof Kozlowski wrote:
> 
> > +	constraints->ena_gpio = of_get_named_gpio_flags(np, "ena-gpios", 0,
> > +							&gpio_flags);
> > +	if (gpio_is_valid(constraints->ena_gpio)) {
> 
> No, this isn't sensible - in what way would an enable control GPIO be a
> constraint?  The whole reason we have separate constraint and config
> structures is that these are different things.  Keep the GPIO setup in
> the configuration.

OK, I'll change it to config.

Best regards,
Krzysztof

[PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Krzysztof Kozlowski]

Use ena_gpio from regulator constraints (filled by parsing generic
bindings) to initialize the GPIO enable control. Support also the old
way: ena_gpio supplied in regulator_config structure.

This also adds a new set_ena_gpio() callback in regulator_ops structure
which driver may provide to actually enable the GPIO control in
hardware.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/core.c         | 96 ++++++++++++++++++++++++++++++----------
 include/linux/regulator/driver.h |  5 +++
 2 files changed, 78 insertions(+), 23 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index bbf93c9caca3..1760184cd8dc 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -26,6 +26,7 @@
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/regmap.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/consumer.h>
@@ -1044,6 +1045,14 @@ static int set_machine_constraints(struct regulator_dev *rdev,
 		}
 	}
 
+	if (rdev->constraints->use_ena_gpio && ops->set_ena_gpio) {
+		ret = ops->set_ena_gpio(rdev);
+		if (ret < 0) {
+			rdev_err(rdev, "failed to set enable GPIO control\n");
+			goto out;
+		}
+	}
+
 	print_constraints(rdev);
 	return 0;
 out:
@@ -1660,36 +1669,36 @@ EXPORT_SYMBOL_GPL(regulator_bulk_unregister_supply_alias);
 
 /* Manage enable GPIO list. Same GPIO pin can be shared among regulators */
 static int regulator_ena_gpio_request(struct regulator_dev *rdev,
-				const struct regulator_config *config)
+					unsigned int gpio,
+					bool gpio_invert,
+					unsigned int gpio_flags)
 {
 	struct regulator_enable_gpio *pin;
 	struct gpio_desc *gpiod;
 	int ret;
 
-	gpiod = gpio_to_desc(config->ena_gpio);
+	gpiod = gpio_to_desc(gpio);
 
 	list_for_each_entry(pin, &regulator_ena_gpio_list, list) {
 		if (pin->gpiod == gpiod) {
-			rdev_dbg(rdev, "GPIO %d is already used\n",
-				config->ena_gpio);
+			rdev_dbg(rdev, "GPIO %d is already used\n", gpio);
 			goto update_ena_gpio_to_rdev;
 		}
 	}
 
-	ret = gpio_request_one(config->ena_gpio,
-				GPIOF_DIR_OUT | config->ena_gpio_flags,
+	ret = gpio_request_one(gpio, GPIOF_DIR_OUT | gpio_flags,
 				rdev_get_name(rdev));
 	if (ret)
 		return ret;
 
 	pin = kzalloc(sizeof(struct regulator_enable_gpio), GFP_KERNEL);
 	if (pin == NULL) {
-		gpio_free(config->ena_gpio);
+		gpio_free(gpio);
 		return -ENOMEM;
 	}
 
 	pin->gpiod = gpiod;
-	pin->ena_gpio_invert = config->ena_gpio_invert;
+	pin->ena_gpio_invert = gpio_invert;
 	list_add(&pin->list, &regulator_ena_gpio_list);
 
 update_ena_gpio_to_rdev:
@@ -1698,6 +1707,59 @@ update_ena_gpio_to_rdev:
 	return 0;
 }
 
+/*
+ * Request GPIO for enable control from regulator_config
+ * or init_data->constraints.
+ */
+static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
+			const struct regulator_config *config,
+			const struct regulator_init_data *init_data)
+{
+	unsigned int gpio_flags;
+	bool gpio_invert;
+	int gpio, ret;
+
+	if (config->ena_gpio || config->ena_gpio_initialized) {
+		gpio = config->ena_gpio;
+		gpio_invert = config->ena_gpio_invert;
+		gpio_flags = config->ena_gpio_flags;
+	} else if (init_data && init_data->constraints.use_ena_gpio) {
+		const struct regulation_constraints *c = &init_data->constraints;
+
+		gpio = c->ena_gpio;
+		gpio_invert = false;
+		gpio_flags = GPIOF_OUT_INIT_HIGH;
+
+		if (c->ena_gpio_flags & OF_GPIO_ACTIVE_LOW) {
+			gpio_invert = true;
+			gpio_flags = GPIOF_OUT_INIT_LOW;
+		}
+
+		if (c->ena_gpio_open_drain)
+			gpio_flags |= GPIOF_OPEN_DRAIN;
+	} else {
+		return 0;
+	}
+
+	if (!gpio_is_valid(gpio))
+		return 0;
+
+	ret = regulator_ena_gpio_request(rdev, gpio, gpio_invert, gpio_flags);
+	if (ret != 0) {
+		rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
+				 gpio, ret);
+		return ret;
+	}
+
+	if (gpio_flags & GPIOF_OUT_INIT_HIGH)
+		rdev->ena_gpio_state = 1;
+
+	if (gpio_invert)
+		rdev->ena_gpio_state = !rdev->ena_gpio_state;
+
+	return 0;
+}
+
 static void regulator_ena_gpio_free(struct regulator_dev *rdev)
 {
 	struct regulator_enable_gpio *pin, *n;
@@ -3650,21 +3712,9 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	dev_set_drvdata(&rdev->dev, rdev);
 
-	if ((config->ena_gpio || config->ena_gpio_initialized) &&
-	    gpio_is_valid(config->ena_gpio)) {
-		ret = regulator_ena_gpio_request(rdev, config);
-		if (ret != 0) {
-			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
-				 config->ena_gpio, ret);
-			goto wash;
-		}
-
-		if (config->ena_gpio_flags & GPIOF_OUT_INIT_HIGH)
-			rdev->ena_gpio_state = 1;
-
-		if (config->ena_gpio_invert)
-			rdev->ena_gpio_state = !rdev->ena_gpio_state;
-	}
+	ret = regulator_ena_gpio_setup(rdev, config, init_data);
+	if (ret != 0)
+		goto wash;
 
 	/* set regulator constraints */
 	if (init_data)
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 28da08e4671f..fe967a0c6ce7 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -118,6 +118,9 @@ struct regulator_linear_range {
  *                       suspended.
  * @set_suspend_mode: Set the operating mode for the regulator when the
  *                    system is suspended.
+ * @set_ena_gpio: Turn on GPIO enable control for given regulator. Called
+ *                by core during registration of regulator when regulator
+ *                was configured for this mode by standard binding.
  *
  * This struct describes regulator operations which can be implemented by
  * regulator chip drivers.
@@ -183,6 +186,8 @@ struct regulator_ops {
 
 	/* set regulator suspend operating mode (defined in consumer.h) */
 	int (*set_suspend_mode) (struct regulator_dev *, unsigned int mode);
+
+	int (*set_ena_gpio)(struct regulator_dev *);
 };
 
 /*
-- 
1.9.1

[PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Krzysztof Kozlowski]

Use ena_gpio from regulator constraints (filled by parsing generic
bindings) to initialize the GPIO enable control. Support also the old
way: ena_gpio supplied in regulator_config structure.

This also adds a new set_ena_gpio() callback in regulator_ops structure
which driver may provide to actually enable the GPIO control in
hardware.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/core.c         | 96 ++++++++++++++++++++++++++++++----------
 include/linux/regulator/driver.h |  5 +++
 2 files changed, 78 insertions(+), 23 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index bbf93c9caca3..1760184cd8dc 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -26,6 +26,7 @@
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/regmap.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/consumer.h>
@@ -1044,6 +1045,14 @@ static int set_machine_constraints(struct regulator_dev *rdev,
 		}
 	}
 
+	if (rdev->constraints->use_ena_gpio && ops->set_ena_gpio) {
+		ret = ops->set_ena_gpio(rdev);
+		if (ret < 0) {
+			rdev_err(rdev, "failed to set enable GPIO control\n");
+			goto out;
+		}
+	}
+
 	print_constraints(rdev);
 	return 0;
 out:
@@ -1660,36 +1669,36 @@ EXPORT_SYMBOL_GPL(regulator_bulk_unregister_supply_alias);
 
 /* Manage enable GPIO list. Same GPIO pin can be shared among regulators */
 static int regulator_ena_gpio_request(struct regulator_dev *rdev,
-				const struct regulator_config *config)
+					unsigned int gpio,
+					bool gpio_invert,
+					unsigned int gpio_flags)
 {
 	struct regulator_enable_gpio *pin;
 	struct gpio_desc *gpiod;
 	int ret;
 
-	gpiod = gpio_to_desc(config->ena_gpio);
+	gpiod = gpio_to_desc(gpio);
 
 	list_for_each_entry(pin, &regulator_ena_gpio_list, list) {
 		if (pin->gpiod == gpiod) {
-			rdev_dbg(rdev, "GPIO %d is already used\n",
-				config->ena_gpio);
+			rdev_dbg(rdev, "GPIO %d is already used\n", gpio);
 			goto update_ena_gpio_to_rdev;
 		}
 	}
 
-	ret = gpio_request_one(config->ena_gpio,
-				GPIOF_DIR_OUT | config->ena_gpio_flags,
+	ret = gpio_request_one(gpio, GPIOF_DIR_OUT | gpio_flags,
 				rdev_get_name(rdev));
 	if (ret)
 		return ret;
 
 	pin = kzalloc(sizeof(struct regulator_enable_gpio), GFP_KERNEL);
 	if (pin == NULL) {
-		gpio_free(config->ena_gpio);
+		gpio_free(gpio);
 		return -ENOMEM;
 	}
 
 	pin->gpiod = gpiod;
-	pin->ena_gpio_invert = config->ena_gpio_invert;
+	pin->ena_gpio_invert = gpio_invert;
 	list_add(&pin->list, &regulator_ena_gpio_list);
 
 update_ena_gpio_to_rdev:
@@ -1698,6 +1707,59 @@ update_ena_gpio_to_rdev:
 	return 0;
 }
 
+/*
+ * Request GPIO for enable control from regulator_config
+ * or init_data->constraints.
+ */
+static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
+			const struct regulator_config *config,
+			const struct regulator_init_data *init_data)
+{
+	unsigned int gpio_flags;
+	bool gpio_invert;
+	int gpio, ret;
+
+	if (config->ena_gpio || config->ena_gpio_initialized) {
+		gpio = config->ena_gpio;
+		gpio_invert = config->ena_gpio_invert;
+		gpio_flags = config->ena_gpio_flags;
+	} else if (init_data && init_data->constraints.use_ena_gpio) {
+		const struct regulation_constraints *c = &init_data->constraints;
+
+		gpio = c->ena_gpio;
+		gpio_invert = false;
+		gpio_flags = GPIOF_OUT_INIT_HIGH;
+
+		if (c->ena_gpio_flags & OF_GPIO_ACTIVE_LOW) {
+			gpio_invert = true;
+			gpio_flags = GPIOF_OUT_INIT_LOW;
+		}
+
+		if (c->ena_gpio_open_drain)
+			gpio_flags |= GPIOF_OPEN_DRAIN;
+	} else {
+		return 0;
+	}
+
+	if (!gpio_is_valid(gpio))
+		return 0;
+
+	ret = regulator_ena_gpio_request(rdev, gpio, gpio_invert, gpio_flags);
+	if (ret != 0) {
+		rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
+				 gpio, ret);
+		return ret;
+	}
+
+	if (gpio_flags & GPIOF_OUT_INIT_HIGH)
+		rdev->ena_gpio_state = 1;
+
+	if (gpio_invert)
+		rdev->ena_gpio_state = !rdev->ena_gpio_state;
+
+	return 0;
+}
+
 static void regulator_ena_gpio_free(struct regulator_dev *rdev)
 {
 	struct regulator_enable_gpio *pin, *n;
@@ -3650,21 +3712,9 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	dev_set_drvdata(&rdev->dev, rdev);
 
-	if ((config->ena_gpio || config->ena_gpio_initialized) &&
-	    gpio_is_valid(config->ena_gpio)) {
-		ret = regulator_ena_gpio_request(rdev, config);
-		if (ret != 0) {
-			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
-				 config->ena_gpio, ret);
-			goto wash;
-		}
-
-		if (config->ena_gpio_flags & GPIOF_OUT_INIT_HIGH)
-			rdev->ena_gpio_state = 1;
-
-		if (config->ena_gpio_invert)
-			rdev->ena_gpio_state = !rdev->ena_gpio_state;
-	}
+	ret = regulator_ena_gpio_setup(rdev, config, init_data);
+	if (ret != 0)
+		goto wash;
 
 	/* set regulator constraints */
 	if (init_data)
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 28da08e4671f..fe967a0c6ce7 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -118,6 +118,9 @@ struct regulator_linear_range {
  *                       suspended.
  * @set_suspend_mode: Set the operating mode for the regulator when the
  *                    system is suspended.
+ * @set_ena_gpio: Turn on GPIO enable control for given regulator. Called
+ *                by core during registration of regulator when regulator
+ *                was configured for this mode by standard binding.
  *
  * This struct describes regulator operations which can be implemented by
  * regulator chip drivers.
@@ -183,6 +186,8 @@ struct regulator_ops {
 
 	/* set regulator suspend operating mode (defined in consumer.h) */
 	int (*set_suspend_mode) (struct regulator_dev *, unsigned int mode);
+
+	int (*set_ena_gpio)(struct regulator_dev *);
 };
 
 /*
-- 
1.9.1

Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2540 bytes --]

On Thu, Nov 27, 2014 at 12:20:50PM +0100, Krzysztof Kozlowski wrote:
> Use ena_gpio from regulator constraints (filled by parsing generic
> bindings) to initialize the GPIO enable control. Support also the old
> way: ena_gpio supplied in regulator_config structure.
> 
> This also adds a new set_ena_gpio() callback in regulator_ops structure
> which driver may provide to actually enable the GPIO control in
> hardware.

This seems really confused like it's trying to work around some other
problem - this all feels like it's at the wrong abstraction level.  As
far as I can tell this is trying to fix bugs in the previous patch and
do some other refactorings (the "also add this other random op" bit
especially) but I'm really not clear what the goal is.

Please try to think if the code you're writing makes sense at the big
picture level rather than just band aiding specific problems you see.
It's also a good idea to keep random code motion separate from
functional changes since it makes it much easier to follow what each is
supposed to do.

> @@ -1044,6 +1045,14 @@ static int set_machine_constraints(struct regulator_dev *rdev,
>  		}
>  	}
>  
> +	if (rdev->constraints->use_ena_gpio && ops->set_ena_gpio) {
> +		ret = ops->set_ena_gpio(rdev);
> +		if (ret < 0) {
> +			rdev_err(rdev, "failed to set enable GPIO control\n");
> +			goto out;
> +		}
> +	}

Why do we need some special magic operation for GPIO based enables
that's separate to any other enable operation?  This seems really
confusing, if the constraint setting doesn't work somehow for GPIO based
enables we should fix that.  Though since this operation takes no
parameters it's hard to see how it's supposed to apply constraints
unless it reparses them which doesn't seem like a good idea...

>  static int regulator_ena_gpio_request(struct regulator_dev *rdev,
> -				const struct regulator_config *config)

> -	ret = gpio_request_one(config->ena_gpio,
> -				GPIOF_DIR_OUT | config->ena_gpio_flags,
> +	ret = gpio_request_one(gpio, GPIOF_DIR_OUT | gpio_flags,
>  				rdev_get_name(rdev));

> +/*
> + * Request GPIO for enable control from regulator_config
> + * or init_data->constraints.
> + */
> +static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
> +			const struct regulator_config *config,
> +			const struct regulator_init_data *init_data)

Why is setting up the GPIO different to requesting it, especially given
that we have an existing function called _request() which still exists?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Mark Brown]

On Thu, Nov 27, 2014 at 12:20:50PM +0100, Krzysztof Kozlowski wrote:
> Use ena_gpio from regulator constraints (filled by parsing generic
> bindings) to initialize the GPIO enable control. Support also the old
> way: ena_gpio supplied in regulator_config structure.
> 
> This also adds a new set_ena_gpio() callback in regulator_ops structure
> which driver may provide to actually enable the GPIO control in
> hardware.

This seems really confused like it's trying to work around some other
problem - this all feels like it's at the wrong abstraction level.  As
far as I can tell this is trying to fix bugs in the previous patch and
do some other refactorings (the "also add this other random op" bit
especially) but I'm really not clear what the goal is.

Please try to think if the code you're writing makes sense at the big
picture level rather than just band aiding specific problems you see.
It's also a good idea to keep random code motion separate from
functional changes since it makes it much easier to follow what each is
supposed to do.

> @@ -1044,6 +1045,14 @@ static int set_machine_constraints(struct regulator_dev *rdev,
>  		}
>  	}
>  
> +	if (rdev->constraints->use_ena_gpio && ops->set_ena_gpio) {
> +		ret = ops->set_ena_gpio(rdev);
> +		if (ret < 0) {
> +			rdev_err(rdev, "failed to set enable GPIO control\n");
> +			goto out;
> +		}
> +	}

Why do we need some special magic operation for GPIO based enables
that's separate to any other enable operation?  This seems really
confusing, if the constraint setting doesn't work somehow for GPIO based
enables we should fix that.  Though since this operation takes no
parameters it's hard to see how it's supposed to apply constraints
unless it reparses them which doesn't seem like a good idea...

>  static int regulator_ena_gpio_request(struct regulator_dev *rdev,
> -				const struct regulator_config *config)

> -	ret = gpio_request_one(config->ena_gpio,
> -				GPIOF_DIR_OUT | config->ena_gpio_flags,
> +	ret = gpio_request_one(gpio, GPIOF_DIR_OUT | gpio_flags,
>  				rdev_get_name(rdev));

> +/*
> + * Request GPIO for enable control from regulator_config
> + * or init_data->constraints.
> + */
> +static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
> +			const struct regulator_config *config,
> +			const struct regulator_init_data *init_data)

Why is setting up the GPIO different to requesting it, especially given
that we have an existing function called _request() which still exists?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/cad8a8a4/attachment.sig>

Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Krzysztof Kozlowski]

On czw, 2014-11-27 at 18:43 +0000, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 12:20:50PM +0100, Krzysztof Kozlowski wrote:
> > Use ena_gpio from regulator constraints (filled by parsing generic
> > bindings) to initialize the GPIO enable control. Support also the old
> > way: ena_gpio supplied in regulator_config structure.
> > 
> > This also adds a new set_ena_gpio() callback in regulator_ops structure
> > which driver may provide to actually enable the GPIO control in
> > hardware.
> 
> This seems really confused like it's trying to work around some other
> problem - this all feels like it's at the wrong abstraction level.  As
> far as I can tell this is trying to fix bugs in the previous patch and
> do some other refactorings (the "also add this other random op" bit
> especially) but I'm really not clear what the goal is.
> 
> Please try to think if the code you're writing makes sense at the big
> picture level rather than just band aiding specific problems you see.
> It's also a good idea to keep random code motion separate from
> functional changes since it makes it much easier to follow what each is
> supposed to do.
> 
> > @@ -1044,6 +1045,14 @@ static int set_machine_constraints(struct regulator_dev *rdev,
> >  		}
> >  	}
> >  
> > +	if (rdev->constraints->use_ena_gpio && ops->set_ena_gpio) {
> > +		ret = ops->set_ena_gpio(rdev);
> > +		if (ret < 0) {
> > +			rdev_err(rdev, "failed to set enable GPIO control\n");
> > +			goto out;
> > +		}
> > +	}
> 
> Why do we need some special magic operation for GPIO based enables
> that's separate to any other enable operation?  This seems really
> confusing, if the constraint setting doesn't work somehow for GPIO based
> enables we should fix that.  Though since this operation takes no
> parameters it's hard to see how it's supposed to apply constraints
> unless it reparses them which doesn't seem like a good idea...

The regulator driver no longer parses GPIO control from DTS. So somehow
it should be notified that regulator core parsed this and GPIO should be
enabled.

That is the purpose of ops->set_ena_gpio() call.

> 
> >  static int regulator_ena_gpio_request(struct regulator_dev *rdev,
> > -				const struct regulator_config *config)
> 
> > -	ret = gpio_request_one(config->ena_gpio,
> > -				GPIOF_DIR_OUT | config->ena_gpio_flags,
> > +	ret = gpio_request_one(gpio, GPIOF_DIR_OUT | gpio_flags,
> >  				rdev_get_name(rdev));
> 
> > +/*
> > + * Request GPIO for enable control from regulator_config
> > + * or init_data->constraints.
> > + */
> > +static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
> > +			const struct regulator_config *config,
> > +			const struct regulator_init_data *init_data)
> 
> Why is setting up the GPIO different to requesting it, especially given
> that we have an existing function called _request() which still exists?

Maybe the name was not a best choice. The setup calls request.

My patchset here tried to retain the compatibility with
"config.ena_gpio" way so the core would accept GPIOs passed in one of
two ways:
1. old: config.ena_gpio,
2. new: parsed by core from DTS.

The request function previously worked only on "config.ena_gpio" and I
changed it here to accept any GPIO. The setup uses one of GPIO methods
(old or new) and calls request with appropriate GPIO.

Anyway this will change after your comments about not using constraints
(patch 3/7). I'll keep your comments about big picture level in mind and
start working on next version.

Thanks for feedback!

Best regards,
Krzysztof

[PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Krzysztof Kozlowski]

On czw, 2014-11-27 at 18:43 +0000, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 12:20:50PM +0100, Krzysztof Kozlowski wrote:
> > Use ena_gpio from regulator constraints (filled by parsing generic
> > bindings) to initialize the GPIO enable control. Support also the old
> > way: ena_gpio supplied in regulator_config structure.
> > 
> > This also adds a new set_ena_gpio() callback in regulator_ops structure
> > which driver may provide to actually enable the GPIO control in
> > hardware.
> 
> This seems really confused like it's trying to work around some other
> problem - this all feels like it's at the wrong abstraction level.  As
> far as I can tell this is trying to fix bugs in the previous patch and
> do some other refactorings (the "also add this other random op" bit
> especially) but I'm really not clear what the goal is.
> 
> Please try to think if the code you're writing makes sense at the big
> picture level rather than just band aiding specific problems you see.
> It's also a good idea to keep random code motion separate from
> functional changes since it makes it much easier to follow what each is
> supposed to do.
> 
> > @@ -1044,6 +1045,14 @@ static int set_machine_constraints(struct regulator_dev *rdev,
> >  		}
> >  	}
> >  
> > +	if (rdev->constraints->use_ena_gpio && ops->set_ena_gpio) {
> > +		ret = ops->set_ena_gpio(rdev);
> > +		if (ret < 0) {
> > +			rdev_err(rdev, "failed to set enable GPIO control\n");
> > +			goto out;
> > +		}
> > +	}
> 
> Why do we need some special magic operation for GPIO based enables
> that's separate to any other enable operation?  This seems really
> confusing, if the constraint setting doesn't work somehow for GPIO based
> enables we should fix that.  Though since this operation takes no
> parameters it's hard to see how it's supposed to apply constraints
> unless it reparses them which doesn't seem like a good idea...

The regulator driver no longer parses GPIO control from DTS. So somehow
it should be notified that regulator core parsed this and GPIO should be
enabled.

That is the purpose of ops->set_ena_gpio() call.

> 
> >  static int regulator_ena_gpio_request(struct regulator_dev *rdev,
> > -				const struct regulator_config *config)
> 
> > -	ret = gpio_request_one(config->ena_gpio,
> > -				GPIOF_DIR_OUT | config->ena_gpio_flags,
> > +	ret = gpio_request_one(gpio, GPIOF_DIR_OUT | gpio_flags,
> >  				rdev_get_name(rdev));
> 
> > +/*
> > + * Request GPIO for enable control from regulator_config
> > + * or init_data->constraints.
> > + */
> > +static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
> > +			const struct regulator_config *config,
> > +			const struct regulator_init_data *init_data)
> 
> Why is setting up the GPIO different to requesting it, especially given
> that we have an existing function called _request() which still exists?

Maybe the name was not a best choice. The setup calls request.

My patchset here tried to retain the compatibility with
"config.ena_gpio" way so the core would accept GPIOs passed in one of
two ways:
1. old: config.ena_gpio,
2. new: parsed by core from DTS.

The request function previously worked only on "config.ena_gpio" and I
changed it here to accept any GPIO. The setup uses one of GPIO methods
(old or new) and calls request with appropriate GPIO.

Anyway this will change after your comments about not using constraints
(patch 3/7). I'll keep your comments about big picture level in mind and
start working on next version.

Thanks for feedback!

Best regards,
Krzysztof

Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]

On Fri, Nov 28, 2014 at 11:30:55AM +0100, Krzysztof Kozlowski wrote:
> On czw, 2014-11-27 at 18:43 +0000, Mark Brown wrote:

> > Why do we need some special magic operation for GPIO based enables
> > that's separate to any other enable operation?  This seems really
> > confusing, if the constraint setting doesn't work somehow for GPIO based
> > enables we should fix that.  Though since this operation takes no
> > parameters it's hard to see how it's supposed to apply constraints
> > unless it reparses them which doesn't seem like a good idea...

> The regulator driver no longer parses GPIO control from DTS. So somehow
> it should be notified that regulator core parsed this and GPIO should be
> enabled.

> That is the purpose of ops->set_ena_gpio() call.

This sort of thing is a sign that we're not saving much by moving the
parsing to the core and perhaps there's more flexiblity here...  It's
also not something that should be in the constraints handling, it's not
something that constrains the regulator.

> > > +static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
> > > +			const struct regulator_config *config,
> > > +			const struct regulator_init_data *init_data)

> > Why is setting up the GPIO different to requesting it, especially given
> > that we have an existing function called _request() which still exists?

> Maybe the name was not a best choice. The setup calls request.

> My patchset here tried to retain the compatibility with
> "config.ena_gpio" way so the core would accept GPIOs passed in one of
> two ways:
> 1. old: config.ena_gpio,
> 2. new: parsed by core from DTS.

> The request function previously worked only on "config.ena_gpio" and I
> changed it here to accept any GPIO. The setup uses one of GPIO methods
> (old or new) and calls request with appropriate GPIO.

We need to support both methods since not all the world is DT.  What I
can't tell is how this code achieves these objectives - it seems to be
an awfully big patch if that's all it's supposed to be doing, I'd expect
just a conditional where we try the two methods in turn.  It may be that
there's a good reason for all this but that needs to be made clear.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Mark Brown]

On Fri, Nov 28, 2014 at 11:30:55AM +0100, Krzysztof Kozlowski wrote:
> On czw, 2014-11-27 at 18:43 +0000, Mark Brown wrote:

> > Why do we need some special magic operation for GPIO based enables
> > that's separate to any other enable operation?  This seems really
> > confusing, if the constraint setting doesn't work somehow for GPIO based
> > enables we should fix that.  Though since this operation takes no
> > parameters it's hard to see how it's supposed to apply constraints
> > unless it reparses them which doesn't seem like a good idea...

> The regulator driver no longer parses GPIO control from DTS. So somehow
> it should be notified that regulator core parsed this and GPIO should be
> enabled.

> That is the purpose of ops->set_ena_gpio() call.

This sort of thing is a sign that we're not saving much by moving the
parsing to the core and perhaps there's more flexiblity here...  It's
also not something that should be in the constraints handling, it's not
something that constrains the regulator.

> > > +static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
> > > +			const struct regulator_config *config,
> > > +			const struct regulator_init_data *init_data)

> > Why is setting up the GPIO different to requesting it, especially given
> > that we have an existing function called _request() which still exists?

> Maybe the name was not a best choice. The setup calls request.

> My patchset here tried to retain the compatibility with
> "config.ena_gpio" way so the core would accept GPIOs passed in one of
> two ways:
> 1. old: config.ena_gpio,
> 2. new: parsed by core from DTS.

> The request function previously worked only on "config.ena_gpio" and I
> changed it here to accept any GPIO. The setup uses one of GPIO methods
> (old or new) and calls request with appropriate GPIO.

We need to support both methods since not all the world is DT.  What I
can't tell is how this code achieves these objectives - it seems to be
an awfully big patch if that's all it's supposed to be doing, I'd expect
just a conditional where we try the two methods in turn.  It may be that
there's a good reason for all this but that needs to be made clear.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141128/21a8a600/attachment.sig>

Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Krzysztof Kozlowski]

On pią, 2014-11-28 at 11:38 +0000, Mark Brown wrote:
> On Fri, Nov 28, 2014 at 11:30:55AM +0100, Krzysztof Kozlowski wrote:
> > On czw, 2014-11-27 at 18:43 +0000, Mark Brown wrote:
> 
> > > Why do we need some special magic operation for GPIO based enables
> > > that's separate to any other enable operation?  This seems really
> > > confusing, if the constraint setting doesn't work somehow for GPIO based
> > > enables we should fix that.  Though since this operation takes no
> > > parameters it's hard to see how it's supposed to apply constraints
> > > unless it reparses them which doesn't seem like a good idea...
> 
> > The regulator driver no longer parses GPIO control from DTS. So somehow
> > it should be notified that regulator core parsed this and GPIO should be
> > enabled.
> 
> > That is the purpose of ops->set_ena_gpio() call.
> 
> This sort of thing is a sign that we're not saving much by moving the
> parsing to the core and perhaps there's more flexiblity here... 

The driver receive callbacks (or exposes other kind of interface) for
other core-generalized code. Recent example is parsing regulator mode
(added by Javier) and .of_map_mode() callback.

I thought how to do this without this additional set_ena_gpio() call.
One way would be to extend the regulator modes (FAST/IDLE/STANDBY/ and
GPIO) but this would look somehow unnatural.

> It's
> also not something that should be in the constraints handling, it's not
> something that constrains the regulator.

Got it.

> > > > +static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
> > > > +			const struct regulator_config *config,
> > > > +			const struct regulator_init_data *init_data)
> 
> > > Why is setting up the GPIO different to requesting it, especially given
> > > that we have an existing function called _request() which still exists?
> 
> > Maybe the name was not a best choice. The setup calls request.
> 
> > My patchset here tried to retain the compatibility with
> > "config.ena_gpio" way so the core would accept GPIOs passed in one of
> > two ways:
> > 1. old: config.ena_gpio,
> > 2. new: parsed by core from DTS.
> 
> > The request function previously worked only on "config.ena_gpio" and I
> > changed it here to accept any GPIO. The setup uses one of GPIO methods
> > (old or new) and calls request with appropriate GPIO.
> 
> We need to support both methods since not all the world is DT.  What I
> can't tell is how this code achieves these objectives - it seems to be
> an awfully big patch if that's all it's supposed to be doing, I'd expect
> just a conditional where we try the two methods in turn.  It may be that
> there's a good reason for all this but that needs to be made clear.

OK, I'll try to better describe the reason behind and to split the
patches (if possible).

Best regards,
Krzysztof

[PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Krzysztof Kozlowski]

On pi?, 2014-11-28 at 11:38 +0000, Mark Brown wrote:
> On Fri, Nov 28, 2014 at 11:30:55AM +0100, Krzysztof Kozlowski wrote:
> > On czw, 2014-11-27 at 18:43 +0000, Mark Brown wrote:
> 
> > > Why do we need some special magic operation for GPIO based enables
> > > that's separate to any other enable operation?  This seems really
> > > confusing, if the constraint setting doesn't work somehow for GPIO based
> > > enables we should fix that.  Though since this operation takes no
> > > parameters it's hard to see how it's supposed to apply constraints
> > > unless it reparses them which doesn't seem like a good idea...
> 
> > The regulator driver no longer parses GPIO control from DTS. So somehow
> > it should be notified that regulator core parsed this and GPIO should be
> > enabled.
> 
> > That is the purpose of ops->set_ena_gpio() call.
> 
> This sort of thing is a sign that we're not saving much by moving the
> parsing to the core and perhaps there's more flexiblity here... 

The driver receive callbacks (or exposes other kind of interface) for
other core-generalized code. Recent example is parsing regulator mode
(added by Javier) and .of_map_mode() callback.

I thought how to do this without this additional set_ena_gpio() call.
One way would be to extend the regulator modes (FAST/IDLE/STANDBY/ and
GPIO) but this would look somehow unnatural.

> It's
> also not something that should be in the constraints handling, it's not
> something that constrains the regulator.

Got it.

> > > > +static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
> > > > +			const struct regulator_config *config,
> > > > +			const struct regulator_init_data *init_data)
> 
> > > Why is setting up the GPIO different to requesting it, especially given
> > > that we have an existing function called _request() which still exists?
> 
> > Maybe the name was not a best choice. The setup calls request.
> 
> > My patchset here tried to retain the compatibility with
> > "config.ena_gpio" way so the core would accept GPIOs passed in one of
> > two ways:
> > 1. old: config.ena_gpio,
> > 2. new: parsed by core from DTS.
> 
> > The request function previously worked only on "config.ena_gpio" and I
> > changed it here to accept any GPIO. The setup uses one of GPIO methods
> > (old or new) and calls request with appropriate GPIO.
> 
> We need to support both methods since not all the world is DT.  What I
> can't tell is how this code achieves these objectives - it seems to be
> an awfully big patch if that's all it's supposed to be doing, I'd expect
> just a conditional where we try the two methods in turn.  It may be that
> there's a good reason for all this but that needs to be made clear.

OK, I'll try to better describe the reason behind and to split the
patches (if possible).

Best regards,
Krzysztof

Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

On Fri, Nov 28, 2014 at 03:14:04PM +0100, Krzysztof Kozlowski wrote:
> On pią, 2014-11-28 at 11:38 +0000, Mark Brown wrote:

> > This sort of thing is a sign that we're not saving much by moving the
> > parsing to the core and perhaps there's more flexiblity here... 

> The driver receive callbacks (or exposes other kind of interface) for
> other core-generalized code. Recent example is parsing regulator mode
> (added by Javier) and .of_map_mode() callback.

Right, but that's actually doing some device specific translation and
successfully factoring out the bulk of the code - the fact that it's
taking parameters and returning data is a good sign.  This is a callback
placed randomly away from any other related code (adding to the
confusion - it's not integrated into the rest of the flow around this at
all) without a clear purpose.

> I thought how to do this without this additional set_ena_gpio() call.
> One way would be to extend the regulator modes (FAST/IDLE/STANDBY/ and
> GPIO) but this would look somehow unnatural.

Yes, that's absolutely hideous.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

[Mark Brown]

On Fri, Nov 28, 2014 at 03:14:04PM +0100, Krzysztof Kozlowski wrote:
> On pi?, 2014-11-28 at 11:38 +0000, Mark Brown wrote:

> > This sort of thing is a sign that we're not saving much by moving the
> > parsing to the core and perhaps there's more flexiblity here... 

> The driver receive callbacks (or exposes other kind of interface) for
> other core-generalized code. Recent example is parsing regulator mode
> (added by Javier) and .of_map_mode() callback.

Right, but that's actually doing some device specific translation and
successfully factoring out the bulk of the code - the fact that it's
taking parameters and returning data is a good sign.  This is a callback
placed randomly away from any other related code (adding to the
confusion - it's not integrated into the rest of the flow around this at
all) without a clear purpose.

> I thought how to do this without this additional set_ena_gpio() call.
> One way would be to extend the regulator modes (FAST/IDLE/STANDBY/ and
> GPIO) but this would look somehow unnatural.

Yes, that's absolutely hideous.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141128/3628c16a/attachment.sig>

[PATCH v4 5/7] regulator: max77686: Add GPIO control

[Krzysztof Kozlowski]

Add enable control over GPIO for regulators supporting this: LDO20,
LDO21, LDO22, buck8 and buck9.

This is needed for proper (and full) configuration of the Maxim 77686
PMIC without creating redundant 'regulator-fixed' entries.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/max77686.c | 58 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index 10d206266ac2..0d6bddad0ef4 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -26,6 +26,7 @@
 #include <linux/bug.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
+#include <linux/of_gpio.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/driver.h>
@@ -46,6 +47,11 @@
 #define MAX77686_DVS_UVSTEP	12500
 
 /*
+ * Value for configuring buck[89] and LDO{20,21,22} as GPIO control.
+ * It is the same as 'off' for other regulators.
+ */
+#define MAX77686_GPIO_CONTROL		0x0
+/*
  * Values used for configuring LDOs and bucks.
  * Forcing low power mode: LDO1, 3-5, 9, 13, 17-26
  */
@@ -82,6 +88,8 @@ enum max77686_ramp_rate {
 };
 
 struct max77686_data {
+	unsigned long long gpio_enabled:MAX77686_REGULATORS;
+
 	/* Array indexed by regulator id */
 	unsigned int opmode[MAX77686_REGULATORS];
 };
@@ -100,6 +108,26 @@ static unsigned int max77686_get_opmode_shift(int id)
 	}
 }
 
+/*
+ * When regulator is configured for GPIO control then it
+ * replaces "normal" mode. Any change from low power mode to normal
+ * should actually change to GPIO control.
+ * Map normal mode to proper value for such regulators.
+ */
+static unsigned int max77686_map_normal_mode(struct max77686_data *max77686,
+					     int id)
+{
+	switch (id) {
+	case MAX77686_BUCK8:
+	case MAX77686_BUCK9:
+	case MAX77686_LDO20 ... MAX77686_LDO22:
+		if (max77686->gpio_enabled & (1 << id))
+			return MAX77686_GPIO_CONTROL;
+	}
+
+	return MAX77686_NORMAL;
+}
+
 /* Some BUCKs and LDOs supports Normal[ON/OFF] mode during suspend */
 static int max77686_set_suspend_disable(struct regulator_dev *rdev)
 {
@@ -136,7 +164,7 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev,
 		val = MAX77686_LDO_LOWPOWER_PWRREQ;
 		break;
 	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
-		val = MAX77686_NORMAL;
+		val = max77686_map_normal_mode(max77686, id);
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
@@ -160,7 +188,7 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
 {
 	unsigned int val;
 	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
-	int ret;
+	int ret, id = rdev_get_id(rdev);
 
 	switch (mode) {
 	case REGULATOR_MODE_STANDBY:			/* switch off */
@@ -170,7 +198,7 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
 		val = MAX77686_LDO_LOWPOWER_PWRREQ;
 		break;
 	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
-		val = MAX77686_NORMAL;
+		val = max77686_map_normal_mode(max77686, id);
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
@@ -184,7 +212,7 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
 	if (ret)
 		return ret;
 
-	max77686->opmode[rdev_get_id(rdev)] = val;
+	max77686->opmode[id] = val;
 	return 0;
 }
 
@@ -197,7 +225,7 @@ static int max77686_enable(struct regulator_dev *rdev)
 	shift = max77686_get_opmode_shift(id);
 
 	if (max77686->opmode[id] == MAX77686_OFF_PWRREQ)
-		max77686->opmode[id] = MAX77686_NORMAL;
+		max77686->opmode[id] = max77686_map_normal_mode(max77686, id);
 
 	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
 				  rdev->desc->enable_mask,
@@ -229,6 +257,25 @@ static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
 				  MAX77686_RAMP_RATE_MASK, ramp_value << 6);
 }
 
+static int max77686_enable_gpio_control(struct regulator_dev *rdev)
+{
+	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
+	int id = rdev_get_id(rdev);
+
+	switch (id) {
+	case MAX77686_BUCK8:
+	case MAX77686_BUCK9:
+	case MAX77686_LDO20 ... MAX77686_LDO22:
+		max77686->gpio_enabled |= (1 << id);
+
+		return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+					rdev->desc->enable_mask,
+					MAX77686_GPIO_CONTROL);
+	}
+
+	return -EINVAL;
+}
+
 static struct regulator_ops max77686_ops = {
 	.list_voltage		= regulator_list_voltage_linear,
 	.map_voltage		= regulator_map_voltage_linear,
@@ -239,6 +286,7 @@ static struct regulator_ops max77686_ops = {
 	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
 	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
 	.set_suspend_mode	= max77686_set_suspend_mode,
+	.set_ena_gpio		= max77686_enable_gpio_control,
 };
 
 static struct regulator_ops max77686_ldo_ops = {
-- 
1.9.1

[PATCH v4 5/7] regulator: max77686: Add GPIO control

[Krzysztof Kozlowski]

Add enable control over GPIO for regulators supporting this: LDO20,
LDO21, LDO22, buck8 and buck9.

This is needed for proper (and full) configuration of the Maxim 77686
PMIC without creating redundant 'regulator-fixed' entries.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/max77686.c | 58 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index 10d206266ac2..0d6bddad0ef4 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -26,6 +26,7 @@
 #include <linux/bug.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
+#include <linux/of_gpio.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/driver.h>
@@ -46,6 +47,11 @@
 #define MAX77686_DVS_UVSTEP	12500
 
 /*
+ * Value for configuring buck[89] and LDO{20,21,22} as GPIO control.
+ * It is the same as 'off' for other regulators.
+ */
+#define MAX77686_GPIO_CONTROL		0x0
+/*
  * Values used for configuring LDOs and bucks.
  * Forcing low power mode: LDO1, 3-5, 9, 13, 17-26
  */
@@ -82,6 +88,8 @@ enum max77686_ramp_rate {
 };
 
 struct max77686_data {
+	unsigned long long gpio_enabled:MAX77686_REGULATORS;
+
 	/* Array indexed by regulator id */
 	unsigned int opmode[MAX77686_REGULATORS];
 };
@@ -100,6 +108,26 @@ static unsigned int max77686_get_opmode_shift(int id)
 	}
 }
 
+/*
+ * When regulator is configured for GPIO control then it
+ * replaces "normal" mode. Any change from low power mode to normal
+ * should actually change to GPIO control.
+ * Map normal mode to proper value for such regulators.
+ */
+static unsigned int max77686_map_normal_mode(struct max77686_data *max77686,
+					     int id)
+{
+	switch (id) {
+	case MAX77686_BUCK8:
+	case MAX77686_BUCK9:
+	case MAX77686_LDO20 ... MAX77686_LDO22:
+		if (max77686->gpio_enabled & (1 << id))
+			return MAX77686_GPIO_CONTROL;
+	}
+
+	return MAX77686_NORMAL;
+}
+
 /* Some BUCKs and LDOs supports Normal[ON/OFF] mode during suspend */
 static int max77686_set_suspend_disable(struct regulator_dev *rdev)
 {
@@ -136,7 +164,7 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev,
 		val = MAX77686_LDO_LOWPOWER_PWRREQ;
 		break;
 	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
-		val = MAX77686_NORMAL;
+		val = max77686_map_normal_mode(max77686, id);
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
@@ -160,7 +188,7 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
 {
 	unsigned int val;
 	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
-	int ret;
+	int ret, id = rdev_get_id(rdev);
 
 	switch (mode) {
 	case REGULATOR_MODE_STANDBY:			/* switch off */
@@ -170,7 +198,7 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
 		val = MAX77686_LDO_LOWPOWER_PWRREQ;
 		break;
 	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
-		val = MAX77686_NORMAL;
+		val = max77686_map_normal_mode(max77686, id);
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
@@ -184,7 +212,7 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
 	if (ret)
 		return ret;
 
-	max77686->opmode[rdev_get_id(rdev)] = val;
+	max77686->opmode[id] = val;
 	return 0;
 }
 
@@ -197,7 +225,7 @@ static int max77686_enable(struct regulator_dev *rdev)
 	shift = max77686_get_opmode_shift(id);
 
 	if (max77686->opmode[id] == MAX77686_OFF_PWRREQ)
-		max77686->opmode[id] = MAX77686_NORMAL;
+		max77686->opmode[id] = max77686_map_normal_mode(max77686, id);
 
 	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
 				  rdev->desc->enable_mask,
@@ -229,6 +257,25 @@ static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
 				  MAX77686_RAMP_RATE_MASK, ramp_value << 6);
 }
 
+static int max77686_enable_gpio_control(struct regulator_dev *rdev)
+{
+	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
+	int id = rdev_get_id(rdev);
+
+	switch (id) {
+	case MAX77686_BUCK8:
+	case MAX77686_BUCK9:
+	case MAX77686_LDO20 ... MAX77686_LDO22:
+		max77686->gpio_enabled |= (1 << id);
+
+		return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+					rdev->desc->enable_mask,
+					MAX77686_GPIO_CONTROL);
+	}
+
+	return -EINVAL;
+}
+
 static struct regulator_ops max77686_ops = {
 	.list_voltage		= regulator_list_voltage_linear,
 	.map_voltage		= regulator_map_voltage_linear,
@@ -239,6 +286,7 @@ static struct regulator_ops max77686_ops = {
 	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
 	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
 	.set_suspend_mode	= max77686_set_suspend_mode,
+	.set_ena_gpio		= max77686_enable_gpio_control,
 };
 
 static struct regulator_ops max77686_ldo_ops = {
-- 
1.9.1

[PATCH v4 6/7] mfd/regulator: dt-bindings: max77686: Document gpio properties

[Krzysztof Kozlowski]

Document usage of ena-gpios properties which turn on external/GPIO
control over regulator.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 Documentation/devicetree/bindings/mfd/max77686.txt | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/max77686.txt b/Documentation/devicetree/bindings/mfd/max77686.txt
index 75fdfaf41831..93c4cd07ffc8 100644
--- a/Documentation/devicetree/bindings/mfd/max77686.txt
+++ b/Documentation/devicetree/bindings/mfd/max77686.txt
@@ -37,8 +37,12 @@ to get matched with their hardware counterparts as follow:
   Regulators which can be turned off during system suspend:
 	-LDOn	:	2, 6-8, 10-12, 14-16,
 	-BUCKn	:	1-4.
-  Use standard regulator bindings for it ('regulator-off-in-suspend').
 
+  Regulators which can be configured for GPIO enable control:
+	-LDOn	:	20, 21, 22
+	-BUCKn	:	8, 9
+  Use standard regulator bindings for these ('regulator-off-in-suspend',
+  'ena-gpios').
 
 Example:
 
@@ -65,4 +69,12 @@ Example:
 				regulator-always-on;
 				regulator-boot-on;
 			};
+
+			buck9_reg {
+				regulator-compatible = "BUCK9";
+				regulator-name = "CAM_ISP_CORE_1.2V";
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <1200000>;
+				ena-gpios = <&gpm0 3 GPIO_ACTIVE_HIGH>;
+			};
 	}
-- 
1.9.1

[PATCH v4 6/7] mfd/regulator: dt-bindings: max77686: Document gpio properties

[Krzysztof Kozlowski]

Document usage of ena-gpios properties which turn on external/GPIO
control over regulator.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 Documentation/devicetree/bindings/mfd/max77686.txt | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/max77686.txt b/Documentation/devicetree/bindings/mfd/max77686.txt
index 75fdfaf41831..93c4cd07ffc8 100644
--- a/Documentation/devicetree/bindings/mfd/max77686.txt
+++ b/Documentation/devicetree/bindings/mfd/max77686.txt
@@ -37,8 +37,12 @@ to get matched with their hardware counterparts as follow:
   Regulators which can be turned off during system suspend:
 	-LDOn	:	2, 6-8, 10-12, 14-16,
 	-BUCKn	:	1-4.
-  Use standard regulator bindings for it ('regulator-off-in-suspend').
 
+  Regulators which can be configured for GPIO enable control:
+	-LDOn	:	20, 21, 22
+	-BUCKn	:	8, 9
+  Use standard regulator bindings for these ('regulator-off-in-suspend',
+  'ena-gpios').
 
 Example:
 
@@ -65,4 +69,12 @@ Example:
 				regulator-always-on;
 				regulator-boot-on;
 			};
+
+			buck9_reg {
+				regulator-compatible = "BUCK9";
+				regulator-name = "CAM_ISP_CORE_1.2V";
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <1200000>;
+				ena-gpios = <&gpm0 3 GPIO_ACTIVE_HIGH>;
+			};
 	}
-- 
1.9.1

[PATCH v4 7/7] ARM: dts: exynos4412-trats: Switch max77686 regulators to GPIO control

[Krzysztof Kozlowski]

Remove fixed regulators (duplicating what max77686 provides) and
add GPIO control to max77686 regulators. Add of_compatible to
voltage-regulators node.

This gives the system full control over those regulators. Previously
the state of such regulators was a mixture of what max77686 driver set
over I2C and what regulator-fixed set through GPIO.

Removal of 'regulator-always-on' from CAM_ISP_CORE_1.2V (buck9) allows
disabling it when it is not used. Previously this regulator was always
enabled because its enable state is a OR of:
 - ENB9 GPIO (turned always on by regulator-fixed),
 - BUCK9EN field in BUCK9CTRL register (off by max77686 through I2C).

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 arch/arm/boot/dts/exynos4412-trats2.dts | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts b/arch/arm/boot/dts/exynos4412-trats2.dts
index 7a68e0832cd6..4ba981eae32e 100644
--- a/arch/arm/boot/dts/exynos4412-trats2.dts
+++ b/arch/arm/boot/dts/exynos4412-trats2.dts
@@ -58,15 +58,6 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		vemmc_reg: regulator-0 {
-			compatible = "regulator-fixed";
-			regulator-name = "VMEM_VDD_2.8V";
-			regulator-min-microvolt = <2800000>;
-			regulator-max-microvolt = <2800000>;
-			gpio = <&gpk0 2 0>;
-			enable-active-high;
-		};
-
 		cam_io_reg: voltage-regulator-1 {
 			compatible = "regulator-fixed";
 			regulator-name = "CAM_SENSOR_A";
@@ -94,16 +85,6 @@
 			enable-active-high;
 		};
 
-		cam_isp_core_reg: voltage-regulator-4 {
-			compatible = "regulator-fixed";
-			regulator-name = "CAM_ISP_CORE_1.2V_EN";
-			regulator-min-microvolt = <1200000>;
-			regulator-max-microvolt = <1200000>;
-			gpio = <&gpm0 3 0>;
-			enable-active-high;
-			regulator-always-on;
-		};
-
 		ps_als_reg: voltage-regulator-5 {
 			compatible = "regulator-fixed";
 			regulator-name = "LED_A_3.0V";
@@ -405,6 +386,7 @@
 					regulator-name = "VTF_2.8V";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
+					ena-gpios = <&gpy2 0 GPIO_ACTIVE_HIGH>;
 				};
 
 				ldo22_reg: ldo22 {
@@ -412,6 +394,7 @@
 					regulator-name = "VMEM_VDD_2.8V";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
+					ena-gpios = <&gpk0 2 GPIO_ACTIVE_HIGH>;
 				};
 
 				ldo23_reg: ldo23 {
@@ -518,6 +501,7 @@
 					regulator-name = "VMEM_VDDF_3.0V";
 					regulator-min-microvolt = <2850000>;
 					regulator-max-microvolt = <2850000>;
+					ena-gpios = <&gpk0 2 GPIO_ACTIVE_HIGH>;
 				};
 
 				buck9_reg: buck9 {
@@ -525,6 +509,7 @@
 					regulator-name = "CAM_ISP_CORE_1.2V";
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1200000>;
+					ena-gpios = <&gpm0 3 GPIO_ACTIVE_HIGH>;
 				};
 			};
 		};
@@ -591,7 +576,7 @@
 		broken-cd;
 		non-removable;
 		card-detect-delay = <200>;
-		vmmc-supply = <&vemmc_reg>;
+		vmmc-supply = <&ldo22_reg>;
 		clock-frequency = <400000000>;
 		samsung,dw-mshc-ciu-div = <0>;
 		samsung,dw-mshc-sdr-timing = <2 3>;
-- 
1.9.1

[PATCH v4 7/7] ARM: dts: exynos4412-trats: Switch max77686 regulators to GPIO control

[Krzysztof Kozlowski]

Remove fixed regulators (duplicating what max77686 provides) and
add GPIO control to max77686 regulators. Add of_compatible to
voltage-regulators node.

This gives the system full control over those regulators. Previously
the state of such regulators was a mixture of what max77686 driver set
over I2C and what regulator-fixed set through GPIO.

Removal of 'regulator-always-on' from CAM_ISP_CORE_1.2V (buck9) allows
disabling it when it is not used. Previously this regulator was always
enabled because its enable state is a OR of:
 - ENB9 GPIO (turned always on by regulator-fixed),
 - BUCK9EN field in BUCK9CTRL register (off by max77686 through I2C).

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 arch/arm/boot/dts/exynos4412-trats2.dts | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts b/arch/arm/boot/dts/exynos4412-trats2.dts
index 7a68e0832cd6..4ba981eae32e 100644
--- a/arch/arm/boot/dts/exynos4412-trats2.dts
+++ b/arch/arm/boot/dts/exynos4412-trats2.dts
@@ -58,15 +58,6 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		vemmc_reg: regulator-0 {
-			compatible = "regulator-fixed";
-			regulator-name = "VMEM_VDD_2.8V";
-			regulator-min-microvolt = <2800000>;
-			regulator-max-microvolt = <2800000>;
-			gpio = <&gpk0 2 0>;
-			enable-active-high;
-		};
-
 		cam_io_reg: voltage-regulator-1 {
 			compatible = "regulator-fixed";
 			regulator-name = "CAM_SENSOR_A";
@@ -94,16 +85,6 @@
 			enable-active-high;
 		};
 
-		cam_isp_core_reg: voltage-regulator-4 {
-			compatible = "regulator-fixed";
-			regulator-name = "CAM_ISP_CORE_1.2V_EN";
-			regulator-min-microvolt = <1200000>;
-			regulator-max-microvolt = <1200000>;
-			gpio = <&gpm0 3 0>;
-			enable-active-high;
-			regulator-always-on;
-		};
-
 		ps_als_reg: voltage-regulator-5 {
 			compatible = "regulator-fixed";
 			regulator-name = "LED_A_3.0V";
@@ -405,6 +386,7 @@
 					regulator-name = "VTF_2.8V";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
+					ena-gpios = <&gpy2 0 GPIO_ACTIVE_HIGH>;
 				};
 
 				ldo22_reg: ldo22 {
@@ -412,6 +394,7 @@
 					regulator-name = "VMEM_VDD_2.8V";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
+					ena-gpios = <&gpk0 2 GPIO_ACTIVE_HIGH>;
 				};
 
 				ldo23_reg: ldo23 {
@@ -518,6 +501,7 @@
 					regulator-name = "VMEM_VDDF_3.0V";
 					regulator-min-microvolt = <2850000>;
 					regulator-max-microvolt = <2850000>;
+					ena-gpios = <&gpk0 2 GPIO_ACTIVE_HIGH>;
 				};
 
 				buck9_reg: buck9 {
@@ -525,6 +509,7 @@
 					regulator-name = "CAM_ISP_CORE_1.2V";
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1200000>;
+					ena-gpios = <&gpm0 3 GPIO_ACTIVE_HIGH>;
 				};
 			};
 		};
@@ -591,7 +576,7 @@
 		broken-cd;
 		non-removable;
 		card-detect-delay = <200>;
-		vmmc-supply = <&vemmc_reg>;
+		vmmc-supply = <&ldo22_reg>;
 		clock-frequency = <400000000>;
 		samsung,dw-mshc-ciu-div = <0>;
 		samsung,dw-mshc-sdr-timing = <2 3>;
-- 
1.9.1