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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- 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 --]