[PATCH] video: mxsfb: get supply regulator optionally

[PATCH] video: mxsfb: get supply regulator optionally

[Stefan Agner]

The lcd-supply is meant to be optional, there are several device-
trees not specifying it and the code handles error values silently.
Therefor, avoid creating a dummy regulator (and the associated
warning) by using devm_regulator_get_optional.

While at it, document that fact also in the device-tree bindings.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 Documentation/devicetree/bindings/display/mxsfb.txt | 3 +++
 drivers/video/fbdev/mxsfb.c                         | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/mxsfb.txt b/Documentation/devicetree/bindings/display/mxsfb.txt
index 96ec517..0bc530f 100644
--- a/Documentation/devicetree/bindings/display/mxsfb.txt
+++ b/Documentation/devicetree/bindings/display/mxsfb.txt
@@ -7,6 +7,9 @@ Required properties:
 - interrupts: Should contain lcdif interrupts
 - display : phandle to display node (see below for details)
 
+Optional properties:
+- lcd-supply: Regulator for LCD supply voltage.
+
 * display node
 
 Required properties:
diff --git a/drivers/video/fbdev/mxsfb.c b/drivers/video/fbdev/mxsfb.c
index 4e6608c..4f7570f 100644
--- a/drivers/video/fbdev/mxsfb.c
+++ b/drivers/video/fbdev/mxsfb.c
@@ -920,7 +920,7 @@ static int mxsfb_probe(struct platform_device *pdev)
 	if (IS_ERR(host->clk_disp_axi))
 		host->clk_disp_axi = NULL;
 
-	host->reg_lcd = devm_regulator_get(&pdev->dev, "lcd");
+	host->reg_lcd = devm_regulator_get_optional(&pdev->dev, "lcd");
 	if (IS_ERR(host->reg_lcd))
 		host->reg_lcd = NULL;
 
-- 
2.9.0

Re: [PATCH] video: mxsfb: get supply regulator optionally

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 749 bytes --]

Hi,

On 04/09/16 07:26, Stefan Agner wrote:
> The lcd-supply is meant to be optional, there are several device-
> trees not specifying it and the code handles error values silently.
> Therefor, avoid creating a dummy regulator (and the associated
> warning) by using devm_regulator_get_optional.
> 
> While at it, document that fact also in the device-tree bindings.

The binding change looks correct, but using
devm_regulator_get_optional() does not sound correct.

devm_regulator_get_optional() is to be used when the device in question
truly can function without the power supply. But if the supply is there,
it's just not controlled by the SW, devm_regulator_get() is to be used.

At least this is my understanding.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] video: mxsfb: get supply regulator optionally

[Stefan Agner]

On 2016-09-06 01:21, Tomi Valkeinen wrote:
> Hi,
> 
> On 04/09/16 07:26, Stefan Agner wrote:
>> The lcd-supply is meant to be optional, there are several device-
>> trees not specifying it and the code handles error values silently.
>> Therefor, avoid creating a dummy regulator (and the associated
>> warning) by using devm_regulator_get_optional.
>>
>> While at it, document that fact also in the device-tree bindings.
> 
> The binding change looks correct, but using
> devm_regulator_get_optional() does not sound correct.
> 
> devm_regulator_get_optional() is to be used when the device in question
> truly can function without the power supply. But if the supply is there,
> it's just not controlled by the SW, devm_regulator_get() is to be used.

The framebuffer device can even function without a display, no problem
there.. Probably not really useful...

devm_regulator_get creates a dummy regulator and a warning. Afaik, the
dummy regulator was meant to be as an aid during development, but not as
a permanent solution. This is what the initial commit of the dummy
regulator says:

> In order to ease transitions with drivers are boards start using regulators
> provide an option to cause all regulator_get() calls to succeed, with a
> dummy always on regulator being supplied where one has not been configured.
> A warning is printed whenever the dummy regulator is used to aid system
> development.

I think we should either make the property mandatory and fix the device
trees or we should fix the driver to support an optional regulator. The
code already supports the reg_lcd being NULL, which is probably mostly
pointless right now as devm_regulator_get always returns a dummy
regulator.

--
Stefan

Re: [PATCH] video: mxsfb: get supply regulator optionally

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On Tue, Sep 06, 2016 at 11:23:31AM -0700, Stefan Agner wrote:
> On 2016-09-06 01:21, Tomi Valkeinen wrote:

> > In order to ease transitions with drivers are boards start using regulators
> > provide an option to cause all regulator_get() calls to succeed, with a
> > dummy always on regulator being supplied where one has not been configured.
> > A warning is printed whenever the dummy regulator is used to aid system
> > development.

> I think we should either make the property mandatory and fix the device
> trees or we should fix the driver to support an optional regulator. The
> code already supports the reg_lcd being NULL, which is probably mostly
> pointless right now as devm_regulator_get always returns a dummy
> regulator.

I think you're agreeing with each other here (and me).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH] video: mxsfb: get supply regulator optionally

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 2745 bytes --]

On 06/09/16 21:23, Stefan Agner wrote:
> On 2016-09-06 01:21, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 04/09/16 07:26, Stefan Agner wrote:
>>> The lcd-supply is meant to be optional, there are several device-
>>> trees not specifying it and the code handles error values silently.
>>> Therefor, avoid creating a dummy regulator (and the associated
>>> warning) by using devm_regulator_get_optional.
>>>
>>> While at it, document that fact also in the device-tree bindings.
>>
>> The binding change looks correct, but using
>> devm_regulator_get_optional() does not sound correct.
>>
>> devm_regulator_get_optional() is to be used when the device in question
>> truly can function without the power supply. But if the supply is there,
>> it's just not controlled by the SW, devm_regulator_get() is to be used.
> 
> The framebuffer device can even function without a display, no problem
> there.. Probably not really useful...

Yes. Of course, the question then becomes, why is the fb driver even
dealing with the LCD's regulator. But yes, I know the answer: because
that's how it has been done =).

> devm_regulator_get creates a dummy regulator and a warning. Afaik, the
> dummy regulator was meant to be as an aid during development, but not as
> a permanent solution. This is what the initial commit of the dummy
> regulator says:

Yep, the fixed regulator is afaik the correct solution to represent
non-controllable regulators.

>> In order to ease transitions with drivers are boards start using regulators
>> provide an option to cause all regulator_get() calls to succeed, with a
>> dummy always on regulator being supplied where one has not been configured.
>> A warning is printed whenever the dummy regulator is used to aid system
>> development.
> 
> I think we should either make the property mandatory and fix the device
> trees or we should fix the driver to support an optional regulator. The
> code already supports the reg_lcd being NULL, which is probably mostly
> pointless right now as devm_regulator_get always returns a dummy
> regulator.

To really clean this up, the LCD driver should be separated from the fb
driver. But that's pointless work on a framework that should be
deprecated (is there a DRM driver for this in the works? =).

I'm fine with the _optional version, that's the easiest cleanup here.
And, I guess, it could be even argued that it's correct in some cases,
as the fb output could go outside the board, to some externally powered
display.

I'm fine with doing more cleanups too, if it eases the maintenance
burden in the future. But I don't see what the cleanups for the device
trees would really give us here.

Mark, what do you say?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] video: mxsfb: get supply regulator optionally

[Rob Herring]

On Sat, Sep 03, 2016 at 09:26:27PM -0700, Stefan Agner wrote:
> The lcd-supply is meant to be optional, there are several device-
> trees not specifying it and the code handles error values silently.
> Therefor, avoid creating a dummy regulator (and the associated
> warning) by using devm_regulator_get_optional.
> 
> While at it, document that fact also in the device-tree bindings.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  Documentation/devicetree/bindings/display/mxsfb.txt | 3 +++

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/video/fbdev/mxsfb.c                         | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)

Re: [PATCH] video: mxsfb: get supply regulator optionally

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 443 bytes --]

