[PATCH v3 0/21] drm/bridge: support chained bridges + panel updates

[PATCH v3 0/21] drm/bridge: support chained bridges + panel updates

[Sam Ravnborg]

This patch-set aims to make connector creation optional
and prepare the bridge drivers for use in a chained setup.

The objective is that all bridge drivers shall support a chained setup
connector creation is moved to the display drivers.
This is just one step on this path.

Third iteration of this patchset covers several drivers,
and a few panel adjustments.

The general approach for the bridge drivers:
- Introduce drm_panel_brigde
- Introduce bridge operations
- Make connector creation optional

v3:
  - Rebase on top of drm-misc-next
  - Address kbuild test robot feedback
 
v2:
  - Updated bus_flags for boe,hv070wsa-100
  - Collected r-b, but did not apply patches yet
  - On the panel side the panel-simple driver gained a default
    connector type for all the dumb panels that do not
    include so in their description.
    With this change panels always provide a connector type,
    and we have the potential to drop most uses of
    devm_drm_panel_bridge_add_typed().
  - Added conversion of a few more bridge drivers

Patches can build but no run-time testing.
So both test and review feedback appreciated!

	Sam

Sam Ravnborg (21):
      drm/panel: add connector type to boe,hv070wsa-100 panel
      drm/panel: panel-simple: add default connector_type
      drm/bridge: tc358764: drop drm_connector_(un)register
      drm/bridge: tc358764: add drm_panel_bridge support
      drm/bridge: tc358764: make connector creation optional
      drm/bridge: tc358767: add drm_panel_bridge support
      drm/bridge: tc358767: add detect bridge operation
      drm/bridge: tc358767: add get_edid bride operation
      drm/bridge: tc358767: make connector creation optional
      drm/bridge: ti-tpd12s015: make connector creation optional
      drm/bridge: parade-ps8622: add drm_panel_bridge support
      drm/bridge: parade-ps8622: make connector creation optional
      drm/bridge: megachips: add helper to create connector
      drm/bridge: megachips: get drm_device from bridge
      drm/bridge: megachips: enable detect bridge operation
      drm/bridge: megachips: add get_edid bridge operation
      drm/bridge: megachips: make connector creation optional
      drm/bridge: nxp-ptn3460: add drm_panel_bridge support
      drm/bridge: nxp-ptn3460: add get_modes bridge operation
      drm/bridge: nxp-ptn3460: make connector creation optional
      drm/bridge: ti-sn65dsi86: add drm_panel_bridge support

 .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c   |  92 +++++++++++-------
 drivers/gpu/drm/bridge/nxp-ptn3460.c               | 107 +++++++++------------
 drivers/gpu/drm/bridge/parade-ps8622.c             |  54 +++--------
 drivers/gpu/drm/bridge/tc358764.c                  |  66 +++----------
 drivers/gpu/drm/bridge/tc358767.c                  |  98 +++++++++++--------
 drivers/gpu/drm/bridge/ti-sn65dsi86.c              |  27 +++---
 drivers/gpu/drm/bridge/ti-tpd12s015.c              |   3 -
 drivers/gpu/drm/panel/panel-simple.c               |  13 ++-
 8 files changed, 216 insertions(+), 244 deletions(-)


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 01/21] drm/panel: add connector type to boe, hv070wsa-100 panel

[Sam Ravnborg]

The boe,hv070wsa-100 panel is a LVDS panel.
Fix connector type to reflect this.

With this change users of this panel do not have to specify the
connector type.

v3:
  - Drop PIXDATA bus_flag, not relevant

v2:
  - Add .bus_format (Laurent)
  - Add .bus_flags

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/panel/panel-simple.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 4f0ec5e5b0aa..7b5d212215e0 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1188,6 +1188,9 @@ static const struct panel_desc boe_hv070wsa = {
 		.width = 154,
 		.height = 90,
 	},
+	.bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH,
+	.connector_type = DRM_MODE_CONNECTOR_LVDS,
 };
 
 static const struct drm_display_mode boe_nv101wxmn51_modes[] = {
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 01/21] drm/panel: add connector type to boe,hv070wsa-100 panel

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:23:57PM +0200, Sam Ravnborg wrote:
> The boe,hv070wsa-100 panel is a LVDS panel.
> Fix connector type to reflect this.
> 
> With this change users of this panel do not have to specify the
> connector type.
> 
> v3:
>   - Drop PIXDATA bus_flag, not relevant
> 
> v2:
>   - Add .bus_format (Laurent)
>   - Add .bus_flags
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 4f0ec5e5b0aa..7b5d212215e0 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1188,6 +1188,9 @@ static const struct panel_desc boe_hv070wsa = {
>  		.width = 154,
>  		.height = 90,
>  	},
> +	.bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH,
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,

While at it, should you set .bpc to 8 ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  };
>  
>  static const struct drm_display_mode boe_nv101wxmn51_modes[] = {

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 02/21] drm/panel: panel-simple: add default connector_type

[Sam Ravnborg]

All panels shall report a connector type.
panel-simple has a lot of panels with no connector_type,
and for these fall back to DPI as the default.

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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 7b5d212215e0..b3ec965721b0 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -500,6 +500,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	struct panel_simple *panel;
 	struct display_timing dt;
 	struct device_node *ddc;
+	int connector_type;
 	int err;
 
 	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
@@ -566,8 +567,13 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 			desc->bpc != 8);
 	}
 
-	drm_panel_init(&panel->base, dev, &panel_simple_funcs,
-		       desc->connector_type);
+	/* Default DRM_MODE_CONNECTOR_DPI if no connector_type is set */
+	if (desc->connector_type != 0)
+		connector_type = desc->connector_type;
+	else
+		connector_type = DRM_MODE_CONNECTOR_DPI;
+
+	drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
 
 	err = drm_panel_of_backlight(&panel->base);
 	if (err)
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/21] drm/panel: panel-simple: add default connector_type

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:23:58PM +0200, Sam Ravnborg wrote:
> All panels shall report a connector type.
> panel-simple has a lot of panels with no connector_type,
> and for these fall back to DPI as the default.
> 
> 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 | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 7b5d212215e0..b3ec965721b0 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -500,6 +500,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  	struct panel_simple *panel;
>  	struct display_timing dt;
>  	struct device_node *ddc;
> +	int connector_type;
>  	int err;
>  
>  	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
> @@ -566,8 +567,13 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  			desc->bpc != 8);
>  	}
>  
> -	drm_panel_init(&panel->base, dev, &panel_simple_funcs,
> -		       desc->connector_type);
> +	/* Default DRM_MODE_CONNECTOR_DPI if no connector_type is set */
> +	if (desc->connector_type != 0)
> +		connector_type = desc->connector_type;
> +	else
> +		connector_type = DRM_MODE_CONNECTOR_DPI;

This avoids a WARN_ON(), which is good, but should we add a dev_warn()
to get this fixed ?

> +
> +	drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
>  
>  	err = drm_panel_of_backlight(&panel->base);
>  	if (err)

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/21] drm/panel: panel-simple: add default connector_type

[Sam Ravnborg]

Hi Laurent.
On Sat, Jul 11, 2020 at 01:11:24AM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> Thank you for the patch.
> 
> On Fri, Jul 03, 2020 at 09:23:58PM +0200, Sam Ravnborg wrote:
> > All panels shall report a connector type.
> > panel-simple has a lot of panels with no connector_type,
> > and for these fall back to DPI as the default.
> > 
> > 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 | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index 7b5d212215e0..b3ec965721b0 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -500,6 +500,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
> >  	struct panel_simple *panel;
> >  	struct display_timing dt;
> >  	struct device_node *ddc;
> > +	int connector_type;
> >  	int err;
> >  
> >  	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
> > @@ -566,8 +567,13 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
> >  			desc->bpc != 8);
> >  	}
> >  
> > -	drm_panel_init(&panel->base, dev, &panel_simple_funcs,
> > -		       desc->connector_type);
> > +	/* Default DRM_MODE_CONNECTOR_DPI if no connector_type is set */
> > +	if (desc->connector_type != 0)
> > +		connector_type = desc->connector_type;
> > +	else
> > +		connector_type = DRM_MODE_CONNECTOR_DPI;
> 
> This avoids a WARN_ON(), which is good, but should we add a dev_warn()
> to get this fixed ?

We need a proper check for all relevant properties for DPI
and the other connectors. Like you already did for LVDS.

The idea is we shall introduce the checks in one series,
so users when they fix the warnings they are all good.

And then we will have to catch less during review which is good.

It is on my TODO list, but wanted to have some other stuff done
first.

So in other words, good to go with this patch or do we need the proper
checks in place first?

	Sam

> 
> > +
> > +	drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
> >  
> >  	err = drm_panel_of_backlight(&panel->base);
> >  	if (err)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/panel: panel-simple: validate panel description

[Sam Ravnborg]

Warn is we detect a panel with missing descriptions.
This is inpsired by a similar patch by Laurent that introduced checks
for LVDS panels - this extends the checks to the reminaing type of
connectors.

This is known to fail for some of the existing panels but added
despite this as we need help from people using the panels to
add the missing info.
The checks are not complete but will catch the most common mistakes.
The checks at the same time serves as documentation for the minimum
required description for a panel.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---

This is my attempt on the validation described in the previous mail.
The assignment of default connector_type will then be a follow-up patch
to this.

	Sam

 drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 2aff93accad5..025a7ccdfcb3 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -549,8 +549,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 			panel_simple_parse_panel_timing_node(dev, panel, &dt);
 	}
 
