[PATCH v1 0/16] drm: panel related updates

[PATCH v1 0/16] drm: panel related updates

[Sam Ravnborg]

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(-)



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 01/16] drm/bridge: tc358767: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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);
 
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 01/16] drm/bridge: tc358767: fix opencoded use of drm_panel_*

[Philipp Zabel]

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 01/16] drm/bridge: tc358767: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 02/16] drm/exynos: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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;
 }
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 02/16] drm/exynos: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 03/16] drm/exynos: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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;
 }
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 03/16] drm/exynos: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 04/16] drm/imx: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 04/16] drm/imx: fix opencoded use of drm_panel_*

[Philipp Zabel]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 05/16] drm/fsl-dcu: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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,
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 05/16] drm/fsl-dcu: fix opencoded use of drm_panel_*

[Stefan Agner]

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,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 05/16] drm/fsl-dcu: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 06/16] drm/msm: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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);
 	}
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 06/16] drm/msm: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 07/16] drm/mxsfb: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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;
 }
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 07/16] drm/mxsfb: fix opencoded use of drm_panel_*

[Stefan Agner]

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;
>  }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 08/16] drm/sti: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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;
 }
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 08/16] drm/sti: fix opencoded use of drm_panel_*

[Benjamin Gaignard]

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
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 09/16] drm/tegra: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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;
 	}
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 09/16] drm/tegra: fix opencoded use of drm_panel_*

[Sam Ravnborg]

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);
>  		if (err > 0)
>  			return err;
>  	}
> -- 
> 2.20.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 10/16] drm/panel: ili9322: move bus_flags to get_modes()

[Sam Ravnborg]

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:
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 10/16] drm/panel: ili9322: move bus_flags to get_modes()

[Linus Walleij]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 11/16] drm/panel: move drm_panel functions to .c file

[Sam Ravnborg]

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
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 11/16] drm/panel: move drm_panel functions to .c file

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 12/16] drm/panel: use inline comments in drm_panel.h

[Sam Ravnborg]

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;
 };
 
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 12/16] drm/panel: use inline comments in drm_panel.h

[Laurent Pinchart]

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.
> +	 */
>  	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;
>  };
>  

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 13/16] drm/panel: drop return code from drm_panel_detach()

[Sam Ravnborg]

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);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 13/16] drm/panel: drop return code from drm_panel_detach()

[Laurent Pinchart]

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);

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 14/16] drm/panel: call prepare/enable only once

[Sam Ravnborg]

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);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 14/16] drm/panel: call prepare/enable only once

[Laurent Pinchart]

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.
> +	 *
>  	 */
>  	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);

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 14/16] drm/panel: call prepare/enable only once

[Emil Velikov]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 14/16] drm/panel: call prepare/enable only once

[Sam Ravnborg]

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.
> > +	 *
> >  	 */
> >  	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);
> 
> -- 
> Regards,
> 
> Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 14/16] drm/panel: call prepare/enable only once

[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.
> > > +	 *
> > >  	 */
> > >  	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);

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 14/16] drm/panel: call prepare/enable only once

[Sean Paul]

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.
> +	 *
>  	 */
>  	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);
> -- 
> 2.20.1
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 14/16] drm/panel: call prepare/enable only once

[Laurent Pinchart]

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.
> > +	 *
> >  	 */
> >  	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);

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 15/16] drm/panel: add backlight support

[Sam Ravnborg]

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
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 15/16] drm/panel: add backlight support

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 16/16] drm/panel: simple: use drm_panel infrastructure

[Sam Ravnborg]

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 = {
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 16/16] drm/panel: simple: use drm_panel infrastructure

[Laurent Pinchart]

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 = {

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 0/16] drm: panel related updates

[Emil Velikov]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel