Hi Thierry, On Mon, Jul 21, 2014 at 1:22 PM, Thierry Reding <thierry.reding@xxxxxxxxx> 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@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. > > 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 -- 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