-	if (desc->connector_type == DRM_MODE_CONNECTOR_LVDS) {
-		/* Catch common mistakes for LVDS panels. */
+	/* Catch common mistakes for panels. */
+	switch (desc->connector_type) {
+	case 0:
+		WARN(desc->connector_type == 0, "specify missing connector_type\n");
+		break;
+	case DRM_MODE_CONNECTOR_LVDS:
 		WARN_ON(desc->bus_flags &
 			~(DRM_BUS_FLAG_DE_LOW |
 			  DRM_BUS_FLAG_DE_HIGH |
@@ -564,6 +568,30 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 		WARN_ON((desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG ||
 			 desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA) &&
 			desc->bpc != 8);
+		break;
+	case DRM_MODE_CONNECTOR_eDP:
+		WARN_ON(desc->bus_format == 0);
+		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
+		break;
+	case DRM_MODE_CONNECTOR_DSI:
+		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
+		break;
+	case DRM_MODE_CONNECTOR_DPI:
+		WARN_ON(desc->bus_flags &
+			~(DRM_BUS_FLAG_DE_LOW |
+			  DRM_BUS_FLAG_DE_HIGH |
+			  DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
+			  DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
+			  DRM_BUS_FLAG_DATA_MSB_TO_LSB |
+			  DRM_BUS_FLAG_DATA_LSB_TO_MSB |
+			  DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE |
+			  DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE));
+		WARN_ON(desc->bus_format == 0);
+		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
+		break;
+	default:
+		WARN(true, "panel has unknown connector_type: %d\n", desc->connector_type);
+		break;
 	}
 
 	drm_panel_init(&panel->base, dev, &panel_simple_funcs,
-- 
2.25.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panel: panel-simple: validate panel description

[Laurent Pinchart]

Hi Sam,

(CC'ing Daniel)

Thank you for the patch.

On Sat, Jul 11, 2020 at 11:47:26AM +0200, Sam Ravnborg wrote:
> Warn is we detect a panel with missing descriptions.

s/is/if/

> This is inpsired by a similar patch by Laurent that introduced checks
> for LVDS panels - this extends the checks to the reminaing type of

s/reminaing type/remaining types/

> connectors.
> 
> This is known to fail for some of the existing panels but added
> despite this as we need help from people using the panels to
> add the missing info.
> The checks are not complete but will catch the most common mistakes.
> The checks at the same time serves as documentation for the minimum
> required description for a panel.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
> 
> This is my attempt on the validation described in the previous mail.
> The assignment of default connector_type will then be a follow-up patch
> to this.
> 
> 	Sam
> 
>  drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 2aff93accad5..025a7ccdfcb3 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -549,8 +549,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  			panel_simple_parse_panel_timing_node(dev, panel, &dt);
>  	}
>  
> -	if (desc->connector_type == DRM_MODE_CONNECTOR_LVDS) {
> -		/* Catch common mistakes for LVDS panels. */
> +	/* Catch common mistakes for panels. */
> +	switch (desc->connector_type) {
> +	case 0:
> +		WARN(desc->connector_type == 0, "specify missing connector_type\n");
> +		break;
> +	case DRM_MODE_CONNECTOR_LVDS:
>  		WARN_ON(desc->bus_flags &
>  			~(DRM_BUS_FLAG_DE_LOW |
>  			  DRM_BUS_FLAG_DE_HIGH |
> @@ -564,6 +568,30 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  		WARN_ON((desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG ||
>  			 desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA) &&
>  			desc->bpc != 8);
> +		break;
> +	case DRM_MODE_CONNECTOR_eDP:
> +		WARN_ON(desc->bus_format == 0);
> +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> +		break;
> +	case DRM_MODE_CONNECTOR_DSI:
> +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> +		break;
> +	case DRM_MODE_CONNECTOR_DPI:
> +		WARN_ON(desc->bus_flags &
> +			~(DRM_BUS_FLAG_DE_LOW |
> +			  DRM_BUS_FLAG_DE_HIGH |
> +			  DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
> +			  DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> +			  DRM_BUS_FLAG_DATA_MSB_TO_LSB |
> +			  DRM_BUS_FLAG_DATA_LSB_TO_MSB |
> +			  DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE |
> +			  DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE));
> +		WARN_ON(desc->bus_format == 0);
> +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> +		break;
> +	default:
> +		WARN(true, "panel has unknown connector_type: %d\n", desc->connector_type);
> +		break;
>  	}

The checks look sane to me. For LVDS we've added the WARN_ON after
checking all LVDS panels [1], so the warning will only get displayed for
new panel drivers. For other types of panel, this will cause lots of
WARN_ON to trigger. On one hand it gets the issues noticed, which should
help fixing them, but on the other hand it will also scare lots of users
and developers. I'm not sure if we should downgrade that to a dev_warn()
for some time until we get at least the majority of the issues fixed.
Daniel, any opinion ?

[1] Actually not quite, I've just sent "[PATCH] drm: panel: simple: Fix
bpc for LG LB070WV8 panel" to fix one bpc issue.

>  	drm_panel_init(&panel->base, dev, &panel_simple_funcs,

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panel: panel-simple: validate panel description

[Sam Ravnborg]

Hi Laurent.

On Sun, Jul 12, 2020 at 01:56:16AM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> (CC'ing Daniel)
> 
> Thank you for the patch.
> 
> On Sat, Jul 11, 2020 at 11:47:26AM +0200, Sam Ravnborg wrote:
> > Warn is we detect a panel with missing descriptions.
> 
> s/is/if/
> 
> > This is inpsired by a similar patch by Laurent that introduced checks
> > for LVDS panels - this extends the checks to the reminaing type of
> 
> s/reminaing type/remaining types/
> 
> > connectors.
> > 
> > This is known to fail for some of the existing panels but added
> > despite this as we need help from people using the panels to
> > add the missing info.
> > The checks are not complete but will catch the most common mistakes.
> > The checks at the same time serves as documentation for the minimum
> > required description for a panel.
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > ---
> > 
> > This is my attempt on the validation described in the previous mail.
> > The assignment of default connector_type will then be a follow-up patch
> > to this.
> > 
> > 	Sam
> > 
> >  drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > index 2aff93accad5..025a7ccdfcb3 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -549,8 +549,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
> >  			panel_simple_parse_panel_timing_node(dev, panel, &dt);
> >  	}
> >  
> > -	if (desc->connector_type == DRM_MODE_CONNECTOR_LVDS) {
> > -		/* Catch common mistakes for LVDS panels. */
> > +	/* Catch common mistakes for panels. */
> > +	switch (desc->connector_type) {
> > +	case 0:
> > +		WARN(desc->connector_type == 0, "specify missing connector_type\n");
> > +		break;
> > +	case DRM_MODE_CONNECTOR_LVDS:
> >  		WARN_ON(desc->bus_flags &
> >  			~(DRM_BUS_FLAG_DE_LOW |
> >  			  DRM_BUS_FLAG_DE_HIGH |
> > @@ -564,6 +568,30 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
> >  		WARN_ON((desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG ||
> >  			 desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA) &&
> >  			desc->bpc != 8);
> > +		break;
> > +	case DRM_MODE_CONNECTOR_eDP:
> > +		WARN_ON(desc->bus_format == 0);
> > +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> > +		break;
> > +	case DRM_MODE_CONNECTOR_DSI:
> > +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> > +		break;
> > +	case DRM_MODE_CONNECTOR_DPI:
> > +		WARN_ON(desc->bus_flags &
> > +			~(DRM_BUS_FLAG_DE_LOW |
> > +			  DRM_BUS_FLAG_DE_HIGH |
> > +			  DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
> > +			  DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> > +			  DRM_BUS_FLAG_DATA_MSB_TO_LSB |
> > +			  DRM_BUS_FLAG_DATA_LSB_TO_MSB |
> > +			  DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE |
> > +			  DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE));
> > +		WARN_ON(desc->bus_format == 0);
> > +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> > +		break;
> > +	default:
> > +		WARN(true, "panel has unknown connector_type: %d\n", desc->connector_type);
> > +		break;
> >  	}
> 
> The checks look sane to me. For LVDS we've added the WARN_ON after
> checking all LVDS panels [1], so the warning will only get displayed for
> new panel drivers. For other types of panel, this will cause lots of
> WARN_ON to trigger. On one hand it gets the issues noticed, which should
> help fixing them, but on the other hand it will also scare lots of users
> and developers. I'm not sure if we should downgrade that to a dev_warn()
> for some time until we get at least the majority of the issues fixed.
> Daniel, any opinion ?

We should be noisy, but not too noisy.
dev_warn() is fine, maybe only dev_info()?

Unless I get other feedback I will convert to dev_info() in the
next version of the patch.
Fixing all panels before we add the checks, is not feasible.
Access to datasheet is not easy/possible for many panels.

	Sam


> 
> [1] Actually not quite, I've just sent "[PATCH] drm: panel: simple: Fix
> bpc for LG LB070WV8 panel" to fix one bpc issue.
> 
> >  	drm_panel_init(&panel->base, dev, &panel_simple_funcs,
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panel: panel-simple: validate panel description

[Laurent Pinchart]

Hi Sam,

On Sun, Jul 12, 2020 at 12:58:19PM +0200, Sam Ravnborg wrote:
> On Sun, Jul 12, 2020 at 01:56:16AM +0300, Laurent Pinchart wrote:
> > Hi Sam,
> > 
> > (CC'ing Daniel)
> > 
> > Thank you for the patch.
> > 
> > On Sat, Jul 11, 2020 at 11:47:26AM +0200, Sam Ravnborg wrote:
> > > Warn is we detect a panel with missing descriptions.
> > 
> > s/is/if/
> > 
> > > This is inpsired by a similar patch by Laurent that introduced checks
> > > for LVDS panels - this extends the checks to the reminaing type of
> > 
> > s/reminaing type/remaining types/
> > 
> > > connectors.
> > > 
> > > This is known to fail for some of the existing panels but added
> > > despite this as we need help from people using the panels to
> > > add the missing info.
> > > The checks are not complete but will catch the most common mistakes.
> > > The checks at the same time serves as documentation for the minimum
> > > required description for a panel.
> > > 
> > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Thierry Reding <thierry.reding@gmail.com>
> > > Cc: Sam Ravnborg <sam@ravnborg.org>
> > > ---
> > > 
> > > This is my attempt on the validation described in the previous mail.
> > > The assignment of default connector_type will then be a follow-up patch
> > > to this.
> > > 
> > > 	Sam
> > > 
> > >  drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++--
> > >  1 file changed, 30 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index 2aff93accad5..025a7ccdfcb3 100644
> > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > @@ -549,8 +549,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
> > >  			panel_simple_parse_panel_timing_node(dev, panel, &dt);
> > >  	}
> > >  
> > > -	if (desc->connector_type == DRM_MODE_CONNECTOR_LVDS) {
> > > -		/* Catch common mistakes for LVDS panels. */
> > > +	/* Catch common mistakes for panels. */
> > > +	switch (desc->connector_type) {
> > > +	case 0:
> > > +		WARN(desc->connector_type == 0, "specify missing connector_type\n");
> > > +		break;
> > > +	case DRM_MODE_CONNECTOR_LVDS:
> > >  		WARN_ON(desc->bus_flags &
> > >  			~(DRM_BUS_FLAG_DE_LOW |
> > >  			  DRM_BUS_FLAG_DE_HIGH |
> > > @@ -564,6 +568,30 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
> > >  		WARN_ON((desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG ||
> > >  			 desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA) &&
> > >  			desc->bpc != 8);
> > > +		break;
> > > +	case DRM_MODE_CONNECTOR_eDP:
> > > +		WARN_ON(desc->bus_format == 0);
> > > +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> > > +		break;
> > > +	case DRM_MODE_CONNECTOR_DSI:
> > > +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> > > +		break;
> > > +	case DRM_MODE_CONNECTOR_DPI:
> > > +		WARN_ON(desc->bus_flags &
> > > +			~(DRM_BUS_FLAG_DE_LOW |
> > > +			  DRM_BUS_FLAG_DE_HIGH |
> > > +			  DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
> > > +			  DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> > > +			  DRM_BUS_FLAG_DATA_MSB_TO_LSB |
> > > +			  DRM_BUS_FLAG_DATA_LSB_TO_MSB |
> > > +			  DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE |
> > > +			  DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE));
> > > +		WARN_ON(desc->bus_format == 0);
> > > +		WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> > > +		break;
> > > +	default:
> > > +		WARN(true, "panel has unknown connector_type: %d\n", desc->connector_type);
> > > +		break;
> > >  	}
> > 
> > The checks look sane to me. For LVDS we've added the WARN_ON after
> > checking all LVDS panels [1], so the warning will only get displayed for
> > new panel drivers. For other types of panel, this will cause lots of
> > WARN_ON to trigger. On one hand it gets the issues noticed, which should
> > help fixing them, but on the other hand it will also scare lots of users
> > and developers. I'm not sure if we should downgrade that to a dev_warn()
> > for some time until we get at least the majority of the issues fixed.
> > Daniel, any opinion ?
> 
> We should be noisy, but not too noisy.
> dev_warn() is fine, maybe only dev_info()?
> 
> Unless I get other feedback I will convert to dev_info() in the
> next version of the patch.
> Fixing all panels before we add the checks, is not feasible.
> Access to datasheet is not easy/possible for many panels.

And that's the whole point of adding the warning, trying to get the
people who have access to the panels to help fixing the problem.

I think a dev_warn() would be a good compromise, the missing info can
have adverse consequences, so a warning seems appropriate.

> > [1] Actually not quite, I've just sent "[PATCH] drm: panel: simple: Fix
> > bpc for LG LB070WV8 panel" to fix one bpc issue.
> > 
> > >  	drm_panel_init(&panel->base, dev, &panel_simple_funcs,

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 03/21] drm/bridge: tc358764: drop drm_connector_(un)register

[Sam Ravnborg]

Drop drm_connector handling that is not needed:

- drm_dev_register() in the display controller driver takes
  care of registering connectors.
  So the call to drm_connector_register() call is not needed in the bridge
  driver.

- Use of drm_connector_unregister() is only required for drivers that
  explicit have called drm_dev_register.

- The reference counting using drm_connector_put() is likewise not needed.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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/tc358764.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
index 5ac1430fab04..a277739fab58 100644
--- a/drivers/gpu/drm/bridge/tc358764.c
+++ b/drivers/gpu/drm/bridge/tc358764.c
@@ -375,7 +375,6 @@ static int tc358764_attach(struct drm_bridge *bridge,
 	drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
 	drm_panel_attach(ctx->panel, &ctx->connector);
 	ctx->connector.funcs->reset(&ctx->connector);
-	drm_connector_register(&ctx->connector);
 
 	return 0;
 }
@@ -384,10 +383,8 @@ static void tc358764_detach(struct drm_bridge *bridge)
 {
 	struct tc358764 *ctx = bridge_to_tc358764(bridge);
 
-	drm_connector_unregister(&ctx->connector);
 	drm_panel_detach(ctx->panel);
 	ctx->panel = NULL;
-	drm_connector_put(&ctx->connector);
 }
 
 static const struct drm_bridge_funcs tc358764_bridge_funcs = {
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 04/21] drm/bridge: tc358764: add drm_panel_bridge support

[Sam Ravnborg]

Prepare the bridge driver for use in a chained setup by
replacing direct use of drm_panel with drm_panel_bridge support.

The bridge panel will use the connector type reported by the panel,
where the connector for this driver hardcode DRM_MODE_CONNECTOR_LVDS.

v2:
  - Use PTR_ERR_OR_ZERO() (kbuild test robot)

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: kbuild test robot <lkp@intel.com>
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/tc358764.c | 57 ++++++++-----------------------
 1 file changed, 14 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
index a277739fab58..e1f052a4f53b 100644
--- a/drivers/gpu/drm/bridge/tc358764.c
+++ b/drivers/gpu/drm/bridge/tc358764.c
@@ -156,7 +156,7 @@ struct tc358764 {
 	struct drm_connector connector;
 	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
 	struct gpio_desc *gpio_reset;
-	struct drm_panel *panel;
+	struct drm_bridge *panel_bridge;
 	int error;
 };
 
@@ -282,7 +282,7 @@ static int tc358764_get_modes(struct drm_connector *connector)
 {
 	struct tc358764 *ctx = connector_to_tc358764(connector);
 
-	return drm_panel_get_modes(ctx->panel, connector);
+	return drm_bridge_get_modes(ctx->panel_bridge, connector);
 }
 
 static const
@@ -298,23 +298,11 @@ static const struct drm_connector_funcs tc358764_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static void tc358764_disable(struct drm_bridge *bridge)
-{
-	struct tc358764 *ctx = bridge_to_tc358764(bridge);
-	int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
-
-	if (ret < 0)
-		dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
-}
-
 static void tc358764_post_disable(struct drm_bridge *bridge)
 {
 	struct tc358764 *ctx = bridge_to_tc358764(bridge);
 	int ret;
 
-	ret = drm_panel_unprepare(ctx->panel);
-	if (ret < 0)
-		dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
 	tc358764_reset(ctx);
 	usleep_range(10000, 15000);
 	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
@@ -335,18 +323,6 @@ static void tc358764_pre_enable(struct drm_bridge *bridge)
 	ret = tc358764_init(ctx);
 	if (ret < 0)
 		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
-	ret = drm_panel_prepare(ctx->panel);
-	if (ret < 0)
-		dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
-}
-
-static void tc358764_enable(struct drm_bridge *bridge)
-{
-	struct tc358764 *ctx = bridge_to_tc358764(bridge);
-	int ret = drm_panel_enable(ctx->panel);
-
-	if (ret < 0)
-		dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
 }
 
 static int tc358764_attach(struct drm_bridge *bridge,
@@ -356,6 +332,11 @@ static int tc358764_attach(struct drm_bridge *bridge,
 	struct drm_device *drm = bridge->dev;
 	int ret;
 
+	ret = drm_bridge_attach(bridge->encoder, ctx->panel_bridge,
+				bridge, flags);
+	if (ret < 0)
+		return ret;
+
 	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
 		DRM_ERROR("Fix bridge driver to make connector optional!");
 		return -EINVAL;
@@ -373,32 +354,21 @@ static int tc358764_attach(struct drm_bridge *bridge,
 	drm_connector_helper_add(&ctx->connector,
 				 &tc358764_connector_helper_funcs);
 	drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
-	drm_panel_attach(ctx->panel, &ctx->connector);
 	ctx->connector.funcs->reset(&ctx->connector);
 
 	return 0;
 }
 
-static void tc358764_detach(struct drm_bridge *bridge)
-{
-	struct tc358764 *ctx = bridge_to_tc358764(bridge);
-
-	drm_panel_detach(ctx->panel);
-	ctx->panel = NULL;
-}
-
 static const struct drm_bridge_funcs tc358764_bridge_funcs = {
-	.disable = tc358764_disable,
 	.post_disable = tc358764_post_disable,
-	.enable = tc358764_enable,
 	.pre_enable = tc358764_pre_enable,
 	.attach = tc358764_attach,
-	.detach = tc358764_detach,
 };
 
 static int tc358764_parse_dt(struct tc358764 *ctx)
 {
 	struct device *dev = ctx->dev;
+	struct drm_panel *panel;
 	int ret;
 
 	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
@@ -407,12 +377,13 @@ static int tc358764_parse_dt(struct tc358764 *ctx)
 		return PTR_ERR(ctx->gpio_reset);
 	}
 
-	ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
-					  NULL);
-	if (ret && ret != -EPROBE_DEFER)
-		dev_err(dev, "cannot find panel (%d)\n", ret);
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
+	if (ret)
+		return ret;
 
-	return ret;
+	ctx->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+
+	return PTR_ERR_OR_ZERO(ctx->panel_bridge);
 }
 
 static int tc358764_configure_regulators(struct tc358764 *ctx)
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 05/21] drm/bridge: tc358764: make connector creation optional

[Sam Ravnborg]

Make the connector creation optional to enable usage of the
tc358764 bridge with the DRM bridge connector helper.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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/tc358764.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
index e1f052a4f53b..4477224e5079 100644
--- a/drivers/gpu/drm/bridge/tc358764.c
+++ b/drivers/gpu/drm/bridge/tc358764.c
@@ -337,10 +337,8 @@ static int tc358764_attach(struct drm_bridge *bridge,
 	if (ret < 0)
 		return ret;
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
+	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+		return 0;
 
 	ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
 	ret = drm_connector_init(drm, &ctx->connector,
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 06/21] drm/bridge: tc358767: add drm_panel_bridge support

[Sam Ravnborg]

Prepare the bridge driver for use in a chained setup by
replacing direct use of drm_panel with drm_panel_bridge support.

The bridge driver assume the panel is optional.
The relevant tests are migrated over to check for the
pnale bridge to keep the same functionality.

Note: the bridge panel will use the connector type from the panel.

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 | 57 +++++++++++++++----------------
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index c2777b226c75..08d483664258 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -244,8 +244,8 @@ struct tc_data {
 	struct drm_dp_aux	aux;
 
 	struct drm_bridge	bridge;
+	struct drm_bridge	*panel_bridge;
 	struct drm_connector	connector;
-	struct drm_panel	*panel;
 
 	/* link settings */
 	struct tc_edp_link	link;
@@ -1236,13 +1236,6 @@ static int tc_stream_disable(struct tc_data *tc)
 	return 0;
 }
 
-static void tc_bridge_pre_enable(struct drm_bridge *bridge)
-{
-	struct tc_data *tc = bridge_to_tc(bridge);
-
-	drm_panel_prepare(tc->panel);
-}
-
 static void tc_bridge_enable(struct drm_bridge *bridge)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
@@ -1266,8 +1259,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 		tc_main_link_disable(tc);
 		return;
 	}
-
-	drm_panel_enable(tc->panel);
 }
 
 static void tc_bridge_disable(struct drm_bridge *bridge)
@@ -1275,8 +1266,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
 
-	drm_panel_disable(tc->panel);
-
 	ret = tc_stream_disable(tc);
 	if (ret < 0)
 		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
@@ -1286,13 +1275,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
 		dev_err(tc->dev, "main link disable error: %d\n", ret);
 }
 
-static void tc_bridge_post_disable(struct drm_bridge *bridge)
-{
-	struct tc_data *tc = bridge_to_tc(bridge);
-
-	drm_panel_unprepare(tc->panel);
-}
-
 static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
 				 const struct drm_display_mode *mode,
 				 struct drm_display_mode *adj)
@@ -1348,9 +1330,11 @@ static int tc_connector_get_modes(struct drm_connector *connector)
 		return 0;
 	}
 
-	count = drm_panel_get_modes(tc->panel, connector);
-	if (count > 0)
-		return count;
+	if (tc->panel_bridge) {
+		count = drm_bridge_get_modes(tc->panel_bridge, connector);
+		if (count > 0)
+			return count;
+	}
 
 	edid = drm_get_edid(connector, &tc->aux.ddc);
 
@@ -1378,7 +1362,7 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
 	int ret;
 
 	if (tc->hpd_pin < 0) {
-		if (tc->panel)
+		if (tc->panel_bridge)
 			return connector_status_connected;
 		else
 			return connector_status_unknown;
@@ -1413,6 +1397,13 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
 	struct drm_device *drm = bridge->dev;
 	int ret;
 
+	if (tc->panel_bridge) {
+		ret = drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
+					&tc->bridge, flags);
+		if (ret < 0)
+			return ret;
+	}
+
 	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
 		DRM_ERROR("Fix bridge driver to make connector optional!");
 		return -EINVAL;
@@ -1421,7 +1412,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
 	/* Create DP/eDP connector */
 	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
 	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
-				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
+				 tc->panel_bridge ? DRM_MODE_CONNECTOR_eDP :
 				 DRM_MODE_CONNECTOR_DisplayPort);
 	if (ret)
 		return ret;
@@ -1435,9 +1426,6 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
 					       DRM_CONNECTOR_POLL_DISCONNECT;
 	}
 
-	if (tc->panel)
-		drm_panel_attach(tc->panel, &tc->connector);
-
 	drm_display_info_set_bus_formats(&tc->connector.display_info,
 					 &bus_format, 1);
 	tc->connector.display_info.bus_flags =
@@ -1453,10 +1441,8 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.attach = tc_bridge_attach,
 	.mode_valid = tc_mode_valid,
 	.mode_set = tc_bridge_mode_set,
-	.pre_enable = tc_bridge_pre_enable,
 	.enable = tc_bridge_enable,
 	.disable = tc_bridge_disable,
-	.post_disable = tc_bridge_post_disable,
 	.mode_fixup = tc_bridge_mode_fixup,
 };
 
@@ -1547,6 +1533,7 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
 static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
+	struct drm_panel *panel;
 	struct tc_data *tc;
 	int ret;
 
@@ -1557,10 +1544,20 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	tc->dev = dev;
 
 	/* port@2 is the output port */
-	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &tc->panel, NULL);
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &panel, NULL);
 	if (ret && ret != -ENODEV)
 		return ret;
 
+	if (panel) {
+		struct drm_bridge *pbridge;
+
+		pbridge = devm_drm_panel_bridge_add(dev, panel);
+		if (IS_ERR(pbridge))
+			return PTR_ERR(pbridge);
+
+		tc->panel_bridge = pbridge;
+	}
+
 	/* Shut down GPIO is optional */
 	tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
 	if (IS_ERR(tc->sd_gpio))
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 06/21] drm/bridge: tc358767: add drm_panel_bridge support

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:02PM +0200, Sam Ravnborg wrote:
> Prepare the bridge driver for use in a chained setup by
> replacing direct use of drm_panel with drm_panel_bridge support.
> 
> The bridge driver assume the panel is optional.
> The relevant tests are migrated over to check for the
> pnale bridge to keep the same functionality.

s/pnale/panel/

> Note: the bridge panel will use the connector type from the panel.
> 
> 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 | 57 +++++++++++++++----------------
>  1 file changed, 27 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index c2777b226c75..08d483664258 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -244,8 +244,8 @@ struct tc_data {
>  	struct drm_dp_aux	aux;
>  
>  	struct drm_bridge	bridge;
> +	struct drm_bridge	*panel_bridge;
>  	struct drm_connector	connector;
> -	struct drm_panel	*panel;
>  
>  	/* link settings */
>  	struct tc_edp_link	link;
> @@ -1236,13 +1236,6 @@ static int tc_stream_disable(struct tc_data *tc)
>  	return 0;
>  }
>  
> -static void tc_bridge_pre_enable(struct drm_bridge *bridge)
> -{
> -	struct tc_data *tc = bridge_to_tc(bridge);
> -
> -	drm_panel_prepare(tc->panel);
> -}
> -
>  static void tc_bridge_enable(struct drm_bridge *bridge)
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
> @@ -1266,8 +1259,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>  		tc_main_link_disable(tc);
>  		return;
>  	}
> -
> -	drm_panel_enable(tc->panel);
>  }
>  
>  static void tc_bridge_disable(struct drm_bridge *bridge)
> @@ -1275,8 +1266,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
>  
> -	drm_panel_disable(tc->panel);
> -
>  	ret = tc_stream_disable(tc);
>  	if (ret < 0)
>  		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
> @@ -1286,13 +1275,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
>  		dev_err(tc->dev, "main link disable error: %d\n", ret);
>  }
>  
> -static void tc_bridge_post_disable(struct drm_bridge *bridge)
> -{
> -	struct tc_data *tc = bridge_to_tc(bridge);
> -
> -	drm_panel_unprepare(tc->panel);
> -}
> -
>  static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
>  				 const struct drm_display_mode *mode,
>  				 struct drm_display_mode *adj)
> @@ -1348,9 +1330,11 @@ static int tc_connector_get_modes(struct drm_connector *connector)
>  		return 0;
>  	}
>  
> -	count = drm_panel_get_modes(tc->panel, connector);
> -	if (count > 0)
> -		return count;
> +	if (tc->panel_bridge) {
> +		count = drm_bridge_get_modes(tc->panel_bridge, connector);
> +		if (count > 0)
> +			return count;
> +	}
>  
>  	edid = drm_get_edid(connector, &tc->aux.ddc);
>  
> @@ -1378,7 +1362,7 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
>  	int ret;
>  
>  	if (tc->hpd_pin < 0) {
> -		if (tc->panel)
> +		if (tc->panel_bridge)
>  			return connector_status_connected;
>  		else
>  			return connector_status_unknown;
> @@ -1413,6 +1397,13 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
>  	struct drm_device *drm = bridge->dev;
>  	int ret;
>  
> +	if (tc->panel_bridge) {
> +		ret = drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
> +					&tc->bridge, flags);
> +		if (ret < 0)
> +			return ret;
> +	}

