| Message ID | 1342617721-14715-2-git-send-email-pawel.moll@arm.com |
|---|---|
| State | New, archived |
| Headers | show |
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.
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ł
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.
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ł
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.
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ł
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.
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++) {
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(-)