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

[Thierry Reding <thierry.reding@gmail.com>]

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

On Fri, Jul 18, 2014 at 02:20:39AM +0530, Ajay kumar wrote:
> +devicetree@vger.kernel.org
> 
> On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
> > Add DT binding documentation for panel-lvds driver.
> >
> > Signed-off-by: Ajay Kumar <ajaykumar.rs@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.

> > +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?

> > +       -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.

> > +       -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.

> > +       -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.

> > +       -panel-width-mm: physical panel width [mm]
> > +       -panel-height-mm: physical panel height [mm]

Same here.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]