With this both this driver and the panel bridge driver will create a
connector. The simplest way to handle that is probably to pass
flags & ~DRM_BRIDGE_ATTACH_NO_CONNECTOR to drm_bridge_attach(). It's a
bit of a hack, but should go away once all users are converted to
!DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>  		DRM_ERROR("Fix bridge driver to make connector optional!");
>  		return -EINVAL;
> @@ -1421,7 +1412,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
>  	/* Create DP/eDP connector */
>  	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
>  	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
> -				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
> +				 tc->panel_bridge ? DRM_MODE_CONNECTOR_eDP :
>  				 DRM_MODE_CONNECTOR_DisplayPort);
>  	if (ret)
>  		return ret;
> @@ -1435,9 +1426,6 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
>  					       DRM_CONNECTOR_POLL_DISCONNECT;
>  	}
>  
> -	if (tc->panel)
> -		drm_panel_attach(tc->panel, &tc->connector);
> -
>  	drm_display_info_set_bus_formats(&tc->connector.display_info,
>  					 &bus_format, 1);
>  	tc->connector.display_info.bus_flags =
> @@ -1453,10 +1441,8 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
>  	.attach = tc_bridge_attach,
>  	.mode_valid = tc_mode_valid,
>  	.mode_set = tc_bridge_mode_set,
> -	.pre_enable = tc_bridge_pre_enable,
>  	.enable = tc_bridge_enable,
>  	.disable = tc_bridge_disable,
> -	.post_disable = tc_bridge_post_disable,
>  	.mode_fixup = tc_bridge_mode_fixup,
>  };
>  
> @@ -1547,6 +1533,7 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
>  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
>  	struct device *dev = &client->dev;
> +	struct drm_panel *panel;
>  	struct tc_data *tc;
>  	int ret;
>  
> @@ -1557,10 +1544,20 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	tc->dev = dev;
>  
>  	/* port@2 is the output port */
> -	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &tc->panel, NULL);
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &panel, NULL);
>  	if (ret && ret != -ENODEV)
>  		return ret;
>  
> +	if (panel) {
> +		struct drm_bridge *pbridge;
> +
> +		pbridge = devm_drm_panel_bridge_add(dev, panel);
> +		if (IS_ERR(pbridge))
> +			return PTR_ERR(pbridge);
> +
> +		tc->panel_bridge = pbridge;
> +	}
> +
>  	/* Shut down GPIO is optional */
>  	tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
>  	if (IS_ERR(tc->sd_gpio))

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 06/21] drm/bridge: tc358767: add drm_panel_bridge support

[Sam Ravnborg]

Hi Laurent.

On Sat, Jul 11, 2020 at 01:19:35AM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> Thank you for the patch.
> 
> On Fri, Jul 03, 2020 at 09:24:02PM +0200, Sam Ravnborg wrote:
> > Prepare the bridge driver for use in a chained setup by
> > replacing direct use of drm_panel with drm_panel_bridge support.
> > 
> > The bridge driver assume the panel is optional.
> > The relevant tests are migrated over to check for the
> > pnale bridge to keep the same functionality.
> 
> s/pnale/panel/
> 
> > Note: the bridge panel will use the connector type from the panel.
> > 
> > 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 | 57 +++++++++++++++----------------
> >  1 file changed, 27 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index c2777b226c75..08d483664258 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -244,8 +244,8 @@ struct tc_data {
> >  	struct drm_dp_aux	aux;
> >  
> >  	struct drm_bridge	bridge;
> > +	struct drm_bridge	*panel_bridge;
> >  	struct drm_connector	connector;
> > -	struct drm_panel	*panel;
> >  
> >  	/* link settings */
> >  	struct tc_edp_link	link;
> > @@ -1236,13 +1236,6 @@ static int tc_stream_disable(struct tc_data *tc)
> >  	return 0;
> >  }
> >  
> > -static void tc_bridge_pre_enable(struct drm_bridge *bridge)
> > -{
> > -	struct tc_data *tc = bridge_to_tc(bridge);
> > -
> > -	drm_panel_prepare(tc->panel);
> > -}
> > -
> >  static void tc_bridge_enable(struct drm_bridge *bridge)
> >  {
> >  	struct tc_data *tc = bridge_to_tc(bridge);
> > @@ -1266,8 +1259,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> >  		tc_main_link_disable(tc);
> >  		return;
> >  	}
> > -
> > -	drm_panel_enable(tc->panel);
> >  }
> >  
> >  static void tc_bridge_disable(struct drm_bridge *bridge)
> > @@ -1275,8 +1266,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
> >  	struct tc_data *tc = bridge_to_tc(bridge);
> >  	int ret;
> >  
> > -	drm_panel_disable(tc->panel);
> > -
> >  	ret = tc_stream_disable(tc);
> >  	if (ret < 0)
> >  		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
> > @@ -1286,13 +1275,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
> >  		dev_err(tc->dev, "main link disable error: %d\n", ret);
> >  }
> >  
> > -static void tc_bridge_post_disable(struct drm_bridge *bridge)
> > -{
> > -	struct tc_data *tc = bridge_to_tc(bridge);
> > -
> > -	drm_panel_unprepare(tc->panel);
> > -}
> > -
> >  static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
> >  				 const struct drm_display_mode *mode,
> >  				 struct drm_display_mode *adj)
> > @@ -1348,9 +1330,11 @@ static int tc_connector_get_modes(struct drm_connector *connector)
> >  		return 0;
> >  	}
> >  
> > -	count = drm_panel_get_modes(tc->panel, connector);
> > -	if (count > 0)
> > -		return count;
> > +	if (tc->panel_bridge) {
> > +		count = drm_bridge_get_modes(tc->panel_bridge, connector);
> > +		if (count > 0)
> > +			return count;
> > +	}
> >  
> >  	edid = drm_get_edid(connector, &tc->aux.ddc);
> >  
> > @@ -1378,7 +1362,7 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
> >  	int ret;
> >  
> >  	if (tc->hpd_pin < 0) {
> > -		if (tc->panel)
> > +		if (tc->panel_bridge)
> >  			return connector_status_connected;
> >  		else
> >  			return connector_status_unknown;
> > @@ -1413,6 +1397,13 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> >  	struct drm_device *drm = bridge->dev;
> >  	int ret;
> >  
> > +	if (tc->panel_bridge) {
> > +		ret = drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
> > +					&tc->bridge, flags);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> 
> With this both this driver and the panel bridge driver will create a
> connector. The simplest way to handle that is probably to pass
> flags & ~DRM_BRIDGE_ATTACH_NO_CONNECTOR to drm_bridge_attach(). It's a
> bit of a hack, but should go away once all users are converted to
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR.

I do not follow you here - sorry.

We have two situations:

display driver creates the connector - and passes DRM_BRIDGE_ATTACH_NO_CONNECTOR.
- bridge driver shall not create connector
- bridge panel shall not create connector

display driver expect bridge to create connector and passes 0
- bridge driver shall not create connector
- bridge panel shall create connector

So the correct logic seems to be:
- if there is a bridge panel do not create a connector in the bridge
- otherwise just follow the flags

I will try to implement this.

	Sam

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> >  	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> >  		DRM_ERROR("Fix bridge driver to make connector optional!");
> >  		return -EINVAL;
> > @@ -1421,7 +1412,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> >  	/* Create DP/eDP connector */
> >  	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
> >  	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
> > -				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
> > +				 tc->panel_bridge ? DRM_MODE_CONNECTOR_eDP :
> >  				 DRM_MODE_CONNECTOR_DisplayPort);
> >  	if (ret)
> >  		return ret;
> > @@ -1435,9 +1426,6 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> >  					       DRM_CONNECTOR_POLL_DISCONNECT;
> >  	}
> >  
> > -	if (tc->panel)
> > -		drm_panel_attach(tc->panel, &tc->connector);
> > -
> >  	drm_display_info_set_bus_formats(&tc->connector.display_info,
> >  					 &bus_format, 1);
> >  	tc->connector.display_info.bus_flags =
> > @@ -1453,10 +1441,8 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
> >  	.attach = tc_bridge_attach,
> >  	.mode_valid = tc_mode_valid,
> >  	.mode_set = tc_bridge_mode_set,
> > -	.pre_enable = tc_bridge_pre_enable,
> >  	.enable = tc_bridge_enable,
> >  	.disable = tc_bridge_disable,
> > -	.post_disable = tc_bridge_post_disable,
> >  	.mode_fixup = tc_bridge_mode_fixup,
> >  };
> >  
> > @@ -1547,6 +1533,7 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
> >  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  {
> >  	struct device *dev = &client->dev;
> > +	struct drm_panel *panel;
> >  	struct tc_data *tc;
> >  	int ret;
> >  
> > @@ -1557,10 +1544,20 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  	tc->dev = dev;
> >  
> >  	/* port@2 is the output port */
> > -	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &tc->panel, NULL);
> > +	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &panel, NULL);
> >  	if (ret && ret != -ENODEV)
> >  		return ret;
> >  
> > +	if (panel) {
> > +		struct drm_bridge *pbridge;
> > +
> > +		pbridge = devm_drm_panel_bridge_add(dev, panel);
> > +		if (IS_ERR(pbridge))
> > +			return PTR_ERR(pbridge);
> > +
> > +		tc->panel_bridge = pbridge;
> > +	}
> > +
> >  	/* Shut down GPIO is optional */
> >  	tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
> >  	if (IS_ERR(tc->sd_gpio))
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 06/21] drm/bridge: tc358767: add drm_panel_bridge support

