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

login
register
mail settings
Submitter Pawel Moll
Date July 18, 2012, 1:22 p.m.
Message ID <1342617721-14715-2-git-send-email-pawel.moll@arm.com>
Download mbox | patch
Permalink /patch/171677/
State New
Headers show

Comments

Pawel Moll - July 18, 2012, 1:22 p.m.
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(-)
Mark Brown - July 23, 2012, 5:46 p.m.
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.
Pawel Moll - July 23, 2012, 6:24 p.m.
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ł
Mark Brown - July 23, 2012, 6:32 p.m.
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.
Pawel Moll - July 23, 2012, 6:42 p.m.
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ł
Mark Brown - July 23, 2012, 6:45 p.m.
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.
Pawel Moll - July 23, 2012, 7:04 p.m.
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ł
Mark Brown - July 23, 2012, 7:12 p.m.
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.

Patch

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++) {