Re: [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver
Hi Thierry, On Mon, Jul 21, 2014 at 1:22 PM, Thierry Reding 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 >> wrote: >> > On Fri, Jul 18, 2014 at 02:20:39AM +0530, Ajay kumar wrote: >> >> +devicet...@vger.kernel.org >> >> >> >> On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar >> >> wrote: >> >> > Add DT binding documentation for panel-lvds driver. >> >> > >> >> > Signed-off-by: Ajay Kumar >> >> > --- >> >> > .../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 000..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
Re: [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver
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 > wrote: > > On Fri, Jul 18, 2014 at 02:20:39AM +0530, Ajay kumar wrote: > >> +devicet...@vger.kernel.org > >> > >> On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar > >> wrote: > >> > Add DT binding documentation for panel-lvds driver. > >> > > >> > Signed-off-by: Ajay Kumar > >> > --- > >> > .../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 000..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
Re: [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver
Hi Thierry, Thanks for your comments. On Fri, Jul 18, 2014 at 4:18 AM, Thierry Reding wrote: > On Fri, Jul 18, 2014 at 02:20:39AM +0530, Ajay kumar wrote: >> +devicet...@vger.kernel.org >> >> On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar wrote: >> > Add DT binding documentation for panel-lvds driver. >> > >> > Signed-off-by: Ajay Kumar >> > --- >> > .../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 000..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 linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver
On Fri, Jul 18, 2014 at 02:20:39AM +0530, Ajay kumar wrote: > +devicet...@vger.kernel.org > > On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar wrote: > > Add DT binding documentation for panel-lvds driver. > > > > Signed-off-by: Ajay Kumar > > --- > > .../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 000..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 pgpACqAvTWT3G.pgp Description: PGP signature
Re: [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver
+devicet...@vger.kernel.org
On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar wrote:
> Add DT binding documentation for panel-lvds driver.
>
> Signed-off-by: Ajay Kumar
> ---
> .../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 000..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"
> +
> +Optional properties:
> + -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.
> + -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.
> + -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.
> + -panel-width-mm: physical panel width [mm]
> + -panel-height-mm: physical panel height [mm]
> +
> +Example:
> +
> + panel-lvds {
> + compatible = "panel-lvds";
> + backlight-enable-gpios = <&gpx3 0 0>;
> + panel-prepare-delay = <40>;
> + panel-enable-delay = <20>;
> + panel-disable-delay = <20>;
> + panel-unprepare-delay = <30>;
> + panel-width-mm = <256>;
> + panel-height-mm = <144>;
> + };
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