[Laurent Pinchart]

Hi Sam,

On Sun, Jul 19, 2020 at 03:06:56PM +0200, Sam Ravnborg wrote:
> On Sat, Jul 11, 2020 at 01:19:35AM +0300, Laurent Pinchart wrote:
> > On Fri, Jul 03, 2020 at 09:24:02PM +0200, Sam Ravnborg wrote:
> > > Prepare the bridge driver for use in a chained setup by
> > > replacing direct use of drm_panel with drm_panel_bridge support.
> > > 
> > > The bridge driver assume the panel is optional.
> > > The relevant tests are migrated over to check for the
> > > pnale bridge to keep the same functionality.
> > 
> > s/pnale/panel/
> > 
> > > Note: the bridge panel will use the connector type from the panel.
> > > 
> > > 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 | 57 +++++++++++++++----------------
> > >  1 file changed, 27 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > > index c2777b226c75..08d483664258 100644
> > > --- a/drivers/gpu/drm/bridge/tc358767.c
> > > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > > @@ -244,8 +244,8 @@ struct tc_data {
> > >  	struct drm_dp_aux	aux;
> > >  
> > >  	struct drm_bridge	bridge;
> > > +	struct drm_bridge	*panel_bridge;
> > >  	struct drm_connector	connector;
> > > -	struct drm_panel	*panel;
> > >  
> > >  	/* link settings */
> > >  	struct tc_edp_link	link;
> > > @@ -1236,13 +1236,6 @@ static int tc_stream_disable(struct tc_data *tc)
> > >  	return 0;
> > >  }
> > >  
> > > -static void tc_bridge_pre_enable(struct drm_bridge *bridge)
> > > -{
> > > -	struct tc_data *tc = bridge_to_tc(bridge);
> > > -
> > > -	drm_panel_prepare(tc->panel);
> > > -}
> > > -
> > >  static void tc_bridge_enable(struct drm_bridge *bridge)
> > >  {
> > >  	struct tc_data *tc = bridge_to_tc(bridge);
> > > @@ -1266,8 +1259,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> > >  		tc_main_link_disable(tc);
> > >  		return;
> > >  	}
> > > -
> > > -	drm_panel_enable(tc->panel);
> > >  }
> > >  
> > >  static void tc_bridge_disable(struct drm_bridge *bridge)
> > > @@ -1275,8 +1266,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
> > >  	struct tc_data *tc = bridge_to_tc(bridge);
> > >  	int ret;
> > >  
> > > -	drm_panel_disable(tc->panel);
> > > -
> > >  	ret = tc_stream_disable(tc);
> > >  	if (ret < 0)
> > >  		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
> > > @@ -1286,13 +1275,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
> > >  		dev_err(tc->dev, "main link disable error: %d\n", ret);
> > >  }
> > >  
> > > -static void tc_bridge_post_disable(struct drm_bridge *bridge)
> > > -{
> > > -	struct tc_data *tc = bridge_to_tc(bridge);
> > > -
> > > -	drm_panel_unprepare(tc->panel);
> > > -}
> > > -
> > >  static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
> > >  				 const struct drm_display_mode *mode,
> > >  				 struct drm_display_mode *adj)
> > > @@ -1348,9 +1330,11 @@ static int tc_connector_get_modes(struct drm_connector *connector)
> > >  		return 0;
> > >  	}
> > >  
> > > -	count = drm_panel_get_modes(tc->panel, connector);
> > > -	if (count > 0)
> > > -		return count;
> > > +	if (tc->panel_bridge) {
> > > +		count = drm_bridge_get_modes(tc->panel_bridge, connector);
> > > +		if (count > 0)
> > > +			return count;
> > > +	}
> > >  
> > >  	edid = drm_get_edid(connector, &tc->aux.ddc);
> > >  
> > > @@ -1378,7 +1362,7 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
> > >  	int ret;
> > >  
> > >  	if (tc->hpd_pin < 0) {
> > > -		if (tc->panel)
> > > +		if (tc->panel_bridge)
> > >  			return connector_status_connected;
> > >  		else
> > >  			return connector_status_unknown;
> > > @@ -1413,6 +1397,13 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> > >  	struct drm_device *drm = bridge->dev;
> > >  	int ret;
> > >  
> > > +	if (tc->panel_bridge) {
> > > +		ret = drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
> > > +					&tc->bridge, flags);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > 
> > With this both this driver and the panel bridge driver will create a
> > connector. The simplest way to handle that is probably to pass
> > flags & ~DRM_BRIDGE_ATTACH_NO_CONNECTOR to drm_bridge_attach(). It's a
> > bit of a hack, but should go away once all users are converted to
> > !DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> 
> I do not follow you here - sorry.
> 
> We have two situations:
> 
> display driver creates the connector - and passes DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> - bridge driver shall not create connector
> - bridge panel shall not create connector
> 
> display driver expect bridge to create connector and passes 0
> - bridge driver shall not create connector
> - bridge panel shall create connector

I meant the other way around, creating a connector here and skipping it
in the bridge panel driver. Which of the two (this driver and the bridge
panel driver) has the most information to implement a connector ?

> So the correct logic seems to be:
> - if there is a bridge panel do not create a connector in the bridge
> - otherwise just follow the flags
> 
> I will try to implement this.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > > +
> > >  	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > >  		DRM_ERROR("Fix bridge driver to make connector optional!");
> > >  		return -EINVAL;
> > > @@ -1421,7 +1412,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> > >  	/* Create DP/eDP connector */
> > >  	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
> > >  	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
> > > -				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
> > > +				 tc->panel_bridge ? DRM_MODE_CONNECTOR_eDP :
> > >  				 DRM_MODE_CONNECTOR_DisplayPort);
> > >  	if (ret)
> > >  		return ret;
> > > @@ -1435,9 +1426,6 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> > >  					       DRM_CONNECTOR_POLL_DISCONNECT;
> > >  	}
> > >  
> > > -	if (tc->panel)
> > > -		drm_panel_attach(tc->panel, &tc->connector);
> > > -
> > >  	drm_display_info_set_bus_formats(&tc->connector.display_info,
> > >  					 &bus_format, 1);
> > >  	tc->connector.display_info.bus_flags =
> > > @@ -1453,10 +1441,8 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
> > >  	.attach = tc_bridge_attach,
> > >  	.mode_valid = tc_mode_valid,
> > >  	.mode_set = tc_bridge_mode_set,
> > > -	.pre_enable = tc_bridge_pre_enable,
> > >  	.enable = tc_bridge_enable,
> > >  	.disable = tc_bridge_disable,
> > > -	.post_disable = tc_bridge_post_disable,
> > >  	.mode_fixup = tc_bridge_mode_fixup,
> > >  };
> > >  
> > > @@ -1547,6 +1533,7 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
> > >  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > >  {
> > >  	struct device *dev = &client->dev;
> > > +	struct drm_panel *panel;
> > >  	struct tc_data *tc;
> > >  	int ret;
> > >  
> > > @@ -1557,10 +1544,20 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > >  	tc->dev = dev;
> > >  
> > >  	/* port@2 is the output port */
> > > -	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &tc->panel, NULL);
> > > +	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &panel, NULL);
> > >  	if (ret && ret != -ENODEV)
> > >  		return ret;
> > >  
> > > +	if (panel) {
> > > +		struct drm_bridge *pbridge;
> > > +
> > > +		pbridge = devm_drm_panel_bridge_add(dev, panel);
> > > +		if (IS_ERR(pbridge))
> > > +			return PTR_ERR(pbridge);
> > > +
> > > +		tc->panel_bridge = pbridge;
> > > +	}
> > > +
> > >  	/* Shut down GPIO is optional */
> > >  	tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
> > >  	if (IS_ERR(tc->sd_gpio))

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 07/21] drm/bridge: tc358767: add detect bridge operation

[Sam Ravnborg]

Prepare the bridge driver for chained operation by adding
support for the detect operation.

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 | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 08d483664258..85973ae728db 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1353,10 +1353,9 @@ static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
 	.get_modes = tc_connector_get_modes,
 };
 
-static enum drm_connector_status tc_connector_detect(struct drm_connector *connector,
-						     bool force)
+static enum drm_connector_status tc_bridge_detect(struct drm_bridge *bridge)
 {
-	struct tc_data *tc = connector_to_tc(connector);
+	struct tc_data *tc = bridge_to_tc(bridge);
 	bool conn;
 	u32 val;
 	int ret;
@@ -1380,6 +1379,14 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
 		return connector_status_disconnected;
 }
 
+static enum drm_connector_status
+tc_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct tc_data *tc = connector_to_tc(connector);
+
+	return tc_bridge_detect(&tc->bridge);
+}
+
 static const struct drm_connector_funcs tc_connector_funcs = {
 	.detect = tc_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -1444,6 +1451,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.enable = tc_bridge_enable,
 	.disable = tc_bridge_disable,
 	.mode_fixup = tc_bridge_mode_fixup,
+	.detect = tc_bridge_detect,
 };
 
 static bool tc_readable_reg(struct device *dev, unsigned int reg)
@@ -1677,6 +1685,8 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return ret;
 
 	tc->bridge.funcs = &tc_bridge_funcs;
+	tc->bridge.ops = DRM_BRIDGE_OP_DETECT;
+
 	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
 
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 07/21] drm/bridge: tc358767: add detect bridge operation

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:03PM +0200, Sam Ravnborg wrote:
> Prepare the bridge driver for chained operation by adding
> support for the detect operation.
> 
> 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 | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 08d483664258..85973ae728db 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1353,10 +1353,9 @@ static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
>  	.get_modes = tc_connector_get_modes,
>  };
>  
> -static enum drm_connector_status tc_connector_detect(struct drm_connector *connector,
> -						     bool force)
> +static enum drm_connector_status tc_bridge_detect(struct drm_bridge *bridge)
>  {
> -	struct tc_data *tc = connector_to_tc(connector);
> +	struct tc_data *tc = bridge_to_tc(bridge);
>  	bool conn;
>  	u32 val;
>  	int ret;
> @@ -1380,6 +1379,14 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
>  		return connector_status_disconnected;
>  }
>  
> +static enum drm_connector_status
> +tc_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct tc_data *tc = connector_to_tc(connector);
> +
> +	return tc_bridge_detect(&tc->bridge);
> +}
> +
>  static const struct drm_connector_funcs tc_connector_funcs = {
>  	.detect = tc_connector_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> @@ -1444,6 +1451,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
>  	.enable = tc_bridge_enable,
>  	.disable = tc_bridge_disable,
>  	.mode_fixup = tc_bridge_mode_fixup,
> +	.detect = tc_bridge_detect,
>  };
>  
>  static bool tc_readable_reg(struct device *dev, unsigned int reg)
> @@ -1677,6 +1685,8 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		return ret;
>  
>  	tc->bridge.funcs = &tc_bridge_funcs;
> +	tc->bridge.ops = DRM_BRIDGE_OP_DETECT;

I think you should set DRM_BRIDGE_OP_DETECT only when tc->hpd_pin is
valid. I would also move the if (tc->hpd_pin < 0) case from
tc_bridge_detect to +tc_connector_detect.

> +
>  	tc->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&tc->bridge);
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 08/21] drm/bridge: tc358767: add get_edid bride operation

[Sam Ravnborg]

Prepare for chained bridge with the addition of
get_edid support.

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 | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 85973ae728db..fb9d57967b2c 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1317,6 +1317,20 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge,
 	tc->mode = *mode;
 }
 
+static struct edid *tc_get_edid(struct drm_bridge *bridge,
+				struct drm_connector *connector)
+{
+	struct tc_data *tc = bridge_to_tc(bridge);
+	struct edid *edid;
+
+	edid = drm_get_edid(connector, &tc->aux.ddc);
+
+	kfree(tc->edid);
+	tc->edid = edid;
+
+	return edid;
+}
+
 static int tc_connector_get_modes(struct drm_connector *connector)
 {
 	struct tc_data *tc = connector_to_tc(connector);
@@ -1336,12 +1350,7 @@ static int tc_connector_get_modes(struct drm_connector *connector)
 			return count;
 	}
 
-	edid = drm_get_edid(connector, &tc->aux.ddc);
-
-	kfree(tc->edid);
-	tc->edid = edid;
-	if (!edid)
-		return 0;
+	edid = tc_get_edid(&tc->bridge, connector);
 
 	drm_connector_update_edid_property(connector, edid);
 	count = drm_add_edid_modes(connector, edid);
@@ -1452,6 +1461,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.disable = tc_bridge_disable,
 	.mode_fixup = tc_bridge_mode_fixup,
 	.detect = tc_bridge_detect,
+	.get_edid = tc_get_edid,
 };
 
 static bool tc_readable_reg(struct device *dev, unsigned int reg)
@@ -1685,7 +1695,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return ret;
 
 	tc->bridge.funcs = &tc_bridge_funcs;
-	tc->bridge.ops = DRM_BRIDGE_OP_DETECT;
+	tc->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
 
 	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 08/21] drm/bridge: tc358767: add get_edid bride operation

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

s/bride/bridge/ in the subject line.

On Fri, Jul 03, 2020 at 09:24:04PM +0200, Sam Ravnborg wrote:
> Prepare for chained bridge with the addition of
> get_edid support.
> 
> 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 | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 85973ae728db..fb9d57967b2c 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1317,6 +1317,20 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge,
>  	tc->mode = *mode;
>  }
>  
> +static struct edid *tc_get_edid(struct drm_bridge *bridge,
> +				struct drm_connector *connector)
> +{
> +	struct tc_data *tc = bridge_to_tc(bridge);
> +	struct edid *edid;
> +
> +	edid = drm_get_edid(connector, &tc->aux.ddc);
> +
> +	kfree(tc->edid);
> +	tc->edid = edid;

The caller (drm_bridge_connector_get_modes_edid()) calls kfree(edid), so
if you want to store it internally, you'll have to make a copy. Can you
skip internal storage altogether by freeing the memory in
tc_connector_get_modes() ?

> +
> +	return edid;
> +}
> +
>  static int tc_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct tc_data *tc = connector_to_tc(connector);
> @@ -1336,12 +1350,7 @@ static int tc_connector_get_modes(struct drm_connector *connector)
>  			return count;
>  	}
>  
> -	edid = drm_get_edid(connector, &tc->aux.ddc);
> -
> -	kfree(tc->edid);
> -	tc->edid = edid;
> -	if (!edid)
> -		return 0;
> +	edid = tc_get_edid(&tc->bridge, connector);
>  
>  	drm_connector_update_edid_property(connector, edid);
>  	count = drm_add_edid_modes(connector, edid);
> @@ -1452,6 +1461,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
>  	.disable = tc_bridge_disable,
>  	.mode_fixup = tc_bridge_mode_fixup,
>  	.detect = tc_bridge_detect,
> +	.get_edid = tc_get_edid,
>  };
>  
>  static bool tc_readable_reg(struct device *dev, unsigned int reg)
> @@ -1685,7 +1695,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		return ret;
>  
>  	tc->bridge.funcs = &tc_bridge_funcs;
> -	tc->bridge.ops = DRM_BRIDGE_OP_DETECT;
> +	tc->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
>  
>  	tc->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&tc->bridge);
> -- 
> 2.25.1
> 

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 09/21] drm/bridge: tc358767: make connector creation optional

[Sam Ravnborg]

Display drivers are in the new model expected to create
the connector using drm_bridge_connector_init().
Allow users of this bridge driver to support the new
model by introducing support for optional connector creation.

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 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index fb9d57967b2c..2eb00d39f619 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1421,8 +1421,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
 	}
 
 	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
+		return 0;
 	}
 
 	/* Create DP/eDP connector */
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 09/21] drm/bridge: tc358767: make connector creation optional

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:05PM +0200, Sam Ravnborg wrote:
> Display drivers are in the new model expected to create
> the connector using drm_bridge_connector_init().
> Allow users of this bridge driver to support the new
> model by introducing support for optional connector creation.
> 
> 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 | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index fb9d57967b2c..2eb00d39f619 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1421,8 +1421,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
>  	}
>  
>  	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> +		return 0;
>  	}

I'd drop the braces too.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	/* Create DP/eDP connector */

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 10/21] drm/bridge: ti-tpd12s015: make connector creation optional

[Sam Ravnborg]

The ti-tpd12s015 do not create any connector, so ignore
the flags argument, just pass it on to the next bridge
in the chain.

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/ti-tpd12s015.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-tpd12s015.c b/drivers/gpu/drm/bridge/ti-tpd12s015.c
index 514cbf0eac75..4f1666422ab2 100644
--- a/drivers/gpu/drm/bridge/ti-tpd12s015.c
+++ b/drivers/gpu/drm/bridge/ti-tpd12s015.c
@@ -43,9 +43,6 @@ static int tpd12s015_attach(struct drm_bridge *bridge,
 	struct tpd12s015_device *tpd = to_tpd12s015(bridge);
 	int ret;
 
-	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
-		return -EINVAL;
-
 	ret = drm_bridge_attach(bridge->encoder, tpd->next_bridge,
 				bridge, flags);
 	if (ret < 0)
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 10/21] drm/bridge: ti-tpd12s015: make connector creation optional

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:06PM +0200, Sam Ravnborg wrote:
> The ti-tpd12s015 do not create any connector, so ignore
> the flags argument, just pass it on to the next bridge
> in the chain.
> 
> 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/ti-tpd12s015.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-tpd12s015.c b/drivers/gpu/drm/bridge/ti-tpd12s015.c
> index 514cbf0eac75..4f1666422ab2 100644
> --- a/drivers/gpu/drm/bridge/ti-tpd12s015.c
> +++ b/drivers/gpu/drm/bridge/ti-tpd12s015.c
> @@ -43,9 +43,6 @@ static int tpd12s015_attach(struct drm_bridge *bridge,
>  	struct tpd12s015_device *tpd = to_tpd12s015(bridge);
>  	int ret;
>  
> -	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> -		return -EINVAL;
> -

The driver is only used by devices that use
DRM_BRIDGE_ATTACH_NO_CONNECTOR. I'd rather keep this check and port
other potential users to DRM_BRIDGE_ATTACH_NO_CONNECTOR instead of
allowing operation in !DRM_BRIDGE_ATTACH_NO_CONNECTOR mode.

>  	ret = drm_bridge_attach(bridge->encoder, tpd->next_bridge,
>  				bridge, flags);
>  	if (ret < 0)

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 11/21] drm/bridge: parade-ps8622: add drm_panel_bridge support

[Sam Ravnborg]

Prepare the bridge driver for use in a chained setup by
replacing direct use of drm_panel with drm_panel_bridge support.

Note: the bridge panel will use the connector type from the panel.

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/parade-ps8622.c | 46 ++++++++------------------
 1 file changed, 13 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
index d789ea2a7fb9..6ab6f60b9091 100644
--- a/drivers/gpu/drm/bridge/parade-ps8622.c
+++ b/drivers/gpu/drm/bridge/parade-ps8622.c
@@ -45,7 +45,7 @@ struct ps8622_bridge {
 	struct drm_connector connector;
 	struct i2c_client *client;
 	struct drm_bridge bridge;
-	struct drm_panel *panel;
+	struct drm_bridge *panel_bridge;
 	struct regulator *v12;
 	struct backlight_device *bl;
 
@@ -365,11 +365,6 @@ static void ps8622_pre_enable(struct drm_bridge *bridge)
 			DRM_ERROR("fails to enable ps8622->v12");
 	}
 
-	if (drm_panel_prepare(ps8622->panel)) {
-		DRM_ERROR("failed to prepare panel\n");
-		return;
-	}
-
 	gpiod_set_value(ps8622->gpio_slp, 1);
 
 	/*
@@ -399,24 +394,8 @@ static void ps8622_pre_enable(struct drm_bridge *bridge)
 	ps8622->enabled = true;
 }
 
-static void ps8622_enable(struct drm_bridge *bridge)
-{
-	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
-
-	if (drm_panel_enable(ps8622->panel)) {
-		DRM_ERROR("failed to enable panel\n");
-		return;
-	}
-}
-
 static void ps8622_disable(struct drm_bridge *bridge)
 {
-	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
-
-	if (drm_panel_disable(ps8622->panel)) {
-		DRM_ERROR("failed to disable panel\n");
-		return;
-	}
 	msleep(PS8622_PWMO_END_T12_MS);
 }
 
@@ -436,11 +415,6 @@ static void ps8622_post_disable(struct drm_bridge *bridge)
 	 */
 	gpiod_set_value(ps8622->gpio_slp, 0);
 
-	if (drm_panel_unprepare(ps8622->panel)) {
-		DRM_ERROR("failed to unprepare panel\n");
-		return;
-	}
-
 	if (ps8622->v12)
 		regulator_disable(ps8622->v12);
 
@@ -461,7 +435,7 @@ static int ps8622_get_modes(struct drm_connector *connector)
 
 	ps8622 = connector_to_ps8622(connector);
 
