On Fri, Oct 24, 2014 at 5:18 PM, Mark Brown broonie@kernel.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.