If we don't stup that call out, we will have build failures for any drivers using that function when .config happens to have CONFIG_REGULATOR=n.
One such case below, found with randconfig
drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c: In function ‘mdp4_kms_init’: drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c:384:2: error: implicit declaration \ of function ‘devm_regulator_get_exclusive’ [-Werror=implicit-function-declaration] mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); ^ drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c:384:16: error: assignment makes \ pointer from integer without a cast [-Werror] mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); ^ Signed-off-by: Felipe Balbi balbi@ti.com --- include/linux/regulator/consumer.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h index d347c80..ff61f3b 100644 --- a/include/linux/regulator/consumer.h +++ b/include/linux/regulator/consumer.h @@ -291,6 +291,11 @@ regulator_get_optional(struct device *dev, const char *id) return ERR_PTR(-ENODEV); }
+static inline struct regulator *__must_check +devm_regulator_get_exclusive(struct device *dev, const char *id) +{ + return ERR_PTR(-ENODEV); +}
static inline struct regulator *__must_check devm_regulator_get_optional(struct device *dev, const char *id)
On Fri, Oct 24, 2014 at 02:15:11PM -0500, Felipe Balbi wrote:
If we don't stup that call out, we will have build failures for any drivers using that function when .config happens to have CONFIG_REGULATOR=n.
One such case below, found with randconfig
drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c: In function ‘mdp4_kms_init’: drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c:384:2: error: implicit declaration \ of function ‘devm_regulator_get_exclusive’ [-Werror=implicit-function-declaration] mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); ^ drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c:384:16: error: assignment makes \ pointer from integer without a cast [-Werror] mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd"); ^ Signed-off-by: Felipe Balbi balbi@ti.com
randconfig attached.
On Fri, Oct 24, 2014 at 02:15:11PM -0500, Felipe Balbi wrote:
If we don't stup that call out, we will have build failures for any drivers using that function when .config happens to have CONFIG_REGULATOR=n.
One such case below, found with randconfig
drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c: In function ‘mdp4_kms_init’: drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c:384:2: error: implicit declaration \
As previously and repeatedly reported the regulator usage in this driver appears extremely problematic, among these problems is that it almost certainly has no sensible reason to be using regulator_get_exclusive() or any variant of it. Sadly every time it's been raised with the video people they've completely ignored the mail so here we are.
Right now not having the stub seems to only be affecting buggy users (which given the use cases for _exclusive() isn't *that* surprising) so I'm more inclined to leave this there in the hope that the users get fixed or we can at least get some sort of dialogue with the relevant maintainers.
Hi,
On Fri, Oct 24, 2014 at 09:11:38PM +0100, Mark Brown wrote:
On Fri, Oct 24, 2014 at 02:15:11PM -0500, Felipe Balbi wrote:
If we don't stup that call out, we will have build failures for any drivers using that function when .config happens to have CONFIG_REGULATOR=n.
One such case below, found with randconfig
drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c: In function ‘mdp4_kms_init’: drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c:384:2: error: implicit declaration \
As previously and repeatedly reported the regulator usage in this driver appears extremely problematic, among these problems is that it almost certainly has no sensible reason to be using regulator_get_exclusive() or any variant of it. Sadly every time it's been raised with the video people they've completely ignored the mail so here we are.
Right now not having the stub seems to only be affecting buggy users (which given the use cases for _exclusive() isn't *that* surprising) so I'm more inclined to leave this there in the hope that the users get fixed or we can at least get some sort of dialogue with the relevant maintainers.
quite frankly, flawed or not, I still think it's wrong of regulator framework to cause a build break during randconfig. Pretty much every other call is stubbed out, why wouldn't this be ? Moreover, if nobody cared to this day, why would this randconfig build break change their minds ?
Not that I really care, it's just yet another build break I need to ignore when build-testing. Whatever.
On Fri, Oct 24, 2014 at 03:18:27PM -0500, Felipe Balbi wrote:
On Fri, Oct 24, 2014 at 09:11:38PM +0100, Mark Brown wrote:
Right now not having the stub seems to only be affecting buggy users (which given the use cases for _exclusive() isn't *that* surprising) so I'm more inclined to leave this there in the hope that the users get fixed or we can at least get some sort of dialogue with the relevant maintainers.
quite frankly, flawed or not, I still think it's wrong of regulator framework to cause a build break during randconfig. Pretty much every other call is stubbed out, why wouldn't this be ? Moreover, if nobody
Well, it wasn't precisely a thought through choice before it happened but when it was reported it wasn't obvious how someone could use a stub (or what that stub should be) so I looked at the code and it just didn't look at all sensible which made me think having the stub was a bad idea.
There are some bits of the regulator API which can quite happily be stubbed out since the stub behaviour is within what could happen in normal usage but there are other bits where the user really has to care about what's happening and should probably depend on the regulator API, this is one of the latter bits.
cared to this day, why would this randconfig build break change their minds ?
For me it's more that I'm not terribly motivated to add code which only serves to enable broken usage; it may be that there's a perfectly good explanation for what the driver is doing but nobody seems to care about it so...
On Fri, Oct 24, 2014 at 4:11 PM, Mark Brown broonie@kernel.org wrote:
On Fri, Oct 24, 2014 at 02:15:11PM -0500, Felipe Balbi wrote:
If we don't stup that call out, we will have build failures for any drivers using that function when .config happens to have CONFIG_REGULATOR=n.
One such case below, found with randconfig
drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c: In function ‘mdp4_kms_init’: drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c:384:2: error: implicit declaration \
As previously and repeatedly reported the regulator usage in this driver appears extremely problematic, among these problems is that it almost certainly has no sensible reason to be using regulator_get_exclusive() or any variant of it. Sadly every time it's been raised with the video people they've completely ignored the mail so here we are.
oh, looks like a case of overambitious mailing list filter rules.. I did not see the earlier threads on the topic.
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.
If you have a recommendation for a better way, I am all ears.
BR, -R
Right now not having the stub seems to only be affecting buggy users (which given the use cases for _exclusive() isn't *that* surprising) so I'm more inclined to leave this there in the hope that the users get fixed or we can at least get some sort of dialogue with the relevant maintainers.
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.
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.
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.
On Fri, Oct 24, 2014 at 05:57:24PM -0400, Rob Clark wrote:
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
No, I saw that - looking at the code in hdmi_init() it's using normal devm_regulator_get() correctly (it appears to be open coding devm_regulator_bulk_get() and likewise for the enables and disables but that's a lot less serious). I can't see anything actively broken with that code other than the error handling on failed enable.
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..
It's a bug elsewhere, if you are getting a dummy regulator on a DT system it means that you don't have a regulator set up for that supply so the core is just assuming that it's always on.
dri-devel@lists.freedesktop.org
-
Felipe Balbi -
Mark Brown -
Rob Clark