-	return drm_panel_get_modes(ps8622->panel, connector);
+	return drm_bridge_get_modes(ps8622->panel_bridge, connector);
 }
 
 static const struct drm_connector_helper_funcs ps8622_connector_helper_funcs = {
@@ -482,6 +456,9 @@ static int ps8622_attach(struct drm_bridge *bridge,
 	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
 	int ret;
 
+	ret = drm_bridge_attach(ps8622->bridge.encoder, ps8622->panel_bridge,
+				&ps8622->bridge, flags);
+
 	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
 		DRM_ERROR("Fix bridge driver to make connector optional!");
 		return -EINVAL;
@@ -505,9 +482,6 @@ static int ps8622_attach(struct drm_bridge *bridge,
 	drm_connector_attach_encoder(&ps8622->connector,
 							bridge->encoder);
 
-	if (ps8622->panel)
-		drm_panel_attach(ps8622->panel, &ps8622->connector);
-
 	drm_helper_hpd_irq_event(ps8622->connector.dev);
 
 	return ret;
@@ -515,7 +489,6 @@ static int ps8622_attach(struct drm_bridge *bridge,
 
 static const struct drm_bridge_funcs ps8622_bridge_funcs = {
 	.pre_enable = ps8622_pre_enable,
-	.enable = ps8622_enable,
 	.disable = ps8622_disable,
 	.post_disable = ps8622_post_disable,
 	.attach = ps8622_attach,
@@ -533,16 +506,23 @@ static int ps8622_probe(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	struct ps8622_bridge *ps8622;
+	struct drm_bridge *pbridge;
+	struct drm_panel *panel;
 	int ret;
 
 	ps8622 = devm_kzalloc(dev, sizeof(*ps8622), GFP_KERNEL);
 	if (!ps8622)
 		return -ENOMEM;
 
-	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &ps8622->panel, NULL);
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL);
 	if (ret)
 		return ret;
 
+	pbridge = devm_drm_panel_bridge_add(dev, panel);
+	if (IS_ERR(pbridge))
+		return PTR_ERR(pbridge);
+
+	ps8622->panel_bridge = pbridge;
 	ps8622->client = client;
 
 	ps8622->v12 = devm_regulator_get(dev, "vdd12");
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 11/21] drm/bridge: parade-ps8622: add drm_panel_bridge support

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:07PM +0200, Sam Ravnborg wrote:
> Prepare the bridge driver for use in a chained setup by
> replacing direct use of drm_panel with drm_panel_bridge support.
> 
> Note: the bridge panel will use the connector type from the panel.
> 
> 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/parade-ps8622.c | 46 ++++++++------------------
>  1 file changed, 13 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
> index d789ea2a7fb9..6ab6f60b9091 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8622.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8622.c
> @@ -45,7 +45,7 @@ struct ps8622_bridge {
>  	struct drm_connector connector;
>  	struct i2c_client *client;
>  	struct drm_bridge bridge;
> -	struct drm_panel *panel;
> +	struct drm_bridge *panel_bridge;
>  	struct regulator *v12;
>  	struct backlight_device *bl;
>  
> @@ -365,11 +365,6 @@ static void ps8622_pre_enable(struct drm_bridge *bridge)
>  			DRM_ERROR("fails to enable ps8622->v12");
>  	}
>  
> -	if (drm_panel_prepare(ps8622->panel)) {
> -		DRM_ERROR("failed to prepare panel\n");
> -		return;
> -	}
> -
>  	gpiod_set_value(ps8622->gpio_slp, 1);
>  
>  	/*
> @@ -399,24 +394,8 @@ static void ps8622_pre_enable(struct drm_bridge *bridge)
>  	ps8622->enabled = true;
>  }
>  
> -static void ps8622_enable(struct drm_bridge *bridge)
> -{
> -	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
> -
> -	if (drm_panel_enable(ps8622->panel)) {
> -		DRM_ERROR("failed to enable panel\n");
> -		return;
> -	}
> -}
> -
>  static void ps8622_disable(struct drm_bridge *bridge)
>  {
> -	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
> -
> -	if (drm_panel_disable(ps8622->panel)) {
> -		DRM_ERROR("failed to disable panel\n");
> -		return;
> -	}
>  	msleep(PS8622_PWMO_END_T12_MS);

What's the point in just waiting in the disable path ?

>  }
>  
> @@ -436,11 +415,6 @@ static void ps8622_post_disable(struct drm_bridge *bridge)
>  	 */
>  	gpiod_set_value(ps8622->gpio_slp, 0);
>  
> -	if (drm_panel_unprepare(ps8622->panel)) {
> -		DRM_ERROR("failed to unprepare panel\n");
> -		return;
> -	}
> -
>  	if (ps8622->v12)
>  		regulator_disable(ps8622->v12);
>  
> @@ -461,7 +435,7 @@ static int ps8622_get_modes(struct drm_connector *connector)
>  
>  	ps8622 = connector_to_ps8622(connector);
>  
> -	return drm_panel_get_modes(ps8622->panel, connector);
> +	return drm_bridge_get_modes(ps8622->panel_bridge, connector);
>  }
>  
>  static const struct drm_connector_helper_funcs ps8622_connector_helper_funcs = {
> @@ -482,6 +456,9 @@ static int ps8622_attach(struct drm_bridge *bridge,
>  	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
>  	int ret;
>  
> +	ret = drm_bridge_attach(ps8622->bridge.encoder, ps8622->panel_bridge,
> +				&ps8622->bridge, flags);

Same comment as earlier in this series about double connector creation.

Don't you need to check ret here ?

> +
>  	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>  		DRM_ERROR("Fix bridge driver to make connector optional!");
>  		return -EINVAL;
> @@ -505,9 +482,6 @@ static int ps8622_attach(struct drm_bridge *bridge,
>  	drm_connector_attach_encoder(&ps8622->connector,
>  							bridge->encoder);
>  
> -	if (ps8622->panel)
> -		drm_panel_attach(ps8622->panel, &ps8622->connector);
> -
>  	drm_helper_hpd_irq_event(ps8622->connector.dev);
>  
>  	return ret;
> @@ -515,7 +489,6 @@ static int ps8622_attach(struct drm_bridge *bridge,
>  
>  static const struct drm_bridge_funcs ps8622_bridge_funcs = {
>  	.pre_enable = ps8622_pre_enable,
> -	.enable = ps8622_enable,
>  	.disable = ps8622_disable,
>  	.post_disable = ps8622_post_disable,
>  	.attach = ps8622_attach,
> @@ -533,16 +506,23 @@ static int ps8622_probe(struct i2c_client *client,
>  {
>  	struct device *dev = &client->dev;
>  	struct ps8622_bridge *ps8622;
> +	struct drm_bridge *pbridge;
> +	struct drm_panel *panel;
>  	int ret;
>  
>  	ps8622 = devm_kzalloc(dev, sizeof(*ps8622), GFP_KERNEL);
>  	if (!ps8622)
>  		return -ENOMEM;
>  
> -	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &ps8622->panel, NULL);
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL);
>  	if (ret)
>  		return ret;
>  
> +	pbridge = devm_drm_panel_bridge_add(dev, panel);
> +	if (IS_ERR(pbridge))
> +		return PTR_ERR(pbridge);
> +
> +	ps8622->panel_bridge = pbridge;
>  	ps8622->client = client;
>  
>  	ps8622->v12 = devm_regulator_get(dev, "vdd12");

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 12/21] drm/bridge: parade-ps8622: make connector creation optional

[Sam Ravnborg]

Make the connector creation optional to enable usage of the
parade-ps8622 bridge with the DRM bridge connector helper.

This change moves drm_helper_hpd_irq_event() call in the attach
function up before the connector creation.

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/parade-ps8622.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
index 6ab6f60b9091..54aa5270d2c9 100644
--- a/drivers/gpu/drm/bridge/parade-ps8622.c
+++ b/drivers/gpu/drm/bridge/parade-ps8622.c
@@ -459,10 +459,8 @@ static int ps8622_attach(struct drm_bridge *bridge,
 	ret = drm_bridge_attach(ps8622->bridge.encoder, ps8622->panel_bridge,
 				&ps8622->bridge, flags);
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
+	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+		return 0;
 
 	if (!bridge->encoder) {
 		DRM_ERROR("Parent encoder object not found");
@@ -482,7 +480,7 @@ static int ps8622_attach(struct drm_bridge *bridge,
 	drm_connector_attach_encoder(&ps8622->connector,
 							bridge->encoder);
 
-	drm_helper_hpd_irq_event(ps8622->connector.dev);
+	drm_helper_hpd_irq_event(ps8622->bridge.dev);
 
 	return ret;
 }
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 12/21] drm/bridge: parade-ps8622: make connector creation optional

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:08PM +0200, Sam Ravnborg wrote:
> Make the connector creation optional to enable usage of the
> parade-ps8622 bridge with the DRM bridge connector helper.
> 
> This change moves drm_helper_hpd_irq_event() call in the attach
> function up before the connector creation.

No it doesn't :-)

And I wonder why that call is actually needed.

> 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/parade-ps8622.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
> index 6ab6f60b9091..54aa5270d2c9 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8622.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8622.c
> @@ -459,10 +459,8 @@ static int ps8622_attach(struct drm_bridge *bridge,
>  	ret = drm_bridge_attach(ps8622->bridge.encoder, ps8622->panel_bridge,
>  				&ps8622->bridge, flags);
>  
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> -	}
> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> +		return 0;
>  
>  	if (!bridge->encoder) {
>  		DRM_ERROR("Parent encoder object not found");
> @@ -482,7 +480,7 @@ static int ps8622_attach(struct drm_bridge *bridge,
>  	drm_connector_attach_encoder(&ps8622->connector,
>  							bridge->encoder);
>  
> -	drm_helper_hpd_irq_event(ps8622->connector.dev);
> +	drm_helper_hpd_irq_event(ps8622->bridge.dev);
>  
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 13/21] drm/bridge: megachips: add helper to create connector

[Sam Ravnborg]

Factor out connector creation to a small helper function.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
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>
---
 .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 47 +++++++++++--------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 6200f12a37e6..258e0525cdcc 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -191,6 +191,32 @@ static const struct drm_connector_funcs ge_b850v3_lvds_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+static int ge_b850v3_lvds_create_connector(struct drm_bridge *bridge)
+{
+	struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector;
+	int ret;
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found");
+		return -ENODEV;
+	}
+
+	connector->polled = DRM_CONNECTOR_POLL_HPD;
+
+	drm_connector_helper_add(connector,
+				 &ge_b850v3_lvds_connector_helper_funcs);
+
+	ret = drm_connector_init(bridge->dev, connector,
+				 &ge_b850v3_lvds_connector_funcs,
+				 DRM_MODE_CONNECTOR_DisplayPort);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	return drm_connector_attach_encoder(connector, bridge->encoder);
+}
+
 static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
 {
 	struct i2c_client *stdp4028_i2c
@@ -209,7 +235,6 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
 static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 				 enum drm_bridge_attach_flags flags)
 {
-	struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector;
 	struct i2c_client *stdp4028_i2c
 			= ge_b850v3_lvds_ptr->stdp4028_i2c;
 	int ret;
@@ -219,25 +244,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 		return -EINVAL;
 	}
 
-	if (!bridge->encoder) {
-		DRM_ERROR("Parent encoder object not found");
-		return -ENODEV;
-	}
-
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
-
-	drm_connector_helper_add(connector,
-				 &ge_b850v3_lvds_connector_helper_funcs);
-
-	ret = drm_connector_init(bridge->dev, connector,
-				 &ge_b850v3_lvds_connector_funcs,
-				 DRM_MODE_CONNECTOR_DisplayPort);
-	if (ret) {
-		DRM_ERROR("Failed to initialize connector with drm\n");
-		return ret;
-	}
-
-	ret = drm_connector_attach_encoder(connector, bridge->encoder);
+	ret = ge_b850v3_lvds_create_connector(bridge);
 	if (ret)
 		return ret;
 
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 13/21] drm/bridge: megachips: add helper to create connector

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:09PM +0200, Sam Ravnborg wrote:
> Factor out connector creation to a small helper function.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Peter Senna Tschudin <peter.senna@gmail.com>
> Cc: Martin Donnelly <martin.donnelly@ge.com>
> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> 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>
> ---
>  .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 47 +++++++++++--------
>  1 file changed, 27 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> index 6200f12a37e6..258e0525cdcc 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> @@ -191,6 +191,32 @@ static const struct drm_connector_funcs ge_b850v3_lvds_connector_funcs = {
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
> +static int ge_b850v3_lvds_create_connector(struct drm_bridge *bridge)
> +{
> +	struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector;

Wow, storing device state in a global variable... :-( How did this go
past review ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	int ret;
> +
> +	if (!bridge->encoder) {
> +		DRM_ERROR("Parent encoder object not found");
> +		return -ENODEV;
> +	}
> +
> +	connector->polled = DRM_CONNECTOR_POLL_HPD;
> +
> +	drm_connector_helper_add(connector,
> +				 &ge_b850v3_lvds_connector_helper_funcs);
> +
> +	ret = drm_connector_init(bridge->dev, connector,
> +				 &ge_b850v3_lvds_connector_funcs,
> +				 DRM_MODE_CONNECTOR_DisplayPort);
> +	if (ret) {
> +		DRM_ERROR("Failed to initialize connector with drm\n");
> +		return ret;
> +	}
> +
> +	return drm_connector_attach_encoder(connector, bridge->encoder);
> +}
> +
>  static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
>  {
>  	struct i2c_client *stdp4028_i2c
> @@ -209,7 +235,6 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
>  static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
>  				 enum drm_bridge_attach_flags flags)
>  {
> -	struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector;
>  	struct i2c_client *stdp4028_i2c
>  			= ge_b850v3_lvds_ptr->stdp4028_i2c;
>  	int ret;
> @@ -219,25 +244,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
>  		return -EINVAL;
>  	}
>  
> -	if (!bridge->encoder) {
> -		DRM_ERROR("Parent encoder object not found");
> -		return -ENODEV;
> -	}
> -
> -	connector->polled = DRM_CONNECTOR_POLL_HPD;
> -
> -	drm_connector_helper_add(connector,
> -				 &ge_b850v3_lvds_connector_helper_funcs);
> -
> -	ret = drm_connector_init(bridge->dev, connector,
> -				 &ge_b850v3_lvds_connector_funcs,
> -				 DRM_MODE_CONNECTOR_DisplayPort);
> -	if (ret) {
> -		DRM_ERROR("Failed to initialize connector with drm\n");
> -		return ret;
> -	}
> -
> -	ret = drm_connector_attach_encoder(connector, bridge->encoder);
> +	ret = ge_b850v3_lvds_create_connector(bridge);
>  	if (ret)
>  		return ret;
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 13/21] drm/bridge: megachips: add helper to create connector

[Sam Ravnborg]

On Sat, Jul 11, 2020 at 01:34:30AM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> Thank you for the patch.
> 
> On Fri, Jul 03, 2020 at 09:24:09PM +0200, Sam Ravnborg wrote:
> > Factor out connector creation to a small helper function.
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Peter Senna Tschudin <peter.senna@gmail.com>
> > Cc: Martin Donnelly <martin.donnelly@ge.com>
> > Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> > 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>
> > ---
> >  .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 47 +++++++++++--------
> >  1 file changed, 27 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > index 6200f12a37e6..258e0525cdcc 100644
> > --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > @@ -191,6 +191,32 @@ static const struct drm_connector_funcs ge_b850v3_lvds_connector_funcs = {
> >  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> >  };
> >  
> > +static int ge_b850v3_lvds_create_connector(struct drm_bridge *bridge)
> > +{
> > +	struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector;
> 
> Wow, storing device state in a global variable... :-( How did this go
> past review ?
Took a short look at eliminating this.
But there are two module entries involved so I droppet it again.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +	int ret;
> > +
> > +	if (!bridge->encoder) {
> > +		DRM_ERROR("Parent encoder object not found");
> > +		return -ENODEV;
> > +	}
> > +
> > +	connector->polled = DRM_CONNECTOR_POLL_HPD;
> > +
> > +	drm_connector_helper_add(connector,
> > +				 &ge_b850v3_lvds_connector_helper_funcs);
> > +
> > +	ret = drm_connector_init(bridge->dev, connector,
> > +				 &ge_b850v3_lvds_connector_funcs,
> > +				 DRM_MODE_CONNECTOR_DisplayPort);
> > +	if (ret) {
> > +		DRM_ERROR("Failed to initialize connector with drm\n");
> > +		return ret;
> > +	}
> > +
> > +	return drm_connector_attach_encoder(connector, bridge->encoder);
> > +}
> > +
> >  static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
> >  {
> >  	struct i2c_client *stdp4028_i2c
> > @@ -209,7 +235,6 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
> >  static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
> >  				 enum drm_bridge_attach_flags flags)
> >  {
> > -	struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector;
> >  	struct i2c_client *stdp4028_i2c
> >  			= ge_b850v3_lvds_ptr->stdp4028_i2c;
> >  	int ret;
> > @@ -219,25 +244,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (!bridge->encoder) {
> > -		DRM_ERROR("Parent encoder object not found");
> > -		return -ENODEV;
> > -	}
> > -
> > -	connector->polled = DRM_CONNECTOR_POLL_HPD;
> > -
> > -	drm_connector_helper_add(connector,
> > -				 &ge_b850v3_lvds_connector_helper_funcs);
> > -
> > -	ret = drm_connector_init(bridge->dev, connector,
> > -				 &ge_b850v3_lvds_connector_funcs,
> > -				 DRM_MODE_CONNECTOR_DisplayPort);
> > -	if (ret) {
> > -		DRM_ERROR("Failed to initialize connector with drm\n");
> > -		return ret;
> > -	}
> > -
> > -	ret = drm_connector_attach_encoder(connector, bridge->encoder);
> > +	ret = ge_b850v3_lvds_create_connector(bridge);
> >  	if (ret)
> >  		return ret;
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 14/21] drm/bridge: megachips: get drm_device from bridge

[Sam Ravnborg]

Fix so drm_device is read from the bridge.
This is a preparation for the connector being optional.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
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/megachips-stdpxxxx-ge-b850v3-fw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 258e0525cdcc..cf1dfbc88acf 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -226,8 +226,8 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
 				  STDP4028_DPTX_IRQ_STS_REG,
 				  STDP4028_DPTX_IRQ_CLEAR);
 
-	if (ge_b850v3_lvds_ptr->connector.dev)
-		drm_kms_helper_hotplug_event(ge_b850v3_lvds_ptr->connector.dev);
+	if (ge_b850v3_lvds_ptr->bridge.dev)
+		drm_kms_helper_hotplug_event(ge_b850v3_lvds_ptr->bridge.dev);
 
 	return IRQ_HANDLED;
 }
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 14/21] drm/bridge: megachips: get drm_device from bridge

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:10PM +0200, Sam Ravnborg wrote:
> Fix so drm_device is read from the bridge.
> This is a preparation for the connector being optional.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Peter Senna Tschudin <peter.senna@gmail.com>
> Cc: Martin Donnelly <martin.donnelly@ge.com>
> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> 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>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> index 258e0525cdcc..cf1dfbc88acf 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> @@ -226,8 +226,8 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
>  				  STDP4028_DPTX_IRQ_STS_REG,
>  				  STDP4028_DPTX_IRQ_CLEAR);
>  
> -	if (ge_b850v3_lvds_ptr->connector.dev)
> -		drm_kms_helper_hotplug_event(ge_b850v3_lvds_ptr->connector.dev);
> +	if (ge_b850v3_lvds_ptr->bridge.dev)
> +		drm_kms_helper_hotplug_event(ge_b850v3_lvds_ptr->bridge.dev);
>  
>  	return IRQ_HANDLED;
>  }

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 15/21] drm/bridge: megachips: enable detect bridge operation

[Sam Ravnborg]

To prepare for use in a chained bridge setup enable the
detect operation.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
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>
---
 .../gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index cf1dfbc88acf..78a9afe8f063 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -163,8 +163,8 @@ drm_connector_helper_funcs ge_b850v3_lvds_connector_helper_funcs = {
 	.mode_valid = ge_b850v3_lvds_mode_valid,
 };
 
-static enum drm_connector_status ge_b850v3_lvds_detect(
-		struct drm_connector *connector, bool force)
+static enum drm_connector_status ge_b850v3_lvds_bridge_detect(
+		struct drm_bridge *bridge)
 {
 	struct i2c_client *stdp4028_i2c =
 			ge_b850v3_lvds_ptr->stdp4028_i2c;
@@ -182,6 +182,12 @@ static enum drm_connector_status ge_b850v3_lvds_detect(
 	return connector_status_unknown;
 }
 
+static enum drm_connector_status ge_b850v3_lvds_detect(
+		struct drm_connector *connector, bool force)
+{
+	return ge_b850v3_lvds_bridge_detect(&ge_b850v3_lvds_ptr->bridge);
+}
+
 static const struct drm_connector_funcs ge_b850v3_lvds_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = ge_b850v3_lvds_detect,
@@ -263,6 +269,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 
 static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {
 	.attach = ge_b850v3_lvds_attach,
+	.detect = ge_b850v3_lvds_bridge_detect,
 };
 
 static int ge_b850v3_lvds_init(struct device *dev)
@@ -317,6 +324,7 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
 
 	/* drm bridge initialization */
 	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
+	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT;
 	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
 
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 15/21] drm/bridge: megachips: enable detect bridge operation

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:11PM +0200, Sam Ravnborg wrote:
> To prepare for use in a chained bridge setup enable the
> detect operation.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Peter Senna Tschudin <peter.senna@gmail.com>
> Cc: Martin Donnelly <martin.donnelly@ge.com>
> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> 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>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> index cf1dfbc88acf..78a9afe8f063 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> @@ -163,8 +163,8 @@ drm_connector_helper_funcs ge_b850v3_lvds_connector_helper_funcs = {
>  	.mode_valid = ge_b850v3_lvds_mode_valid,
>  };
>  
> -static enum drm_connector_status ge_b850v3_lvds_detect(
> -		struct drm_connector *connector, bool force)
> +static enum drm_connector_status ge_b850v3_lvds_bridge_detect(
> +		struct drm_bridge *bridge)
>  {
>  	struct i2c_client *stdp4028_i2c =
>  			ge_b850v3_lvds_ptr->stdp4028_i2c;
> @@ -182,6 +182,12 @@ static enum drm_connector_status ge_b850v3_lvds_detect(
>  	return connector_status_unknown;
>  }
>  
> +static enum drm_connector_status ge_b850v3_lvds_detect(
> +		struct drm_connector *connector, bool force)
> +{
> +	return ge_b850v3_lvds_bridge_detect(&ge_b850v3_lvds_ptr->bridge);
> +}
> +
>  static const struct drm_connector_funcs ge_b850v3_lvds_connector_funcs = {
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.detect = ge_b850v3_lvds_detect,
> @@ -263,6 +269,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
>  
>  static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {
>  	.attach = ge_b850v3_lvds_attach,
> +	.detect = ge_b850v3_lvds_bridge_detect,
>  };
>  
>  static int ge_b850v3_lvds_init(struct device *dev)
> @@ -317,6 +324,7 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
>  
>  	/* drm bridge initialization */
>  	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
> +	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT;
>  	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 16/21] drm/bridge: megachips: add get_edid bridge operation

[Sam Ravnborg]

To prepare for a chained bridge setup add support for the
get_edid bridge operation.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
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>
---
 .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 26 +++++++++++++------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 78a9afe8f063..5f06e18f0a61 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -131,21 +131,29 @@ static u8 *stdp2690_get_edid(struct i2c_client *client)
 	return NULL;
 }
 
-static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
+static struct edid *ge_b850v3_lvds_get_edid(
+		struct drm_bridge *bridge, struct drm_connector *connector)
 {
 	struct i2c_client *client;
-	int num_modes = 0;
 
 	client = ge_b850v3_lvds_ptr->stdp2690_i2c;
 
 	kfree(ge_b850v3_lvds_ptr->edid);
 	ge_b850v3_lvds_ptr->edid = (struct edid *)stdp2690_get_edid(client);
 
-	if (ge_b850v3_lvds_ptr->edid) {
-		drm_connector_update_edid_property(connector,
-						      ge_b850v3_lvds_ptr->edid);
-		num_modes = drm_add_edid_modes(connector,
-					       ge_b850v3_lvds_ptr->edid);
+	return ge_b850v3_lvds_ptr->edid;
+}
+
+static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
+{
+	struct edid *edid;
+	int num_modes = 0;
+
+	edid = ge_b850v3_lvds_get_edid(&ge_b850v3_lvds_ptr->bridge, connector);
+
+	if (edid) {
+		drm_connector_update_edid_property(connector, edid);
+		num_modes = drm_add_edid_modes(connector, edid);
 	}
 
 	return num_modes;
@@ -270,6 +278,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {
 	.attach = ge_b850v3_lvds_attach,
 	.detect = ge_b850v3_lvds_bridge_detect,
+	.get_edid = ge_b850v3_lvds_get_edid,
 };
 
 static int ge_b850v3_lvds_init(struct device *dev)
@@ -324,7 +333,8 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
 
 	/* drm bridge initialization */
 	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
-	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT;
+	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT |
+					 DRM_BRIDGE_OP_EDID;
 	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
 
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 16/21] drm/bridge: megachips: add get_edid bridge operation

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:12PM +0200, Sam Ravnborg wrote:
> To prepare for a chained bridge setup add support for the
> get_edid bridge operation.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Peter Senna Tschudin <peter.senna@gmail.com>
> Cc: Martin Donnelly <martin.donnelly@ge.com>
> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> 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>
> ---
>  .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 26 +++++++++++++------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> index 78a9afe8f063..5f06e18f0a61 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> @@ -131,21 +131,29 @@ static u8 *stdp2690_get_edid(struct i2c_client *client)
>  	return NULL;
>  }
>  
> -static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
> +static struct edid *ge_b850v3_lvds_get_edid(
> +		struct drm_bridge *bridge, struct drm_connector *connector)
>  {
>  	struct i2c_client *client;
> -	int num_modes = 0;
>  
>  	client = ge_b850v3_lvds_ptr->stdp2690_i2c;
>  
>  	kfree(ge_b850v3_lvds_ptr->edid);
>  	ge_b850v3_lvds_ptr->edid = (struct edid *)stdp2690_get_edid(client);
>  
> -	if (ge_b850v3_lvds_ptr->edid) {
> -		drm_connector_update_edid_property(connector,
> -						      ge_b850v3_lvds_ptr->edid);
> -		num_modes = drm_add_edid_modes(connector,
> -					       ge_b850v3_lvds_ptr->edid);
> +	return ge_b850v3_lvds_ptr->edid;

As pointed out earlier in this series, you can't store a pointer to the
edid, if will get freed by the caller. Fortunately it doesn't seem to be
needed here either.

> +}
> +
> +static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
> +{
> +	struct edid *edid;
> +	int num_modes = 0;
> +
> +	edid = ge_b850v3_lvds_get_edid(&ge_b850v3_lvds_ptr->bridge, connector);
> +
> +	if (edid) {
> +		drm_connector_update_edid_property(connector, edid);
> +		num_modes = drm_add_edid_modes(connector, edid);
>  	}
>  
>  	return num_modes;
> @@ -270,6 +278,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
>  static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {
>  	.attach = ge_b850v3_lvds_attach,
>  	.detect = ge_b850v3_lvds_bridge_detect,
> +	.get_edid = ge_b850v3_lvds_get_edid,
>  };
>  
>  static int ge_b850v3_lvds_init(struct device *dev)
> @@ -324,7 +333,8 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
>  
>  	/* drm bridge initialization */
>  	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
> -	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT;
> +	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT |
> +					 DRM_BRIDGE_OP_EDID;
>  	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 17/21] drm/bridge: megachips: make connector creation optional

[Sam Ravnborg]

Make the connector creation optional to enable usage of the
megachips-stdpxxxx-ge-b850v3-fw bridge with the DRM bridge connector helper.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
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>
---
 .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 5f06e18f0a61..991417ab35b6 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -251,16 +251,6 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 {
 	struct i2c_client *stdp4028_i2c
 			= ge_b850v3_lvds_ptr->stdp4028_i2c;
-	int ret;
-
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
-
-	ret = ge_b850v3_lvds_create_connector(bridge);
-	if (ret)
-		return ret;
 
 	/* Configures the bridge to re-enable interrupts after each ack. */
 	i2c_smbus_write_word_data(stdp4028_i2c,
@@ -272,7 +262,10 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 				  STDP4028_DPTX_IRQ_EN_REG,
 				  STDP4028_DPTX_IRQ_CONFIG);
 
-	return 0;
+	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+		return 0;
+
+	return ge_b850v3_lvds_create_connector(bridge);
 }
 
 static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 17/21] drm/bridge: megachips: make connector creation optional

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:13PM +0200, Sam Ravnborg wrote:
> Make the connector creation optional to enable usage of the
> megachips-stdpxxxx-ge-b850v3-fw bridge with the DRM bridge connector helper.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Peter Senna Tschudin <peter.senna@gmail.com>
> Cc: Martin Donnelly <martin.donnelly@ge.com>
> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> 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>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> index 5f06e18f0a61..991417ab35b6 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> @@ -251,16 +251,6 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
>  {
>  	struct i2c_client *stdp4028_i2c
>  			= ge_b850v3_lvds_ptr->stdp4028_i2c;
> -	int ret;
> -
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> -	}
> -
> -	ret = ge_b850v3_lvds_create_connector(bridge);
> -	if (ret)
> -		return ret;
>  
>  	/* Configures the bridge to re-enable interrupts after each ack. */
>  	i2c_smbus_write_word_data(stdp4028_i2c,
> @@ -272,7 +262,10 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
>  				  STDP4028_DPTX_IRQ_EN_REG,
>  				  STDP4028_DPTX_IRQ_CONFIG);
>  
> -	return 0;
> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> +		return 0;
> +
> +	return ge_b850v3_lvds_create_connector(bridge);
>  }
>  
>  static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 18/21] drm/bridge: nxp-ptn3460: add drm_panel_bridge support

