Hi Thierry,
On Mon, Jul 21, 2014 at 1:44 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> 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@xxxxxxxxxxx>
>> ---
>> .../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"
Ok. Will fix this.
>> 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.
Ok.
>> @@ -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.
Ok. Will fix this.
>> @@ -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.
Ok.
>> 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.
Right, I am planning to use panel-simple driver by adding drm_display_mode
structure for the lvds panels. So, I would not need this change.
>> @@ -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.
Same explanation as above. Need not store panel_node inside private context.
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
