[PATCH v2 0/2] Regulator control of physmap-ed chip's VPP

[PATCH v2 0/2] Regulator control of physmap-ed chip's VPP

[Pawel Moll]

Hello again,

After posting the previous, GPIO-based series (see
http://thread.gmane.org/gmane.linux.drivers.mtd/42502) I had a go
with the regulator version. It turned out to be much cleaner, so
that's what I'd propose for merge...

Regards

Pawel

Pawel Moll (2):
  mtd: maps: physmap: Add VPP regulator control
  mtd: maps: physmap_of: Add VPP regulator control

 .../devicetree/bindings/mtd/mtd-physmap.txt        |    4 ++-
 drivers/mtd/maps/physmap.c                         |   24 ++++++++++++-----
 drivers/mtd/maps/physmap_of.c                      |   28 ++++++++++++++++++++
 3 files changed, 48 insertions(+), 8 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/2] mtd: maps: physmap: Add VPP regulator control

[Pawel Moll]

Add an optional regulator control of the VPP line.
"vpp" supply must be provided by the board description.

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 drivers/mtd/maps/physmap.c |   24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
index 21b0b71..57d79ea 100644
--- a/drivers/mtd/maps/physmap.c
+++ b/drivers/mtd/maps/physmap.c
@@ -20,6 +20,8 @@
 #include <linux/mtd/physmap.h>
 #include <linux/mtd/concat.h>
 #include <linux/io.h>
+#include <linux/err.h>
+#include <linux/regulator/consumer.h>
 
 #define MAX_RESOURCES		4
 
@@ -29,6 +31,7 @@ struct physmap_flash_info {
 	struct map_info		map[MAX_RESOURCES];
 	spinlock_t		vpp_lock;
 	int			vpp_refcnt;
+	struct regulator	*vpp_regulator;
 };
 
 static int physmap_flash_remove(struct platform_device *dev)
@@ -70,19 +73,22 @@ static void physmap_set_vpp(struct map_info *map, int state)
 
 	pdev = (struct platform_device *)map->map_priv_1;
 	physmap_data = pdev->dev.platform_data;
+	info = platform_get_drvdata(pdev);
 
-	if (!physmap_data->set_vpp)
+	if (!physmap_data->set_vpp && !info->vpp_regulator)
 		return;
 
-	info = platform_get_drvdata(pdev);
-
 	spin_lock_irqsave(&info->vpp_lock, flags);
-	if (state) {
-		if (++info->vpp_refcnt == 1)    /* first nested 'on' */
+	if (state && ++info->vpp_refcnt == 1) { /* first nested 'on' */
+		if (physmap_data->set_vpp)
 			physmap_data->set_vpp(pdev, 1);
-	} else {
-		if (--info->vpp_refcnt == 0)    /* last nested 'off' */
+		else
+			regulator_enable(info->vpp_regulator);
+	} else if (!state && --info->vpp_refcnt == 0) { /* last nested 'off' */
+		if (physmap_data->set_vpp)
 			physmap_data->set_vpp(pdev, 0);
+		else
+			regulator_disable(info->vpp_regulator);
 	}
 	spin_unlock_irqrestore(&info->vpp_lock, flags);
 }
@@ -123,6 +129,10 @@ static int physmap_flash_probe(struct platform_device *dev)
 			goto err_out;
 	}
 
+	info->vpp_regulator = devm_regulator_get(&dev->dev, "vpp");
+	if (IS_ERR(info->vpp_regulator))
+		info->vpp_regulator = NULL;
+
 	platform_set_drvdata(dev, info);
 
 	for (i = 0; i < dev->num_resources; i++) {
-- 
1.7.9.5

Re: [PATCH v2 1/2] mtd: maps: physmap: Add VPP regulator control

[Mark Brown]

On Wed, Jul 18, 2012 at 02:22:00PM +0100, Pawel Moll wrote:

> +	info->vpp_regulator = devm_regulator_get(&dev->dev, "vpp");
> +	if (IS_ERR(info->vpp_regulator))
> +		info->vpp_regulator = NULL;
> +

No, this is very bad karma.  You shouldn't just silently ignore the
error here, if we didn't get the supply it's probably important and
otherwise why bother checking the error at all?  Systems should use
a fixed voltage regulator to stub out the supply if there's not any
runtime contol.

At a bare minimum you should be passing back -EPROBE_DEFER if you get
that.

Re: [PATCH v2 1/2] mtd: maps: physmap: Add VPP regulator control

[Pawel Moll]

On Mon, 2012-07-23 at 18:46 +0100, Mark Brown wrote:
> > +	info->vpp_regulator = devm_regulator_get(&dev->dev, "vpp");
> > +	if (IS_ERR(info->vpp_regulator))
> > +		info->vpp_regulator = NULL;
> > +
> 
> No, this is very bad karma.  You shouldn't just silently ignore the
> error here, if we didn't get the supply it's probably important and
> otherwise why bother checking the error at all?  

The reason is simple - to make the regulator completely optional. That's
also why I check the existence of "vpp-supply" property in the OF
version.

> Systems should use
> a fixed voltage regulator to stub out the supply if there's not any
> runtime contol.

I know this discussion and I appreciate your point of view. The fact
that I disagree doesn't matter here. Simply speaking - I'd rather drop
this patch completely than break current (as in: no regulator necessary)
behaviour of the physmap driver. I'm not ready to take all the flak from
dozens of hmm...unhappy users :-)

> At a bare minimum you should be passing back -EPROBE_DEFER if you get
> that.

Which is, if I'm not mistaken, exactly what is returned when no supply
is defined by the board. Which gets us to the starting point. As I said
- if this is deemed unacceptable, I'll simply forget about this patch.

Regards

Paweł

Re: [PATCH v2 1/2] mtd: maps: physmap: Add VPP regulator control

[Mark Brown]

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

On Mon, Jul 23, 2012 at 07:24:21PM +0100, Pawel Moll wrote:
> On Mon, 2012-07-23 at 18:46 +0100, Mark Brown wrote:
> > > +	info->vpp_regulator = devm_regulator_get(&dev->dev, "vpp");
> > > +	if (IS_ERR(info->vpp_regulator))
> > > +		info->vpp_regulator = NULL;

> > No, this is very bad karma.  You shouldn't just silently ignore the
> > error here, if we didn't get the supply it's probably important and
> > otherwise why bother checking the error at all?  

> The reason is simple - to make the regulator completely optional. That's
> also why I check the existence of "vpp-supply" property in the OF
> version.

Yes, I know what you think you're doing here but the fact remains that
you're just guessing about why you got an error, perhaps it's due to
there not being anything there but perhaps it's something important.
The thing that's particularly bad here is that your change will just
silently ignore the error which is far from awesome, at the very least
it ought to complain (though I don't think that's a good idea).

It shouldn't be that hard to find the in-tree users...

> > At a bare minimum you should be passing back -EPROBE_DEFER if you get
> > that.

> Which is, if I'm not mistaken, exactly what is returned when no supply
> is defined by the board. Which gets us to the starting point. As I said
> - if this is deemed unacceptable, I'll simply forget about this patch.

Well, in the DT case it'll probably start returning -ENODEV soon if
there's no supply binding set up (which would get you back to your
current case), given that you're from ARM probably that's covering all
the cases you're especially interested in.  Otherwise we just can't tell
why we didn't find the regulator.

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

Re: [PATCH v2 1/2] mtd: maps: physmap: Add VPP regulator control

[Pawel Moll]

On Mon, 2012-07-23 at 19:32 +0100, Mark Brown wrote:
> The thing that's particularly bad here is that your change will just
> silently ignore the error which is far from awesome, at the very least
> it ought to complain (though I don't think that's a good idea).

I know, I don't like that myself - that's probably why I don't mind
dropping that change. Particularly that non-DT boards can always pass
set_vpp via platform data. And that's what will probably have to happen
here.

> It shouldn't be that hard to find the in-tree users...

It was in case of SMSC ethernet drivers ;-)

> Well, in the DT case it'll probably start returning -ENODEV soon if
> there's no supply binding set up (which would get you back to your
> current case), 

This would be perfect. Will it happen for 3.7? If so, I'll drop the
non-OF patch and make the OF one rely on -ENODEV.

> given that you're from ARM probably that's covering all
> the cases you're especially interested in.

