On Tue, May 13, 2014 at 1:35 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> 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@xxxxxxxxxxx>
>> ---
>> .../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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
