Message-ID: <CAF6AEGsDhv6V8ggOsLHgk6YgWEaL4iV1yX4qOdA_=KzCAWqR=w@mail.gmail.com>
Date:	Fri, 24 Oct 2014 17:57:24 -0400
From:	Rob Clark <robdclark@...il.com>
To:	Mark Brown <broonie@...nel.org>
Cc:	Felipe Balbi <balbi@...com>, lgirdwood@...il.com,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	David Airlie <airlied@...ux.ie>,
	David Brown <davidb@...eaurora.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH] regulator: stub out devm_regulator_get_exclusive

On Fri, Oct 24, 2014 at 5:18 PM, Mark Brown <broonie@...nel.org> wrote:
> On Fri, Oct 24, 2014 at 04:36:24PM -0400, Rob Clark wrote:
>
>> iirc, I was using _get_exclusive() in a few places where I wanted to
>> be sure not to get dummy-regulator in cases where I should
>> -EPROBE_DEFER instead (since probe order with DT is slightly
>> hilarious, and since I depend on a few other drivers I end up
>> deferring at least a couple times at boot)... I don't quite remember
>> the details.  But afaict regulator_get() still allows dummy-regulator,
>> which is what I specifically don't want.
>
> No, this is actually making things worse.  You will only get a dummy
> regulator from regulator_get() if no regulator at all is mapped in the
> DT, if one is mapped then you'll always get either an -EPROBE_DEFER or
> the real regulator.  Right now the driver is broken with respect to
> -EPROBE_DEFER since it just completely ignores the error code and
> carries on happily if any error is returned which means that the
> behaviour is going to be unstable on any given system, what happens will
> depend on probe order which could in turn depend on what's been built
> modular and so on.

Oh, you are only looking at the one in mdp4_kms_init(), which could
possibly be a simple regulator_get().  Look instead at the ones in
hdmi_init(), where I need to know whether to defer or not.  At one
point I was having a problem getting dummy regulators with
regulator_get() but that could have been a symptom of another problem.
I will re-try regulator_get() next time I am working on the kernel
part of the driver..

The mdp4_kms->vdd logic is a bit of a hack right now, since we are
missing a driver upstream for that regulator (and just relying on
bootloader to leave it on for us).

BR,
-R

> As far as I can tell the only thing the driver does with the regulator
> it's grabbing exclusively is enable it in probe() and that's going to
> work just as well with the dummy regulator anyway so I can't see any
> reason to worry if the driver is getting one.
>
>> If you have a recommendation for a better way, I am all ears.
>
> Just use regulator_get() (or better, devm_regulator_get()) and pay
> attention to the errors.  The driver should also disable the regulator
> on remove and I'd be surprised if the other two regulators shouldn't be
> using a normal _get() too.  If there is a good reason to use _optional()
> then the code should be changed to use ERR_PTR() rather than NULL to
> check for missing regulators and the driver needs to keep them enabled
> as long as it's using them.
>
> Given that the two optional regulators are only set to one specific
> value it's a bit surprising that the DT doesn't do this but I guess it's
> possible there could be broken DTs out there that do give permission for
> set_voltage() for a range rather than specifying the correct voltage.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists