Hi Thierry, Thanks for your comments. On Fri, Jul 18, 2014 at 4:18 AM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > On Fri, Jul 18, 2014 at 02:20:39AM +0530, Ajay kumar wrote: >> +devicetree@xxxxxxxxxxxxxxx >> >> On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx> wrote: >> > Add DT binding documentation for panel-lvds driver. >> > >> > Signed-off-by: Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx> >> > --- >> > .../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. >> > +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. >> > + -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). 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? >> > + -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. >> > + -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. Thanks and Regards, Ajay Kumar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
