On Fri, Jul 18, 2014 at 02:13:49AM +0530, Ajay Kumar wrote:
> Add drm_panel controls to support powerup/down of the
> eDP panel, if one is present at the sink side.
>
> Signed-off-by: Ajay Kumar <ajaykumar.rs at samsung.com>
> ---
> .../devicetree/bindings/video/exynos_dp.txt | 2 +
> drivers/gpu/drm/exynos/Kconfig | 1 +
> drivers/gpu/drm/exynos/exynos_dp_core.c | 45
> ++++++++++++++++----
> drivers/gpu/drm/exynos/exynos_dp_core.h | 2 +
> 4 files changed, 41 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt
> b/Documentation/devicetree/bindings/video/exynos_dp.txt
> index 53dbccf..c029a09 100644
> --- a/Documentation/devicetree/bindings/video/exynos_dp.txt
> +++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
> @@ -51,6 +51,8 @@ Required properties for dp-controller:
> LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
> - display-timings: timings for the connected panel as described by
> Documentation/devicetree/bindings/video/display-timing.txt
> + -edp-panel:
> + phandle for the edp/lvds drm_panel node.
There's really no need for the edp- prefix here. Also please don't use
drm_panel in the binding description since it makes no sense outside of
DRM (and bindings need to be OS agnostic).
Also: "eDP" and "LVDS"
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
> b/drivers/gpu/drm/exynos/exynos_dp_core.c
> index a94b114..b3d0d9b 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> @@ -28,6 +28,7 @@
> #include <drm/drmP.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_panel.h>
> #include <drm/bridge/ptn3460.h>
>
> #include "exynos_drm_drv.h"
> @@ -1029,6 +1030,9 @@ static int exynos_dp_create_connector(struct
> exynos_drm_display *display,
> drm_connector_register(connector);
> drm_mode_connector_attach_encoder(connector, encoder);
>
> + if (dp->edp_panel)
> + drm_panel_attach(dp->edp_panel, &dp->connector);
This function can fail, so you really need to do some error-checking
here.
> @@ -1063,11 +1067,13 @@ static void exynos_dp_poweron(struct exynos_dp_device
> *dp)
> if (dp->dpms_mode == DRM_MODE_DPMS_ON)
> return;
>
> + drm_panel_prepare(dp->edp_panel);
> clk_prepare_enable(dp->clock);
> exynos_dp_phy_init(dp);
> exynos_dp_init_dp(dp);
> enable_irq(dp->irq);
> exynos_dp_setup(dp);
> + drm_panel_enable(dp->edp_panel);
These two as well. clk_prepare_enable() can also fail by the way.
exynos_dp_init_dp() can also fail theoretically (it returns int) but
never returns anything other than 0, so it could just as well return
void.
> @@ -1075,10 +1081,12 @@ static void exynos_dp_poweroff(struct
> exynos_dp_device *dp)
> if (dp->dpms_mode != DRM_MODE_DPMS_ON)
> return;
>
> + drm_panel_disable(dp->edp_panel);
> disable_irq(dp->irq);
> flush_work(&dp->hotplug_work);
> exynos_dp_phy_exit(dp);
> clk_disable_unprepare(dp->clock);
> + drm_panel_unprepare(dp->edp_panel);
> }
And these two also.
> static void exynos_dp_dpms(struct exynos_drm_display *display, int mode)
> @@ -1209,8 +1217,17 @@ err:
> static int exynos_dp_dt_parse_panel(struct exynos_dp_device *dp)
> {
> int ret;
> + struct device_node *videomode_parent;
> +
> + /* Receive video timing info from panel node
> + * if there is a panel node
> + */
> + if (dp->panel_node)
> + videomode_parent = dp->panel_node;
> + else
> + videomode_parent = dp->dev->of_node;
>
> - ret = of_get_videomode(dp->dev->of_node, &dp->panel.vm,
> + ret = of_get_videomode(videomode_parent, &dp->panel.vm,
You shouldn't be grabbing the video timings from some random node like
this. Instead you should use the DRM panel's .get_modes() function to
query the supported modes. The panel may not (in fact, should not, at
least under most circumstances) define video timings in the device tree.
> @@ -1341,6 +1353,21 @@ static int exynos_dp_probe(struct platform_device
> *pdev)
> if (ret)
> return ret;
>
> + dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
> + GFP_KERNEL);
> + if (!dp)
> + return -ENOMEM;
> +
> + dp->panel_node = of_parse_phandle(dev->of_node, "edp-panel", 0);
There should be no need to store the struct device_node * obtained here
in the context like this.
> + if (dp->panel_node) {
> + dp->edp_panel = of_drm_find_panel(dp->panel_node);
> + of_node_put(dp->panel_node);
Especially since after this that pointer may become invalid.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140721/1da77184/attachment.sig>
