The first 9 patches replaces direct use of the drm_panel function pointers with their drm_panel_* counterparts. The function pointers are only supposed to be used by the drm_panel infrastructure and direct use are discouraged.
ili9322 is updated to handle bus_flags in get_modes like everyone else. This is in preparation for a later patch series where controller becomes an arugument to get_modes() and not like today where drm_panel is attached to a controller.
The remaining patches move functionality to the drm_panel core that today are repeated in many drivers. As preparation for this the inline functions are moved to drm_panel.c and kernel-doc is made inline. panel-simple is updated to benefit from the additional infrastructure and is an example for the simplifications that can be done.
The patchset has been tested on my embedded target, and build tested.
Feedback welcome!
The "fix opencoded" patches are all independent and can be applied out of order. They were kept here to keep panel related patches in one series.
Sam
Cc: Alexios Zavras alexios.zavras@intel.com Cc: Alison Wang alison.wang@nxp.com Cc: Allison Randal allison@lohutok.net Cc: Andrzej Hajda a.hajda@samsung.com Cc: Benjamin Gaignard benjamin.gaignard@linaro.org Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie Cc: Enrico Weigelt info@metux.net Cc: Fabio Estevam festevam@gmail.com Cc: Gwan-gyeong Mun gwan-gyeong.mun@intel.com Cc: Inki Dae inki.dae@samsung.com Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: Jonas Karlman jonas@kwiboo.se Cc: Jonathan Hunter jonathanh@nvidia.com Cc: Joonyoung Shim jy0922.shim@samsung.com Cc: Krzysztof Kozlowski krzk@kernel.org Cc: Kukjin Kim kgene@kernel.org Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Cc: Linus Walleij linus.walleij@linaro.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-tegra@vger.kernel.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Marek Vasut marex@denx.de Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: NXP Linux Team linux-imx@nxp.com Cc: Pengutronix Kernel Team kernel@pengutronix.de Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Sam Ravnborg sam@ravnborg.org Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Sean Paul sean@poorly.run Cc: Seung-Woo Kim sw0312.kim@samsung.com Cc: Shawn Guo shawnguo@kernel.org Cc: Stefan Agner stefan@agner.ch Cc: Thierry Reding thierry.reding@gmail.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Vincent Abriou vincent.abriou@st.com
Sam Ravnborg (16): drm/bridge: tc358767: fix opencoded use of drm_panel_* drm/exynos: fix opencoded use of drm_panel_* drm/exynos: fix opencoded use of drm_panel_* drm/imx: fix opencoded use of drm_panel_* drm/fsl-dcu: fix opencoded use of drm_panel_* drm/msm: fix opencoded use of drm_panel_* drm/mxsfb: fix opencoded use of drm_panel_* drm/sti: fix opencoded use of drm_panel_* drm/tegra: fix opencoded use of drm_panel_* drm/panel: ili9322: move bus_flags to get_modes() drm/panel: move drm_panel functions to .c file drm/panel: use inline comments in drm_panel.h drm/panel: drop return code from drm_panel_detach() drm/panel: call prepare/enable only once drm/panel: add backlight support drm/panel: simple: use drm_panel infrastructure
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 3 +- drivers/gpu/drm/bridge/tc358767.c | 10 +- drivers/gpu/drm/drm_panel.c | 185 ++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_dpi.c | 4 +- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 10 +- drivers/gpu/drm/imx/imx-ldb.c | 11 +- drivers/gpu/drm/imx/parallel-display.c | 11 +- .../gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 2 +- drivers/gpu/drm/mxsfb/mxsfb_out.c | 2 +- drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 34 ++- drivers/gpu/drm/panel/panel-simple.c | 73 +------ drivers/gpu/drm/sti/sti_dvo.c | 8 +- drivers/gpu/drm/tegra/output.c | 2 +- include/drm/drm_panel.h | 227 +++++++++++---------- 15 files changed, 349 insertions(+), 235 deletions(-)
Replace open coded version with call to drm_panel_get_modes().
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/tc358767.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 42f03a985ac0..cebc8e620820 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1312,7 +1312,7 @@ static int tc_connector_get_modes(struct drm_connector *connector) { struct tc_data *tc = connector_to_tc(connector); struct edid *edid; - unsigned int count; + int count; int ret;
ret = tc_get_display_props(tc); @@ -1321,11 +1321,9 @@ static int tc_connector_get_modes(struct drm_connector *connector) return 0; }
- if (tc->panel && tc->panel->funcs && tc->panel->funcs->get_modes) { - count = tc->panel->funcs->get_modes(tc->panel); - if (count > 0) - return count; - } + count = drm_panel_get_modes(tc->panel); + if (count > 0) + return count;
edid = drm_get_edid(connector, &tc->aux.ddc);
On Sun, 2019-08-04 at 22:16 +0200, Sam Ravnborg wrote:
Replace open coded version with call to drm_panel_get_modes().
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/bridge/tc358767.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 42f03a985ac0..cebc8e620820 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1312,7 +1312,7 @@ static int tc_connector_get_modes(struct drm_connector *connector) { struct tc_data *tc = connector_to_tc(connector); struct edid *edid;
- unsigned int count;
- int count;
This looks like it also fixes a potential bug ...
int ret;
ret = tc_get_display_props(tc); @@ -1321,11 +1321,9 @@ static int tc_connector_get_modes(struct drm_connector *connector) return 0; }
- if (tc->panel && tc->panel->funcs && tc->panel->funcs->get_modes) {
count = tc->panel->funcs->get_modes(tc->panel);if (count > 0)
... when .get_modes returns a negative value.
return count;- }
count = drm_panel_get_modes(tc->panel);
if (count > 0)
return count;edid = drm_get_edid(connector, &tc->aux.ddc);
Reviewed-by: Philipp Zabel p.zabel@pengutronix.de
regards Philipp
Hi Philipp.
On Mon, Aug 05, 2019 at 11:35:56AM +0200, Philipp Zabel wrote:
On Sun, 2019-08-04 at 22:16 +0200, Sam Ravnborg wrote:
Replace open coded version with call to drm_panel_get_modes().
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net
drivers/gpu/drm/bridge/tc358767.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 42f03a985ac0..cebc8e620820 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1312,7 +1312,7 @@ static int tc_connector_get_modes(struct drm_connector *connector) { struct tc_data *tc = connector_to_tc(connector); struct edid *edid;
- unsigned int count;
- int count;
This looks like it also fixes a potential bug ...
get_modes() return either 0 or number of modes. The negative return values come into play in drm_panel_get_modes() when panel for example s NULL.
I will add this to changelog before I apply to avoid any misunderstanding.
Sam
drm_panel_attach() will check if there is a controller already attached - drop the check in the driver.
Use drm_panel_get_modes() so the driver no longer uses the function pointer.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Inki Dae inki.dae@samsung.com Cc: Joonyoung Shim jy0922.shim@samsung.com Cc: Seung-Woo Kim sw0312.kim@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Kukjin Kim kgene@kernel.org Cc: Krzysztof Kozlowski krzk@kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org --- drivers/gpu/drm/exynos/exynos_drm_dpi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c index 3cebb19ec1c4..5479ff71cbc6 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c @@ -43,7 +43,7 @@ exynos_dpi_detect(struct drm_connector *connector, bool force) { struct exynos_dpi *ctx = connector_to_dpi(connector);
- if (ctx->panel && !ctx->panel->connector) + if (ctx->panel) drm_panel_attach(ctx->panel, &ctx->connector);
return connector_status_connected; @@ -85,7 +85,7 @@ static int exynos_dpi_get_modes(struct drm_connector *connector) }
if (ctx->panel) - return ctx->panel->funcs->get_modes(ctx->panel); + return drm_panel_get_modes(ctx->panel);
return 0; }
On Sun, Aug 04, 2019 at 10:16:23PM +0200, Sam Ravnborg wrote:
drm_panel_attach() will check if there is a controller already attached - drop the check in the driver.
Use drm_panel_get_modes() so the driver no longer uses the function pointer.
Applied to drm-misc-next.
Sam
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Inki Dae inki.dae@samsung.com Cc: Joonyoung Shim jy0922.shim@samsung.com Cc: Seung-Woo Kim sw0312.kim@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Kukjin Kim kgene@kernel.org Cc: Krzysztof Kozlowski krzk@kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org
drivers/gpu/drm/exynos/exynos_drm_dpi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c index 3cebb19ec1c4..5479ff71cbc6 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c @@ -43,7 +43,7 @@ exynos_dpi_detect(struct drm_connector *connector, bool force) { struct exynos_dpi *ctx = connector_to_dpi(connector);
- if (ctx->panel && !ctx->panel->connector)
if (ctx->panel) drm_panel_attach(ctx->panel, &ctx->connector);
return connector_status_connected;
@@ -85,7 +85,7 @@ static int exynos_dpi_get_modes(struct drm_connector *connector) }
if (ctx->panel)
return ctx->panel->funcs->get_modes(ctx->panel);
return drm_panel_get_modes(ctx->panel);return 0;
}
2.20.1
Call via drm_panel_get_modes().
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Inki Dae inki.dae@samsung.com Cc: Joonyoung Shim jy0922.shim@samsung.com Cc: Seung-Woo Kim sw0312.kim@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Kukjin Kim kgene@kernel.org Cc: Krzysztof Kozlowski krzk@kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 6926cee91b36..36b02b456d9c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1460,7 +1460,7 @@ static int exynos_dsi_get_modes(struct drm_connector *connector) struct exynos_dsi *dsi = connector_to_dsi(connector);
if (dsi->panel) - return dsi->panel->funcs->get_modes(dsi->panel); + return drm_panel_get_modes(dsi->panel);
return 0; }
On Sun, Aug 04, 2019 at 10:16:24PM +0200, Sam Ravnborg wrote:
Call via drm_panel_get_modes().
Applied to drm-misc-next.
Sam
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Inki Dae inki.dae@samsung.com Cc: Joonyoung Shim jy0922.shim@samsung.com Cc: Seung-Woo Kim sw0312.kim@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Kukjin Kim kgene@kernel.org Cc: Krzysztof Kozlowski krzk@kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 6926cee91b36..36b02b456d9c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1460,7 +1460,7 @@ static int exynos_dsi_get_modes(struct drm_connector *connector) struct exynos_dsi *dsi = connector_to_dsi(connector);
if (dsi->panel)
return dsi->panel->funcs->get_modes(dsi->panel);
return drm_panel_get_modes(dsi->panel);return 0;
}
2.20.1
Use the drm_panel_get_modes() function to get the modes.
This patch leave one test for the function pointer: panel->funcs->get_modes
This is used to check if the panel may have any modes. There is no direct replacement. We may be able to just check that drm_panel_get_modes() return > 0, but as this is not the same functionality it is left for later.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Shawn Guo shawnguo@kernel.org Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Pengutronix Kernel Team kernel@pengutronix.de Cc: Fabio Estevam festevam@gmail.com Cc: NXP Linux Team linux-imx@nxp.com Cc: linux-arm-kernel@lists.infradead.org --- drivers/gpu/drm/imx/imx-ldb.c | 11 ++++------- drivers/gpu/drm/imx/parallel-display.c | 11 ++++------- 2 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c index db461b6a257f..695f307f36b2 100644 --- a/drivers/gpu/drm/imx/imx-ldb.c +++ b/drivers/gpu/drm/imx/imx-ldb.c @@ -124,14 +124,11 @@ static void imx_ldb_ch_set_bus_format(struct imx_ldb_channel *imx_ldb_ch, static int imx_ldb_connector_get_modes(struct drm_connector *connector) { struct imx_ldb_channel *imx_ldb_ch = con_to_imx_ldb_ch(connector); - int num_modes = 0; + int num_modes;
- if (imx_ldb_ch->panel && imx_ldb_ch->panel->funcs && - imx_ldb_ch->panel->funcs->get_modes) { - num_modes = imx_ldb_ch->panel->funcs->get_modes(imx_ldb_ch->panel); - if (num_modes > 0) - return num_modes; - } + num_modes = drm_panel_get_modes(imx_ldb_ch->panel); + if (num_modes > 0) + return num_modes;
if (!imx_ldb_ch->edid && imx_ldb_ch->ddc) imx_ldb_ch->edid = drm_get_edid(connector, imx_ldb_ch->ddc); diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index 2e51b2fade75..e7ce17503ae1 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -47,14 +47,11 @@ static int imx_pd_connector_get_modes(struct drm_connector *connector) { struct imx_parallel_display *imxpd = con_to_imxpd(connector); struct device_node *np = imxpd->dev->of_node; - int num_modes = 0; + int num_modes;
- if (imxpd->panel && imxpd->panel->funcs && - imxpd->panel->funcs->get_modes) { - num_modes = imxpd->panel->funcs->get_modes(imxpd->panel); - if (num_modes > 0) - return num_modes; - } + num_modes = drm_panel_get_modes(imxpd->panel); + if (num_modes > 0) + return num_modes;
if (imxpd->edid) { drm_connector_update_edid_property(connector, imxpd->edid);
On Sun, 2019-08-04 at 22:16 +0200, Sam Ravnborg wrote:
Use the drm_panel_get_modes() function to get the modes.
This patch leave one test for the function pointer: panel->funcs->get_modes
This is used to check if the panel may have any modes. There is no direct replacement. We may be able to just check that drm_panel_get_modes() return > 0, but as this is not the same functionality it is left for later.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Pengutronix Kernel Team kernel@pengutronix.de Cc: Fabio Estevam festevam@gmail.com Cc: NXP Linux Team linux-imx@nxp.com Cc: linux-arm-kernel@lists.infradead.org
Acked-by: Philipp Zabel p.zabel@pengutronix.de
regards Philipp
Use drm_panel_get_modes() to access modes. This has a nice side effect to simplify the code.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Stefan Agner stefan@agner.ch Cc: Alison Wang alison.wang@nxp.com --- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c index 279d83eaffc0..a92fd6c70b09 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c @@ -65,17 +65,9 @@ static const struct drm_connector_funcs fsl_dcu_drm_connector_funcs = { static int fsl_dcu_drm_connector_get_modes(struct drm_connector *connector) { struct fsl_dcu_drm_connector *fsl_connector; - int (*get_modes)(struct drm_panel *panel); - int num_modes = 0;
fsl_connector = to_fsl_dcu_connector(connector); - if (fsl_connector->panel && fsl_connector->panel->funcs && - fsl_connector->panel->funcs->get_modes) { - get_modes = fsl_connector->panel->funcs->get_modes; - num_modes = get_modes(fsl_connector->panel); - } - - return num_modes; + return drm_panel_get_modes(fsl_connector->panel); }
static int fsl_dcu_drm_connector_mode_valid(struct drm_connector *connector,
On 2019-08-04 22:16, Sam Ravnborg wrote:
Use drm_panel_get_modes() to access modes. This has a nice side effect to simplify the code.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Stefan Agner stefan@agner.ch Cc: Alison Wang alison.wang@nxp.com
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c index 279d83eaffc0..a92fd6c70b09 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c @@ -65,17 +65,9 @@ static const struct drm_connector_funcs fsl_dcu_drm_connector_funcs = { static int fsl_dcu_drm_connector_get_modes(struct drm_connector *connector) { struct fsl_dcu_drm_connector *fsl_connector;
int (*get_modes)(struct drm_panel *panel);
int num_modes = 0;
fsl_connector = to_fsl_dcu_connector(connector);
if (fsl_connector->panel && fsl_connector->panel->funcs &&
fsl_connector->panel->funcs->get_modes) {get_modes = fsl_connector->panel->funcs->get_modes;num_modes = get_modes(fsl_connector->panel);}
return num_modes;
- return drm_panel_get_modes(fsl_connector->panel);
Oh, that old code looks rather messy. Thanks for the simplification!
This behaves slightly different since it now returns -EINVAL or -ENOSYS, but that is what we want.
Acked-by: Stefan Agner stefan@agner.ch
-- Stefan
}
static int fsl_dcu_drm_connector_mode_valid(struct drm_connector *connector,
Hi Stefan.
Thanks for the feedback.
On Mon, Aug 05, 2019 at 11:16:26AM +0200, Stefan Agner wrote:
On 2019-08-04 22:16, Sam Ravnborg wrote:
Use drm_panel_get_modes() to access modes. This has a nice side effect to simplify the code.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Stefan Agner stefan@agner.ch Cc: Alison Wang alison.wang@nxp.com
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c index 279d83eaffc0..a92fd6c70b09 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c @@ -65,17 +65,9 @@ static const struct drm_connector_funcs fsl_dcu_drm_connector_funcs = { static int fsl_dcu_drm_connector_get_modes(struct drm_connector *connector) { struct fsl_dcu_drm_connector *fsl_connector;
int (*get_modes)(struct drm_panel *panel);
int num_modes = 0;
fsl_connector = to_fsl_dcu_connector(connector);
if (fsl_connector->panel && fsl_connector->panel->funcs &&
fsl_connector->panel->funcs->get_modes) {get_modes = fsl_connector->panel->funcs->get_modes;num_modes = get_modes(fsl_connector->panel);}
return num_modes;
- return drm_panel_get_modes(fsl_connector->panel);
Oh, that old code looks rather messy. Thanks for the simplification!
This behaves slightly different since it now returns -EINVAL or -ENOSYS, but that is what we want.
You are right, and I will add this to the changelog when I apply.
Sam
Use the function drm_panel_get_modes().
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Alexios Zavras alexios.zavras@intel.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Allison Randal allison@lohutok.net Cc: Sam Ravnborg sam@ravnborg.org Cc: Enrico Weigelt info@metux.net --- drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c index ecef4f5b9f26..0e21252fd1d6 100644 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c @@ -55,7 +55,7 @@ static int mdp4_lvds_connector_get_modes(struct drm_connector *connector) if (panel) { drm_panel_attach(panel, connector);
- ret = panel->funcs->get_modes(panel); + ret = drm_panel_get_modes(panel);
drm_panel_detach(panel); }
On Sun, Aug 04, 2019 at 10:16:27PM +0200, Sam Ravnborg wrote:
Use the function drm_panel_get_modes().
Applied to drm-misc-next.
Sam
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Alexios Zavras alexios.zavras@intel.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Allison Randal allison@lohutok.net Cc: Sam Ravnborg sam@ravnborg.org Cc: Enrico Weigelt info@metux.net
drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c index ecef4f5b9f26..0e21252fd1d6 100644 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c @@ -55,7 +55,7 @@ static int mdp4_lvds_connector_get_modes(struct drm_connector *connector) if (panel) { drm_panel_attach(panel, connector);
ret = panel->funcs->get_modes(panel);
ret = drm_panel_get_modes(panel);drm_panel_detach(panel); }
-- 2.20.1
Use the drm_panel_get_modes() function.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Marek Vasut marex@denx.de Cc: Stefan Agner stefan@agner.ch Cc: Shawn Guo shawnguo@kernel.org Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Pengutronix Kernel Team kernel@pengutronix.de Cc: Fabio Estevam festevam@gmail.com Cc: NXP Linux Team linux-imx@nxp.com Cc: linux-arm-kernel@lists.infradead.org --- drivers/gpu/drm/mxsfb/mxsfb_out.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c b/drivers/gpu/drm/mxsfb/mxsfb_out.c index 231d016c6f47..be36f4d6cc96 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_out.c @@ -30,7 +30,7 @@ static int mxsfb_panel_get_modes(struct drm_connector *connector) drm_connector_to_mxsfb_drm_private(connector);
if (mxsfb->panel) - return mxsfb->panel->funcs->get_modes(mxsfb->panel); + return drm_panel_get_modes(mxsfb->panel);
return 0; }
On 2019-08-04 22:16, Sam Ravnborg wrote:
Use the drm_panel_get_modes() function.
Looks good to me,
Acked-by: Stefan Agner stefan@agner.ch
-- Stefan
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Marek Vasut marex@denx.de Cc: Stefan Agner stefan@agner.ch Cc: Shawn Guo shawnguo@kernel.org Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Pengutronix Kernel Team kernel@pengutronix.de Cc: Fabio Estevam festevam@gmail.com Cc: NXP Linux Team linux-imx@nxp.com Cc: linux-arm-kernel@lists.infradead.org
drivers/gpu/drm/mxsfb/mxsfb_out.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c b/drivers/gpu/drm/mxsfb/mxsfb_out.c index 231d016c6f47..be36f4d6cc96 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_out.c @@ -30,7 +30,7 @@ static int mxsfb_panel_get_modes(struct drm_connector *connector) drm_connector_to_mxsfb_drm_private(connector);
if (mxsfb->panel)
return mxsfb->panel->funcs->get_modes(mxsfb->panel);
return drm_panel_get_modes(mxsfb->panel);return 0;
}
Use the drm_panel_(enable|disable|get_modes) functions.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Benjamin Gaignard benjamin.gaignard@linaro.org Cc: Vincent Abriou vincent.abriou@st.com --- drivers/gpu/drm/sti/sti_dvo.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c index 9e6d5d8b7030..e55870190bf5 100644 --- a/drivers/gpu/drm/sti/sti_dvo.c +++ b/drivers/gpu/drm/sti/sti_dvo.c @@ -221,8 +221,7 @@ static void sti_dvo_disable(struct drm_bridge *bridge)
writel(0x00000000, dvo->regs + DVO_DOF_CFG);
- if (dvo->panel) - dvo->panel->funcs->disable(dvo->panel); + drm_panel_disable(dvo->panel);
/* Disable/unprepare dvo clock */ clk_disable_unprepare(dvo->clk_pix); @@ -262,8 +261,7 @@ static void sti_dvo_pre_enable(struct drm_bridge *bridge) if (clk_prepare_enable(dvo->clk)) DRM_ERROR("Failed to prepare/enable dvo clk\n");
- if (dvo->panel) - dvo->panel->funcs->enable(dvo->panel); + drm_panel_enable(dvo->panel);
/* Set LUT */ writel(config->lowbyte, dvo->regs + DVO_LUT_PROG_LOW); @@ -340,7 +338,7 @@ static int sti_dvo_connector_get_modes(struct drm_connector *connector) struct sti_dvo *dvo = dvo_connector->dvo;
if (dvo->panel) - return dvo->panel->funcs->get_modes(dvo->panel); + return drm_panel_get_modes(dvo->panel);
return 0; }
Le dim. 4 août 2019 à 22:17, Sam Ravnborg sam@ravnborg.org a écrit :
Use the drm_panel_(enable|disable|get_modes) functions.
Applied on drm-misc-next, Thanks.
Benjamin
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Benjamin Gaignard benjamin.gaignard@linaro.org Cc: Vincent Abriou vincent.abriou@st.com
drivers/gpu/drm/sti/sti_dvo.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c index 9e6d5d8b7030..e55870190bf5 100644 --- a/drivers/gpu/drm/sti/sti_dvo.c +++ b/drivers/gpu/drm/sti/sti_dvo.c @@ -221,8 +221,7 @@ static void sti_dvo_disable(struct drm_bridge *bridge)
writel(0x00000000, dvo->regs + DVO_DOF_CFG);
if (dvo->panel)dvo->panel->funcs->disable(dvo->panel);
drm_panel_disable(dvo->panel); /* Disable/unprepare dvo clock */ clk_disable_unprepare(dvo->clk_pix);@@ -262,8 +261,7 @@ static void sti_dvo_pre_enable(struct drm_bridge *bridge) if (clk_prepare_enable(dvo->clk)) DRM_ERROR("Failed to prepare/enable dvo clk\n");
if (dvo->panel)dvo->panel->funcs->enable(dvo->panel);
drm_panel_enable(dvo->panel); /* Set LUT */ writel(config->lowbyte, dvo->regs + DVO_LUT_PROG_LOW);@@ -340,7 +338,7 @@ static int sti_dvo_connector_get_modes(struct drm_connector *connector) struct sti_dvo *dvo = dvo_connector->dvo;
if (dvo->panel)
return dvo->panel->funcs->get_modes(dvo->panel);
return drm_panel_get_modes(dvo->panel); return 0;}
2.20.1
Use the drm_panel_get_modes function.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Jonathan Hunter jonathanh@nvidia.com Cc: linux-tegra@vger.kernel.org --- drivers/gpu/drm/tegra/output.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c index 274cb955e2e1..52b8396ec2dc 100644 --- a/drivers/gpu/drm/tegra/output.c +++ b/drivers/gpu/drm/tegra/output.c @@ -23,7 +23,7 @@ int tegra_output_connector_get_modes(struct drm_connector *connector) * ignore any other means of obtaining a mode. */ if (output->panel) { - err = output->panel->funcs->get_modes(output->panel); + err = drm_panel_get_modes(output->panel); if (err > 0) return err; }
On Sun, Aug 04, 2019 at 10:16:30PM +0200, Sam Ravnborg wrote:
Use the drm_panel_get_modes function.
Applied to drm-misc-next.
Sam
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Jonathan Hunter jonathanh@nvidia.com Cc: linux-tegra@vger.kernel.org
drivers/gpu/drm/tegra/output.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c index 274cb955e2e1..52b8396ec2dc 100644 --- a/drivers/gpu/drm/tegra/output.c +++ b/drivers/gpu/drm/tegra/output.c @@ -23,7 +23,7 @@ int tegra_output_connector_get_modes(struct drm_connector *connector) * ignore any other means of obtaining a mode. */ if (output->panel) {
err = output->panel->funcs->get_modes(output->panel);
err = drm_panel_get_modes(output->panel);-- 2.20.1
To prepare the driver to receive drm_connector only in the get_modes() callback, move bus_flags handling to ili9322_get_modes().
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 34 +++++++++----------- 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9322.c b/drivers/gpu/drm/panel/panel-ilitek-ili9322.c index 53dd1e128795..3c58f63adbf7 100644 --- a/drivers/gpu/drm/panel/panel-ilitek-ili9322.c +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9322.c @@ -349,7 +349,6 @@ static const struct regmap_config ili9322_regmap_config = {
static int ili9322_init(struct drm_panel *panel, struct ili9322 *ili) { - struct drm_connector *connector = panel->connector; u8 reg; int ret; int i; @@ -407,23 +406,11 @@ static int ili9322_init(struct drm_panel *panel, struct ili9322 *ili) * Polarity and inverted color order for RGB input. * None of this applies in the BT.656 mode. */ - if (ili->conf->dclk_active_high) { + reg = 0; + if (ili->conf->dclk_active_high) reg = ILI9322_POL_DCLK; - connector->display_info.bus_flags |= - DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; - } else { - reg = 0; - connector->display_info.bus_flags |= - DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE; - } - if (ili->conf->de_active_high) { + if (ili->conf->de_active_high) reg |= ILI9322_POL_DE; - connector->display_info.bus_flags |= - DRM_BUS_FLAG_DE_HIGH; - } else { - connector->display_info.bus_flags |= - DRM_BUS_FLAG_DE_LOW; - } if (ili->conf->hsync_active_high) reg |= ILI9322_POL_HSYNC; if (ili->conf->vsync_active_high) @@ -659,9 +646,20 @@ static int ili9322_get_modes(struct drm_panel *panel) struct drm_connector *connector = panel->connector; struct ili9322 *ili = panel_to_ili9322(panel); struct drm_display_mode *mode; + struct drm_display_info *info; + + info = &connector->display_info; + info->width_mm = ili->conf->width_mm; + info->height_mm = ili->conf->height_mm; + if (ili->conf->dclk_active_high) + info->bus_flags |= DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; + else + info->bus_flags |= DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE;
- connector->display_info.width_mm = ili->conf->width_mm; - connector->display_info.height_mm = ili->conf->height_mm; + if (ili->conf->de_active_high) + info->bus_flags |= DRM_BUS_FLAG_DE_HIGH; + else + info->bus_flags |= DRM_BUS_FLAG_DE_LOW;
switch (ili->input) { case ILI9322_INPUT_SRGB_DUMMY_320X240:
On Sun, Aug 4, 2019 at 10:17 PM Sam Ravnborg sam@ravnborg.org wrote:
To prepare the driver to receive drm_connector only in the get_modes() callback, move bus_flags handling to ili9322_get_modes().
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org
OK I don't see where this is going but I trust you so: Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
Move inline functions from include/drm/drm_panel.h to drm_panel.c. This is in preparation for follow-up patches that will add extra logic to the functions. As they are no longer static inline, EXPORT them.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_panel.c | 96 +++++++++++++++++++++++++++++++++++ include/drm/drm_panel.h | 99 +++---------------------------------- 2 files changed, 104 insertions(+), 91 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index dbd5b873e8f2..9946b8d9bacc 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -54,6 +54,102 @@ void drm_panel_init(struct drm_panel *panel) } EXPORT_SYMBOL(drm_panel_init);
+/** + * drm_panel_prepare - power on a panel + * @panel: DRM panel + * + * Calling this function will enable power and deassert any reset signals to + * the panel. After this has completed it is possible to communicate with any + * integrated circuitry via a command bus. + * + * Return: 0 on success or a negative error code on failure. + */ +int drm_panel_prepare(struct drm_panel *panel) +{ + if (panel && panel->funcs && panel->funcs->prepare) + return panel->funcs->prepare(panel); + + return panel ? -ENOSYS : -EINVAL; +} +EXPORT_SYMBOL(drm_panel_prepare); + +/** + * drm_panel_enable - enable a panel + * @panel: DRM panel + * + * Calling this function will cause the panel display drivers to be turned on + * and the backlight to be enabled. Content will be visible on screen after + * this call completes. + * + * Return: 0 on success or a negative error code on failure. + */ +int drm_panel_enable(struct drm_panel *panel) +{ + if (panel && panel->funcs && panel->funcs->enable) + return panel->funcs->enable(panel); + + return panel ? -ENOSYS : -EINVAL; +} +EXPORT_SYMBOL(drm_panel_enable); + +/** + * drm_panel_disable - disable a panel + * @panel: DRM panel + * + * This will typically turn off the panel's backlight or disable the display + * drivers. For smart panels it should still be possible to communicate with + * the integrated circuitry via any command bus after this call. + * + * Return: 0 on success or a negative error code on failure. + */ +int drm_panel_disable(struct drm_panel *panel) +{ + if (panel && panel->funcs && panel->funcs->disable) + return panel->funcs->disable(panel); + + return panel ? -ENOSYS : -EINVAL; +} +EXPORT_SYMBOL(drm_panel_disable); + +/** + * drm_panel_unprepare - power off a panel + * @panel: DRM panel + * + * Calling this function will completely power off a panel (assert the panel's + * reset, turn off power supplies, ...). After this function has completed, it + * is usually no longer possible to communicate with the panel until another + * call to drm_panel_prepare(). + * + * Return: 0 on success or a negative error code on failure. + */ +int drm_panel_unprepare(struct drm_panel *panel) +{ + if (panel && panel->funcs && panel->funcs->unprepare) + return panel->funcs->unprepare(panel); + + return panel ? -ENOSYS : -EINVAL; +} +EXPORT_SYMBOL(drm_panel_unprepare); + +/** + * drm_panel_get_modes - probe the available display modes of a panel + * @panel: DRM panel + * + * The modes probed from the panel are automatically added to the connector + * that the panel is attached to. + * + * Return: The number of modes available from the panel on success or a + * negative error code on failure. + */ +int drm_panel_get_modes(struct drm_panel *panel) +{ + if (panel && panel->funcs && panel->funcs->get_modes) + return panel->funcs->get_modes(panel); + + return panel ? -ENOSYS : -EINVAL; +} +EXPORT_SYMBOL(drm_panel_get_modes); + /** * drm_panel_add - add a panel to the global registry * @panel: panel to add diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 26377836141c..053d611656b9 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -97,97 +97,6 @@ struct drm_panel { struct list_head list; };
-/** - * drm_disable_unprepare - power off a panel - * @panel: DRM panel - * - * Calling this function will completely power off a panel (assert the panel's - * reset, turn off power supplies, ...). After this function has completed, it - * is usually no longer possible to communicate with the panel until another - * call to drm_panel_prepare(). - * - * Return: 0 on success or a negative error code on failure. - */ -static inline int drm_panel_unprepare(struct drm_panel *panel) -{ - if (panel && panel->funcs && panel->funcs->unprepare) - return panel->funcs->unprepare(panel); - - return panel ? -ENOSYS : -EINVAL; -} - -/** - * drm_panel_disable - disable a panel - * @panel: DRM panel - * - * This will typically turn off the panel's backlight or disable the display - * drivers. For smart panels it should still be possible to communicate with - * the integrated circuitry via any command bus after this call. - * - * Return: 0 on success or a negative error code on failure. - */ -static inline int drm_panel_disable(struct drm_panel *panel) -{ - if (panel && panel->funcs && panel->funcs->disable) - return panel->funcs->disable(panel); - - return panel ? -ENOSYS : -EINVAL; -} - -/** - * drm_panel_prepare - power on a panel - * @panel: DRM panel - * - * Calling this function will enable power and deassert any reset signals to - * the panel. After this has completed it is possible to communicate with any - * integrated circuitry via a command bus. - * - * Return: 0 on success or a negative error code on failure. - */ -static inline int drm_panel_prepare(struct drm_panel *panel) -{ - if (panel && panel->funcs && panel->funcs->prepare) - return panel->funcs->prepare(panel); - - return panel ? -ENOSYS : -EINVAL; -} - -/** - * drm_panel_enable - enable a panel - * @panel: DRM panel - * - * Calling this function will cause the panel display drivers to be turned on - * and the backlight to be enabled. Content will be visible on screen after - * this call completes. - * - * Return: 0 on success or a negative error code on failure. - */ -static inline int drm_panel_enable(struct drm_panel *panel) -{ - if (panel && panel->funcs && panel->funcs->enable) - return panel->funcs->enable(panel); - - return panel ? -ENOSYS : -EINVAL; -} - -/** - * drm_panel_get_modes - probe the available display modes of a panel - * @panel: DRM panel - * - * The modes probed from the panel are automatically added to the connector - * that the panel is attached to. - * - * Return: The number of modes available from the panel on success or a - * negative error code on failure. - */ -static inline int drm_panel_get_modes(struct drm_panel *panel) -{ - if (panel && panel->funcs && panel->funcs->get_modes) - return panel->funcs->get_modes(panel); - - return panel ? -ENOSYS : -EINVAL; -} - void drm_panel_init(struct drm_panel *panel);
int drm_panel_add(struct drm_panel *panel); @@ -196,6 +105,14 @@ void drm_panel_remove(struct drm_panel *panel); int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector); int drm_panel_detach(struct drm_panel *panel);
+int drm_panel_prepare(struct drm_panel *panel); +int drm_panel_unprepare(struct drm_panel *panel); + +int drm_panel_enable(struct drm_panel *panel); +int drm_panel_disable(struct drm_panel *panel); + +int drm_panel_get_modes(struct drm_panel *panel); + #if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL) struct drm_panel *of_drm_find_panel(const struct device_node *np); #else
Hi Sam,
Thank you for the patch.
On Sun, Aug 04, 2019 at 10:16:32PM +0200, Sam Ravnborg wrote:
Move inline functions from include/drm/drm_panel.h to drm_panel.c. This is in preparation for follow-up patches that will add extra logic to the functions. As they are no longer static inline, EXPORT them.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_panel.c | 96 +++++++++++++++++++++++++++++++++++ include/drm/drm_panel.h | 99 +++---------------------------------- 2 files changed, 104 insertions(+), 91 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index dbd5b873e8f2..9946b8d9bacc 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -54,6 +54,102 @@ void drm_panel_init(struct drm_panel *panel) } EXPORT_SYMBOL(drm_panel_init);
+/**
- drm_panel_prepare - power on a panel
- @panel: DRM panel
- Calling this function will enable power and deassert any reset signals to
- the panel. After this has completed it is possible to communicate with any
- integrated circuitry via a command bus.
- Return: 0 on success or a negative error code on failure.
- */
+int drm_panel_prepare(struct drm_panel *panel) +{
- if (panel && panel->funcs && panel->funcs->prepare)
return panel->funcs->prepare(panel);- return panel ? -ENOSYS : -EINVAL;
+} +EXPORT_SYMBOL(drm_panel_prepare);
+/**
- drm_panel_enable - enable a panel
- @panel: DRM panel
- Calling this function will cause the panel display drivers to be turned on
- and the backlight to be enabled. Content will be visible on screen after
- this call completes.
- Return: 0 on success or a negative error code on failure.
- */
+int drm_panel_enable(struct drm_panel *panel) +{
- if (panel && panel->funcs && panel->funcs->enable)
return panel->funcs->enable(panel);- return panel ? -ENOSYS : -EINVAL;
+} +EXPORT_SYMBOL(drm_panel_enable);
+/**
- drm_panel_disable - disable a panel
- @panel: DRM panel
- This will typically turn off the panel's backlight or disable the display
- drivers. For smart panels it should still be possible to communicate with
- the integrated circuitry via any command bus after this call.
- Return: 0 on success or a negative error code on failure.
- */
+int drm_panel_disable(struct drm_panel *panel) +{
- if (panel && panel->funcs && panel->funcs->disable)
return panel->funcs->disable(panel);- return panel ? -ENOSYS : -EINVAL;
+} +EXPORT_SYMBOL(drm_panel_disable);
+/**
- drm_panel_unprepare - power off a panel
- @panel: DRM panel
- Calling this function will completely power off a panel (assert the panel's
- reset, turn off power supplies, ...). After this function has completed, it
- is usually no longer possible to communicate with the panel until another
- call to drm_panel_prepare().
- Return: 0 on success or a negative error code on failure.
- */
+int drm_panel_unprepare(struct drm_panel *panel) +{
- if (panel && panel->funcs && panel->funcs->unprepare)
return panel->funcs->unprepare(panel);- return panel ? -ENOSYS : -EINVAL;
+} +EXPORT_SYMBOL(drm_panel_unprepare);
+/**
- drm_panel_get_modes - probe the available display modes of a panel
- @panel: DRM panel
- The modes probed from the panel are automatically added to the connector
- that the panel is attached to.
- Return: The number of modes available from the panel on success or a
- negative error code on failure.
- */
+int drm_panel_get_modes(struct drm_panel *panel) +{
- if (panel && panel->funcs && panel->funcs->get_modes)
return panel->funcs->get_modes(panel);- return panel ? -ENOSYS : -EINVAL;
+} +EXPORT_SYMBOL(drm_panel_get_modes);
/**
- drm_panel_add - add a panel to the global registry
- @panel: panel to add
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 26377836141c..053d611656b9 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -97,97 +97,6 @@ struct drm_panel { struct list_head list; };
-/**
- drm_disable_unprepare - power off a panel
- @panel: DRM panel
- Calling this function will completely power off a panel (assert the panel's
- reset, turn off power supplies, ...). After this function has completed, it
- is usually no longer possible to communicate with the panel until another
- call to drm_panel_prepare().
- Return: 0 on success or a negative error code on failure.
- */
-static inline int drm_panel_unprepare(struct drm_panel *panel) -{
- if (panel && panel->funcs && panel->funcs->unprepare)
return panel->funcs->unprepare(panel);- return panel ? -ENOSYS : -EINVAL;
-}
-/**
- drm_panel_disable - disable a panel
- @panel: DRM panel
- This will typically turn off the panel's backlight or disable the display
- drivers. For smart panels it should still be possible to communicate with
- the integrated circuitry via any command bus after this call.
- Return: 0 on success or a negative error code on failure.
- */
-static inline int drm_panel_disable(struct drm_panel *panel) -{
- if (panel && panel->funcs && panel->funcs->disable)
return panel->funcs->disable(panel);- return panel ? -ENOSYS : -EINVAL;
-}
-/**
- drm_panel_prepare - power on a panel
- @panel: DRM panel
- Calling this function will enable power and deassert any reset signals to
- the panel. After this has completed it is possible to communicate with any
- integrated circuitry via a command bus.
- Return: 0 on success or a negative error code on failure.
- */
-static inline int drm_panel_prepare(struct drm_panel *panel) -{
- if (panel && panel->funcs && panel->funcs->prepare)
return panel->funcs->prepare(panel);- return panel ? -ENOSYS : -EINVAL;
-}
-/**
- drm_panel_enable - enable a panel
- @panel: DRM panel
- Calling this function will cause the panel display drivers to be turned on
- and the backlight to be enabled. Content will be visible on screen after
- this call completes.
- Return: 0 on success or a negative error code on failure.
- */
-static inline int drm_panel_enable(struct drm_panel *panel) -{
- if (panel && panel->funcs && panel->funcs->enable)
return panel->funcs->enable(panel);- return panel ? -ENOSYS : -EINVAL;
-}
-/**
- drm_panel_get_modes - probe the available display modes of a panel
- @panel: DRM panel
- The modes probed from the panel are automatically added to the connector
- that the panel is attached to.
- Return: The number of modes available from the panel on success or a
- negative error code on failure.
- */
-static inline int drm_panel_get_modes(struct drm_panel *panel) -{
- if (panel && panel->funcs && panel->funcs->get_modes)
return panel->funcs->get_modes(panel);- return panel ? -ENOSYS : -EINVAL;
-}
void drm_panel_init(struct drm_panel *panel);
int drm_panel_add(struct drm_panel *panel); @@ -196,6 +105,14 @@ void drm_panel_remove(struct drm_panel *panel); int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector); int drm_panel_detach(struct drm_panel *panel);
+int drm_panel_prepare(struct drm_panel *panel); +int drm_panel_unprepare(struct drm_panel *panel);
+int drm_panel_enable(struct drm_panel *panel); +int drm_panel_disable(struct drm_panel *panel);
Nitpicking, I would keep the order of the declarations aligned with the definitions. prepare - enable - disable - unprepare and prepare - unprepare - enable - disable are both fine with me, as long as they're consistent.
Apart from that,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
+int drm_panel_get_modes(struct drm_panel *panel);
#if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL) struct drm_panel *of_drm_find_panel(const struct device_node *np); #else
Inline comments provide better space for additional comments. Comments was slightly edited to follow the normal style, but no change to actual content. Used the opportuniy to change the order in drm_panel_funcs to follow the order they will be used by a panel.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch --- include/drm/drm_panel.h | 82 +++++++++++++++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 16 deletions(-)
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 053d611656b9..5e62deea49ba 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -36,14 +36,6 @@ struct display_timing;
/** * struct drm_panel_funcs - perform operations on a given panel - * @disable: disable panel (turn off back light, etc.) - * @unprepare: turn off panel - * @prepare: turn on panel and perform set up - * @enable: enable panel (turn on back light, etc.) - * @get_modes: add modes to the connector that the panel is attached to and - * return the number of modes added - * @get_timings: copy display timings into the provided array and return - * the number of display timings available * * The .prepare() function is typically called before the display controller * starts to transmit video data. Panel drivers can use this to turn the panel @@ -69,31 +61,89 @@ struct display_timing; * the panel. This is the job of the .unprepare() function. */ struct drm_panel_funcs { - int (*disable)(struct drm_panel *panel); - int (*unprepare)(struct drm_panel *panel); + /** + * @prepare: + * + * Turn on panel and perform set up. + */ int (*prepare)(struct drm_panel *panel); + + /** + * @enable: + * + * Enable panel (turn on back light, etc.). + */ int (*enable)(struct drm_panel *panel); + + /** + * @disable: + * + * Disable panel (turn off back light, etc.). + */ + int (*disable)(struct drm_panel *panel); + + /** + * @unprepare: + * + * Turn off panel. + */ + int (*unprepare)(struct drm_panel *panel); + + /** + * @get_modes: + * + * Add modes to the connector that the panel is attached to and + * return the number of modes added. + */ int (*get_modes)(struct drm_panel *panel); + + /** + * @get_timings: + * + * Copy display timings into the provided array and return + * the number of display timings available. + */ int (*get_timings)(struct drm_panel *panel, unsigned int num_timings, struct display_timing *timings); };
/** * struct drm_panel - DRM panel object - * @drm: DRM device owning the panel - * @connector: DRM connector that the panel is attached to - * @dev: parent device of the panel - * @link: link from panel device (supplier) to DRM device (consumer) - * @funcs: operations that can be performed on the panel - * @list: panel entry in registry */ struct drm_panel { + /** + * @drm: + * + * DRM device owning the panel. + */ struct drm_device *drm; + + /** + * @connector: + * + * DRM connector that the panel is attached to. + */ struct drm_connector *connector; + + /** + * @dev: + * + * Parent device of the panel. + */ struct device *dev;
+ /** + * @funcs: + * + * Operations that can be performed on the panel. + */ const struct drm_panel_funcs *funcs;
+ /** + * @list: + * + * Panel entry in registry. + */ struct list_head list; };
Hi Sam,
Thank you for the patch.
On Sun, Aug 04, 2019 at 10:16:33PM +0200, Sam Ravnborg wrote:
Inline comments provide better space for additional comments. Comments was slightly edited to follow the normal style, but no change to actual content. Used the opportuniy to change the order in drm_panel_funcs to follow the order they will be used by a panel.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
include/drm/drm_panel.h | 82 +++++++++++++++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 16 deletions(-)
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 053d611656b9..5e62deea49ba 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -36,14 +36,6 @@ struct display_timing;
/**
- struct drm_panel_funcs - perform operations on a given panel
- @disable: disable panel (turn off back light, etc.)
- @unprepare: turn off panel
- @prepare: turn on panel and perform set up
- @enable: enable panel (turn on back light, etc.)
- @get_modes: add modes to the connector that the panel is attached to and
- return the number of modes added
- @get_timings: copy display timings into the provided array and return
- the number of display timings available
- The .prepare() function is typically called before the display controller
- starts to transmit video data. Panel drivers can use this to turn the panel
@@ -69,31 +61,89 @@ struct display_timing;
- the panel. This is the job of the .unprepare() function.
*/ struct drm_panel_funcs {
- int (*disable)(struct drm_panel *panel);
- int (*unprepare)(struct drm_panel *panel);
- /**
* @prepare:** Turn on panel and perform set up.*/- /**
* @enable:** Enable panel (turn on back light, etc.).*/- /**
* @disable:** Disable panel (turn off back light, etc.).*/- int (*disable)(struct drm_panel *panel);
- /**
* @unprepare:** Turn off panel.*/- int (*unprepare)(struct drm_panel *panel);
- /**
* @get_modes:** Add modes to the connector that the panel is attached to and* return the number of modes added.*/- /**
* @get_timings:** Copy display timings into the provided array and return* the number of display timings available.*/};
/**
- struct drm_panel - DRM panel object
- @drm: DRM device owning the panel
- @connector: DRM connector that the panel is attached to
- @dev: parent device of the panel
- @link: link from panel device (supplier) to DRM device (consumer)
- @funcs: operations that can be performed on the panel
*/
- @list: panel entry in registry
struct drm_panel {
/**
* @drm:** DRM device owning the panel.*/struct drm_device *drm;
/**
* @connector:** DRM connector that the panel is attached to.*/struct drm_connector *connector;
/**
* @dev:** Parent device of the panel.*/struct device *dev;
/**
* @funcs:** Operations that can be performed on the panel.*/const struct drm_panel_funcs *funcs;
/**
* @list:** Panel entry in registry.*/struct list_head list;
};
There are no errors that can be reported by this function, so drop the return code. Fix the only bridge driver that checked the return result.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Gwan-gyeong Mun gwan-gyeong.mun@intel.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Linus Walleij linus.walleij@linaro.org --- drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 3 +-- drivers/gpu/drm/drm_panel.c | 6 +----- include/drm/drm_panel.h | 2 +- 3 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index f2f7f69d6cc3..22885dceaa17 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1780,8 +1780,7 @@ void analogix_dp_unbind(struct analogix_dp_device *dp) if (dp->plat_data->panel) { if (drm_panel_unprepare(dp->plat_data->panel)) DRM_ERROR("failed to turnoff the panel\n"); - if (drm_panel_detach(dp->plat_data->panel)) - DRM_ERROR("failed to detach the panel\n"); + drm_panel_detach(dp->plat_data->panel); }
drm_dp_aux_unregister(&dp->aux); diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index 9946b8d9bacc..da19d5b4a2f4 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -219,15 +219,11 @@ EXPORT_SYMBOL(drm_panel_attach); * * This function should not be called by the panel device itself. It * is only for the drm device that called drm_panel_attach(). - * - * Return: 0 on success or a negative error code on failure. */ -int drm_panel_detach(struct drm_panel *panel) +void drm_panel_detach(struct drm_panel *panel) { panel->connector = NULL; panel->drm = NULL; - - return 0; } EXPORT_SYMBOL(drm_panel_detach);
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 5e62deea49ba..624bd15ecfab 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -153,7 +153,7 @@ int drm_panel_add(struct drm_panel *panel); void drm_panel_remove(struct drm_panel *panel);
int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector); -int drm_panel_detach(struct drm_panel *panel); +void drm_panel_detach(struct drm_panel *panel);
int drm_panel_prepare(struct drm_panel *panel); int drm_panel_unprepare(struct drm_panel *panel);
Hi Sam,
Thank you for the patch.
On Sun, Aug 04, 2019 at 10:16:34PM +0200, Sam Ravnborg wrote:
There are no errors that can be reported by this function, so drop the return code. Fix the only bridge driver that checked the return result.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Gwan-gyeong Mun gwan-gyeong.mun@intel.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Linus Walleij linus.walleij@linaro.org
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 3 +-- drivers/gpu/drm/drm_panel.c | 6 +----- include/drm/drm_panel.h | 2 +- 3 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index f2f7f69d6cc3..22885dceaa17 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1780,8 +1780,7 @@ void analogix_dp_unbind(struct analogix_dp_device *dp) if (dp->plat_data->panel) { if (drm_panel_unprepare(dp->plat_data->panel)) DRM_ERROR("failed to turnoff the panel\n");
if (drm_panel_detach(dp->plat_data->panel))DRM_ERROR("failed to detach the panel\n");
drm_panel_detach(dp->plat_data->panel);}
drm_dp_aux_unregister(&dp->aux);
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index 9946b8d9bacc..da19d5b4a2f4 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -219,15 +219,11 @@ EXPORT_SYMBOL(drm_panel_attach);
- This function should not be called by the panel device itself. It
- is only for the drm device that called drm_panel_attach().
*/
- Return: 0 on success or a negative error code on failure.
-int drm_panel_detach(struct drm_panel *panel) +void drm_panel_detach(struct drm_panel *panel) { panel->connector = NULL; panel->drm = NULL;
- return 0;
} EXPORT_SYMBOL(drm_panel_detach);
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 5e62deea49ba..624bd15ecfab 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -153,7 +153,7 @@ int drm_panel_add(struct drm_panel *panel); void drm_panel_remove(struct drm_panel *panel);
int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector); -int drm_panel_detach(struct drm_panel *panel); +void drm_panel_detach(struct drm_panel *panel);
int drm_panel_prepare(struct drm_panel *panel); int drm_panel_unprepare(struct drm_panel *panel);
Many panel drivers duplicate logic to prevent prepare to be called for a panel that is already prepared. Likewise for enable.
Implement this logic in drm_panel so the individual drivers no longer needs this. A panel is considered prepared/enabled only if the prepare/enable call succeeds. For disable/unprepare it is unconditionally considered disabled/unprepared.
This allows calls to prepare/enable again, even if there were some issue disabling a regulator or similar during disable/unprepare.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- include/drm/drm_panel.h | 21 ++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index da19d5b4a2f4..0853764040de 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); */ int drm_panel_prepare(struct drm_panel *panel) { - if (panel && panel->funcs && panel->funcs->prepare) - return panel->funcs->prepare(panel); + int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL; + if (!panel) + return -EINVAL; + + if (panel->prepared) + return 0; + + if (panel->funcs && panel->funcs->prepare) + ret = panel->funcs->prepare(panel); + + if (ret >= 0) + panel->prepared = true; + + return ret; } EXPORT_SYMBOL(drm_panel_prepare);
@@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); */ int drm_panel_enable(struct drm_panel *panel) { - if (panel && panel->funcs && panel->funcs->enable) - return panel->funcs->enable(panel); + int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL; + if (!panel) + return -EINVAL; + + if (panel->enabled) + return 0; + + if (panel->funcs && panel->funcs->enable) + ret = panel->funcs->enable(panel); + + if (ret >= 0) + panel->enabled = true; + + return ret; } EXPORT_SYMBOL(drm_panel_enable);
@@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); */ int drm_panel_disable(struct drm_panel *panel) { - if (panel && panel->funcs && panel->funcs->disable) - return panel->funcs->disable(panel); + int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL; + if (!panel) + return -EINVAL; + + if (!panel->enabled) + return 0; + + if (panel->funcs && panel->funcs->disable) + ret = panel->funcs->disable(panel); + + panel->enabled = false; + + return ret; } EXPORT_SYMBOL(drm_panel_disable);
@@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); */ int drm_panel_unprepare(struct drm_panel *panel) { - if (panel && panel->funcs && panel->funcs->unprepare) - return panel->funcs->unprepare(panel); + int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL; + if (!panel) + return -EINVAL; + + if (!panel->prepared) + return 0; + + if (panel->funcs && panel->funcs->unprepare) + ret = panel->funcs->unprepare(panel); + + panel->prepared = false; + + return ret; } EXPORT_SYMBOL(drm_panel_unprepare);
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 624bd15ecfab..7493500fc9bd 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -65,6 +65,9 @@ struct drm_panel_funcs { * @prepare: * * Turn on panel and perform set up. + * When the panel is successfully prepared the prepare() function + * will not be called again until the panel has been unprepared. + * */ int (*prepare)(struct drm_panel *panel);
@@ -72,6 +75,8 @@ struct drm_panel_funcs { * @enable: * * Enable panel (turn on back light, etc.). + * When the panel is successfully enabled the enable() function + * will not be called again until the panel has been disabled. */ int (*enable)(struct drm_panel *panel);
@@ -79,6 +84,7 @@ struct drm_panel_funcs { * @disable: * * Disable panel (turn off back light, etc.). + * If the panel is already disabled the disable() function is not called. */ int (*disable)(struct drm_panel *panel);
@@ -86,6 +92,7 @@ struct drm_panel_funcs { * @unprepare: * * Turn off panel. + * If the panel is already unprepared the unprepare() function is not called. */ int (*unprepare)(struct drm_panel *panel);
@@ -145,6 +152,20 @@ struct drm_panel { * Panel entry in registry. */ struct list_head list; + + /** + * @prepared: + * + * Set to true when the panel is successfully prepared. + */ + bool prepared; + + /** + * @enabled: + * + * Set to true when the panel is successfully enabled. + */ + bool enabled; };
void drm_panel_init(struct drm_panel *panel);
Hi Sam,
Thank you for the patch.
On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote:
Many panel drivers duplicate logic to prevent prepare to be called for a panel that is already prepared. Likewise for enable.
Implement this logic in drm_panel so the individual drivers no longer needs this. A panel is considered prepared/enabled only if the prepare/enable call succeeds. For disable/unprepare it is unconditionally considered disabled/unprepared.
This allows calls to prepare/enable again, even if there were some issue disabling a regulator or similar during disable/unprepare.
Is this the right place to handle this ? Shouldn't the upper layers ensure than enable/disable and prepare/unprepare are correcty balanced, and not called multiple times ? Adding enabled and prepared state to drm_panel not only doesn't align well with atomic state handling, but also would hide issues in upper layers that should really be fixed there.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- include/drm/drm_panel.h | 21 ++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index da19d5b4a2f4..0853764040de 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); */ int drm_panel_prepare(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->prepare)
return panel->funcs->prepare(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (panel->prepared)
return 0;- if (panel->funcs && panel->funcs->prepare)
ret = panel->funcs->prepare(panel);- if (ret >= 0)
panel->prepared = true;- return ret;
} EXPORT_SYMBOL(drm_panel_prepare);
@@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); */ int drm_panel_enable(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->enable)
return panel->funcs->enable(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (panel->enabled)
return 0;- if (panel->funcs && panel->funcs->enable)
ret = panel->funcs->enable(panel);- if (ret >= 0)
panel->enabled = true;- return ret;
} EXPORT_SYMBOL(drm_panel_enable);
@@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); */ int drm_panel_disable(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->disable)
return panel->funcs->disable(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (!panel->enabled)
return 0;- if (panel->funcs && panel->funcs->disable)
ret = panel->funcs->disable(panel);- panel->enabled = false;
- return ret;
} EXPORT_SYMBOL(drm_panel_disable);
@@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); */ int drm_panel_unprepare(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->unprepare)
return panel->funcs->unprepare(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (!panel->prepared)
return 0;- if (panel->funcs && panel->funcs->unprepare)
ret = panel->funcs->unprepare(panel);- panel->prepared = false;
- return ret;
} EXPORT_SYMBOL(drm_panel_unprepare);
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 624bd15ecfab..7493500fc9bd 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -65,6 +65,9 @@ struct drm_panel_funcs { * @prepare: * * Turn on panel and perform set up.
* When the panel is successfully prepared the prepare() function* will not be called again until the panel has been unprepared.*@@ -72,6 +75,8 @@ struct drm_panel_funcs { * @enable: * * Enable panel (turn on back light, etc.).
* When the panel is successfully enabled the enable() function* will not be called again until the panel has been disabled.@@ -79,6 +84,7 @@ struct drm_panel_funcs { * @disable: * * Disable panel (turn off back light, etc.).
* If the panel is already disabled the disable() function is not called.@@ -86,6 +92,7 @@ struct drm_panel_funcs { * @unprepare: * * Turn off panel.
* If the panel is already unprepared the unprepare() function is not called.@@ -145,6 +152,20 @@ struct drm_panel { * Panel entry in registry. */ struct list_head list;
- /**
* @prepared:** Set to true when the panel is successfully prepared.*/- bool prepared;
- /**
* @enabled:** Set to true when the panel is successfully enabled.*/- bool enabled;
};
void drm_panel_init(struct drm_panel *panel);
On 2019/08/05, Laurent Pinchart wrote:
Hi Sam,
Thank you for the patch.
On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote:
Many panel drivers duplicate logic to prevent prepare to be called for a panel that is already prepared. Likewise for enable.
Implement this logic in drm_panel so the individual drivers no longer needs this. A panel is considered prepared/enabled only if the prepare/enable call succeeds. For disable/unprepare it is unconditionally considered disabled/unprepared.
This allows calls to prepare/enable again, even if there were some issue disabling a regulator or similar during disable/unprepare.
Is this the right place to handle this ? Shouldn't the upper layers ensure than enable/disable and prepare/unprepare are correcty balanced, and not called multiple times ? Adding enabled and prepared state to drm_panel not only doesn't align well with atomic state handling, but also would hide issues in upper layers that should really be fixed there.
Fully agreed. Mistakes happen - hiding them, by returning "success" does not sound like a wise approach.
HTH Emil
Hi Laurent.
On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote:
Many panel drivers duplicate logic to prevent prepare to be called for a panel that is already prepared. Likewise for enable.
Implement this logic in drm_panel so the individual drivers no longer needs this. A panel is considered prepared/enabled only if the prepare/enable call succeeds. For disable/unprepare it is unconditionally considered disabled/unprepared.
This allows calls to prepare/enable again, even if there were some issue disabling a regulator or similar during disable/unprepare.
Is this the right place to handle this ? Shouldn't the upper layers ensure than enable/disable and prepare/unprepare are correcty balanced, and not called multiple times ? Adding enabled and prepared state to drm_panel not only doesn't align well with atomic state handling, but also would hide issues in upper layers that should really be fixed there.
The main rationale behind starting on this was that ~15 panel drivers already implements logic to prevent the prepare/enable/disable/unprepare functions to be called out of order. $ cd drivers/gpu/drm/panel/; git grep enabled | grep bool | wc -l
Several of the panel drivers also implements a mipi_dsi_driver.shutdown() or platform_driver.shutdown(). To the best of my knowledge we cannot guarantee that the upper layers have done the proper disable()/unprepare() dance before a shutdown. So the flags exists to allow the driver to unconditionally call disable() / unprepare() in the shutdown methods. Same goes for *_driver.remove()
One improvement could be to detect if the panel is prepare() when upper layers call enable() and warn/error in this situation. With the current implementation this is not checked at all. Likewise for unprepare() (require it was never enabled or disable() was caled first)
I claim the check exists for the benefit of .remove and .shutdown, so we could also check if prepare() or enable() is called twice.
Adding logic to call prepare() automagically would hide probems in upper layers and this was only briefly considered - and discarded as hiding bugs.
So to sum up: - Moving the checks from drivers to the core is a good thing - The core shall check that a panel is prepared when enable is called and error out if not (or warn). - The core shall check that a panel is disabled when unprepare is called and error out if not (or warn). The core shall check if prepare() and enable() is called out of order.
The patch needs to be extended to cover the last three points.
Laurent / Emil / Thierry - agree/comments?
Note: Did a quick round to see if could spot any wrong use of drm_panel_* functions. Most looked good, but then I did not do a throughly check.
bridge/analogix/analogix_dp_core.c looks fishy. Looks like analogix_dp_prepare_panel() is a nop the way it is called. I did not look too much on this, maybe I am wrong.
Sam
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- include/drm/drm_panel.h | 21 ++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index da19d5b4a2f4..0853764040de 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); */ int drm_panel_prepare(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->prepare)
return panel->funcs->prepare(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (panel->prepared)
return 0;- if (panel->funcs && panel->funcs->prepare)
ret = panel->funcs->prepare(panel);- if (ret >= 0)
panel->prepared = true;- return ret;
} EXPORT_SYMBOL(drm_panel_prepare);
@@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); */ int drm_panel_enable(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->enable)
return panel->funcs->enable(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (panel->enabled)
return 0;- if (panel->funcs && panel->funcs->enable)
ret = panel->funcs->enable(panel);- if (ret >= 0)
panel->enabled = true;- return ret;
} EXPORT_SYMBOL(drm_panel_enable);
@@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); */ int drm_panel_disable(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->disable)
return panel->funcs->disable(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (!panel->enabled)
return 0;- if (panel->funcs && panel->funcs->disable)
ret = panel->funcs->disable(panel);- panel->enabled = false;
- return ret;
} EXPORT_SYMBOL(drm_panel_disable);
@@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); */ int drm_panel_unprepare(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->unprepare)
return panel->funcs->unprepare(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (!panel->prepared)
return 0;- if (panel->funcs && panel->funcs->unprepare)
ret = panel->funcs->unprepare(panel);- panel->prepared = false;
- return ret;
} EXPORT_SYMBOL(drm_panel_unprepare);
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 624bd15ecfab..7493500fc9bd 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -65,6 +65,9 @@ struct drm_panel_funcs { * @prepare: * * Turn on panel and perform set up.
* When the panel is successfully prepared the prepare() function* will not be called again until the panel has been unprepared.*@@ -72,6 +75,8 @@ struct drm_panel_funcs { * @enable: * * Enable panel (turn on back light, etc.).
* When the panel is successfully enabled the enable() function* will not be called again until the panel has been disabled.@@ -79,6 +84,7 @@ struct drm_panel_funcs { * @disable: * * Disable panel (turn off back light, etc.).
* If the panel is already disabled the disable() function is not called.@@ -86,6 +92,7 @@ struct drm_panel_funcs { * @unprepare: * * Turn off panel.
* If the panel is already unprepared the unprepare() function is not called.@@ -145,6 +152,20 @@ struct drm_panel { * Panel entry in registry. */ struct list_head list;
- /**
* @prepared:** Set to true when the panel is successfully prepared.*/- bool prepared;
- /**
* @enabled:** Set to true when the panel is successfully enabled.*/- bool enabled;
};
void drm_panel_init(struct drm_panel *panel);
-- Regards,
Laurent Pinchart
Hi Sam,
On Mon, Aug 05, 2019 at 06:51:17PM +0200, Sam Ravnborg wrote:
On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote:
Many panel drivers duplicate logic to prevent prepare to be called for a panel that is already prepared. Likewise for enable.
Implement this logic in drm_panel so the individual drivers no longer needs this. A panel is considered prepared/enabled only if the prepare/enable call succeeds. For disable/unprepare it is unconditionally considered disabled/unprepared.
This allows calls to prepare/enable again, even if there were some issue disabling a regulator or similar during disable/unprepare.
Is this the right place to handle this ? Shouldn't the upper layers ensure than enable/disable and prepare/unprepare are correcty balanced, and not called multiple times ? Adding enabled and prepared state to drm_panel not only doesn't align well with atomic state handling, but also would hide issues in upper layers that should really be fixed there.
The main rationale behind starting on this was that ~15 panel drivers already implements logic to prevent the prepare/enable/disable/unprepare functions to be called out of order. $ cd drivers/gpu/drm/panel/; git grep enabled | grep bool | wc -l
Several of the panel drivers also implements a mipi_dsi_driver.shutdown() or platform_driver.shutdown(). To the best of my knowledge we cannot guarantee that the upper layers have done the proper disable()/unprepare() dance before a shutdown.
If the display controller drivers all behaved correctly, their .shutdown() implementation would disable all the output, and thus disable the panels. I think that's the best way forward, and we should ideally remove .shutdown() from the panel drivers, as otherwise the panel may be disabled before the display driver .shutdown() operation is called, and weird things can then happen.
This being said, guaranteeing proper operation of the display controller drivers isn't easy, so I'm not calling for removing .shutdown() from panel drivers right now, but I think we shouldn't accept that operation in new panel drivers going forward.
So the flags exists to allow the driver to unconditionally call disable() / unprepare() in the shutdown methods. Same goes for *_driver.remove()
I'd rather get rid of the hacks instead of trying to refactor them in generic hack-support helpers ;-) But I get your point, as an interim measure this is probably our best option.
One improvement could be to detect if the panel is prepare() when upper layers call enable() and warn/error in this situation. With the current implementation this is not checked at all. Likewise for unprepare() (require it was never enabled or disable() was caled first)
I claim the check exists for the benefit of .remove and .shutdown, so we could also check if prepare() or enable() is called twice.
Adding logic to call prepare() automagically would hide probems in upper layers and this was only briefly considered - and discarded as hiding bugs.
I agree with you.
So to sum up:
- Moving the checks from drivers to the core is a good thing
- The core shall check that a panel is prepared when enable is called and error out if not (or warn).
- The core shall check that a panel is disabled when unprepare is called and error out if not (or warn). The core shall check if prepare() and enable() is called out of order.
The patch needs to be extended to cover the last three points.
Laurent / Emil / Thierry - agree/comments?
Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Maybe with a note in the Documentation/gpu/todo.rst to remove .shutdown() in panel drivers as a long term goal ?
Note: Did a quick round to see if could spot any wrong use of drm_panel_* functions. Most looked good, but then I did not do a throughly check.
bridge/analogix/analogix_dp_core.c looks fishy. Looks like analogix_dp_prepare_panel() is a nop the way it is called. I did not look too much on this, maybe I am wrong.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- include/drm/drm_panel.h | 21 ++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index da19d5b4a2f4..0853764040de 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); */ int drm_panel_prepare(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->prepare)
return panel->funcs->prepare(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (panel->prepared)
return 0;- if (panel->funcs && panel->funcs->prepare)
ret = panel->funcs->prepare(panel);- if (ret >= 0)
panel->prepared = true;- return ret;
} EXPORT_SYMBOL(drm_panel_prepare);
@@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); */ int drm_panel_enable(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->enable)
return panel->funcs->enable(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (panel->enabled)
return 0;- if (panel->funcs && panel->funcs->enable)
ret = panel->funcs->enable(panel);- if (ret >= 0)
panel->enabled = true;- return ret;
} EXPORT_SYMBOL(drm_panel_enable);
@@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); */ int drm_panel_disable(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->disable)
return panel->funcs->disable(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (!panel->enabled)
return 0;- if (panel->funcs && panel->funcs->disable)
ret = panel->funcs->disable(panel);- panel->enabled = false;
- return ret;
} EXPORT_SYMBOL(drm_panel_disable);
@@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); */ int drm_panel_unprepare(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->unprepare)
return panel->funcs->unprepare(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (!panel->prepared)
return 0;- if (panel->funcs && panel->funcs->unprepare)
ret = panel->funcs->unprepare(panel);- panel->prepared = false;
- return ret;
} EXPORT_SYMBOL(drm_panel_unprepare);
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 624bd15ecfab..7493500fc9bd 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -65,6 +65,9 @@ struct drm_panel_funcs { * @prepare: * * Turn on panel and perform set up.
* When the panel is successfully prepared the prepare() function* will not be called again until the panel has been unprepared.*@@ -72,6 +75,8 @@ struct drm_panel_funcs { * @enable: * * Enable panel (turn on back light, etc.).
* When the panel is successfully enabled the enable() function* will not be called again until the panel has been disabled.@@ -79,6 +84,7 @@ struct drm_panel_funcs { * @disable: * * Disable panel (turn off back light, etc.).
* If the panel is already disabled the disable() function is not called.@@ -86,6 +92,7 @@ struct drm_panel_funcs { * @unprepare: * * Turn off panel.
* If the panel is already unprepared the unprepare() function is not called.@@ -145,6 +152,20 @@ struct drm_panel { * Panel entry in registry. */ struct list_head list;
- /**
* @prepared:** Set to true when the panel is successfully prepared.*/- bool prepared;
- /**
* @enabled:** Set to true when the panel is successfully enabled.*/- bool enabled;
};
void drm_panel_init(struct drm_panel *panel);
On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote:
Many panel drivers duplicate logic to prevent prepare to be called for a panel that is already prepared. Likewise for enable.
Implement this logic in drm_panel so the individual drivers no longer needs this. A panel is considered prepared/enabled only if the prepare/enable call succeeds. For disable/unprepare it is unconditionally considered disabled/unprepared.
Hi Sam, I did a similar thing a few years ago [1]. IIRC it was well received and just needed some nits cleaned up. Unfortunately I lost interest^W^W switched to other things and dropped it. So thanks for picking this back up!
Fast forward to today, I still think it's a good idea but I want to make sure this won't negatively interact with the self refresh helpers. With the helpers in place, it's possible to call disable consecutively (ie: once to enter self refresh and again to actually shut down). I did a quick pass and it looks like this patch might break that behavior, so you might want to take that into account.
Sean
[1]- https://patchwork.freedesktop.org/series/30712/
This allows calls to prepare/enable again, even if there were some issue disabling a regulator or similar during disable/unprepare.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- include/drm/drm_panel.h | 21 ++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index da19d5b4a2f4..0853764040de 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); */ int drm_panel_prepare(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->prepare)
return panel->funcs->prepare(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (panel->prepared)
return 0;- if (panel->funcs && panel->funcs->prepare)
ret = panel->funcs->prepare(panel);- if (ret >= 0)
panel->prepared = true;- return ret;
} EXPORT_SYMBOL(drm_panel_prepare);
@@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); */ int drm_panel_enable(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->enable)
return panel->funcs->enable(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (panel->enabled)
return 0;- if (panel->funcs && panel->funcs->enable)
ret = panel->funcs->enable(panel);- if (ret >= 0)
panel->enabled = true;- return ret;
} EXPORT_SYMBOL(drm_panel_enable);
@@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); */ int drm_panel_disable(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->disable)
return panel->funcs->disable(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (!panel->enabled)
return 0;- if (panel->funcs && panel->funcs->disable)
ret = panel->funcs->disable(panel);- panel->enabled = false;
- return ret;
} EXPORT_SYMBOL(drm_panel_disable);
@@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); */ int drm_panel_unprepare(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->unprepare)
return panel->funcs->unprepare(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (!panel->prepared)
return 0;- if (panel->funcs && panel->funcs->unprepare)
ret = panel->funcs->unprepare(panel);- panel->prepared = false;
- return ret;
} EXPORT_SYMBOL(drm_panel_unprepare);
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 624bd15ecfab..7493500fc9bd 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -65,6 +65,9 @@ struct drm_panel_funcs { * @prepare: * * Turn on panel and perform set up.
* When the panel is successfully prepared the prepare() function* will not be called again until the panel has been unprepared.*@@ -72,6 +75,8 @@ struct drm_panel_funcs { * @enable: * * Enable panel (turn on back light, etc.).
* When the panel is successfully enabled the enable() function* will not be called again until the panel has been disabled.@@ -79,6 +84,7 @@ struct drm_panel_funcs { * @disable: * * Disable panel (turn off back light, etc.).
* If the panel is already disabled the disable() function is not called.@@ -86,6 +92,7 @@ struct drm_panel_funcs { * @unprepare: * * Turn off panel.
* If the panel is already unprepared the unprepare() function is not called.@@ -145,6 +152,20 @@ struct drm_panel { * Panel entry in registry. */ struct list_head list;
- /**
* @prepared:** Set to true when the panel is successfully prepared.*/- bool prepared;
- /**
* @enabled:** Set to true when the panel is successfully enabled.*/- bool enabled;
};
void drm_panel_init(struct drm_panel *panel);
2.20.1
Hi Sean,
On Mon, Aug 05, 2019 at 01:01:20PM -0400, Sean Paul wrote:
On Sun, Aug 04, 2019 at 10:16:35PM +0200, Sam Ravnborg wrote:
Many panel drivers duplicate logic to prevent prepare to be called for a panel that is already prepared. Likewise for enable.
Implement this logic in drm_panel so the individual drivers no longer needs this. A panel is considered prepared/enabled only if the prepare/enable call succeeds. For disable/unprepare it is unconditionally considered disabled/unprepared.
Hi Sam, I did a similar thing a few years ago [1]. IIRC it was well received and just needed some nits cleaned up. Unfortunately I lost interest^W^W switched to other things and dropped it. So thanks for picking this back up!
Fast forward to today, I still think it's a good idea but I want to make sure this won't negatively interact with the self refresh helpers. With the helpers in place, it's possible to call disable consecutively (ie: once to enter self refresh and again to actually shut down). I did a quick pass and it looks like this patch might break that behavior, so you might want to take that into account.
Is this semantics documented somewhere ? The documentation of the panel disable operation is pretty terse, and we need to explicitly state who of the caller and callee needs to track the state.
[1]- https://patchwork.freedesktop.org/series/30712/
This allows calls to prepare/enable again, even if there were some issue disabling a regulator or similar during disable/unprepare.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_panel.c | 66 ++++++++++++++++++++++++++++++------- include/drm/drm_panel.h | 21 ++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index da19d5b4a2f4..0853764040de 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -66,10 +66,21 @@ EXPORT_SYMBOL(drm_panel_init); */ int drm_panel_prepare(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->prepare)
return panel->funcs->prepare(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (panel->prepared)
return 0;- if (panel->funcs && panel->funcs->prepare)
ret = panel->funcs->prepare(panel);- if (ret >= 0)
panel->prepared = true;- return ret;
} EXPORT_SYMBOL(drm_panel_prepare);
@@ -85,10 +96,21 @@ EXPORT_SYMBOL(drm_panel_prepare); */ int drm_panel_enable(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->enable)
return panel->funcs->enable(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (panel->enabled)
return 0;- if (panel->funcs && panel->funcs->enable)
ret = panel->funcs->enable(panel);- if (ret >= 0)
panel->enabled = true;- return ret;
} EXPORT_SYMBOL(drm_panel_enable);
@@ -104,10 +126,20 @@ EXPORT_SYMBOL(drm_panel_enable); */ int drm_panel_disable(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->disable)
return panel->funcs->disable(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (!panel->enabled)
return 0;- if (panel->funcs && panel->funcs->disable)
ret = panel->funcs->disable(panel);- panel->enabled = false;
- return ret;
} EXPORT_SYMBOL(drm_panel_disable);
@@ -124,10 +156,20 @@ EXPORT_SYMBOL(drm_panel_disable); */ int drm_panel_unprepare(struct drm_panel *panel) {
- if (panel && panel->funcs && panel->funcs->unprepare)
return panel->funcs->unprepare(panel);
- int ret = -ENOSYS;
- return panel ? -ENOSYS : -EINVAL;
- if (!panel)
return -EINVAL;- if (!panel->prepared)
return 0;- if (panel->funcs && panel->funcs->unprepare)
ret = panel->funcs->unprepare(panel);- panel->prepared = false;
- return ret;
} EXPORT_SYMBOL(drm_panel_unprepare);
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 624bd15ecfab..7493500fc9bd 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -65,6 +65,9 @@ struct drm_panel_funcs { * @prepare: * * Turn on panel and perform set up.
* When the panel is successfully prepared the prepare() function* will not be called again until the panel has been unprepared.*@@ -72,6 +75,8 @@ struct drm_panel_funcs { * @enable: * * Enable panel (turn on back light, etc.).
* When the panel is successfully enabled the enable() function* will not be called again until the panel has been disabled.@@ -79,6 +84,7 @@ struct drm_panel_funcs { * @disable: * * Disable panel (turn off back light, etc.).
* If the panel is already disabled the disable() function is not called.@@ -86,6 +92,7 @@ struct drm_panel_funcs { * @unprepare: * * Turn off panel.
* If the panel is already unprepared the unprepare() function is not called.@@ -145,6 +152,20 @@ struct drm_panel { * Panel entry in registry. */ struct list_head list;
- /**
* @prepared:** Set to true when the panel is successfully prepared.*/- bool prepared;
- /**
* @enabled:** Set to true when the panel is successfully enabled.*/- bool enabled;
};
void drm_panel_init(struct drm_panel *panel);
Panels often supports backlight as specified in a device tree. Update the drm_panel infrastructure to support this to simplify the drivers.
With this the panel driver just needs to add the following to the probe() function:
err = drm_panel_of_backlight(panel); if (err) return err;
Then drm_panel will handle all the rest.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_panel.c | 41 +++++++++++++++++++++++++++++++++++++ include/drm/drm_panel.h | 23 +++++++++++++++++++++ 2 files changed, 64 insertions(+)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index 0853764040de..d8139674b883 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -21,6 +21,7 @@ * DEALINGS IN THE SOFTWARE. */
+#include <linux/backlight.h> #include <linux/err.h> #include <linux/module.h>
@@ -110,6 +111,7 @@ int drm_panel_enable(struct drm_panel *panel) if (ret >= 0) panel->enabled = true;
+ backlight_enable(panel->backlight); return ret; } EXPORT_SYMBOL(drm_panel_enable); @@ -134,6 +136,8 @@ int drm_panel_disable(struct drm_panel *panel) if (!panel->enabled) return 0;
+ backlight_disable(panel->backlight); + if (panel->funcs && panel->funcs->disable) ret = panel->funcs->disable(panel);
@@ -308,6 +312,43 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np) EXPORT_SYMBOL(of_drm_find_panel); #endif
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE +/** + * drm_panel_of_backlight - use backlight device node for backlight + * @panel: DRM panel + * + * Use this function to enable backlight handling if your panel + * uses device tree and has a backlight handle. + * + * When panel is enabled backlight will be enabled after a + * successfull call to &drm_panel_funcs.enable() + * + * When panel is disabled backlight will be disabled before the + * call to &drm_panel_funcs.disable(). + * + * A typical implementation for a panel driver supporting device tree + * will call this function and then backlight just works. + * + * Return: 0 on success or a negative error code on failure. + */ +int drm_panel_of_backlight(struct drm_panel *panel) +{ + struct backlight_device *backlight; + + if (!panel || !panel->dev) + return -EINVAL; + + backlight = devm_of_find_backlight(panel->dev); + + if (IS_ERR(backlight)) + return PTR_ERR(backlight); + + panel->backlight = backlight; + return 0; +} +EXPORT_SYMBOL(drm_panel_of_backlight); +#endif + MODULE_AUTHOR("Thierry Reding treding@nvidia.com"); MODULE_DESCRIPTION("DRM panel infrastructure"); MODULE_LICENSE("GPL and additional rights"); diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 7493500fc9bd..31349c2393b7 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -28,6 +28,7 @@ #include <linux/errno.h> #include <linux/list.h>
+struct backlight_device; struct device_node; struct drm_connector; struct drm_device; @@ -59,6 +60,10 @@ struct display_timing; * * To save power when no video data is transmitted, a driver can power down * the panel. This is the job of the .unprepare() function. + * + * Backlight can be handled automatically if configured using + * drm_panel_of_backlight(). Then the driver do not need to implement the + * functionality to enable/disable backlight. */ struct drm_panel_funcs { /** @@ -139,6 +144,15 @@ struct drm_panel { */ struct device *dev;
+ /** + * @backlight: + * + * Backlight device, used to turn on backlight after + * the call to enable(), and to turn off + * backlight before call to disable(). + */ + struct backlight_device *backlight; + /** * @funcs: * @@ -193,4 +207,13 @@ static inline struct drm_panel *of_drm_find_panel(const struct device_node *np) } #endif
+#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) && defined(CONFIG_DRM_PANEL) +int drm_panel_of_backlight(struct drm_panel *panel); +#else +static inline int drm_panel_of_backlight(struct drm_panel *panel) +{ + return -EINVAL; +} +#endif + #endif
Hi Sam,
Thank you for the patch.
On Sun, Aug 04, 2019 at 10:16:36PM +0200, Sam Ravnborg wrote:
Panels often supports backlight as specified in a device tree. Update the drm_panel infrastructure to support this to simplify the drivers.
With this the panel driver just needs to add the following to the probe() function:
err = drm_panel_of_backlight(panel); if (err) return err;Then drm_panel will handle all the rest.
Do you have an example on how this will simplify drivers ? How many existing panel drivers would benefit from this, and do you plan to convert them ?
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_panel.c | 41 +++++++++++++++++++++++++++++++++++++ include/drm/drm_panel.h | 23 +++++++++++++++++++++ 2 files changed, 64 insertions(+)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index 0853764040de..d8139674b883 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -21,6 +21,7 @@
- DEALINGS IN THE SOFTWARE.
*/
+#include <linux/backlight.h> #include <linux/err.h> #include <linux/module.h>
@@ -110,6 +111,7 @@ int drm_panel_enable(struct drm_panel *panel) if (ret >= 0) panel->enabled = true;
- backlight_enable(panel->backlight); return ret;
} EXPORT_SYMBOL(drm_panel_enable); @@ -134,6 +136,8 @@ int drm_panel_disable(struct drm_panel *panel) if (!panel->enabled) return 0;
- backlight_disable(panel->backlight);
- if (panel->funcs && panel->funcs->disable) ret = panel->funcs->disable(panel);
@@ -308,6 +312,43 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np) EXPORT_SYMBOL(of_drm_find_panel); #endif
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE +/**
- drm_panel_of_backlight - use backlight device node for backlight
- @panel: DRM panel
- Use this function to enable backlight handling if your panel
- uses device tree and has a backlight handle.
- When panel is enabled backlight will be enabled after a
- successfull call to &drm_panel_funcs.enable()
- When panel is disabled backlight will be disabled before the
- call to &drm_panel_funcs.disable().
- A typical implementation for a panel driver supporting device tree
- will call this function and then backlight just works.
- Return: 0 on success or a negative error code on failure.
- */
+int drm_panel_of_backlight(struct drm_panel *panel) +{
- struct backlight_device *backlight;
- if (!panel || !panel->dev)
return -EINVAL;- backlight = devm_of_find_backlight(panel->dev);
- if (IS_ERR(backlight))
return PTR_ERR(backlight);- panel->backlight = backlight;
- return 0;
+} +EXPORT_SYMBOL(drm_panel_of_backlight); +#endif
MODULE_AUTHOR("Thierry Reding treding@nvidia.com"); MODULE_DESCRIPTION("DRM panel infrastructure"); MODULE_LICENSE("GPL and additional rights"); diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 7493500fc9bd..31349c2393b7 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -28,6 +28,7 @@ #include <linux/errno.h> #include <linux/list.h>
+struct backlight_device; struct device_node; struct drm_connector; struct drm_device; @@ -59,6 +60,10 @@ struct display_timing;
- To save power when no video data is transmitted, a driver can power down
- the panel. This is the job of the .unprepare() function.
- Backlight can be handled automatically if configured using
- drm_panel_of_backlight(). Then the driver do not need to implement the
*/
- functionality to enable/disable backlight.
struct drm_panel_funcs { /** @@ -139,6 +144,15 @@ struct drm_panel { */ struct device *dev;
- /**
* @backlight:** Backlight device, used to turn on backlight after* the call to enable(), and to turn off* backlight before call to disable().*/- struct backlight_device *backlight;
- /**
- @funcs:
@@ -193,4 +207,13 @@ static inline struct drm_panel *of_drm_find_panel(const struct device_node *np) } #endif
+#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) && defined(CONFIG_DRM_PANEL) +int drm_panel_of_backlight(struct drm_panel *panel);
I would expect callers of this function to depend on (or select) CONFIG_DRM_PANEL, so I would drop it from here.
+#else +static inline int drm_panel_of_backlight(struct drm_panel *panel) +{
- return -EINVAL;
Maybe -ENOSYS ?
+} +#endif
#endif
Use drm_panel infrastrucute: - drm_panel has guards for calling disable/enable twice - drm_panel has backlight support
To use the drm_panel infrastructure use the drm_panel_* variants for prepare/enable/disable/unprepare.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/panel/panel-simple.c | 73 +++++----------------------- 1 file changed, 11 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index bff7578f84dd..c7eed34f2c9c 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -21,7 +21,6 @@ * DEALINGS IN THE SOFTWARE. */
-#include <linux/backlight.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> #include <linux/module.h> @@ -98,13 +97,10 @@ struct panel_desc {
struct panel_simple { struct drm_panel base; - bool prepared; - bool enabled; bool no_hpd;
const struct panel_desc *desc;
- struct backlight_device *backlight; struct regulator *supply; struct i2c_adapter *ddc;
@@ -232,20 +228,9 @@ static int panel_simple_disable(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel);
- if (!p->enabled) - return 0; - - if (p->backlight) { - p->backlight->props.power = FB_BLANK_POWERDOWN; - p->backlight->props.state |= BL_CORE_FBBLANK; - backlight_update_status(p->backlight); - } - if (p->desc->delay.disable) msleep(p->desc->delay.disable);
- p->enabled = false; - return 0; }
@@ -253,9 +238,6 @@ static int panel_simple_unprepare(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel);
- if (!p->prepared) - return 0; - gpiod_set_value_cansleep(p->enable_gpio, 0);
regulator_disable(p->supply); @@ -263,8 +245,6 @@ static int panel_simple_unprepare(struct drm_panel *panel) if (p->desc->delay.unprepare) msleep(p->desc->delay.unprepare);
- p->prepared = false; - return 0; }
@@ -274,9 +254,6 @@ static int panel_simple_prepare(struct drm_panel *panel) unsigned int delay; int err;
- if (p->prepared) - return 0; - err = regulator_enable(p->supply); if (err < 0) { dev_err(panel->dev, "failed to enable supply: %d\n", err); @@ -291,8 +268,6 @@ static int panel_simple_prepare(struct drm_panel *panel) if (delay) msleep(delay);
- p->prepared = true; - return 0; }
@@ -300,20 +275,9 @@ static int panel_simple_enable(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel);
- if (p->enabled) - return 0; - if (p->desc->delay.enable) msleep(p->desc->delay.enable);
- if (p->backlight) { - p->backlight->props.state &= ~BL_CORE_FBBLANK; - p->backlight->props.power = FB_BLANK_UNBLANK; - backlight_update_status(p->backlight); - } - - p->enabled = true; - return 0; }
@@ -413,7 +377,7 @@ static void panel_simple_parse_panel_timing_node(struct device *dev,
static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) { - struct device_node *backlight, *ddc; + struct device_node *ddc; struct panel_simple *panel; struct display_timing dt; int err; @@ -422,8 +386,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) if (!panel) return -ENOMEM;
- panel->enabled = false; - panel->prepared = false; panel->desc = desc;
panel->no_hpd = of_property_read_bool(dev->of_node, "no-hpd"); @@ -441,24 +403,13 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) return err; }
- backlight = of_parse_phandle(dev->of_node, "backlight", 0); - if (backlight) { - panel->backlight = of_find_backlight_by_node(backlight); - of_node_put(backlight); - - if (!panel->backlight) - return -EPROBE_DEFER; - } - ddc = of_parse_phandle(dev->of_node, "ddc-i2c-bus", 0); if (ddc) { panel->ddc = of_find_i2c_adapter_by_node(ddc); of_node_put(ddc);
- if (!panel->ddc) { - err = -EPROBE_DEFER; - goto free_backlight; - } + if (!panel->ddc) + return -EPROBE_DEFER; }
if (!of_get_display_timing(dev->of_node, "panel-timing", &dt)) @@ -468,6 +419,10 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) panel->base.dev = dev; panel->base.funcs = &panel_simple_funcs;
+ err = drm_panel_of_backlight(&panel->base); + if (err) + goto free_ddc; + err = drm_panel_add(&panel->base); if (err < 0) goto free_ddc; @@ -479,9 +434,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) free_ddc: if (panel->ddc) put_device(&panel->ddc->dev); -free_backlight: - if (panel->backlight) - put_device(&panel->backlight->dev);
return err; } @@ -492,15 +444,12 @@ static int panel_simple_remove(struct device *dev)
drm_panel_remove(&panel->base);
- panel_simple_disable(&panel->base); - panel_simple_unprepare(&panel->base); + drm_panel_disable(&panel->base); + drm_panel_unprepare(&panel->base);
if (panel->ddc) put_device(&panel->ddc->dev);
- if (panel->backlight) - put_device(&panel->backlight->dev); - return 0; }
@@ -508,8 +457,8 @@ static void panel_simple_shutdown(struct device *dev) { struct panel_simple *panel = dev_get_drvdata(dev);
- panel_simple_disable(&panel->base); - panel_simple_unprepare(&panel->base); + drm_panel_disable(&panel->base); + drm_panel_unprepare(&panel->base); }
static const struct drm_display_mode ampire_am_480272h3tmqw_t01h_mode = {
Hi Sam,
Thank you for the patch.
On Sun, Aug 04, 2019 at 10:16:37PM +0200, Sam Ravnborg wrote:
Use drm_panel infrastrucute:
- drm_panel has guards for calling disable/enable twice
As stated in the review of the corresponding patch, I think those checks should be dropped, but not moved to the panel core.
- drm_panel has backlight support
This answers my first question in the review of 15/16 :-)
To use the drm_panel infrastructure use the drm_panel_* variants for prepare/enable/disable/unprepare.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org
The change looks good overall,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
but this is pending an agreement on what to do with the multiple prepare/enable guards.
drivers/gpu/drm/panel/panel-simple.c | 73 +++++----------------------- 1 file changed, 11 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index bff7578f84dd..c7eed34f2c9c 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -21,7 +21,6 @@
- DEALINGS IN THE SOFTWARE.
*/
-#include <linux/backlight.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> #include <linux/module.h> @@ -98,13 +97,10 @@ struct panel_desc {
struct panel_simple { struct drm_panel base;
bool prepared;
bool enabled; bool no_hpd;
const struct panel_desc *desc;
struct backlight_device *backlight; struct regulator *supply; struct i2c_adapter *ddc;
@@ -232,20 +228,9 @@ static int panel_simple_disable(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel);
if (!p->enabled)
return 0;if (p->backlight) {
p->backlight->props.power = FB_BLANK_POWERDOWN;p->backlight->props.state |= BL_CORE_FBBLANK;backlight_update_status(p->backlight);}
if (p->desc->delay.disable) msleep(p->desc->delay.disable);
p->enabled = false;
return 0;
}
@@ -253,9 +238,6 @@ static int panel_simple_unprepare(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel);
if (!p->prepared)
return 0;gpiod_set_value_cansleep(p->enable_gpio, 0);
regulator_disable(p->supply);
@@ -263,8 +245,6 @@ static int panel_simple_unprepare(struct drm_panel *panel) if (p->desc->delay.unprepare) msleep(p->desc->delay.unprepare);
- p->prepared = false;
- return 0;
}
@@ -274,9 +254,6 @@ static int panel_simple_prepare(struct drm_panel *panel) unsigned int delay; int err;
- if (p->prepared)
return 0;- err = regulator_enable(p->supply); if (err < 0) { dev_err(panel->dev, "failed to enable supply: %d\n", err);
@@ -291,8 +268,6 @@ static int panel_simple_prepare(struct drm_panel *panel) if (delay) msleep(delay);
- p->prepared = true;
- return 0;
}
@@ -300,20 +275,9 @@ static int panel_simple_enable(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel);
if (p->enabled)
return 0;if (p->desc->delay.enable) msleep(p->desc->delay.enable);
if (p->backlight) {
p->backlight->props.state &= ~BL_CORE_FBBLANK;p->backlight->props.power = FB_BLANK_UNBLANK;backlight_update_status(p->backlight);}
p->enabled = true;
return 0;
}
@@ -413,7 +377,7 @@ static void panel_simple_parse_panel_timing_node(struct device *dev,
static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) {
- struct device_node *backlight, *ddc;
- struct device_node *ddc; struct panel_simple *panel; struct display_timing dt; int err;
@@ -422,8 +386,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) if (!panel) return -ENOMEM;
panel->enabled = false;
panel->prepared = false; panel->desc = desc;
panel->no_hpd = of_property_read_bool(dev->of_node, "no-hpd");
@@ -441,24 +403,13 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) return err; }
backlight = of_parse_phandle(dev->of_node, "backlight", 0);
if (backlight) {
panel->backlight = of_find_backlight_by_node(backlight);of_node_put(backlight);if (!panel->backlight)return -EPROBE_DEFER;}
ddc = of_parse_phandle(dev->of_node, "ddc-i2c-bus", 0); if (ddc) { panel->ddc = of_find_i2c_adapter_by_node(ddc); of_node_put(ddc);
if (!panel->ddc) {err = -EPROBE_DEFER;goto free_backlight;}
if (!panel->ddc)return -EPROBE_DEFER;}
if (!of_get_display_timing(dev->of_node, "panel-timing", &dt))
@@ -468,6 +419,10 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) panel->base.dev = dev; panel->base.funcs = &panel_simple_funcs;
- err = drm_panel_of_backlight(&panel->base);
- if (err)
goto free_ddc;- err = drm_panel_add(&panel->base); if (err < 0) goto free_ddc;
@@ -479,9 +434,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) free_ddc: if (panel->ddc) put_device(&panel->ddc->dev); -free_backlight:
- if (panel->backlight)
put_device(&panel->backlight->dev);
This looks weird, where
return err; } @@ -492,15 +444,12 @@ static int panel_simple_remove(struct device *dev)
drm_panel_remove(&panel->base);
- panel_simple_disable(&panel->base);
- panel_simple_unprepare(&panel->base);
drm_panel_disable(&panel->base);
drm_panel_unprepare(&panel->base);
if (panel->ddc) put_device(&panel->ddc->dev);
- if (panel->backlight)
put_device(&panel->backlight->dev);- return 0;
}
@@ -508,8 +457,8 @@ static void panel_simple_shutdown(struct device *dev) { struct panel_simple *panel = dev_get_drvdata(dev);
- panel_simple_disable(&panel->base);
- panel_simple_unprepare(&panel->base);
- drm_panel_disable(&panel->base);
- drm_panel_unprepare(&panel->base);
}
static const struct drm_display_mode ampire_am_480272h3tmqw_t01h_mode = {
On 2019/08/04, Sam Ravnborg wrote:
The first 9 patches replaces direct use of the drm_panel function pointers with their drm_panel_* counterparts. The function pointers are only supposed to be used by the drm_panel infrastructure and direct use are discouraged.
ili9322 is updated to handle bus_flags in get_modes like everyone else. This is in preparation for a later patch series where controller becomes an arugument to get_modes() and not like today where drm_panel is attached to a controller.
The remaining patches move functionality to the drm_panel core that today are repeated in many drivers. As preparation for this the inline functions are moved to drm_panel.c and kernel-doc is made inline. panel-simple is updated to benefit from the additional infrastructure and is an example for the simplifications that can be done.
The patchset has been tested on my embedded target, and build tested.
Feedback welcome!
The "fix opencoded" patches are all independent and can be applied out of order. They were kept here to keep panel related patches in one series.
Sam
Thanks for working on this Sam.
Patches 1-13 are: Reviewed-by: Emil Velikov emil.velikov@collabora.com
-Emil
dri-devel@lists.freedesktop.org
-
Benjamin Gaignard -
Emil Velikov -
Laurent Pinchart -
Linus Walleij -
Philipp Zabel -
Sam Ravnborg -
Sean Paul -
Stefan Agner