[Sam Ravnborg]

Prepare the bridge driver for use in a chained setup by
replacing direct use of drm_panel with drm_panel_bridge support.

Note: the bridge panel will use the connector type from the panel.

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/nxp-ptn3460.c | 51 ++++++++--------------------
 1 file changed, 14 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index 438e566ce0a4..0bd9f0e451b3 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -30,7 +30,7 @@ struct ptn3460_bridge {
 	struct i2c_client *client;
 	struct drm_bridge bridge;
 	struct edid *edid;
-	struct drm_panel *panel;
+	struct drm_bridge *panel_bridge;
 	struct gpio_desc *gpio_pd_n;
 	struct gpio_desc *gpio_rst_n;
 	u32 edid_emulation;
@@ -127,11 +127,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
 	usleep_range(10, 20);
 	gpiod_set_value(ptn_bridge->gpio_rst_n, 1);
 
-	if (drm_panel_prepare(ptn_bridge->panel)) {
-		DRM_ERROR("failed to prepare panel\n");
-		return;
-	}
-
 	/*
 	 * There's a bug in the PTN chip where it falsely asserts hotplug before
 	 * it is fully functional. We're forced to wait for the maximum start up
@@ -146,16 +141,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
 	ptn_bridge->enabled = true;
 }
 
-static void ptn3460_enable(struct drm_bridge *bridge)
-{
-	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
-
-	if (drm_panel_enable(ptn_bridge->panel)) {
-		DRM_ERROR("failed to enable panel\n");
-		return;
-	}
-}
-
 static void ptn3460_disable(struct drm_bridge *bridge)
 {
 	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
@@ -165,25 +150,10 @@ static void ptn3460_disable(struct drm_bridge *bridge)
 
 	ptn_bridge->enabled = false;
 
-	if (drm_panel_disable(ptn_bridge->panel)) {
-		DRM_ERROR("failed to disable panel\n");
-		return;
-	}
-
 	gpiod_set_value(ptn_bridge->gpio_rst_n, 1);
 	gpiod_set_value(ptn_bridge->gpio_pd_n, 0);
 }
 
-static void ptn3460_post_disable(struct drm_bridge *bridge)
-{
-	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
-
-	if (drm_panel_unprepare(ptn_bridge->panel)) {
-		DRM_ERROR("failed to unprepare panel\n");
-		return;
-	}
-}
-
 static int ptn3460_get_modes(struct drm_connector *connector)
 {
 	struct ptn3460_bridge *ptn_bridge;
@@ -242,6 +212,11 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
 	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
 	int ret;
 
+	ret = drm_bridge_attach(bridge->encoder, ptn_bridge->panel_bridge,
+				bridge, flags);
+	if (ret < 0)
+		return ret;
+
 	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
 		DRM_ERROR("Fix bridge driver to make connector optional!");
 		return -EINVAL;
@@ -265,9 +240,6 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
 	drm_connector_attach_encoder(&ptn_bridge->connector,
 							bridge->encoder);
 
-	if (ptn_bridge->panel)
-		drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector);
-
 	drm_helper_hpd_irq_event(ptn_bridge->connector.dev);
 
 	return ret;
@@ -275,9 +247,7 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
 
 static const struct drm_bridge_funcs ptn3460_bridge_funcs = {
 	.pre_enable = ptn3460_pre_enable,
-	.enable = ptn3460_enable,
 	.disable = ptn3460_disable,
-	.post_disable = ptn3460_post_disable,
 	.attach = ptn3460_bridge_attach,
 };
 
@@ -286,6 +256,8 @@ static int ptn3460_probe(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	struct ptn3460_bridge *ptn_bridge;
+	struct drm_bridge *pbridge;
+	struct drm_panel *panel;
 	int ret;
 
 	ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL);
@@ -293,10 +265,15 @@ static int ptn3460_probe(struct i2c_client *client,
 		return -ENOMEM;
 	}
 
-	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &ptn_bridge->panel, NULL);
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL);
 	if (ret)
 		return ret;
 
+	pbridge = devm_drm_panel_bridge_add(dev, panel);
+	if (IS_ERR(pbridge))
+		return PTR_ERR(pbridge);
+
+	ptn_bridge->panel_bridge = pbridge;
 	ptn_bridge->client = client;
 
 	ptn_bridge->gpio_pd_n = devm_gpiod_get(&client->dev, "powerdown",
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 18/21] drm/bridge: nxp-ptn3460: add drm_panel_bridge support

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:14PM +0200, Sam Ravnborg wrote:
> Prepare the bridge driver for use in a chained setup by
> replacing direct use of drm_panel with drm_panel_bridge support.
> 
> Note: the bridge panel will use the connector type from the panel.
> 
> 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/nxp-ptn3460.c | 51 ++++++++--------------------
>  1 file changed, 14 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> index 438e566ce0a4..0bd9f0e451b3 100644
> --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> @@ -30,7 +30,7 @@ struct ptn3460_bridge {
>  	struct i2c_client *client;
>  	struct drm_bridge bridge;
>  	struct edid *edid;
> -	struct drm_panel *panel;
> +	struct drm_bridge *panel_bridge;
>  	struct gpio_desc *gpio_pd_n;
>  	struct gpio_desc *gpio_rst_n;
>  	u32 edid_emulation;
> @@ -127,11 +127,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>  	usleep_range(10, 20);
>  	gpiod_set_value(ptn_bridge->gpio_rst_n, 1);
>  
> -	if (drm_panel_prepare(ptn_bridge->panel)) {
> -		DRM_ERROR("failed to prepare panel\n");
> -		return;
> -	}
> -
>  	/*
>  	 * There's a bug in the PTN chip where it falsely asserts hotplug before
>  	 * it is fully functional. We're forced to wait for the maximum start up
> @@ -146,16 +141,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>  	ptn_bridge->enabled = true;
>  }
>  
> -static void ptn3460_enable(struct drm_bridge *bridge)
> -{
> -	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
> -
> -	if (drm_panel_enable(ptn_bridge->panel)) {
> -		DRM_ERROR("failed to enable panel\n");
> -		return;
> -	}
> -}
> -
>  static void ptn3460_disable(struct drm_bridge *bridge)
>  {
>  	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
> @@ -165,25 +150,10 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>  
>  	ptn_bridge->enabled = false;
>  
> -	if (drm_panel_disable(ptn_bridge->panel)) {
> -		DRM_ERROR("failed to disable panel\n");
> -		return;
> -	}
> -
>  	gpiod_set_value(ptn_bridge->gpio_rst_n, 1);
>  	gpiod_set_value(ptn_bridge->gpio_pd_n, 0);
>  }
>  
> -static void ptn3460_post_disable(struct drm_bridge *bridge)
> -{
> -	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
> -
> -	if (drm_panel_unprepare(ptn_bridge->panel)) {
> -		DRM_ERROR("failed to unprepare panel\n");
> -		return;
> -	}
> -}
> -
>  static int ptn3460_get_modes(struct drm_connector *connector)
>  {
>  	struct ptn3460_bridge *ptn_bridge;
> @@ -242,6 +212,11 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
>  	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
>  	int ret;
>  
> +	ret = drm_bridge_attach(bridge->encoder, ptn_bridge->panel_bridge,
> +				bridge, flags);

Same as earlier in the series about the flags.

> +	if (ret < 0)
> +		return ret;
> +
>  	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>  		DRM_ERROR("Fix bridge driver to make connector optional!");
>  		return -EINVAL;
> @@ -265,9 +240,6 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
>  	drm_connector_attach_encoder(&ptn_bridge->connector,
>  							bridge->encoder);
>  
> -	if (ptn_bridge->panel)
> -		drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector);
> -
>  	drm_helper_hpd_irq_event(ptn_bridge->connector.dev);
>  
>  	return ret;
> @@ -275,9 +247,7 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
>  
>  static const struct drm_bridge_funcs ptn3460_bridge_funcs = {
>  	.pre_enable = ptn3460_pre_enable,
> -	.enable = ptn3460_enable,
>  	.disable = ptn3460_disable,
> -	.post_disable = ptn3460_post_disable,
>  	.attach = ptn3460_bridge_attach,
>  };
>  
> @@ -286,6 +256,8 @@ static int ptn3460_probe(struct i2c_client *client,
>  {
>  	struct device *dev = &client->dev;
>  	struct ptn3460_bridge *ptn_bridge;
> +	struct drm_bridge *pbridge;
> +	struct drm_panel *panel;
>  	int ret;
>  
>  	ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL);
> @@ -293,10 +265,15 @@ static int ptn3460_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  	}
>  
> -	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &ptn_bridge->panel, NULL);
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL);
>  	if (ret)
>  		return ret;
>  
> +	pbridge = devm_drm_panel_bridge_add(dev, panel);
> +	if (IS_ERR(pbridge))
> +		return PTR_ERR(pbridge);
> +
> +	ptn_bridge->panel_bridge = pbridge;
>  	ptn_bridge->client = client;
>  
>  	ptn_bridge->gpio_pd_n = devm_gpiod_get(&client->dev, "powerdown",

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 19/21] drm/bridge: nxp-ptn3460: add get_modes bridge operation

[Sam Ravnborg]

Add the get_modes() bridge operation to prepare for
use as a chained bridge.
Add helper function that is also used by the connector.

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/nxp-ptn3460.c | 52 ++++++++++++++++++----------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index 0bd9f0e451b3..e253c185f94c 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -154,17 +154,13 @@ static void ptn3460_disable(struct drm_bridge *bridge)
 	gpiod_set_value(ptn_bridge->gpio_pd_n, 0);
 }
 
-static int ptn3460_get_modes(struct drm_connector *connector)
+static struct edid *ptn3460_get_edid(struct drm_bridge *bridge,
+				     struct drm_connector *connector)
 {
-	struct ptn3460_bridge *ptn_bridge;
-	u8 *edid;
-	int ret, num_modes = 0;
+	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
 	bool power_off;
-
-	ptn_bridge = connector_to_ptn3460(connector);
-
-	if (ptn_bridge->edid)
-		return drm_add_edid_modes(connector, ptn_bridge->edid);
+	u8 *edid;
+	int ret;
 
 	power_off = !ptn_bridge->enabled;
 	ptn3460_pre_enable(&ptn_bridge->bridge);
@@ -172,30 +168,46 @@ static int ptn3460_get_modes(struct drm_connector *connector)
 	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
 	if (!edid) {
 		DRM_ERROR("Failed to allocate EDID\n");
-		return 0;
+		return NULL;
 	}
 
 	ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid,
-			EDID_LENGTH);
+				 EDID_LENGTH);
 	if (ret) {
 		kfree(edid);
-		goto out;
+		return NULL;
 	}
 
+	if (power_off)
+		ptn3460_disable(&ptn_bridge->bridge);
+
+	kfree(ptn_bridge->edid);
 	ptn_bridge->edid = (struct edid *)edid;
-	drm_connector_update_edid_property(connector, ptn_bridge->edid);
 
-	num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
+	return ptn_bridge->edid;
+}
 
-out:
-	if (power_off)
-		ptn3460_disable(&ptn_bridge->bridge);
+static int ptn3460_connector_get_modes(struct drm_connector *connector)
+{
+	struct ptn3460_bridge *ptn_bridge;
+	struct edid *edid;
+
+	ptn_bridge = connector_to_ptn3460(connector);
+
+	if (ptn_bridge->edid)
+		return drm_add_edid_modes(connector, ptn_bridge->edid);
+
+	edid = ptn3460_get_edid(&ptn_bridge->bridge, connector);
+	if (!edid)
+		return 0;
+
+	drm_connector_update_edid_property(connector, edid);
 
-	return num_modes;
+	return drm_add_edid_modes(connector, edid);
 }
 
 static const struct drm_connector_helper_funcs ptn3460_connector_helper_funcs = {
-	.get_modes = ptn3460_get_modes,
+	.get_modes = ptn3460_connector_get_modes,
 };
 
 static const struct drm_connector_funcs ptn3460_connector_funcs = {
@@ -249,6 +261,7 @@ static const struct drm_bridge_funcs ptn3460_bridge_funcs = {
 	.pre_enable = ptn3460_pre_enable,
 	.disable = ptn3460_disable,
 	.attach = ptn3460_bridge_attach,
+	.get_edid = ptn3460_get_edid,
 };
 
 static int ptn3460_probe(struct i2c_client *client,
@@ -304,6 +317,7 @@ static int ptn3460_probe(struct i2c_client *client,
 	}
 
 	ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
+	ptn_bridge->bridge.ops = DRM_BRIDGE_OP_EDID;
 	ptn_bridge->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ptn_bridge->bridge);
 
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 19/21] drm/bridge: nxp-ptn3460: add get_modes bridge operation

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:15PM +0200, Sam Ravnborg wrote:
> Add the get_modes() bridge operation to prepare for
> use as a chained bridge.
> Add helper function that is also used by the connector.
> 
> 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/nxp-ptn3460.c | 52 ++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> index 0bd9f0e451b3..e253c185f94c 100644
> --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> @@ -154,17 +154,13 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>  	gpiod_set_value(ptn_bridge->gpio_pd_n, 0);
>  }
>  
> -static int ptn3460_get_modes(struct drm_connector *connector)
> +static struct edid *ptn3460_get_edid(struct drm_bridge *bridge,
> +				     struct drm_connector *connector)
>  {
> -	struct ptn3460_bridge *ptn_bridge;
> -	u8 *edid;
> -	int ret, num_modes = 0;
> +	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
>  	bool power_off;
> -
> -	ptn_bridge = connector_to_ptn3460(connector);
> -
> -	if (ptn_bridge->edid)
> -		return drm_add_edid_modes(connector, ptn_bridge->edid);
> +	u8 *edid;
> +	int ret;
>  
>  	power_off = !ptn_bridge->enabled;
>  	ptn3460_pre_enable(&ptn_bridge->bridge);
> @@ -172,30 +168,46 @@ static int ptn3460_get_modes(struct drm_connector *connector)
>  	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
>  	if (!edid) {
>  		DRM_ERROR("Failed to allocate EDID\n");
> -		return 0;
> +		return NULL;

Don't you need to power off in the error path ?

>  	}
>  
>  	ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid,
> -			EDID_LENGTH);
> +				 EDID_LENGTH);
>  	if (ret) {
>  		kfree(edid);
> -		goto out;
> +		return NULL;

And here too ?

>  	}
>  
> +	if (power_off)
> +		ptn3460_disable(&ptn_bridge->bridge);
> +
> +	kfree(ptn_bridge->edid);
>  	ptn_bridge->edid = (struct edid *)edid;
> -	drm_connector_update_edid_property(connector, ptn_bridge->edid);
>  
> -	num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
> +	return ptn_bridge->edid;

Same comment as earlier in this series about storing the edid.

> +}
>  
> -out:
> -	if (power_off)
> -		ptn3460_disable(&ptn_bridge->bridge);
> +static int ptn3460_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct ptn3460_bridge *ptn_bridge;
> +	struct edid *edid;
> +
> +	ptn_bridge = connector_to_ptn3460(connector);

Maybe

	struct ptn3460_bridge *ptn_bridge = connector_to_ptn3460(connector);

?

> +
> +	if (ptn_bridge->edid)
> +		return drm_add_edid_modes(connector, ptn_bridge->edid);
> +
> +	edid = ptn3460_get_edid(&ptn_bridge->bridge, connector);
> +	if (!edid)
> +		return 0;
> +
> +	drm_connector_update_edid_property(connector, edid);
>  
> -	return num_modes;
> +	return drm_add_edid_modes(connector, edid);
>  }
>  
>  static const struct drm_connector_helper_funcs ptn3460_connector_helper_funcs = {
> -	.get_modes = ptn3460_get_modes,
> +	.get_modes = ptn3460_connector_get_modes,
>  };
>  
>  static const struct drm_connector_funcs ptn3460_connector_funcs = {
> @@ -249,6 +261,7 @@ static const struct drm_bridge_funcs ptn3460_bridge_funcs = {
>  	.pre_enable = ptn3460_pre_enable,
>  	.disable = ptn3460_disable,
>  	.attach = ptn3460_bridge_attach,
> +	.get_edid = ptn3460_get_edid,
>  };
>  
>  static int ptn3460_probe(struct i2c_client *client,
> @@ -304,6 +317,7 @@ static int ptn3460_probe(struct i2c_client *client,
>  	}
>  
>  	ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
> +	ptn_bridge->bridge.ops = DRM_BRIDGE_OP_EDID;
>  	ptn_bridge->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ptn_bridge->bridge);
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 20/21] drm/bridge: nxp-ptn3460: make connector creation optional

[Sam Ravnborg]

Make the connector creation optional to enable usage of the
nxp-ptn3460 bridge with the DRM bridge connector helper.

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/nxp-ptn3460.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index e253c185f94c..6a65657087f9 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -229,10 +229,8 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
 	if (ret < 0)
 		return ret;
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
+	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+		return 0;
 
 	if (!bridge->encoder) {
 		DRM_ERROR("Parent encoder object not found");
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 20/21] drm/bridge: nxp-ptn3460: make connector creation optional

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:16PM +0200, Sam Ravnborg wrote:
> Make the connector creation optional to enable usage of the
> nxp-ptn3460 bridge with the DRM bridge connector helper.
> 
> 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>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/nxp-ptn3460.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> index e253c185f94c..6a65657087f9 100644
> --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> @@ -229,10 +229,8 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
>  	if (ret < 0)
>  		return ret;
>  
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> -	}
> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> +		return 0;
>  
>  	if (!bridge->encoder) {
>  		DRM_ERROR("Parent encoder object not found");

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 21/21] drm/bridge: ti-sn65dsi86: add drm_panel_bridge support

[Sam Ravnborg]

Prepare the bridge driver for use in a chained setup by
replacing direct use of drm_panel with drm_panel_bridge support.

Note: the bridge panel will use the connector type from the panel.

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/ti-sn65dsi86.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 0f75bb2d7f56..ecf0693e3018 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -144,7 +144,7 @@ struct ti_sn_bridge {
 	struct device_node		*host_node;
 	struct mipi_dsi_device		*dsi;
 	struct clk			*refclk;
-	struct drm_panel		*panel;
+	struct drm_bridge		*panel_bridge;
 	struct gpio_desc		*enable_gpio;
 	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
 	int				dp_lanes;
@@ -263,7 +263,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 {
 	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
 
-	return drm_panel_get_modes(pdata->panel, connector);
+	return drm_bridge_get_modes(pdata->panel_bridge, connector);
 }
 
 static enum drm_mode_status
@@ -395,9 +395,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 	pdata->dsi = dsi;
 
 	/* attach panel to bridge */
-	drm_panel_attach(pdata->panel, &pdata->connector);
-
-	return 0;
+	return drm_bridge_attach(bridge->encoder, pdata->panel_bridge,
+				 bridge, flags);
 
 err_dsi_attach:
 	mipi_dsi_device_unregister(dsi);
@@ -410,16 +409,12 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
 
-	drm_panel_disable(pdata->panel);
-
 	/* disable video stream */
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
 	/* semi auto link training mode OFF */
 	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0);
 	/* disable DP PLL */
 	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
