On Tue, May 13, 2014 at 1:35 PM, Thierry Reding thierry.reding@gmail.com wrote:
On Fri, May 09, 2014 at 08:23:01PM +0530, Ajay Kumar wrote:
implement basic panel controls as a drm_bridge so that the existing bridges can make use of it.
The driver assumes that it is the last entity in the bridge chain.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++
Can we please stop adding files to this directory? Device tree bindings are supposed to be OS agnostic, but DRM is specific to Linux and should not be used when describing hardware.
One should not be adding a devicetree node if it is not describing the actual hardware?
diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
[...]
-led-en-gpio:
eDP panel LED enable GPIO.
Indicates which GPIO needs to be powered up as output
to enable the backlight.
Since this is used to control a backlight, then this should really be a separate node to describe the backlight device (and use the corresponding backlight driver) rather than duplicating a subset of that functionality.
I am stressing this point again! Backlight should ideally be enabled only after: 1) Panel is powered up (LCD_VDD) 2) HPD is thrown. 3) Valid data starts coming from the encoder(DP in our case) All the above 3 are controlled inside drm, but pwm-backlight is independent of drm. So, we let the pwm_config happen in pwm-backlight driver and enable backlight GPIO(BL_EN) inside drm, after valid video data starts coming out of the encoder. As you said, we can get this GPIO from a backlight device node, but since this GPIO is related to panel also, I think conceptually its not wrong to have this GPIO binding defined in a panel node.
-panel-pre-enable-delay:
delay value in ms required for panel_pre_enable process
Delay in ms needed for the eDP panel LCD unit to
powerup, and delay needed between panel_VCC and
video_enable.
What are panel_VCC or video_enable?
It is the time taken for the panel to throw a valid HPD from the point we enabled LCD_VCC. After HPD is thrown, we can start sending video data.
-panel-enable-delay:
delay value in ms required for panel_enable process
Delay in ms needed for the eDP panel backlight/LED unit
to powerup, and delay needed between video_enable and
BL_EN.
Similarily, what does BL_EN stand for?
Backlight Enable, same as "enable-gpios" in pwm-backlight driver.
bridge-panel {
compatible = "drm-bridge,panel";
Again, drm- doesn't mean anything outside of Linux (and maybe BSD), therefore shouldn't be used to describe hardware in device tree.
Agreed. :)
diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c
[...]
This duplicates much of the functionality that panels should provide. I think a better solution would be to properly integrate panels with bridges.
True, ideally I should be calling drm_panel functions from a simple dummy bridge which sits at the end of the bridge chain. But, since you are not convinced with having pre_enable and post_disable calls for panels, I had no other way to do this, than stuffing these panel controls inside bridge! :( See the discussion here. I have already explained this! http://www.spinics.net/lists/dri-devel/msg57973.html
Ajay