| Message ID | 1405629839-12086-4-git-send-email-ajaykumar.rs@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Jul 18, 2014 at 02:13:49AM +0530, Ajay Kumar wrote: [...] > diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig [...] > depends on DRM_EXYNOS_FIMD && ARCH_EXYNOS && (DRM_PTN3460=n || DRM_PTN3460=y || DRM_PTN3460=DRM_EXYNOS) One of the reasons the current way of implementing bridges is that it doesn't scale, as shown by this ridiculous dependency. In patch [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver you add support for another type of bridge but it doesn't update this dependency. I suspect that in order for this to work properly you'll need to extend the dependency like so (rewritten to make it somewhat cleaner): depends on DRM_EXYNOS_FIMD && ARCH_EXYNOS depends on DRM_PTN3460=n || DRM_PTN3460=y || DRM_PTN3460=DRM_EXYNOS depends on DRM_PS8622=n || DRM_PS8622=y || DRM_PS8622=DRM_EXYNOS And you'll need one more of these lines for each new bridge chip that you want to support. Thierry
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@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
Hi Thierry, On Mon, Jul 21, 2014 at 1:44 PM, Thierry Reding <thierry.reding@gmail.com> 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@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" 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Optional properties for dp-controller: -interlaced: diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig index 178d2a9..fd1c070 100644 --- a/drivers/gpu/drm/exynos/Kconfig +++ b/drivers/gpu/drm/exynos/Kconfig @@ -52,6 +52,7 @@ config DRM_EXYNOS_DP bool "EXYNOS DRM DP driver support" depends on DRM_EXYNOS_FIMD && ARCH_EXYNOS && (DRM_PTN3460=n || DRM_PTN3460=y || DRM_PTN3460=DRM_EXYNOS) default DRM_EXYNOS + select DRM_PANEL help This enables support for DP device. 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); + return 0; } @@ -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); } static void exynos_dp_poweroff(struct exynos_dp_device *dp) @@ -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); } 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, OF_USE_NATIVE_MODE); if (ret) { DRM_ERROR("failed: of_get_videomode() : %d\n", ret); @@ -1224,16 +1241,10 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data) struct platform_device *pdev = to_platform_device(dev); struct drm_device *drm_dev = data; struct resource *res; - struct exynos_dp_device *dp; + struct exynos_dp_device *dp = exynos_dp_display.ctx; unsigned int irq_flags; - int ret = 0; - dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device), - GFP_KERNEL); - if (!dp) - return -ENOMEM; - dp->dev = &pdev->dev; dp->dpms_mode = DRM_MODE_DPMS_OFF; @@ -1307,7 +1318,6 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data) disable_irq(dp->irq); dp->drm_dev = drm_dev; - exynos_dp_display.ctx = dp; platform_set_drvdata(pdev, &exynos_dp_display); @@ -1334,6 +1344,8 @@ static const struct component_ops exynos_dp_ops = { static int exynos_dp_probe(struct platform_device *pdev) { + struct device *dev = &pdev->dev; + struct exynos_dp_device *dp; int ret; ret = exynos_drm_component_add(&pdev->dev, EXYNOS_DEVICE_TYPE_CONNECTOR, @@ -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); + if (dp->panel_node) { + dp->edp_panel = of_drm_find_panel(dp->panel_node); + of_node_put(dp->panel_node); + if (!dp->edp_panel) + return -EPROBE_DEFER; + } + + exynos_dp_display.ctx = dp; + ret = component_add(&pdev->dev, &exynos_dp_ops); if (ret) exynos_drm_component_del(&pdev->dev, diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.h b/drivers/gpu/drm/exynos/exynos_dp_core.h index 02cc4f9..3c29e4e 100644 --- a/drivers/gpu/drm/exynos/exynos_dp_core.h +++ b/drivers/gpu/drm/exynos/exynos_dp_core.h @@ -146,9 +146,11 @@ struct link_train { struct exynos_dp_device { struct device *dev; + struct device_node *panel_node; struct drm_device *drm_dev; struct drm_connector connector; struct drm_encoder *encoder; + struct drm_panel *edp_panel; struct clk *clock; unsigned int irq; void __iomem *reg_base;
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@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(-)