On Wed, Sep 07, 2016 at 10:20:25AM +0300, Tomi Valkeinen wrote:

> I'm fine with the _optional version, that's the easiest cleanup here.
> And, I guess, it could be even argued that it's correct in some cases,
> as the fb output could go outside the board, to some externally powered
> display.

I'd *prefer* to see the supplies being specified, if only for the
encouragement of the others.  But if you want to do the optional thing
anyway...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] video: mxsfb: get supply regulator optionally

[Stefan Agner]

Hi Tomi,

I vote to merge this patch, see below:

On 2016-09-07 00:20, Tomi Valkeinen wrote:
> On 06/09/16 21:23, Stefan Agner wrote:
>> On 2016-09-06 01:21, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 04/09/16 07:26, Stefan Agner wrote:
>>>> The lcd-supply is meant to be optional, there are several device-
>>>> trees not specifying it and the code handles error values silently.
>>>> Therefor, avoid creating a dummy regulator (and the associated
>>>> warning) by using devm_regulator_get_optional.
>>>>
>>>> While at it, document that fact also in the device-tree bindings.
>>>
>>> The binding change looks correct, but using
>>> devm_regulator_get_optional() does not sound correct.
>>>
>>> devm_regulator_get_optional() is to be used when the device in question
>>> truly can function without the power supply. But if the supply is there,
>>> it's just not controlled by the SW, devm_regulator_get() is to be used.
>>
>> The framebuffer device can even function without a display, no problem
>> there.. Probably not really useful...
> 
> Yes. Of course, the question then becomes, why is the fb driver even
> dealing with the LCD's regulator. But yes, I know the answer: because
> that's how it has been done =).

Agreed, this property really shouldn't be part of the display controller
node.

The device tree should describe the hardware, and that is clearly not
how the hardware is wired up...

The DRM solution to this is having a separate panel node with a
(mandatory) power-supply property.

> 
>> devm_regulator_get creates a dummy regulator and a warning. Afaik, the
>> dummy regulator was meant to be as an aid during development, but not as
>> a permanent solution. This is what the initial commit of the dummy
>> regulator says:
> 
> Yep, the fixed regulator is afaik the correct solution to represent
> non-controllable regulators.
> 
>>> In order to ease transitions with drivers are boards start using regulators
>>> provide an option to cause all regulator_get() calls to succeed, with a
>>> dummy always on regulator being supplied where one has not been configured.
>>> A warning is printed whenever the dummy regulator is used to aid system
>>> development.
>>
>> I think we should either make the property mandatory and fix the device
>> trees or we should fix the driver to support an optional regulator. The
>> code already supports the reg_lcd being NULL, which is probably mostly
>> pointless right now as devm_regulator_get always returns a dummy
>> regulator.
> 
> To really clean this up, the LCD driver should be separated from the fb
> driver. But that's pointless work on a framework that should be
> deprecated (is there a DRM driver for this in the works? =).

Not that I am aware of, but I am considering it actually...

> 
> I'm fine with the _optional version, that's the easiest cleanup here.
> And, I guess, it could be even argued that it's correct in some cases,
> as the fb output could go outside the board, to some externally powered
> display.
> 
> I'm fine with doing more cleanups too, if it eases the maintenance
> burden in the future. But I don't see what the cleanups for the device
> trees would really give us here.
> 
> Mark, what do you say?

So this is what mark said:

On 2016-09-12 16:47, Mark Brown wrote:
> On Wed, Sep 07, 2016 at 10:20:25AM +0300, Tomi Valkeinen wrote:
> 
>> I'm fine with the _optional version, that's the easiest cleanup here.
>> And, I guess, it could be even argued that it's correct in some cases,
>> as the fb output could go outside the board, to some externally powered
>> display.
> 
> I'd *prefer* to see the supplies being specified, if only for the
> encouragement of the others.  But if you want to do the optional thing
> anyway...

In this case we really don't want people encourage using this
property... Making it optional is not the solution, but a band aid until
we have a proper solution (and deprecate the property...)

--
Stefan