-
-	drm_panel_unprepare(pdata->panel);
 }
 
 static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
@@ -780,8 +775,6 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	/* enable video stream */
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE,
 			   VSTREAM_ENABLE);
-
-	drm_panel_enable(pdata->panel);
 }
 
 static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
@@ -811,8 +804,6 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 	 */
 	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
 			   HPD_DISABLE);
-
-	drm_panel_prepare(pdata->panel);
 }
 
 static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
@@ -1163,6 +1154,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
 	struct ti_sn_bridge *pdata;
+	struct drm_bridge *bridge;
+	struct drm_panel *panel;
 	int ret;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
@@ -1185,12 +1178,18 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 	pdata->dev = &client->dev;
 
 	ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0,
-					  &pdata->panel, NULL);
+					  &panel, NULL);
 	if (ret) {
 		DRM_ERROR("could not find any panel node\n");
 		return ret;
 	}
 
+	bridge = devm_drm_panel_bridge_add(pdata->dev, panel);
+	if (IS_ERR(bridge))
+		return PTR_ERR(bridge);
+
+	pdata->panel_bridge = bridge;
+
 	dev_set_drvdata(&client->dev, pdata);
 
 	pdata->enable_gpio = devm_gpiod_get(pdata->dev, "enable",
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 21/21] drm/bridge: ti-sn65dsi86: add drm_panel_bridge support

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:24:17PM +0200, Sam Ravnborg wrote:
> Prepare the bridge driver for use in a chained setup by
> replacing direct use of drm_panel with drm_panel_bridge support.
> 
> Note: the bridge panel will use the connector type from the panel.
> 
> 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/ti-sn65dsi86.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 0f75bb2d7f56..ecf0693e3018 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -144,7 +144,7 @@ struct ti_sn_bridge {
>  	struct device_node		*host_node;
>  	struct mipi_dsi_device		*dsi;
>  	struct clk			*refclk;
> -	struct drm_panel		*panel;
> +	struct drm_bridge		*panel_bridge;
>  	struct gpio_desc		*enable_gpio;
>  	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
>  	int				dp_lanes;
> @@ -263,7 +263,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
>  
> -	return drm_panel_get_modes(pdata->panel, connector);
> +	return drm_bridge_get_modes(pdata->panel_bridge, connector);
>  }
>  
>  static enum drm_mode_status
> @@ -395,9 +395,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>  	pdata->dsi = dsi;
>  
>  	/* attach panel to bridge */
> -	drm_panel_attach(pdata->panel, &pdata->connector);
> -
> -	return 0;
> +	return drm_bridge_attach(bridge->encoder, pdata->panel_bridge,
> +				 bridge, flags);

Same comment as earlier in this series regarding the flags.

I suppose attaching the panel bridge will be moved before creating the
connector in a future patch series ?

>  
>  err_dsi_attach:
>  	mipi_dsi_device_unregister(dsi);
> @@ -410,16 +409,12 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
>  {
>  	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>  
> -	drm_panel_disable(pdata->panel);
> -
>  	/* disable video stream */
>  	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
>  	/* semi auto link training mode OFF */
>  	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0);
>  	/* disable DP PLL */
>  	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
> -
> -	drm_panel_unprepare(pdata->panel);
>  }
>  
>  static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
> @@ -780,8 +775,6 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>  	/* enable video stream */
>  	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE,
>  			   VSTREAM_ENABLE);
> -
> -	drm_panel_enable(pdata->panel);
>  }
>  
>  static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> @@ -811,8 +804,6 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
>  	 */
>  	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
>  			   HPD_DISABLE);
> -
> -	drm_panel_prepare(pdata->panel);
>  }
>  
>  static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> @@ -1163,6 +1154,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>  			      const struct i2c_device_id *id)
>  {
>  	struct ti_sn_bridge *pdata;
> +	struct drm_bridge *bridge;
> +	struct drm_panel *panel;
>  	int ret;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> @@ -1185,12 +1178,18 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>  	pdata->dev = &client->dev;
>  
>  	ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0,
> -					  &pdata->panel, NULL);
> +					  &panel, NULL);
>  	if (ret) {
>  		DRM_ERROR("could not find any panel node\n");
>  		return ret;
>  	}
>  
> +	bridge = devm_drm_panel_bridge_add(pdata->dev, panel);
> +	if (IS_ERR(bridge))
> +		return PTR_ERR(bridge);
> +
> +	pdata->panel_bridge = bridge;
> +
>  	dev_set_drvdata(&client->dev, pdata);
>  
>  	pdata->enable_gpio = devm_gpiod_get(pdata->dev, "enable",

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel