[PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver

[Ajay kumar]

Hi Thierry,

On Mon, Jul 21, 2014 at 1:22 PM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> On Fri, Jul 18, 2014 at 12:18:05PM +0530, Ajay kumar wrote:
>> Hi Thierry,
>>
>> Thanks for your comments.
>>
>> On Fri, Jul 18, 2014 at 4:18 AM, Thierry Reding
>> <thierry.reding at gmail.com> wrote:
>> > On Fri, Jul 18, 2014 at 02:20:39AM +0530, Ajay kumar wrote:
>> >> +devicetree at vger.kernel.org
>> >>
>> >> On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar <ajaykumar.rs at samsung.com> wrote:
>> >> > Add DT binding documentation for panel-lvds driver.
>> >> >
>> >> > Signed-off-by: Ajay Kumar <ajaykumar.rs at samsung.com>
>> >> > ---
>> >> >  .../devicetree/bindings/panel/panel-lvds.txt       |   50 ++++++++++++++++++++
>> >> >  1 file changed, 50 insertions(+)
>> >> >  create mode 100644 Documentation/devicetree/bindings/panel/panel-lvds.txt
>> >> >
>> >> > diff --git a/Documentation/devicetree/bindings/panel/panel-lvds.txt b/Documentation/devicetree/bindings/panel/panel-lvds.txt
>> >> > new file mode 100644
>> >> > index 0000000..fdf91da2
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/panel/panel-lvds.txt
>> >> > @@ -0,0 +1,50 @@
>> >> > +panel interface for eDP/lvds panels
>> >> > +
>> >> > +Required properties:
>> >> > +  - compatible: "panel-lvds"
>> >
>> > I think I've said this before. I oppose the addition of this binding. We
>> > need to list a device-specific compatible value here, wildcards like
>> > this aren't a good choice. And then if we have that compatible value we
>> > can move most of the optional properties below into a driver.
>> Yes, "panel-lvds" is a wildcard for all lvds panels.
>> And since lvds panels from different vendors have different values
>> for power_up_delay, delay from video_to_backlight etc, I thought its
>> better to pick them up from device tree.
>
> What I'm saying is that we shouldn't be adding this type of wildcard.
> Instead the compatible property needs to list the specific type of panel
> and since the driver will match on that specific compatible, the values
> for the delays etc. are all implicit and don't have to be listed in
> device tree.
>
>> >> > +Optional properties:
>> >
>> > If all these properties are optional, then what happens if a device tree
>> > node doesn't contain any of them? Doesn't that turn the driver into one
>> > big no-op?
>> No! We need to provide lcd-supply and backlight-supply.
>
> What are those? I don't see them described anywhere.
>
>> >> > +       -lcd-enable-gpios:
>> >> > +               panel LCD poweron GPIO.
>> >> > +                       Indicates which GPIO needs to be powered up as output
>> >> > +                       to powerup/enable the switch to the LCD panel.
>> >> > +       -backlight-enable-gpios:
>> >> > +               panel LED enable GPIO.
>> >> > +                       Indicates which GPIO needs to be powered up as output
>> >> > +                       to enable the backlight.
>> >
>> > I've also said before that this really belongs in a backlight driver.
>> > Chances are that you'll want to have a way to set the brightness of the
>> > backlight as well, so simply an enable GPIO won't be good enough.
>> Ok. I can handle this in backlight driver itself (with some minor glitches).
>
> You can make it work without glitches as well and we've discussed this
> before.
>
>> But, how do I map bridge functions to panel functions now?
>> The bridge supports (pre_enable, enable, disable and post_disable) which I map
>> with (prepare, enable, disable and unprepare) of the panel, using a sample layer
>> called bridge to panel_binder.
>> Moving out the backlight control from panel means I really don't need
>> those extra
>> panel calls(prepare and unprepare)!
>> Then how to distribute 2 panel calls(enable and disable) across 4 bridge calls?
>
> The .prepare() and .unprepare() functions are useful irrespective of
> backlight control. What you want to separate is powering up the panel
> from enabling the backlight. So .prepare() could be what's doing the
> power up of the panel (and possibly other initialization required to
> make it work) and .enable() could be as simple as turning on the
> backlight.
>
> What I'm saying is rather than use a backlight-enable-gpios property in
> the panel, that property should be part of the backlight device and the
> GPIO can then be toggled by the backlight driver when the backlight is
> enabled.
>
>> >> > +       -panel-prepare-delay:
>> >> > +               delay value in ms required for panel_prepare process
>> >> > +                       Delay in ms needed for the panel LCD unit to
>> >> > +                       powerup completely.
>> >> > +                       ex: delay needed till eDP panel throws HPD.
>> >> > +                           delay needed so that we cans tart reading edid.
>> >
>> > If the panel signals HPD then we don't need this delay at all and we
>> > should just wait for HPD instead.
>> Not always for HPD, we need to wait for EDID module as well.
>
> I always thought that HPD signalled readiness from the panel to provide
> EDID, too. Are there really panels that need additional delays after HPD
> has been signalled until the EDID can be read? Are there maybe other
> ways to detect that EDID can't be read?
>
>> >> > +       -panel-enable-delay:
>> >> > +               delay value in ms required for panel_enable process
>> >> > +                       Delay in ms needed for the panel backlight/LED unit
>> >> > +                       to powerup, and delay needed between video_enable and
>> >> > +                       backlight_enable.
>> >> > +       -panel-disable-delay:
>> >> > +               delay value in ms required for panel_disable process
>> >> > +                       Delay in ms needed for the panel backlight/LED unit
>> >> > +                       powerdown, and delay needed between backlight_disable
>> >> > +                       and video_disable.
>> >> > +       -panel-unprepare-delay:
>> >> > +               delay value in ms required for panel_post_disable process
>> >> > +                       Delay in ms needed for the panel LCD unit to
>> >> > +                       to powerdown completely, and the minimum delay needed
>> >> > +                       before powering it on again.
>> >
>> > These delays are all panel specific and they don't vary per board, so
>> > they shouldn't go into the device tree at all.
>> But, fetching them from device tree would allow us to support all lvds
>> panels in this single driver.
>
> You can do that even without having this data in device tree. My point
> is that this is redundant information once you know the panel's specific
> compatible value. Therefore there shouldn't be a need to list this in
> device tree again.
>
> Thierry
I am planning to use panel-simple driver for the lvds panels. Let me test
using that and if things workout well, then will drop this entire driver.

Ajay