Indeed physmap_of is what really interests me. I added the non-DT code
to keep symmetry, but after all nature is not symmetrical after
all... ;-)

Cheers!

Paweł

Re: [PATCH v2 1/2] mtd: maps: physmap: Add VPP regulator control

[Mark Brown]

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

On Mon, Jul 23, 2012 at 07:42:09PM +0100, Pawel Moll wrote:
> On Mon, 2012-07-23 at 19:32 +0100, Mark Brown wrote:

> > The thing that's particularly bad here is that your change will just
> > silently ignore the error which is far from awesome, at the very least
> > it ought to complain (though I don't think that's a good idea).

> I know, I don't like that myself - that's probably why I don't mind
> dropping that change. Particularly that non-DT boards can always pass
> set_vpp via platform data. And that's what will probably have to happen
> here.

The MMC subsystem went down that path initially but has recently
converted to using regulators more normally.  It provides an explict
callback but uses regulators otherwise IIRC which seems sensible.

> > It shouldn't be that hard to find the in-tree users...

> It was in case of SMSC ethernet drivers ;-)

I think that's more a case of the submitters not looking than anything
else TBH.

> > Well, in the DT case it'll probably start returning -ENODEV soon if
> > there's no supply binding set up (which would get you back to your
> > current case), 

> This would be perfect. Will it happen for 3.7? If so, I'll drop the
> non-OF patch and make the OF one rely on -ENODEV.

It'll happen if anyone does the work; I've not currently got any useful
systems that run DT and don't have a particularly large amount of spare
time.  People were muttering about it, though, for much the reasons you
mentioned.

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

Re: [PATCH v2 1/2] mtd: maps: physmap: Add VPP regulator control

[Pawel Moll]

On Mon, 2012-07-23 at 19:45 +0100, Mark Brown wrote:
> The MMC subsystem went down that path initially but has recently
> converted to using regulators more normally.  It provides an explict
> callback but uses regulators otherwise IIRC which seems sensible.

Yeesss...

