Adding the dri-devel mailing list on Cc, since this discussion is
outgrowing its original intent.
On Thu, Oct 17, 2013 at 02:47:52AM +0200, Laurent Pinchart wrote:
> Hi Thierry,
>
> Thank you for the patch.
>
> On Wednesday 16 October 2013 20:25:12 Thierry Reding wrote:
> > Add a driver for simple panels. Such panels can have a regulator that
> > provides the supply voltage and a separate GPIO to enable the panel.
> > Optionally the panels can have a backlight associated with them so it
> > can be enabled or disabled according to the panel's power management
> > mode.
> >
> > Support is added for three panels: An AU Optronics 10.1" WSVGA, a
> > Chunghwa Picture Tubes 10.1" WXGA and a Panasonic 10.1 WUXGA TFT LCD
> > panel.
> >
> > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> > ---
> > .../devicetree/bindings/panel/auo,b101aw03.txt | 7 +
> > .../bindings/panel/chunghwa,claa101wb03.txt | 7 +
> > .../bindings/panel/panasonic,vvx10f004b00.txt | 7 +
> > .../devicetree/bindings/panel/simple-panel.txt | 21 ++
> > drivers/gpu/drm/Makefile | 1 +
> > drivers/gpu/drm/panel/Kconfig | 13 +
> > drivers/gpu/drm/panel/Makefile | 1 +
> > drivers/gpu/drm/panel/panel-simple.c | 335 ++++++++++++++++++
> > 8 files changed, 392 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/panel/auo,b101aw03.txt
> > create mode 100644
> > Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt create
> > mode 100644
> > Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt create
> > mode 100644 Documentation/devicetree/bindings/panel/simple-panel.txt create
> > mode 100644 drivers/gpu/drm/panel/Makefile
> > create mode 100644 drivers/gpu/drm/panel/panel-simple.c
> >
> > diff --git a/Documentation/devicetree/bindings/panel/auo,b101aw03.txt
> > b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt new file mode
> > 100644
> > index 0000000..72e088a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/panel/auo,b101aw03.txt
> > @@ -0,0 +1,7 @@
> > +AU Optronics Corporation 10.1" WSVGA TFT LCD panel
> > +
> > +Required properties:
> > +- compatible: should be "auo,b101aw03"
> > +
> > +This binding is compatible with the simple-panel binding, which is
> > specified +in simple-panel.txt in this directory.
> > diff --git
> > a/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt
> > b/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt new file
> > mode 100644
> > index 0000000..0ab2c05
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt
> > @@ -0,0 +1,7 @@
> > +Chunghwa Picture Tubes Ltd. 10.1" WXGA TFT LCD panel
> > +
> > +Required properties:
> > +- compatible: should be "chunghwa,claa101wb03"
> > +
> > +This binding is compatible with the simple-panel binding, which is
> > specified +in simple-panel.txt in this directory.
> > diff --git
> > a/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt
> > b/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt new
> > file mode 100644
> > index 0000000..d328b03
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt
> > @@ -0,0 +1,7 @@
> > +Panasonic Corporation 10.1" WUXGA TFT LCD panel
> > +
> > +Required properties:
> > +- compatible: should be "panasonic,vvx10f004b00"
> > +
> > +This binding is compatible with the simple-panel binding, which is
> > specified +in simple-panel.txt in this directory.
> > diff --git a/Documentation/devicetree/bindings/panel/simple-panel.txt
> > b/Documentation/devicetree/bindings/panel/simple-panel.txt new file mode
> > 100644
> > index 0000000..1341bbf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/panel/simple-panel.txt
> > @@ -0,0 +1,21 @@
> > +Simple display panel
> > +
> > +Required properties:
> > +- power-supply: regulator to provide the supply voltage
> > +
> > +Optional properties:
> > +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
> > +- enable-gpios: GPIO pin to enable or disable the panel
> > +- backlight: phandle of the backlight device attached to the panel
> > +
> > +Example:
> > +
> > + panel: panel {
> > + compatible = "cptt,claa101wb01";
> > + ddc-i2c-bus = <&panelddc>;
> > +
> > + power-supply = <&vdd_pnl_reg>;
> > + enable-gpios = <&gpio 90 0>;
> > +
> > + backlight = <&backlight>;
> > + };
>
> My biggest concern here is that this will not be compatible with the CDF DT
> bindings. They're not complete yet, but they will require connections between
> entities to be described in DT, in a way very similar (or actually identical)
> to the V4L2 DT bindings, documented in
> Documentation/devicetree/bindings/media/video-interfaces.txt. Could you have a
> look at that ? Please ignore all optional properties beside remote-endpoint,
> as they're V4L2 specific.
Okay, so if I understand correctly, translating those bindings to panel
nodes would look somewhat like this:
dc: display-controller {
ports {
port@0 {
remote-endpoint = <&panel>;
};
};
};
panel: panel {
ports {
port@0 {
remote-endpoint = <&dc>;
};
};
};
The above leaves out any of the other, non-relevant properties. Does
that sound about right? That seems like an overly complex amount of data
to describe just a simple panel where the only connection between it and
the display hardware is the data bus and I was sort of expecting the CDF
to provide some sort of shortcut for setups where the connection is 1:1
with no means to perform any configuration of the bus.
> I also plan to specify video bus parameters in DT for CDF, but this hasn't
> been finalized yet.
That effectively blocks any of the code that I've been working on until
CDF has been merged. It's been discussed for over a year now, and there
has even been significant pushback on using CDF in DRM, so even if CDF
is eventually merged, that will still leave DRM with no way to turn on
a simple panel.
I keep wondering why we absolutely must have compatibility between CDF
and this simple panel binding. DT content is supposed to concern itself
with the description of hardware only. What about the following isn't an
accurate description of the panel hardware?
panel: panel {
compatible = "cptt,claa101wb01";
power-supply = <&vdd_pnl_reg>;
enable-gpios = <&gpio 90 0>;
backlight = <&backlight>;
};
dsi-controller {
panel = <&panel>;
};
Note how the above is also not incompatible with anything that the CDF
(to be) mandates, so it could easily be extended with any nodes or
properties that CDF needs to work properly without breaking backwards-
compatibility.
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c
[...]
> > +/*
> > + * Copyright (C) 2013, NVIDIA Corporation. All rights reserved.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sub
> > license,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> > + * next paragraph) shall be included in all copies or substantial portions
> > + * of the Software.
>
> This contradicts MODULE_LICENSE("GPL v2") below.
True, I'll fix that up.
> > +/* TODO: convert to gpiod_*() API once it's been merged */
> > +#define GPIO_ACTIVE_LOW (1 << 0)
>
> Why can't you just #include <dt-bindings/gpio/gpio.h> ?
This is supposed to be something completely driver internal. Keeping it
here make that more explicit. It shouldn't matter much anyway, since by
now these patches have about 0 chance of making it into 3.13, so I'll
just convert them to gpiod.
> > + backlight_update_status(p->backlight);
> > + }
> > +
> > + if (gpio_is_valid(p->enable_gpio)) {
> > + if (p->enable_gpio_flags & GPIO_ACTIVE_LOW)
> > + gpio_set_value(p->enable_gpio, 1);
> > + else
> > + gpio_set_value(p->enable_gpio, 0);
> > + }
> > +
> > + regulator_disable(p->supply);
> > + p->enabled = false;
> > +}
> > +
> > +static void panel_simple_enable(struct drm_panel *panel)
> > +{
> > + struct panel_simple *p = to_panel_simple(panel);
> > + int err;
> > +
> > + if (p->enabled)
> > + return;
> > +
> > + err = regulator_enable(p->supply);
> > + if (err < 0)
> > + dev_err(panel->dev, "failed to enable supply: %d\n", err);
>
> Is that really a non-fatal error ? Shouldn't the enable operation return an
> int ?
There's no way to propagate this in DRM, so why go through the trouble
of returning the error? Furthermore, there's nothing that the caller
could do to remedy the situation anyway.
> > +static const struct drm_display_mode auo_b101aw03_mode = {
> > + .clock = 51450,
> > + .hdisplay = 1024,
> > + .hsync_start = 1024 + 156,
> > + .hsync_end = 1024 + 156 + 8,
> > + .htotal = 1024 + 156 + 8 + 156,
> > + .vdisplay = 600,
> > + .vsync_start = 600 + 16,
> > + .vsync_end = 600 + 16 + 6,
> > + .vtotal = 600 + 16 + 6 + 16,
> > + .vrefresh = 60,
> > +};
>
> Instead of hardcoding the modes in the driver, which would then require to be
> updated for every new simple panel model (and we know there are lots of them),
> why don't you specify the modes in the panel DT node ? The simple panel driver
> would then become much more generic. It would also allow board designers to
> tweak h/v sync timings depending on the system requirements.
Sigh... we keep second-guessing ourselves. Back at the time when power
sequences were designed (and NAKed at the last minute), we all decided
that the right thing to do would be to use specific compatible values
for individual panels, because that would allow us to encode the power
sequencing within the driver. And when we already have the panel model
encoded in the compatible value, we might just as well encode the mode
within the driver for that panel. Otherwise we'll have to keep adding
the same mode timings for every board that uses the same panel.
I do agree though that it might be useful to tweak the mode in case the
default one doesn't work. How about we provide a means to override the
mode encoded in the driver using one specified in the DT? That's
obviously a backwards-compatible change, so it could be added if or when
it becomes necessary.
Thierry
Attachment:
pgpp5T4UsDoSy.pgp
Description: PGP signature
