linux-kernel - Re: [PATCH] video: mxsfb: get supply regulator optionally

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1812b2bf-bd66-3553-4c2d-3d2b08603ca0@ti.com>
Date:   Wed, 7 Sep 2016 10:20:25 +0300
From:   Tomi Valkeinen <tomi.valkeinen@...com>
To:     Stefan Agner <stefan@...er.ch>
CC:     <plagnioj@...osoft.com>, <robh+dt@...nel.org>,
        <mark.rutland@....com>, <devicetree@...r.kernel.org>,
        <linux-fbdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] video: mxsfb: get supply regulator optionally

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



Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)