#ifdef CONFIG_REGULATOR
        /* If we're using the regulator framework, try to fetch a regulator */
        host->vcc = regulator_get(&dev->dev, "vmmc");
        if (IS_ERR(host->vcc))
                host->vcc = NULL;
        else {
                int mask = mmc_regulator_get_ocrmask(host->vcc);

> > This would be perfect. Will it happen for 3.7? If so, I'll drop the
> > non-OF patch and make the OF one rely on -ENODEV.
> 
> It'll happen if anyone does the work; I've not currently got any useful
> systems that run DT and don't have a particularly large amount of spare
> time.  People were muttering about it, though, for much the reasons you
> mentioned.

Ok, I take the hint, will look into it.

Paweł

Re: [PATCH v2 1/2] mtd: maps: physmap: Add VPP regulator control

[Mark Brown]

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

On Mon, Jul 23, 2012 at 08:04:59PM +0100, Pawel Moll wrote:
> On Mon, 2012-07-23 at 19:45 +0100, Mark Brown wrote:

> #ifdef CONFIG_REGULATOR
>         /* If we're using the regulator framework, try to fetch a regulator */
>         host->vcc = regulator_get(&dev->dev, "vmmc");
>         if (IS_ERR(host->vcc))
>                 host->vcc = NULL;
>         else {
>                 int mask = mmc_regulator_get_ocrmask(host->vcc);

Oh, yeah.  Well, MMC regulator integration has historically been
entertaining so I guess any move towards standardisation is good :/

> > It'll happen if anyone does the work; I've not currently got any useful
> > systems that run DT and don't have a particularly large amount of spare
> > time.  People were muttering about it, though, for much the reasons you
> > mentioned.

> Ok, I take the hint, will look into it.

That'd be great, people were really talking about it so it may be that
someone will get to it soon, I don't know though.

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

[PATCH 2/2] mtd: maps: physmap_of: Add VPP regulator control

[Pawel Moll]

Add an optional regulator control of the VPP line and a
standard "vpp-supply" tree property.

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 .../devicetree/bindings/mtd/mtd-physmap.txt        |    4 ++-
 drivers/mtd/maps/physmap_of.c                      |   28 ++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
index a63c2bd7..40cb554 100644
--- a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
+++ b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
@@ -13,10 +13,11 @@ file systems on embedded devices.
    device width times the number of interleaved chips.
  - device-width : (optional) Width of a single mtd chip.  If
    omitted, assumed to be equal to 'bank-width'.
+ - vpp-supply : (optional) phandle to a regulator controlling VPP
+   line of the chips.
  - #address-cells, #size-cells : Must be present if the device has
    sub-nodes representing partitions (see below).  In this case
    both #address-cells and #size-cells must be equal to 1.
-
 For JEDEC compatible devices, the following additional properties
 are defined:
 
@@ -33,6 +34,7 @@ Example:
 		reg = <ff000000 01000000>;
 		bank-width = <4>;
 		device-width = <1>;
+		vpp-supply = <&flash_vpp_reg>
 		#address-cells = <1>;
 		#size-cells = <1>;
 		fs@0 {
diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 2e6fb68..9f34320 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -17,6 +17,7 @@
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/device.h>
+#include <linux/err.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/map.h>
 #include <linux/mtd/partitions.h>
@@ -24,6 +25,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
 struct of_flash_list {
@@ -74,6 +76,18 @@ static int of_flash_remove(struct platform_device *dev)
 	return 0;
 }
 
+static void of_flash_set_vpp_regulator(struct map_info *map, int state)
+{
+	/* map_priv_1 = VPP regulator pointer */
+	struct regulator *vpp_regulator = (struct regulator *)map->map_priv_1;
+
+	/* map_priv_2 = "reference counter" */
+	if (state && !map->map_priv_2++)
+		WARN_ON(regulator_enable(vpp_regulator));
+	else if (!state && !--map->map_priv_2)
+		WARN_ON(regulator_disable(vpp_regulator));
+}
+
 /* Helper function to handle probing of the obsolete "direct-mapped"
  * compatible binding, which has an extra "probe-type" property
  * describing the type of flash probe necessary. */
@@ -169,6 +183,7 @@ static int __devinit of_flash_probe(struct platform_device *dev)
 	struct mtd_info **mtd_list = NULL;
 	resource_size_t res_size;
 	struct mtd_part_parser_data ppdata;
+	struct regulator *vpp_regulator = NULL;
 
 	match = of_match_device(of_flash_match, &dev->dev);
 	if (!match)
@@ -200,6 +215,14 @@ static int __devinit of_flash_probe(struct platform_device *dev)
 
 	dev_set_drvdata(&dev->dev, info);
 
+	if (of_get_property(dp, "vpp-supply", NULL)) {
+		vpp_regulator = devm_regulator_get(&dev->dev, "vpp");
+		if (IS_ERR(vpp_regulator)) {
+			err = PTR_ERR(vpp_regulator);
+			goto err_flash_remove;
+		}
+	}
+
 	mtd_list = kzalloc(sizeof(*mtd_list) * count, GFP_KERNEL);
 	if (!mtd_list)
 		goto err_flash_remove;
@@ -235,6 +258,11 @@ static int __devinit of_flash_probe(struct platform_device *dev)
 		info->list[i].map.phys = res.start;
 		info->list[i].map.size = res_size;
 		info->list[i].map.bankwidth = be32_to_cpup(width);
+		if (vpp_regulator) {
+			info->list[i].map.map_priv_1 =
+					(unsigned long)vpp_regulator;
+			info->list[i].map.set_vpp = of_flash_set_vpp_regulator;
+		}
 
 		err = -ENOMEM;
 		info->list[i].map.virt = ioremap(info->list[i].map.phys,
-- 
1.7.9.5

Re: [PATCH v2 0/2] Regulator control of physmap-ed chip's VPP

[Artem Bityutskiy]

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

On Wed, 2012-07-18 at 14:21 +0100, Pawel Moll wrote:
> Hello again,
> 
> After posting the previous, GPIO-based series (see
> http://thread.gmane.org/gmane.linux.drivers.mtd/42502) I had a go
> with the regulator version. It turned out to be much cleaner, so
> that's what I'd propose for merge...

Could you please re-send with the ARM mailing list in CC (because we do
not have many people who'd care much about relulators and DT here).

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]