[PATCH 0/8] regulator: max77686: Add GPIO control

[PATCH 0/8] regulator: max77686: Add GPIO control

[Krzysztof Kozlowski]

Hi,


This patch helps in proper description of max77686 regulators in DTS by
allowing to control them over GPIO. This allows removal of
fixed regulators from DTS which duplicate the description of hardware.

The first five patches are cleanups, including board support removal.

The whole patchset should be taken at once.

Patchset is rebased on next-20141023 AND:
1. ARM: EXYNOS: Call regulator suspend prepare/finish
   https://lkml.org/lkml/2014/10/20/545
2. regulator: max77686/trats2: Disable some regulators in suspend
   https://lkml.org/lkml/2014/10/27/280


Best regards,
Krzysztof

Krzysztof Kozlowski (8):
  regulator: max77802: Remove support for board files
  regulator: max77686: Remove support for board files
  mfd: max77686/802: Remove support for board files
  regulator: max77686: Make regulator_desc array const
  regulator: max77686: Initialize opmode explicitly to normal mode
  regulator: max77686: Add external GPIO control
  mfd/regulator: dt-bindings: max77686: Document gpio property
  ARM: dts: exynos4412-trats: Switch max77686 regulators to GPIO control

 Documentation/devicetree/bindings/mfd/max77686.txt |   3 +
 arch/arm/boot/dts/exynos4412-trats2.dts            |  25 +---
 drivers/mfd/max77686.c                             |  23 ---
 drivers/regulator/max77686.c                       | 158 +++++++++++++++------
 drivers/regulator/max77802.c                       |  44 ++----
 include/linux/mfd/max77686-private.h               |   1 -
 include/linux/mfd/max77686.h                       |  22 ---
 7 files changed, 141 insertions(+), 135 deletions(-)

-- 
1.9.1

[PATCH 1/8] regulator: max77802: Remove support for board files

[Krzysztof Kozlowski]

The driver is used only on Exynos based boards with DTS support.
Convert the driver to DTS-only version.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/max77802.c | 44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/drivers/regulator/max77802.c b/drivers/regulator/max77802.c
index 5839c4509e1f..61d03e9f8acf 100644
--- a/drivers/regulator/max77802.c
+++ b/drivers/regulator/max77802.c
@@ -70,6 +70,9 @@ static unsigned int ramp_table_77802_4bit[] = {
 };
 
 struct max77802_regulator_prv {
+	struct max77686_regulator_data *regulators;
+	int num_regulators;
+
 	unsigned int opmode[MAX77802_REG_MAX];
 };
 
@@ -517,9 +520,8 @@ static struct regulator_desc regulators[] = {
 	regulator_77802_desc_n_ldo(35, 2, 1),
 };
 
-#ifdef CONFIG_OF
-static int max77802_pmic_dt_parse_pdata(struct platform_device *pdev,
-					struct max77686_platform_data *pdata)
+static int max77802_pmic_dt_parse(struct platform_device *pdev,
+				struct max77802_regulator_prv *max77802)
 {
 	struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
 	struct device_node *pmic_np, *regulators_np;
@@ -534,15 +536,15 @@ static int max77802_pmic_dt_parse_pdata(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	pdata->num_regulators = ARRAY_SIZE(regulators);
+	max77802->num_regulators = ARRAY_SIZE(regulators);
 	rdata = devm_kzalloc(&pdev->dev, sizeof(*rdata) *
-			     pdata->num_regulators, GFP_KERNEL);
+			     max77802->num_regulators, GFP_KERNEL);
 	if (!rdata) {
 		of_node_put(regulators_np);
 		return -ENOMEM;
 	}
 
-	for (i = 0; i < pdata->num_regulators; i++) {
+	for (i = 0; i < max77802->num_regulators; i++) {
 		rmatch.name = regulators[i].name;
 		rmatch.init_data = NULL;
 		rmatch.of_node = NULL;
@@ -557,44 +559,28 @@ static int max77802_pmic_dt_parse_pdata(struct platform_device *pdev,
 		rdata[i].id = regulators[i].id;
 	}
 
-	pdata->regulators = rdata;
+	max77802->regulators = rdata;
 	of_node_put(regulators_np);
 
 	return 0;
 }
-#else
-static int max77802_pmic_dt_parse_pdata(struct platform_device *pdev,
-					struct max77686_platform_data *pdata)
-{
-	return 0;
-}
-#endif /* CONFIG_OF */
 
 static int max77802_pmic_probe(struct platform_device *pdev)
 {
 	struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
-	struct max77686_platform_data *pdata = dev_get_platdata(iodev->dev);
 	struct max77802_regulator_prv *max77802;
 	int i, ret = 0, val;
 	struct regulator_config config = { };
 
-	/* This is allocated by the MFD driver */
-	if (!pdata) {
-		dev_err(&pdev->dev, "no platform data found for regulator\n");
-		return -ENODEV;
-	}
-
 	max77802 = devm_kzalloc(&pdev->dev,
 				sizeof(struct max77802_regulator_prv),
 				GFP_KERNEL);
 	if (!max77802)
 		return -ENOMEM;
 
-	if (iodev->dev->of_node) {
-		ret = max77802_pmic_dt_parse_pdata(pdev, pdata);
-		if (ret)
-			return ret;
-	}
+	ret = max77802_pmic_dt_parse(pdev, max77802);
+	if (ret)
+		return ret;
 
 	config.dev = iodev->dev;
 	config.regmap = iodev->regmap;
@@ -603,11 +589,11 @@ static int max77802_pmic_probe(struct platform_device *pdev)
 
 	for (i = 0; i < MAX77802_REG_MAX; i++) {
 		struct regulator_dev *rdev;
-		int id = pdata->regulators[i].id;
+		int id = max77802->regulators[i].id;
 		int shift = max77802_get_opmode_shift(id);
 
-		config.init_data = pdata->regulators[i].initdata;
-		config.of_node = pdata->regulators[i].of_node;
+		config.init_data = max77802->regulators[i].initdata;
+		config.of_node = max77802->regulators[i].of_node;
 
 		ret = regmap_read(iodev->regmap, regulators[i].enable_reg, &val);
 		val = val >> shift & MAX77802_OPMODE_MASK;
-- 
1.9.1

Re: [PATCH 1/8] regulator: max77802: Remove support for board files

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> The driver is used only on Exynos based boards with DTS support.
> Convert the driver to DTS-only version.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/regulator/max77802.c | 44 +++++++++++++++-----------------------------
>  1 file changed, 15 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/regulator/max77802.c b/drivers/regulator/max77802.c
> index 5839c4509e1f..61d03e9f8acf 100644
> --- a/drivers/regulator/max77802.c
> +++ b/drivers/regulator/max77802.c

The original ChromeOS max77xxx driver supported both DT and platform data
configuration and that's why the max77802 driver was not DT-only.

But I looked at the ChromeOS kernel now and indeed even in that tree there
are no board files using the max77802 platform data so I agree that it
should be removed.

Acked-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Best regards,
Javier

Re: [PATCH 1/8] regulator: max77802: Remove support for board files

[Krzysztof Kozlowski]

On pon, 2014-10-27 at 20:36 +0100, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> > The driver is used only on Exynos based boards with DTS support.
> > Convert the driver to DTS-only version.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  drivers/regulator/max77802.c | 44 +++++++++++++++-----------------------------
> >  1 file changed, 15 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/regulator/max77802.c b/drivers/regulator/max77802.c
> > index 5839c4509e1f..61d03e9f8acf 100644
> > --- a/drivers/regulator/max77802.c
> > +++ b/drivers/regulator/max77802.c
> 
> The original ChromeOS max77xxx driver supported both DT and platform data
> configuration and that's why the max77802 driver was not DT-only.

If you mean custom vendor kernel (probably 3.4, 3.8 or 3.10 with board
files) then we shouldn't care. It is out of tree. The driver is for the
mainline kernel.

> 
> But I looked at the ChromeOS kernel now and indeed even in that tree there
> are no board files using the max77802 platform data so I agree that it
> should be removed.
> 
> Acked-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Thanks! However Mark had some comments which will reorganize the patch
completely. I'll be sending next version and probably I would need your
ack and testing again.

Best regards,
Krzysztof

> 
> Best regards,
> Javier

Re: [PATCH 1/8] regulator: max77802: Remove support for board files

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/28/2014 09:45 AM, Krzysztof Kozlowski wrote:
> 
> Thanks! However Mark had some comments which will reorganize the patch
> completely. I'll be sending next version and probably I would need your
> ack and testing again.
> 

Sure, I can test your patches if you don't have access to a machine with
the max77802 PMIC.

Best regards,
Javier

[PATCH 2/8] regulator: max77686: Remove support for board files

[Krzysztof Kozlowski]

The driver is used only on Exynos4 based boards with DTS support.
Convert the driver to DTS-only version.

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

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index c6bf6f79bd2a..04af7a2b0910 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -82,6 +82,9 @@ enum max77686_ramp_rate {
 };
 
 struct max77686_data {
+	struct max77686_regulator_data *regulators;
+	int num_regulators;
+
 	unsigned int opmode[MAX77686_REGULATORS];
 };
 
@@ -430,9 +433,8 @@ static struct regulator_desc regulators[] = {
 	regulator_desc_buck(9),
 };
 
-#ifdef CONFIG_OF
-static int max77686_pmic_dt_parse_pdata(struct platform_device *pdev,
-					struct max77686_platform_data *pdata)
+static int max77686_pmic_dt_parse(struct platform_device *pdev,
+					struct max77686_data *max77686)
 {
 	struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
 	struct device_node *pmic_np, *regulators_np;
@@ -447,15 +449,15 @@ static int max77686_pmic_dt_parse_pdata(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	pdata->num_regulators = ARRAY_SIZE(regulators);
+	max77686->num_regulators = ARRAY_SIZE(regulators);
 	rdata = devm_kzalloc(&pdev->dev, sizeof(*rdata) *
-			     pdata->num_regulators, GFP_KERNEL);
+			     max77686->num_regulators, GFP_KERNEL);
 	if (!rdata) {
 		of_node_put(regulators_np);
 		return -ENOMEM;
 	}
 
-	for (i = 0; i < pdata->num_regulators; i++) {
+	for (i = 0; i < max77686->num_regulators; i++) {
 		rmatch.name = regulators[i].name;
 		rmatch.init_data = NULL;
 		rmatch.of_node = NULL;
@@ -464,51 +466,36 @@ static int max77686_pmic_dt_parse_pdata(struct platform_device *pdev,
 		rdata[i].of_node = rmatch.of_node;
 	}
 
-	pdata->regulators = rdata;
+	max77686->regulators = rdata;
 	of_node_put(regulators_np);
 
 	return 0;
 }
-#else
-static int max77686_pmic_dt_parse_pdata(struct platform_device *pdev,
-					struct max77686_platform_data *pdata)
-{
-	return 0;
-}
-#endif /* CONFIG_OF */
 
 static int max77686_pmic_probe(struct platform_device *pdev)
 {
 	struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
-	struct max77686_platform_data *pdata = dev_get_platdata(iodev->dev);
 	struct max77686_data *max77686;
 	int i, ret = 0;
 	struct regulator_config config = { };
 
 	dev_dbg(&pdev->dev, "%s\n", __func__);
 
-	if (!pdata) {
-		dev_err(&pdev->dev, "no platform data found for regulator\n");
-		return -ENODEV;
-	}
+	max77686 = devm_kzalloc(&pdev->dev, sizeof(struct max77686_data),
+				GFP_KERNEL);
+	if (!max77686)
+		return -ENOMEM;
 
-	if (iodev->dev->of_node) {
-		ret = max77686_pmic_dt_parse_pdata(pdev, pdata);
-		if (ret)
-			return ret;
-	}
+	ret = max77686_pmic_dt_parse(pdev, max77686);
+	if (ret)
+		return ret;
 
-	if (pdata->num_regulators != MAX77686_REGULATORS) {
+	if (max77686->num_regulators != MAX77686_REGULATORS) {
 		dev_err(&pdev->dev,
 			"Invalid initial data for regulator's initialiation\n");
 		return -EINVAL;
 	}
 
-	max77686 = devm_kzalloc(&pdev->dev, sizeof(struct max77686_data),
-				GFP_KERNEL);
-	if (!max77686)
-		return -ENOMEM;
-
 	config.dev = &pdev->dev;
 	config.regmap = iodev->regmap;
 	config.driver_data = max77686;
@@ -517,8 +504,8 @@ static int max77686_pmic_probe(struct platform_device *pdev)
 	for (i = 0; i < MAX77686_REGULATORS; i++) {
 		struct regulator_dev *rdev;
 
-		config.init_data = pdata->regulators[i].initdata;
-		config.of_node = pdata->regulators[i].of_node;
+		config.init_data = max77686->regulators[i].initdata;
+		config.of_node = max77686->regulators[i].of_node;
 
 		max77686->opmode[i] = regulators[i].enable_mask >>
 						max77686_get_opmode_shift(i);
-- 
1.9.1

Re: [PATCH 2/8] regulator: max77686: Remove support for board files

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> The driver is used only on Exynos4 based boards with DTS support.
> Convert the driver to DTS-only version.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/regulator/max77686.c | 51 +++++++++++++++++---------------------------
>  1 file changed, 19 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> index c6bf6f79bd2a..04af7a2b0910 100644
> --- a/drivers/regulator/max77686.c
> +++ b/drivers/regulator/max77686.c

I agree with the patch but shouldn't the mfd driver depends on OF if is DT-only?

Best regards,
Javier

Re: [PATCH 2/8] regulator: max77686: Remove support for board files

[Mark Brown]

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

On Mon, Oct 27, 2014 at 04:03:40PM +0100, Krzysztof Kozlowski wrote:
> The driver is used only on Exynos4 based boards with DTS support.
> Convert the driver to DTS-only version.

This doesn't seem like a particularly persuasive reason honestly and if
you're going to mess around with this stuff please fix it properly...

>  struct max77686_data {
> +	struct max77686_regulator_data *regulators;
> +	int num_regulators;
> +

...the only reason for this array and the specification of the number of
regulators is to make platform data easier to do...

> -	pdata->num_regulators = ARRAY_SIZE(regulators);
> +	max77686->num_regulators = ARRAY_SIZE(regulators);

...and indeed we wind up with a constant here anyway.  At the very least
it'd be better to pull the parsing into the registration, right now we
have the code still laid out for platform data so it's hard to see it as
a win in cleanup terms.  If we're not translating into platform data we
shouldn't need to keep this stuff around outside of the probe function.

What would be even better would be to convert to use the standard DT
parsing with regulators_node and of_match specified in the descriptor
and then delete most of this code entirely.

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

Re: [PATCH 2/8] regulator: max77686: Remove support for board files

[Krzysztof Kozlowski]

On wto, 2014-10-28 at 00:37 +0000, Mark Brown wrote:
> On Mon, Oct 27, 2014 at 04:03:40PM +0100, Krzysztof Kozlowski wrote:
> > The driver is used only on Exynos4 based boards with DTS support.
> > Convert the driver to DTS-only version.
> 
> This doesn't seem like a particularly persuasive reason honestly and if
> you're going to mess around with this stuff please fix it properly...
> 
> >  struct max77686_data {
> > +	struct max77686_regulator_data *regulators;
> > +	int num_regulators;
> > +
> 
> ...the only reason for this array and the specification of the number of
> regulators is to make platform data easier to do...
> 
> > -	pdata->num_regulators = ARRAY_SIZE(regulators);
> > +	max77686->num_regulators = ARRAY_SIZE(regulators);
> 
> ...and indeed we wind up with a constant here anyway.  At the very least
> it'd be better to pull the parsing into the registration, right now we
> have the code still laid out for platform data so it's hard to see it as
> a win in cleanup terms.  If we're not translating into platform data we
> shouldn't need to keep this stuff around outside of the probe function.
> 
> What would be even better would be to convert to use the standard DT
> parsing with regulators_node and of_match specified in the descriptor
> and then delete most of this code entirely.

You're right, there's a lot to clean in this driver.

The only caveat is that I can't test the max77802 regulator so I
refrained from doing intrusive changes to it. I'll try to clean up both
drivers and make max77802 as RFT.

Best regards,
Krzysztof

[PATCH 3/8] mfd: max77686/802: Remove support for board files

[Krzysztof Kozlowski]

The driver is used only on Exynos based boards with DTS support.
Convert the driver to DTS-only version. This simplifies a little the
code:
1. No dead (unused) entries in platform_data structure.
2. More code removed (from all three patches: 34 insertions(+), 107
   deletions).
3. Regulator driver does not depend on allocated memory
   from MFD driver.
4. It makes also easier extending the regulator driver

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/mfd/max77686.c               | 23 -----------------------
 include/linux/mfd/max77686-private.h |  1 -
 include/linux/mfd/max77686.h         | 22 ----------------------
 3 files changed, 46 deletions(-)

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..ea8501dfc865 100644
--- a/include/linux/mfd/max77686.h
+++ b/include/linux/mfd/max77686.h
@@ -136,26 +136,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 3/8] mfd: max77686/802: Remove support for board files

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> The driver is used only on Exynos based boards with DTS support.
> Convert the driver to DTS-only version. This simplifies a little the
> code:
> 1. No dead (unused) entries in platform_data structure.
> 2. More code removed (from all three patches: 34 insertions(+), 107
>    deletions).
> 3. Regulator driver does not depend on allocated memory
>    from MFD driver.
> 4. It makes also easier extending the regulator driver
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/mfd/max77686.c               | 23 -----------------------
>  include/linux/mfd/max77686-private.h |  1 -
>  include/linux/mfd/max77686.h         | 22 ----------------------
>  3 files changed, 46 deletions(-)
> 
> 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

As I said in Patch 2/8, I think this driver should depend on OF since
is now DT-only. Regulator drivers don't need to do this since they
already depend on MFD_MAX77686. With that change feel free to add:

Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Please also add my Reviewed-by tag to Patch 2/8 since I just noticed
that I forgot to add it.

Best regards,
Javier

Re: [PATCH 3/8] mfd: max77686/802: Remove support for board files

[Krzysztof Kozlowski]

On pon, 2014-10-27 at 20:48 +0100, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> > The driver is used only on Exynos based boards with DTS support.
> > Convert the driver to DTS-only version. This simplifies a little the
> > code:
> > 1. No dead (unused) entries in platform_data structure.
> > 2. More code removed (from all three patches: 34 insertions(+), 107
> >    deletions).
> > 3. Regulator driver does not depend on allocated memory
> >    from MFD driver.
> > 4. It makes also easier extending the regulator driver
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  drivers/mfd/max77686.c               | 23 -----------------------
> >  include/linux/mfd/max77686-private.h |  1 -
> >  include/linux/mfd/max77686.h         | 22 ----------------------
> >  3 files changed, 46 deletions(-)
> > 
> > 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
> 
> As I said in Patch 2/8, I think this driver should depend on OF since
> is now DT-only. Regulator drivers don't need to do this since they
> already depend on MFD_MAX77686. With that change feel free to add:
> 
> Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> 
> Please also add my Reviewed-by tag to Patch 2/8 since I just noticed
> that I forgot to add it.

Thank you, I'll add depends on OF.

Best regards,
Krzysztof
> 
> Best regards,
> Javier

[PATCH 4/8] regulator: max77686: Make regulator_desc array const

[Krzysztof Kozlowski]

The regulator_register() expects array of 'regulator_desc' to be const.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/regulator/max77686.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index 04af7a2b0910..7930b4f82c0b 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -395,7 +395,7 @@ static struct regulator_ops max77686_buck_dvs_ops = {
 			<< MAX77686_OPMODE_BUCK234_SHIFT,		\
 }
 
-static struct regulator_desc regulators[] = {
+static const struct regulator_desc regulators[] = {
 	regulator_desc_ldo1_low(1),
 	regulator_desc_ldo_low(2),
 	regulator_desc_ldo(3),
-- 
1.9.1

Re: [PATCH 4/8] regulator: max77686: Make regulator_desc array const

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> The regulator_register() expects array of 'regulator_desc' to be const.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/regulator/max77686.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> index 04af7a2b0910..7930b4f82c0b 100644
> --- a/drivers/regulator/max77686.c
> +++ b/drivers/regulator/max77686.c
> @@ -395,7 +395,7 @@ static struct regulator_ops max77686_buck_dvs_ops = {
>  			<< MAX77686_OPMODE_BUCK234_SHIFT,		\
>  }
>  
> -static struct regulator_desc regulators[] = {
> +static const struct regulator_desc regulators[] = {
>  	regulator_desc_ldo1_low(1),
>  	regulator_desc_ldo_low(2),
>  	regulator_desc_ldo(3),
> 

Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Best regards,
Javier

Re: [PATCH 4/8] regulator: max77686: Make regulator_desc array const

[Mark Brown]

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

On Mon, Oct 27, 2014 at 04:03:42PM +0100, Krzysztof Kozlowski wrote:
> The regulator_register() expects array of 'regulator_desc' to be const.

Applied, thanks.

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

[PATCH 5/8] regulator: max77686: Initialize opmode explicitly to normal mode

[Krzysztof Kozlowski]

Minor nit: Initialize the opmode for each regulator to normal mode in a
readable explicit way.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/regulator/max77686.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index 7930b4f82c0b..e5738c363f07 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -507,8 +507,8 @@ static int max77686_pmic_probe(struct platform_device *pdev)
 		config.init_data = max77686->regulators[i].initdata;
 		config.of_node = max77686->regulators[i].of_node;
 
-		max77686->opmode[i] = regulators[i].enable_mask >>
-						max77686_get_opmode_shift(i);
+		max77686->opmode[i] = MAX77686_NORMAL;
+
 		rdev = devm_regulator_register(&pdev->dev,
 						&regulators[i], &config);
 		if (IS_ERR(rdev)) {
-- 
1.9.1

Re: [PATCH 5/8] regulator: max77686: Initialize opmode explicitly to normal mode

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> Minor nit: Initialize the opmode for each regulator to normal mode in a
> readable explicit way.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  drivers/regulator/max77686.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> index 7930b4f82c0b..e5738c363f07 100644
> --- a/drivers/regulator/max77686.c
> +++ b/drivers/regulator/max77686.c
> @@ -507,8 +507,8 @@ static int max77686_pmic_probe(struct platform_device *pdev)
>  		config.init_data = max77686->regulators[i].initdata;
>  		config.of_node = max77686->regulators[i].of_node;
>  
> -		max77686->opmode[i] = regulators[i].enable_mask >>
> -						max77686_get_opmode_shift(i);
> +		max77686->opmode[i] = MAX77686_NORMAL;
> +
>  		rdev = devm_regulator_register(&pdev->dev,
>  						&regulators[i], &config);
>  		if (IS_ERR(rdev)) {
> 

Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Best regards,
Javier

[PATCH 6/8] regulator: max77686: Add external GPIO control

[Krzysztof Kozlowski]

Add 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 | 101 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 96 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index e5738c363f07..c54a8603f5b2 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 external control.
+ * It is the same as 'off' for other regulators.
+ */
+#define MAX77686_EXT_CONTROL		0x0
+/*
  * Values used for configuring LDOs and bucks.
  * Forcing low power mode: LDO1, 3-5, 9, 13, 17-26
  */
@@ -85,6 +91,9 @@ struct max77686_data {
 	struct max77686_regulator_data *regulators;
 	int num_regulators;
 
+	/* Array of size num_regulators with GPIOs for external control. */
+	int *ext_control_gpio;
+
 	unsigned int opmode[MAX77686_REGULATORS];
 };
 
@@ -102,6 +111,28 @@ static unsigned int max77686_get_opmode_shift(int id)
 	}
 }
 
+/*
+ * When regulator is configured for external control then it
+ * replaces "normal" mode. Any change from low power mode to normal
+ * should actually change to external control.
+ * Map normal mode to proper value for such regulators.
+ */
+static int max77686_map_normal_mode(struct max77686_data *max77686, int id)
+{
+	switch (id) {
+	case MAX77686_BUCK8:
+	case MAX77686_BUCK9:
+		if (gpio_is_valid(max77686->ext_control_gpio[id]))
+			return MAX77686_EXT_CONTROL;
+
+	case MAX77686_LDO20 ... MAX77686_LDO22:
+		if (gpio_is_valid(max77686->ext_control_gpio[id]))
+			return MAX77686_EXT_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)
 {
@@ -138,7 +169,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",
@@ -162,7 +193,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 */
@@ -172,7 +203,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",
@@ -186,7 +217,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;
 }
 
@@ -199,7 +230,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,
@@ -433,6 +464,42 @@ static const struct regulator_desc regulators[] = {
 	regulator_desc_buck(9),
 };
 
+static int max77686_enable_ext_control(struct regulator_dev *rdev)
+{
+	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+					rdev->desc->enable_mask,
+					MAX77686_EXT_CONTROL);
+}
+
+static void max77686_pmic_dt_parse_ext_control_gpio(struct platform_device *pdev,
+		struct max77686_regulator_data *rdata,
+		struct max77686_data *max77686)
+{
+	int *gpio = max77686->ext_control_gpio;
+	unsigned int i;
+	unsigned int valid_regulators[5] = { MAX77686_LDO20, MAX77686_LDO21,
+		MAX77686_LDO22, MAX77686_BUCK8, MAX77686_BUCK9 };
+
+	/*
+	 * 0 is a valid GPIO so initialize all GPIOs to negative value
+	 * to indicate that external control won't be used for this regulator.
+	 */
+	for (i = 0; i < max77686->num_regulators; i++)
+		max77686->ext_control_gpio[i] = -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(valid_regulators); i++) {
+		unsigned int reg = valid_regulators[i];
+
+		if (!rdata[reg].initdata || !rdata[reg].of_node)
+			continue;
+
+		gpio[reg] = of_get_named_gpio(rdata[reg].of_node, "gpio", 0);
+		if (gpio_is_valid(gpio[reg]))
+			dev_dbg(&pdev->dev, "Using GPIO %d for ext-control over %d/%s\n",
+				gpio[reg], reg, rdata[reg].of_node->name);
+	}
+}
+
 static int max77686_pmic_dt_parse(struct platform_device *pdev,
 					struct max77686_data *max77686)
 {
@@ -457,6 +524,14 @@ static int max77686_pmic_dt_parse(struct platform_device *pdev,
 		return -ENOMEM;
 	}
 
+	max77686->ext_control_gpio = devm_kmalloc_array(&pdev->dev,
+			max77686->num_regulators,
+			sizeof(*max77686->ext_control_gpio), GFP_KERNEL);
+	if (!max77686->ext_control_gpio) {
+		of_node_put(regulators_np);
+		return -ENOMEM;
+	}
+
 	for (i = 0; i < max77686->num_regulators; i++) {
 		rmatch.name = regulators[i].name;
 		rmatch.init_data = NULL;
@@ -466,7 +541,9 @@ static int max77686_pmic_dt_parse(struct platform_device *pdev,
 		rdata[i].of_node = rmatch.of_node;
 	}
 
+	max77686_pmic_dt_parse_ext_control_gpio(pdev, rdata, max77686);
 	max77686->regulators = rdata;
+
 	of_node_put(regulators_np);
 
 	return 0;
@@ -499,6 +576,7 @@ static int max77686_pmic_probe(struct platform_device *pdev)
 	config.dev = &pdev->dev;
 	config.regmap = iodev->regmap;
 	config.driver_data = max77686;
+	config.ena_gpio_flags = GPIOF_OUT_INIT_HIGH;
 	platform_set_drvdata(pdev, max77686);
 
 	for (i = 0; i < MAX77686_REGULATORS; i++) {
@@ -506,6 +584,8 @@ static int max77686_pmic_probe(struct platform_device *pdev)
 
 		config.init_data = max77686->regulators[i].initdata;
 		config.of_node = max77686->regulators[i].of_node;
+		config.ena_gpio = max77686->ext_control_gpio[i];
+		config.ena_gpio_initialized = true;
 
 		max77686->opmode[i] = MAX77686_NORMAL;
 
@@ -516,6 +596,17 @@ static int max77686_pmic_probe(struct platform_device *pdev)
 				"regulator init failed for %d\n", i);
 			return PTR_ERR(rdev);
 		}
+
+		if (gpio_is_valid(config.ena_gpio)) {
+			ret = max77686_enable_ext_control(rdev);
+
+			if (ret < 0) {
+				dev_err(&pdev->dev,
+					"regulator enable ext control failed for %d\n",
+					i);
+				return ret;
+			}
+		}
 	}
 
 	return 0;
-- 
1.9.1

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> @@ -85,6 +91,9 @@ struct max77686_data {
>  	struct max77686_regulator_data *regulators;
>  	int num_regulators;
>  
> +	/* Array of size num_regulators with GPIOs for external control. */
> +	int *ext_control_gpio;
> +

The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
interface (Documentation/gpio/consumer.txt). Could you please use the later?

Best regards,
Javier

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Krzysztof Kozlowski]

On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> > @@ -85,6 +91,9 @@ struct max77686_data {
> >  	struct max77686_regulator_data *regulators;
> >  	int num_regulators;
> >  
> > +	/* Array of size num_regulators with GPIOs for external control. */
> > +	int *ext_control_gpio;
> > +
> 
> The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
> interface (Documentation/gpio/consumer.txt). Could you please use the later?

Sure, I can. Please have in mind that regulator core still accepts old
GPIO so I will have to use desc_to_gpio(). That should work... and
should be future-ready.

Thanks for feedback,
Krzysztof

> 
> Best regards,
> Javier

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Krzysztof Kozlowski]

On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
> On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
> > Hello Krzysztof,
> > 
> > On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> > > @@ -85,6 +91,9 @@ struct max77686_data {
> > >  	struct max77686_regulator_data *regulators;
> > >  	int num_regulators;
> > >  
> > > +	/* Array of size num_regulators with GPIOs for external control. */
> > > +	int *ext_control_gpio;
> > > +
> > 
> > The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
> > interface (Documentation/gpio/consumer.txt). Could you please use the later?
> 
> Sure, I can. Please have in mind that regulator core still accepts old
> GPIO so I will have to use desc_to_gpio(). That should work... and
> should be future-ready.

It seems I was too hasty... I think usage of the new gpiod API implies
completely different bindings.

The gpiod_get() gets GPIO from a device level, not from given sub-node
pointer. This means that you cannot have DTS like this:
ldo21_reg: ldo21 {
	regulator-compatible = "LDO21";
	regulator-name = "VTF_2.8V";
	regulator-min-microvolt = <2800000>;
	regulator-max-microvolt = <2800000>;
	ec-gpio = <&gpy2 0 0>;
};

ldo22_reg: ldo22 {
	regulator-compatible = "LDO22";
	regulator-name = "VMEM_VDD_2.8V";
	regulator-min-microvolt = <2800000>;
	regulator-max-microvolt = <2800000>;
	ec-gpio = <&gpk0 2 0>;
};


I could put GPIOs in device node:

max77686_pmic@09 {
	compatible = "maxim,max77686";
	interrupt-parent = <&gpx0>;
	interrupts = <7 0>;
	reg = <0x09>;
	#clock-cells = <1>;
	ldo21-gpio = <&gpy2 0 0>;
	ldo22-gpio = <&gpk0 2 0>;

	ldo21_reg: ldo21 {
		regulator-compatible = "LDO21";
		regulator-name = "VTF_2.8V";
		regulator-min-microvolt = <2800000>;
		regulator-max-microvolt = <2800000>;
	};

	ldo22_reg: ldo22 {
		regulator-compatible = "LDO22";
		regulator-name = "VMEM_VDD_2.8V";
		regulator-min-microvolt = <2800000>;
		regulator-max-microvolt = <2800000>;
	};

This would work but I don't like it. The properties of a regulator are
above the node configuring that regulator.

Any ideas?

Best regards,
Krzysztof

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Krzysztof Kozlowski]

On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
> On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
> > On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
> > > Hello Krzysztof,
> > > 
> > > On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> > > > @@ -85,6 +91,9 @@ struct max77686_data {
> > > >  	struct max77686_regulator_data *regulators;
> > > >  	int num_regulators;
> > > >  
> > > > +	/* Array of size num_regulators with GPIOs for external control. */
> > > > +	int *ext_control_gpio;
> > > > +
> > > 
> > > The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
> > > interface (Documentation/gpio/consumer.txt). Could you please use the later?
> > 
> > Sure, I can. Please have in mind that regulator core still accepts old
> > GPIO so I will have to use desc_to_gpio(). That should work... and
> > should be future-ready.
> 
> It seems I was too hasty... I think usage of the new gpiod API implies
> completely different bindings.
> 
> The gpiod_get() gets GPIO from a device level, not from given sub-node
> pointer. This means that you cannot have DTS like this:
> ldo21_reg: ldo21 {
> 	regulator-compatible = "LDO21";
> 	regulator-name = "VTF_2.8V";
> 	regulator-min-microvolt = <2800000>;
> 	regulator-max-microvolt = <2800000>;
> 	ec-gpio = <&gpy2 0 0>;
> };
> 
> ldo22_reg: ldo22 {
> 	regulator-compatible = "LDO22";
> 	regulator-name = "VMEM_VDD_2.8V";
> 	regulator-min-microvolt = <2800000>;
> 	regulator-max-microvolt = <2800000>;
> 	ec-gpio = <&gpk0 2 0>;
> };
> 
> 
> I could put GPIOs in device node:
> 
> max77686_pmic@09 {
> 	compatible = "maxim,max77686";
> 	interrupt-parent = <&gpx0>;
> 	interrupts = <7 0>;
> 	reg = <0x09>;
> 	#clock-cells = <1>;
> 	ldo21-gpio = <&gpy2 0 0>;
> 	ldo22-gpio = <&gpk0 2 0>;
> 
> 	ldo21_reg: ldo21 {
> 		regulator-compatible = "LDO21";
> 		regulator-name = "VTF_2.8V";
> 		regulator-min-microvolt = <2800000>;
> 		regulator-max-microvolt = <2800000>;
> 	};
> 
> 	ldo22_reg: ldo22 {
> 		regulator-compatible = "LDO22";
> 		regulator-name = "VMEM_VDD_2.8V";
> 		regulator-min-microvolt = <2800000>;
> 		regulator-max-microvolt = <2800000>;
> 	};
> 
> This would work but I don't like it. The properties of a regulator are
> above the node configuring that regulator.
> 
> Any ideas?
> 

Continuing talking to myself... I found another problem - GPIO cannot be
requested more than once (-EBUSY). In case of this driver (and board:
Trats2) one GPIO is connected to regulators. The legacy GPIO API and
regulator core handle this.

With new GPIO API I would have to implement some additional steps in
such case...

So there are 2 issues:
1. Cannot put GPIO property in regulator node.
2. Cannot request some GPIO more than once.

I'm going back to legacy API for now.

Best regards,
Krzysztof

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Javier Martinez Canillas]

[adding Linus and Alexandre to the cc list]

Hello Krzysztof,

On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
> On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
>> On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
>> > On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
>> > > Hello Krzysztof,
>> > > 
>> > > On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
>> > > > @@ -85,6 +91,9 @@ struct max77686_data {
>> > > >  	struct max77686_regulator_data *regulators;
>> > > >  	int num_regulators;
>> > > >  
>> > > > +	/* Array of size num_regulators with GPIOs for external control. */
>> > > > +	int *ext_control_gpio;
>> > > > +
>> > > 
>> > > The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
>> > > interface (Documentation/gpio/consumer.txt). Could you please use the later?
>> > 
>> > Sure, I can. Please have in mind that regulator core still accepts old
>> > GPIO so I will have to use desc_to_gpio(). That should work... and
>> > should be future-ready.
>> 
>> It seems I was too hasty... I think usage of the new gpiod API implies
>> completely different bindings.
>> 
>> The gpiod_get() gets GPIO from a device level, not from given sub-node
>> pointer. This means that you cannot have DTS like this:
>> ldo21_reg: ldo21 {
>> 	regulator-compatible = "LDO21";
>> 	regulator-name = "VTF_2.8V";
>> 	regulator-min-microvolt = <2800000>;
>> 	regulator-max-microvolt = <2800000>;
>> 	ec-gpio = <&gpy2 0 0>;
>> };
>> 
>> ldo22_reg: ldo22 {
>> 	regulator-compatible = "LDO22";
>> 	regulator-name = "VMEM_VDD_2.8V";
>> 	regulator-min-microvolt = <2800000>;
>> 	regulator-max-microvolt = <2800000>;
>> 	ec-gpio = <&gpk0 2 0>;
>> };
>> 
>> 
>> I could put GPIOs in device node:
>> 
>> max77686_pmic@09 {
>> 	compatible = "maxim,max77686";
>> 	interrupt-parent = <&gpx0>;
>> 	interrupts = <7 0>;
>> 	reg = <0x09>;
>> 	#clock-cells = <1>;
>> 	ldo21-gpio = <&gpy2 0 0>;
>> 	ldo22-gpio = <&gpk0 2 0>;
>> 
>> 	ldo21_reg: ldo21 {
>> 		regulator-compatible = "LDO21";
>> 		regulator-name = "VTF_2.8V";
>> 		regulator-min-microvolt = <2800000>;
>> 		regulator-max-microvolt = <2800000>;
>> 	};
>> 
>> 	ldo22_reg: ldo22 {
>> 		regulator-compatible = "LDO22";
>> 		regulator-name = "VMEM_VDD_2.8V";
>> 		regulator-min-microvolt = <2800000>;
>> 		regulator-max-microvolt = <2800000>;
>> 	};
>> 
>> This would work but I don't like it. The properties of a regulator are
>> above the node configuring that regulator.
>> 
>> Any ideas?
>> 
> 
> Continuing talking to myself... I found another problem - GPIO cannot be
> requested more than once (-EBUSY). In case of this driver (and board:
> Trats2) one GPIO is connected to regulators. The legacy GPIO API and
> regulator core handle this.
> 
> With new GPIO API I would have to implement some additional steps in
> such case...
> 
> So there are 2 issues:
> 1. Cannot put GPIO property in regulator node.
> 2. Cannot request some GPIO more than once.
> 
> I'm going back to legacy API for now.
> 

I've added the GPIO maintainers as cc so hopefully they can comment on this.

> Best regards,
> Krzysztof
> 

Best regards,
Javier

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Alexandre Courbot]

Hi, and thanks for bringing this issue to us!

On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> [adding Linus and Alexandre to the cc list]
>
> Hello Krzysztof,
>
> On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
>> On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
>>> On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
>>> > On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
>>> > > Hello Krzysztof,
>>> > >
>>> > > On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
>>> > > > @@ -85,6 +91,9 @@ struct max77686_data {
>>> > > >        struct max77686_regulator_data *regulators;
>>> > > >        int num_regulators;
>>> > > >
>>> > > > +      /* Array of size num_regulators with GPIOs for external control. */
>>> > > > +      int *ext_control_gpio;
>>> > > > +
>>> > >
>>> > > The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
>>> > > interface (Documentation/gpio/consumer.txt). Could you please use the later?
>>> >
>>> > Sure, I can. Please have in mind that regulator core still accepts old
>>> > GPIO so I will have to use desc_to_gpio(). That should work... and
>>> > should be future-ready.
>>>
>>> It seems I was too hasty... I think usage of the new gpiod API implies
>>> completely different bindings.
>>>
>>> The gpiod_get() gets GPIO from a device level, not from given sub-node
>>> pointer. This means that you cannot have DTS like this:
>>> ldo21_reg: ldo21 {
>>>      regulator-compatible = "LDO21";
>>>      regulator-name = "VTF_2.8V";
>>>      regulator-min-microvolt = <2800000>;
>>>      regulator-max-microvolt = <2800000>;
>>>      ec-gpio = <&gpy2 0 0>;
>>> };
>>>
>>> ldo22_reg: ldo22 {
>>>      regulator-compatible = "LDO22";
>>>      regulator-name = "VMEM_VDD_2.8V";
>>>      regulator-min-microvolt = <2800000>;
>>>      regulator-max-microvolt = <2800000>;
>>>      ec-gpio = <&gpk0 2 0>;
>>> };
>>>
>>>
>>> I could put GPIOs in device node:
>>>
>>> max77686_pmic@09 {
>>>      compatible = "maxim,max77686";
>>>      interrupt-parent = <&gpx0>;
>>>      interrupts = <7 0>;
>>>      reg = <0x09>;
>>>      #clock-cells = <1>;
>>>      ldo21-gpio = <&gpy2 0 0>;
>>>      ldo22-gpio = <&gpk0 2 0>;
>>>
>>>      ldo21_reg: ldo21 {
>>>              regulator-compatible = "LDO21";
>>>              regulator-name = "VTF_2.8V";
>>>              regulator-min-microvolt = <2800000>;
>>>              regulator-max-microvolt = <2800000>;
>>>      };
>>>
>>>      ldo22_reg: ldo22 {
>>>              regulator-compatible = "LDO22";
>>>              regulator-name = "VMEM_VDD_2.8V";
>>>              regulator-min-microvolt = <2800000>;
>>>              regulator-max-microvolt = <2800000>;
>>>      };
>>>
>>> This would work but I don't like it. The properties of a regulator are
>>> above the node configuring that regulator.
>>>
>>> Any ideas?
>>>
>>
>> Continuing talking to myself... I found another problem - GPIO cannot be
>> requested more than once (-EBUSY). In case of this driver (and board:
>> Trats2) one GPIO is connected to regulators. The legacy GPIO API and
>> regulator core handle this.
>>
>> With new GPIO API I would have to implement some additional steps in
>> such case...
>>
>> So there are 2 issues:
>> 1. Cannot put GPIO property in regulator node.

For this problem you will probably want to use the
dev(m)_get_named_gpiod_from_child() function from the following patch:

https://lkml.org/lkml/2014/10/6/529

It should reach -next soon now.

>> 2. Cannot request some GPIO more than once.

We have been confronted to this problem with the regulator core as well:

http://marc.info/?l=linux-arm-kernel&m=140417649119733&w=1

I have a draft patch that allows GPIOs to be requested by several
clients. What prevented me from submitting it was that I wanted to
make sure the different requested configurations were compatible, but
maybe I am overthinking this. There are also a couple of other patches
that this depends on (like removal of the big descs array), so I don't
think it will be available too soon, sadly.

So maybe your best shot for now is to keep using the integer API, as
much as I hate it. Once we become able to request the same GPIO
several times, you should be good to switch to descriptors. Sorry this
has not been done faster.

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Krzysztof Kozlowski]

On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
> Hi, and thanks for bringing this issue to us!
> 
> On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
> > [adding Linus and Alexandre to the cc list]
> >
> > Hello Krzysztof,
> >
> > On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
> >> On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
> >>> On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
> >>> > On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
> >>> > > Hello Krzysztof,
> >>> > >
> >>> > > On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> >>> > > > @@ -85,6 +91,9 @@ struct max77686_data {
> >>> > > >        struct max77686_regulator_data *regulators;
> >>> > > >        int num_regulators;
> >>> > > >
> >>> > > > +      /* Array of size num_regulators with GPIOs for external control. */
> >>> > > > +      int *ext_control_gpio;
> >>> > > > +
> >>> > >
> >>> > > The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
> >>> > > interface (Documentation/gpio/consumer.txt). Could you please use the later?
> >>> >
> >>> > Sure, I can. Please have in mind that regulator core still accepts old
> >>> > GPIO so I will have to use desc_to_gpio(). That should work... and
> >>> > should be future-ready.
> >>>
> >>> It seems I was too hasty... I think usage of the new gpiod API implies
> >>> completely different bindings.
> >>>
> >>> The gpiod_get() gets GPIO from a device level, not from given sub-node
> >>> pointer. This means that you cannot have DTS like this:
> >>> ldo21_reg: ldo21 {
> >>>      regulator-compatible = "LDO21";
> >>>      regulator-name = "VTF_2.8V";
> >>>      regulator-min-microvolt = <2800000>;
> >>>      regulator-max-microvolt = <2800000>;
> >>>      ec-gpio = <&gpy2 0 0>;
> >>> };
> >>>
> >>> ldo22_reg: ldo22 {
> >>>      regulator-compatible = "LDO22";
> >>>      regulator-name = "VMEM_VDD_2.8V";
> >>>      regulator-min-microvolt = <2800000>;
> >>>      regulator-max-microvolt = <2800000>;
> >>>      ec-gpio = <&gpk0 2 0>;
> >>> };
> >>>
> >>>
> >>> I could put GPIOs in device node:
> >>>
> >>> max77686_pmic@09 {
> >>>      compatible = "maxim,max77686";
> >>>      interrupt-parent = <&gpx0>;
> >>>      interrupts = <7 0>;
> >>>      reg = <0x09>;
> >>>      #clock-cells = <1>;
> >>>      ldo21-gpio = <&gpy2 0 0>;
> >>>      ldo22-gpio = <&gpk0 2 0>;
> >>>
> >>>      ldo21_reg: ldo21 {
> >>>              regulator-compatible = "LDO21";
> >>>              regulator-name = "VTF_2.8V";
> >>>              regulator-min-microvolt = <2800000>;
> >>>              regulator-max-microvolt = <2800000>;
> >>>      };
> >>>
> >>>      ldo22_reg: ldo22 {
> >>>              regulator-compatible = "LDO22";
> >>>              regulator-name = "VMEM_VDD_2.8V";
> >>>              regulator-min-microvolt = <2800000>;
> >>>              regulator-max-microvolt = <2800000>;
> >>>      };
> >>>
> >>> This would work but I don't like it. The properties of a regulator are
> >>> above the node configuring that regulator.
> >>>
> >>> Any ideas?
> >>>
> >>
> >> Continuing talking to myself... I found another problem - GPIO cannot be
> >> requested more than once (-EBUSY). In case of this driver (and board:
> >> Trats2) one GPIO is connected to regulators. The legacy GPIO API and
> >> regulator core handle this.
> >>
> >> With new GPIO API I would have to implement some additional steps in
> >> such case...
> >>
> >> So there are 2 issues:
> >> 1. Cannot put GPIO property in regulator node.
> 
> For this problem you will probably want to use the
> dev(m)_get_named_gpiod_from_child() function from the following patch:
> 
> https://lkml.org/lkml/2014/10/6/529
> 
> It should reach -next soon now.

Thanks! Probably I would switch to "top" level gpios property anyway
(other reasons) but it would be valuable in some cases to specify them
per child node.

> 
> >> 2. Cannot request some GPIO more than once.
> 
> We have been confronted to this problem with the regulator core as well:
> 
> http://marc.info/?l=linux-arm-kernel&m=140417649119733&w=1
> 
> I have a draft patch that allows GPIOs to be requested by several
> clients. What prevented me from submitting it was that I wanted to
> make sure the different requested configurations were compatible, but
> maybe I am overthinking this. There are also a couple of other patches
> that this depends on (like removal of the big descs array), so I don't
> think it will be available too soon, sadly.

Shouldn't be the nature of get()/put() interface to allow multiple
requests? To me it was a kind of intuitive that I could do another
devm_gpiod_get() for the same gpio. But then it hit me with EBUSY :).

> 
> So maybe your best shot for now is to keep using the integer API, as
> much as I hate it. Once we become able to request the same GPIO
> several times, you should be good to switch to descriptors. Sorry this
> has not been done faster.

I'll do it legacy way but I'll try to use bindings gpiolib-safe. This
way future transition in the driver should not affect bindings.

Best regards,
Krzysztof

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Alexandre Courbot]

On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
>> Hi, and thanks for bringing this issue to us!
>>
>> On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
>> <javier.martinez@collabora.co.uk> wrote:
>> > [adding Linus and Alexandre to the cc list]
>> >
>> > Hello Krzysztof,
>> >
>> > On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
>> >> On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
>> >>> On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
>> >>> > On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
>> >>> > > Hello Krzysztof,
>> >>> > >
>> >>> > > On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
>> >>> > > > @@ -85,6 +91,9 @@ struct max77686_data {
>> >>> > > >        struct max77686_regulator_data *regulators;
>> >>> > > >        int num_regulators;
>> >>> > > >
>> >>> > > > +      /* Array of size num_regulators with GPIOs for external control. */
>> >>> > > > +      int *ext_control_gpio;
>> >>> > > > +
>> >>> > >
>> >>> > > The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
>> >>> > > interface (Documentation/gpio/consumer.txt). Could you please use the later?
>> >>> >
>> >>> > Sure, I can. Please have in mind that regulator core still accepts old
>> >>> > GPIO so I will have to use desc_to_gpio(). That should work... and
>> >>> > should be future-ready.
>> >>>
>> >>> It seems I was too hasty... I think usage of the new gpiod API implies
>> >>> completely different bindings.
>> >>>
>> >>> The gpiod_get() gets GPIO from a device level, not from given sub-node
>> >>> pointer. This means that you cannot have DTS like this:
>> >>> ldo21_reg: ldo21 {
>> >>>      regulator-compatible = "LDO21";
>> >>>      regulator-name = "VTF_2.8V";
>> >>>      regulator-min-microvolt = <2800000>;
>> >>>      regulator-max-microvolt = <2800000>;
>> >>>      ec-gpio = <&gpy2 0 0>;
>> >>> };
>> >>>
>> >>> ldo22_reg: ldo22 {
>> >>>      regulator-compatible = "LDO22";
>> >>>      regulator-name = "VMEM_VDD_2.8V";
>> >>>      regulator-min-microvolt = <2800000>;
>> >>>      regulator-max-microvolt = <2800000>;
>> >>>      ec-gpio = <&gpk0 2 0>;
>> >>> };
>> >>>
>> >>>
>> >>> I could put GPIOs in device node:
>> >>>
>> >>> max77686_pmic@09 {
>> >>>      compatible = "maxim,max77686";
>> >>>      interrupt-parent = <&gpx0>;
>> >>>      interrupts = <7 0>;
>> >>>      reg = <0x09>;
>> >>>      #clock-cells = <1>;
>> >>>      ldo21-gpio = <&gpy2 0 0>;
>> >>>      ldo22-gpio = <&gpk0 2 0>;
>> >>>
>> >>>      ldo21_reg: ldo21 {
>> >>>              regulator-compatible = "LDO21";
>> >>>              regulator-name = "VTF_2.8V";
>> >>>              regulator-min-microvolt = <2800000>;
>> >>>              regulator-max-microvolt = <2800000>;
>> >>>      };
>> >>>
>> >>>      ldo22_reg: ldo22 {
>> >>>              regulator-compatible = "LDO22";
>> >>>              regulator-name = "VMEM_VDD_2.8V";
>> >>>              regulator-min-microvolt = <2800000>;
>> >>>              regulator-max-microvolt = <2800000>;
>> >>>      };
>> >>>
>> >>> This would work but I don't like it. The properties of a regulator are
>> >>> above the node configuring that regulator.
>> >>>
>> >>> Any ideas?
>> >>>
>> >>
>> >> Continuing talking to myself... I found another problem - GPIO cannot be
>> >> requested more than once (-EBUSY). In case of this driver (and board:
>> >> Trats2) one GPIO is connected to regulators. The legacy GPIO API and
>> >> regulator core handle this.
>> >>
>> >> With new GPIO API I would have to implement some additional steps in
>> >> such case...
>> >>
>> >> So there are 2 issues:
>> >> 1. Cannot put GPIO property in regulator node.
>>
>> For this problem you will probably want to use the
>> dev(m)_get_named_gpiod_from_child() function from the following patch:
>>
>> https://lkml.org/lkml/2014/10/6/529
>>
>> It should reach -next soon now.
>
> Thanks! Probably I would switch to "top" level gpios property anyway
> (other reasons) but it would be valuable in some cases to specify them
> per child node.

Mmm, but doesn't it make more sense to have them in the child nodes?
Besides if the bindings of this driver have already been published,
I'm afraid you will have to maintain backward compability.

>
>>
>> >> 2. Cannot request some GPIO more than once.
>>
>> We have been confronted to this problem with the regulator core as well:
>>
>> http://marc.info/?l=linux-arm-kernel&m=140417649119733&w=1
>>
>> I have a draft patch that allows GPIOs to be requested by several
>> clients. What prevented me from submitting it was that I wanted to
>> make sure the different requested configurations were compatible, but
>> maybe I am overthinking this. There are also a couple of other patches
>> that this depends on (like removal of the big descs array), so I don't
>> think it will be available too soon, sadly.
>
> Shouldn't be the nature of get()/put() interface to allow multiple
> requests?

Only if it makes sense for the resource to be requested multiple
times. GPIOs are kind of a difficult case here. Consumers could ask
for e.g. conflicting directions. But for cases where it should work I
agree that multiple consumers would be welcome.

> To me it was a kind of intuitive that I could do another
> devm_gpiod_get() for the same gpio. But then it hit me with EBUSY :).
>
>>
>> So maybe your best shot for now is to keep using the integer API, as
>> much as I hate it. Once we become able to request the same GPIO
>> several times, you should be good to switch to descriptors. Sorry this
>> has not been done faster.
>
> I'll do it legacy way but I'll try to use bindings gpiolib-safe. This
> way future transition in the driver should not affect bindings.

For DT bindings, please refer to these brand-new instructions:

https://lkml.org/lkml/2014/10/29/98

Personally I think having the GPIO phandle in the child node would
be the most intuitive. You will also have to use the "-gpios" suffix, no
"-gpio", if you can still change the bindings.

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Krzysztof Kozlowski]

On pią, 2014-10-31 at 12:31 +0900, Alexandre Courbot wrote:
> On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
> > On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
> >> Hi, and thanks for bringing this issue to us!
> >>
> >> On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
> >> <javier.martinez@collabora.co.uk> wrote:
> >> > [adding Linus and Alexandre to the cc list]
> >> >
> >> > Hello Krzysztof,
> >> >
> >> > On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
> >> >> On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
> >> >>> On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
> >> >>> > On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
> >> >>> > > Hello Krzysztof,
> >> >>> > >
> >> >>> > > On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> >> >>> > > > @@ -85,6 +91,9 @@ struct max77686_data {
> >> >>> > > >        struct max77686_regulator_data *regulators;
> >> >>> > > >        int num_regulators;
> >> >>> > > >
> >> >>> > > > +      /* Array of size num_regulators with GPIOs for external control. */
> >> >>> > > > +      int *ext_control_gpio;
> >> >>> > > > +
> >> >>> > >
> >> >>> > > The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
> >> >>> > > interface (Documentation/gpio/consumer.txt). Could you please use the later?
> >> >>> >
> >> >>> > Sure, I can. Please have in mind that regulator core still accepts old
> >> >>> > GPIO so I will have to use desc_to_gpio(). That should work... and
> >> >>> > should be future-ready.
> >> >>>
> >> >>> It seems I was too hasty... I think usage of the new gpiod API implies
> >> >>> completely different bindings.
> >> >>>
> >> >>> The gpiod_get() gets GPIO from a device level, not from given sub-node
> >> >>> pointer. This means that you cannot have DTS like this:
> >> >>> ldo21_reg: ldo21 {
> >> >>>      regulator-compatible = "LDO21";
> >> >>>      regulator-name = "VTF_2.8V";
> >> >>>      regulator-min-microvolt = <2800000>;
> >> >>>      regulator-max-microvolt = <2800000>;
> >> >>>      ec-gpio = <&gpy2 0 0>;
> >> >>> };
> >> >>>
> >> >>> ldo22_reg: ldo22 {
> >> >>>      regulator-compatible = "LDO22";
> >> >>>      regulator-name = "VMEM_VDD_2.8V";
> >> >>>      regulator-min-microvolt = <2800000>;
> >> >>>      regulator-max-microvolt = <2800000>;
> >> >>>      ec-gpio = <&gpk0 2 0>;
> >> >>> };
> >> >>>
> >> >>>
> >> >>> I could put GPIOs in device node:
> >> >>>
> >> >>> max77686_pmic@09 {
> >> >>>      compatible = "maxim,max77686";
> >> >>>      interrupt-parent = <&gpx0>;
> >> >>>      interrupts = <7 0>;
> >> >>>      reg = <0x09>;
> >> >>>      #clock-cells = <1>;
> >> >>>      ldo21-gpio = <&gpy2 0 0>;
> >> >>>      ldo22-gpio = <&gpk0 2 0>;
> >> >>>
> >> >>>      ldo21_reg: ldo21 {
> >> >>>              regulator-compatible = "LDO21";
> >> >>>              regulator-name = "VTF_2.8V";
> >> >>>              regulator-min-microvolt = <2800000>;
> >> >>>              regulator-max-microvolt = <2800000>;
> >> >>>      };
> >> >>>
> >> >>>      ldo22_reg: ldo22 {
> >> >>>              regulator-compatible = "LDO22";
> >> >>>              regulator-name = "VMEM_VDD_2.8V";
> >> >>>              regulator-min-microvolt = <2800000>;
> >> >>>              regulator-max-microvolt = <2800000>;
> >> >>>      };
> >> >>>
> >> >>> This would work but I don't like it. The properties of a regulator are
> >> >>> above the node configuring that regulator.
> >> >>>
> >> >>> Any ideas?
> >> >>>
> >> >>
> >> >> Continuing talking to myself... I found another problem - GPIO cannot be
> >> >> requested more than once (-EBUSY). In case of this driver (and board:
> >> >> Trats2) one GPIO is connected to regulators. The legacy GPIO API and
> >> >> regulator core handle this.
> >> >>
> >> >> With new GPIO API I would have to implement some additional steps in
> >> >> such case...
> >> >>
> >> >> So there are 2 issues:
> >> >> 1. Cannot put GPIO property in regulator node.
> >>
> >> For this problem you will probably want to use the
> >> dev(m)_get_named_gpiod_from_child() function from the following patch:
> >>
> >> https://lkml.org/lkml/2014/10/6/529
> >>
> >> It should reach -next soon now.
> >
> > Thanks! Probably I would switch to "top" level gpios property anyway
> > (other reasons) but it would be valuable in some cases to specify them
> > per child node.
> 
> Mmm, but doesn't it make more sense to have them in the child nodes?

Yes, it makes more sense. Using old way of parsing regulators from DT it
was straightforward.

However new DT style parsing (regulator_of_get_init_data()) does the
basic parsing stuff and this removes a lot of code from driver. The
driver no longer parses all regulaotrs but the regulator core does it.
Unfortunately regulator core does not parse custom bindings (like such
GPIOs) so I would have to iterate once again through all regulators just
to find "gpios" property.

It is simpler just to do something like (5 regulators can be controlled
by GPIO):

max77686_pmic_dt_parse_gpio_control(struct platform_device *pdev, *gpio)
{
  gpio[MAX77686_LDO20] = of_get_named_gpio(np, "ldo20-gpios", 0);
  gpio[MAX77686_LDO21] = of_get_named_gpio(np, "ldo21-gpios", 0);
  gpio[MAX77686_LDO22] = of_get_named_gpio(np, "ldo22-gpios", 0);
  gpio[MAX77686_BUCK8] = of_get_named_gpio(np, "buck8-gpios", 0);
  gpio[MAX77686_BUCK9] = of_get_named_gpio(np, "buck9-gpios", 0);
}

> Besides if the bindings of this driver have already been published,
> I'm afraid you will have to maintain backward compability.

These are new bindings. Driver exists but I am adding new functionality:
the "GPIO enable control".

> >>
> >> >> 2. Cannot request some GPIO more than once.
> >>
> >> We have been confronted to this problem with the regulator core as well:
> >>
> >> http://marc.info/?l=linux-arm-kernel&m=140417649119733&w=1
> >>
> >> I have a draft patch that allows GPIOs to be requested by several
> >> clients. What prevented me from submitting it was that I wanted to
> >> make sure the different requested configurations were compatible, but
> >> maybe I am overthinking this. There are also a couple of other patches
> >> that this depends on (like removal of the big descs array), so I don't
> >> think it will be available too soon, sadly.
> >
> > Shouldn't be the nature of get()/put() interface to allow multiple
> > requests?
> 
> Only if it makes sense for the resource to be requested multiple
> times. GPIOs are kind of a difficult case here. Consumers could ask
> for e.g. conflicting directions.

Right, I haven't thought about conflicts.

> But for cases where it should work I
> agree that multiple consumers would be welcome.
> 
> > To me it was a kind of intuitive that I could do another
> > devm_gpiod_get() for the same gpio. But then it hit me with EBUSY :).
> >
> >>
> >> So maybe your best shot for now is to keep using the integer API, as
> >> much as I hate it. Once we become able to request the same GPIO
> >> several times, you should be good to switch to descriptors. Sorry this
> >> has not been done faster.
> >
> > I'll do it legacy way but I'll try to use bindings gpiolib-safe. This
> > way future transition in the driver should not affect bindings.
> 
> For DT bindings, please refer to these brand-new instructions:
> 
> https://lkml.org/lkml/2014/10/29/98
> 
> Personally I think having the GPIO phandle in the child node would
> be the most intuitive. You will also have to use the "-gpios" suffix, no
> "-gpio", if you can still change the bindings.

Thanks! I'll adjust to new style.

Best regards,
Krzysztof

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Mark Brown]

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

On Fri, Oct 31, 2014 at 08:51:38AM +0100, Krzysztof Kozlowski wrote:

> However new DT style parsing (regulator_of_get_init_data()) does the
> basic parsing stuff and this removes a lot of code from driver. The
> driver no longer parses all regulaotrs but the regulator core does it.
> Unfortunately regulator core does not parse custom bindings (like such
> GPIOs) so I would have to iterate once again through all regulators just
> to find "gpios" property.

We could always add a callback for the driver to handle any custom
properties...  one of the advantages of an OS like Linux is that we can
improve the core code.

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

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Krzysztof Kozlowski]

On pią, 2014-10-31 at 10:32 +0000, Mark Brown wrote:
> On Fri, Oct 31, 2014 at 08:51:38AM +0100, Krzysztof Kozlowski wrote:
> 
> > However new DT style parsing (regulator_of_get_init_data()) does the
> > basic parsing stuff and this removes a lot of code from driver. The
> > driver no longer parses all regulaotrs but the regulator core does it.
> > Unfortunately regulator core does not parse custom bindings (like such
> > GPIOs) so I would have to iterate once again through all regulators just
> > to find "gpios" property.
> 
> We could always add a callback for the driver to handle any custom
> properties...  one of the advantages of an OS like Linux is that we can
> improve the core code.

Then I'll add it.

Mark, what device should be assigned to "config.dev" during registration
of regulators? The regulator driver's device or its parent (MFD main
driver)?

Various drivers do this differently.

Best regards,
Krzysztof

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Mark Brown]

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

On Fri, Oct 31, 2014 at 12:45:47PM +0100, Krzysztof Kozlowski wrote:

> Then I'll add it.

> Mark, what device should be assigned to "config.dev" during registration
> of regulators? The regulator driver's device or its parent (MFD main
> driver)?

> Various drivers do this differently.

Normally the parent device.

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

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Krzysztof Kozlowski]

On pią, 2014-10-31 at 10:32 +0000, Mark Brown wrote:
> On Fri, Oct 31, 2014 at 08:51:38AM +0100, Krzysztof Kozlowski wrote:
> 
> > However new DT style parsing (regulator_of_get_init_data()) does the
> > basic parsing stuff and this removes a lot of code from driver. The
> > driver no longer parses all regulaotrs but the regulator core does it.
> > Unfortunately regulator core does not parse custom bindings (like such
> > GPIOs) so I would have to iterate once again through all regulators just
> > to find "gpios" property.
> 
> We could always add a callback for the driver to handle any custom
> properties...  one of the advantages of an OS like Linux is that we can
> improve the core code.

I thought about this - adding a callback, called on each child in
regulator_of_get_init_data(). However the reason behind this callback is
to parse GPIO and set config.ena_gpio. However in that context the
regulator_config is const so the callback cannot change it. Unless it
casts it to non-const... which isn't what we want I think.

So now I wonder whether adding generic bindings for ena_gpio make sense.
These would look like bindings for fixed-regulator (with "ena-" prefix).
Unfortunately this would duplicate a little the ena_gpio in
regulator_config... but to me it seems more appropriate.

What do you think about adding generic bindings for ena_gpio?

Best regards,
Krzysztof

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Mark Brown]

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

On Mon, Nov 03, 2014 at 01:07:02PM +0100, Krzysztof Kozlowski wrote:
> On pią, 2014-10-31 at 10:32 +0000, Mark Brown wrote:

> > We could always add a callback for the driver to handle any custom
> > properties...  one of the advantages of an OS like Linux is that we can
> > improve the core code.

> I thought about this - adding a callback, called on each child in
> regulator_of_get_init_data(). However the reason behind this callback is
> to parse GPIO and set config.ena_gpio. However in that context the
> regulator_config is const so the callback cannot change it. Unless it
> casts it to non-const... which isn't what we want I think.

> So now I wonder whether adding generic bindings for ena_gpio make sense.
> These would look like bindings for fixed-regulator (with "ena-" prefix).
> Unfortunately this would duplicate a little the ena_gpio in
> regulator_config... but to me it seems more appropriate.

> What do you think about adding generic bindings for ena_gpio?

Well, if you only want this for enable GPIO control (sorry, I'm really
not reading a lot of these long threads when it looks like there'll be a
resubmit anyway) we can always add a way for drivers to specify the name
of a property to look at easily enough.

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

Re: [PATCH 6/8] regulator: max77686: Add external GPIO control

[Alexandre Courbot]

On Fri, Oct 31, 2014 at 4:51 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On pią, 2014-10-31 at 12:31 +0900, Alexandre Courbot wrote:
>> On Fri, Oct 31, 2014 at 12:03 AM, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>> > On czw, 2014-10-30 at 22:56 +0900, Alexandre Courbot wrote:
>> >> Hi, and thanks for bringing this issue to us!
>> >>
>> >> On Wed, Oct 29, 2014 at 7:49 PM, Javier Martinez Canillas
>> >> <javier.martinez@collabora.co.uk> wrote:
>> >> > [adding Linus and Alexandre to the cc list]
>> >> >
>> >> > Hello Krzysztof,
>> >> >
>> >> > On 10/29/2014 11:42 AM, Krzysztof Kozlowski wrote:
>> >> >> On wto, 2014-10-28 at 13:11 +0100, Krzysztof Kozlowski wrote:
>> >> >>> On wto, 2014-10-28 at 09:52 +0100, Krzysztof Kozlowski wrote:
>> >> >>> > On pon, 2014-10-27 at 21:03 +0100, Javier Martinez Canillas wrote:
>> >> >>> > > Hello Krzysztof,
>> >> >>> > >
>> >> >>> > > On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
>> >> >>> > > > @@ -85,6 +91,9 @@ struct max77686_data {
>> >> >>> > > >        struct max77686_regulator_data *regulators;
>> >> >>> > > >        int num_regulators;
>> >> >>> > > >
>> >> >>> > > > +      /* Array of size num_regulators with GPIOs for external control. */
>> >> >>> > > > +      int *ext_control_gpio;
>> >> >>> > > > +
>> >> >>> > >
>> >> >>> > > The integer-based GPIO API is deprecated in favor of the descriptor-based GPIO
>> >> >>> > > interface (Documentation/gpio/consumer.txt). Could you please use the later?
>> >> >>> >
>> >> >>> > Sure, I can. Please have in mind that regulator core still accepts old
>> >> >>> > GPIO so I will have to use desc_to_gpio(). That should work... and
>> >> >>> > should be future-ready.
>> >> >>>
>> >> >>> It seems I was too hasty... I think usage of the new gpiod API implies
>> >> >>> completely different bindings.
>> >> >>>
>> >> >>> The gpiod_get() gets GPIO from a device level, not from given sub-node
>> >> >>> pointer. This means that you cannot have DTS like this:
>> >> >>> ldo21_reg: ldo21 {
>> >> >>>      regulator-compatible = "LDO21";
>> >> >>>      regulator-name = "VTF_2.8V";
>> >> >>>      regulator-min-microvolt = <2800000>;
>> >> >>>      regulator-max-microvolt = <2800000>;
>> >> >>>      ec-gpio = <&gpy2 0 0>;
>> >> >>> };
>> >> >>>
>> >> >>> ldo22_reg: ldo22 {
>> >> >>>      regulator-compatible = "LDO22";
>> >> >>>      regulator-name = "VMEM_VDD_2.8V";
>> >> >>>      regulator-min-microvolt = <2800000>;
>> >> >>>      regulator-max-microvolt = <2800000>;
>> >> >>>      ec-gpio = <&gpk0 2 0>;
>> >> >>> };
>> >> >>>
>> >> >>>
>> >> >>> I could put GPIOs in device node:
>> >> >>>
>> >> >>> max77686_pmic@09 {
>> >> >>>      compatible = "maxim,max77686";
>> >> >>>      interrupt-parent = <&gpx0>;
>> >> >>>      interrupts = <7 0>;
>> >> >>>      reg = <0x09>;
>> >> >>>      #clock-cells = <1>;
>> >> >>>      ldo21-gpio = <&gpy2 0 0>;
>> >> >>>      ldo22-gpio = <&gpk0 2 0>;
>> >> >>>
>> >> >>>      ldo21_reg: ldo21 {
>> >> >>>              regulator-compatible = "LDO21";
>> >> >>>              regulator-name = "VTF_2.8V";
>> >> >>>              regulator-min-microvolt = <2800000>;
>> >> >>>              regulator-max-microvolt = <2800000>;
>> >> >>>      };
>> >> >>>
>> >> >>>      ldo22_reg: ldo22 {
>> >> >>>              regulator-compatible = "LDO22";
>> >> >>>              regulator-name = "VMEM_VDD_2.8V";
>> >> >>>              regulator-min-microvolt = <2800000>;
>> >> >>>              regulator-max-microvolt = <2800000>;
>> >> >>>      };
>> >> >>>
>> >> >>> This would work but I don't like it. The properties of a regulator are
>> >> >>> above the node configuring that regulator.
>> >> >>>
>> >> >>> Any ideas?
>> >> >>>
>> >> >>
>> >> >> Continuing talking to myself... I found another problem - GPIO cannot be
>> >> >> requested more than once (-EBUSY). In case of this driver (and board:
>> >> >> Trats2) one GPIO is connected to regulators. The legacy GPIO API and
>> >> >> regulator core handle this.
>> >> >>
>> >> >> With new GPIO API I would have to implement some additional steps in
>> >> >> such case...
>> >> >>
>> >> >> So there are 2 issues:
>> >> >> 1. Cannot put GPIO property in regulator node.
>> >>
>> >> For this problem you will probably want to use the
>> >> dev(m)_get_named_gpiod_from_child() function from the following patch:
>> >>
>> >> https://lkml.org/lkml/2014/10/6/529
>> >>
>> >> It should reach -next soon now.
>> >
>> > Thanks! Probably I would switch to "top" level gpios property anyway
>> > (other reasons) but it would be valuable in some cases to specify them
>> > per child node.
>>
>> Mmm, but doesn't it make more sense to have them in the child nodes?
>
> Yes, it makes more sense. Using old way of parsing regulators from DT it
> was straightforward.
>
> However new DT style parsing (regulator_of_get_init_data()) does the
> basic parsing stuff and this removes a lot of code from driver. The
> driver no longer parses all regulaotrs but the regulator core does it.
> Unfortunately regulator core does not parse custom bindings (like such
> GPIOs) so I would have to iterate once again through all regulators just
> to find "gpios" property.
>
> It is simpler just to do something like (5 regulators can be controlled
> by GPIO):
>
> max77686_pmic_dt_parse_gpio_control(struct platform_device *pdev, *gpio)
> {
>   gpio[MAX77686_LDO20] = of_get_named_gpio(np, "ldo20-gpios", 0);
>   gpio[MAX77686_LDO21] = of_get_named_gpio(np, "ldo21-gpios", 0);
>   gpio[MAX77686_LDO22] = of_get_named_gpio(np, "ldo22-gpios", 0);
>   gpio[MAX77686_BUCK8] = of_get_named_gpio(np, "buck8-gpios", 0);
>   gpio[MAX77686_BUCK9] = of_get_named_gpio(np, "buck9-gpios", 0);
> }

It is simpler from the driver's perspective, but if I understand
correctly DT bindings are not supposed to be adapted to make life
easier for a particular implementation. If the driver needs to make an
additional pass into the child nodes, then so be it, as long as the
nodes describe the hardware accurately and in a way that is easy to
understand. You can always adapt the driver core to handle your
use-case better, but once DT bindings are published, they are set in
stone.

[PATCH 7/8] mfd/regulator: dt-bindings: max77686: Document gpio property

[Krzysztof Kozlowski]

Document usage of gpio property which turns on external control over
regulator.

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

diff --git a/Documentation/devicetree/bindings/mfd/max77686.txt b/Documentation/devicetree/bindings/mfd/max77686.txt
index 75fdfaf41831..e41dfa7e2b90 100644
--- a/Documentation/devicetree/bindings/mfd/max77686.txt
+++ b/Documentation/devicetree/bindings/mfd/max77686.txt
@@ -39,6 +39,9 @@ to get matched with their hardware counterparts as follow:
 	-BUCKn	:	1-4.
   Use standard regulator bindings for it ('regulator-off-in-suspend').
 
+  Optional properties:
+  - gpio : GPIO to use for external control. Valid only for LDO20, LDO21,
+    LDO22, BUCK8 and BUCK9 regulators.
 
 Example:
 
-- 
1.9.1

Re: [PATCH 7/8] mfd/regulator: dt-bindings: max77686: Document gpio property

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> Document usage of gpio property which turns on external control over
> regulator.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  Documentation/devicetree/bindings/mfd/max77686.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/max77686.txt b/Documentation/devicetree/bindings/mfd/max77686.txt
> index 75fdfaf41831..e41dfa7e2b90 100644
> --- a/Documentation/devicetree/bindings/mfd/max77686.txt
> +++ b/Documentation/devicetree/bindings/mfd/max77686.txt
> @@ -39,6 +39,9 @@ to get matched with their hardware counterparts as follow:
>  	-BUCKn	:	1-4.
>    Use standard regulator bindings for it ('regulator-off-in-suspend').
>  
> +  Optional properties:
> +  - gpio : GPIO to use for external control. Valid only for LDO20, LDO21,
> +    LDO22, BUCK8 and BUCK9 regulators.

When using the descriptor-based GPIO interface the standard is to use as the DT
property name a prefix documenting the GPIO function + a -gpios or -gpio suffix.
So this should be something like ec-gpio.

Best regards,
Javier

Re: [PATCH 7/8] mfd/regulator: dt-bindings: max77686: Document gpio property

[Krzysztof Kozlowski]

On pon, 2014-10-27 at 21:15 +0100, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 10/27/2014 04:03 PM, Krzysztof Kozlowski wrote:
> > Document usage of gpio property which turns on external control over
> > regulator.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/max77686.txt | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/max77686.txt b/Documentation/devicetree/bindings/mfd/max77686.txt
> > index 75fdfaf41831..e41dfa7e2b90 100644
> > --- a/Documentation/devicetree/bindings/mfd/max77686.txt
> > +++ b/Documentation/devicetree/bindings/mfd/max77686.txt
> > @@ -39,6 +39,9 @@ to get matched with their hardware counterparts as follow:
> >  	-BUCKn	:	1-4.
> >    Use standard regulator bindings for it ('regulator-off-in-suspend').
> >  
> > +  Optional properties:
> > +  - gpio : GPIO to use for external control. Valid only for LDO20, LDO21,
> > +    LDO22, BUCK8 and BUCK9 regulators.
> 
> When using the descriptor-based GPIO interface the standard is to use as the DT
> property name a prefix documenting the GPIO function + a -gpios or -gpio suffix.
> So this should be something like ec-gpio.

OK.

Best regards,
Krzysztof

> 
> Best regards,
> Javier

[PATCH 8/8] 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.

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 by regulator-fixed),
 - BUCK9EN field in BUCK9CTRL register (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..ac1baff7130d 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>;
+					gpio = <&gpy2 0 0>;
 				};
 
 				ldo22_reg: ldo22 {
@@ -412,6 +394,7 @@
 					regulator-name = "VMEM_VDD_2.8V";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
+					gpio = <&gpk0 2 0>;
 				};
 
 				ldo23_reg: ldo23 {
@@ -518,6 +501,7 @@
 					regulator-name = "VMEM_VDDF_3.0V";
 					regulator-min-microvolt = <2850000>;
 					regulator-max-microvolt = <2850000>;
+					gpio = <&gpk0 2 0>;
 				};
 
 				buck9_reg: buck9 {
@@ -525,6 +509,7 @@
 					regulator-name = "CAM_ISP_CORE_1.2V";
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1200000>;
+					gpio = <&gpm0 3 0>;
 				};
 			};
 		};
@@ -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