[PATCH v1 0/4] drm: tc358764: support drm bridge connector helper

[PATCH v1 0/4] drm: tc358764: support drm bridge connector helper

[Sam Ravnborg]

Somehow Laurent tricked me into updating a bridge driver.
In reality, having updated a driver yourself makes for much
better understanding of what is going on.
So in order to provide proper review feedback I deciced to give
it a spin. tc358764 was selected as it is a simple bridge driver,
so a good driver to start out with.

We are moving to a model where the panel tell what connector
it uses - so this patch assumes this model is in place.

The only device tree in the kernel that uses tc358764
use the boe,hv070wsa-100 panel, so update this panel to report
the correct connector type to support the model described above.

The "drop drm_connector_(un)register" patch drops some
unnessesary calls related to drm_connector.

Next update tc358764 to use drm_panel_bridge - which was a
nice simplification of the driver.

The last patch to support optional connector creation was
then simple to implement.

The patchset has not seen any run-time test.
So testing feedback is appreciated. The only in-kernel
user seems to be: exynos5250-arndale

Review feedback is likewise appreciated/expected - bridge
drivers is not my expertise area.

	Sam


Sam Ravnborg (4):
      drm/panel: add connector type to boe,hv070wsa-100 panel
      drm/bridge: tc358764: drop drm_connector_(un)register
      drm/bridge: tc358764: add drm_panel_bridge support
      drm/bridge: tc358764: make connector creation optional

 drivers/gpu/drm/bridge/tc358764.c    | 68 ++++++++++--------------------------
 drivers/gpu/drm/panel/panel-simple.c |  1 +
 2 files changed, 19 insertions(+), 50 deletions(-)


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

[PATCH v1 1/4] 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.

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

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 44a1f5dfb571..749115c98ee2 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1059,6 +1059,7 @@ static const struct panel_desc boe_hv070wsa = {
 		.width = 154,
 		.height = 90,
 	},
+	.connector_type = DRM_MODE_CONNECTOR_LVDS,
 };
 
 static const struct drm_display_mode boe_nv101wxmn51_modes[] = {
-- 
2.20.1

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

Re: [PATCH v1 1/4] drm/panel: add connector type to boe, hv070wsa-100 panel

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Tue, Apr 14, 2020 at 10:47:24AM +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.
> 
> 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 44a1f5dfb571..749115c98ee2 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1059,6 +1059,7 @@ static const struct panel_desc boe_hv070wsa = {
>  		.width = 154,
>  		.height = 90,
>  	},
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,

While at it, could you set .bus_format too ? It's particularly important
for LVDS panels.

>  };
>  
>  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 v1 2/4] drm/bridge: tc358764: drop drm_connector_(un)register

[Sam Ravnborg]

Drop drm_connector handling that is not needed:

- drm_dev_register() in the display controlelr 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>
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.20.1

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

Re: [PATCH v1 2/4] drm/bridge: tc358764: drop drm_connector_(un)register

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Tue, Apr 14, 2020 at 10:47:25AM +0200, Sam Ravnborg wrote:
> Drop drm_connector handling that is not needed:
> 
> - drm_dev_register() in the display controlelr driver takes

s/controlelr/controller/

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

-- 
Regards,

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

[PATCH v1 3/4] 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.

Note that this change requires that the panel reports the correct
type of 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/tc358764.c | 59 +++++++++----------------------
 1 file changed, 16 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
index a277739fab58..a5abf15e5c7f 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, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	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,15 @@ 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);
+	if (IS_ERR(ctx->panel_bridge))
+		return PTR_ERR(ctx->panel_bridge);
+
+	return 0;
 }
 
 static int tc358764_configure_regulators(struct tc358764 *ctx)
-- 
2.20.1

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

Re: [PATCH v1 3/4] drm/bridge: tc358764: add drm_panel_bridge support

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Tue, Apr 14, 2020 at 10:47:26AM +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 that this change requires that the panel reports the correct
> type of 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/tc358764.c | 59 +++++++++----------------------
>  1 file changed, 16 insertions(+), 43 deletions(-)

I like the diffstat :-)

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

> 
> diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
> index a277739fab58..a5abf15e5c7f 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, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	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,15 @@ 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);
> +	if (IS_ERR(ctx->panel_bridge))
> +		return PTR_ERR(ctx->panel_bridge);
> +
> +	return 0;
>  }
>  
>  static int tc358764_configure_regulators(struct tc358764 *ctx)

-- 
Regards,

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

[PATCH v1 4/4] 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>
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 a5abf15e5c7f..4ebabea86040 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.20.1

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

Re: [PATCH v1 4/4] drm/bridge: tc358764: make connector creation optional

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Tue, Apr 14, 2020 at 10:47:27AM +0200, Sam Ravnborg wrote:
> 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>
> 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>

Thank you for the conversion. "Just a few" more bridge drivers to go,
and then there will be no more excuse not to use the DRM bridge
connector helper :-)

> ---
>  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 a5abf15e5c7f..4ebabea86040 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,

-- 
Regards,

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

Re: [PATCH v1 4/4] drm/bridge: tc358764: make connector creation optional

[Sam Ravnborg]

On Tue, Apr 14, 2020 at 12:38:16PM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> Thank you for the patch.
> 
> On Tue, Apr 14, 2020 at 10:47:27AM +0200, Sam Ravnborg wrote:
> > 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>
> > 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>

Thanks. I will wait a few days for more feedback before applying.
 
> Thank you for the conversion. "Just a few" more bridge drivers to go,
> and then there will be no more excuse not to use the DRM bridge
> connector helper :-)

A quick grep for ATTACH and drm_bridge_attach_flags gave me following list:

adv7511				ongoing, Laurent
analogix/analogix-anx6345	todo
analogix/analogix-anx78xx	todo
analogix/analogix_dp_core	todo
cdns-dsi			done?
display-connector.c		done
lvds-codec			done?
megachips-stdpxxxx-ge-b850v3-fw	todo
nwl-dsi				(ongoing), Guido
nxp-ptn3460			todo
panel.c				done
parade-ps8622			todo
parade-ps8640			done?
sii902x				todo
sil-sii8620			done?
simple-bridge			ongoing, Laurent
synopsys/dw-hdmi		todo
synopsys/dw-mipi-dsi		done?
tc358764			ongoing, Sam
tc358767			todo
tc358768			done?
thc63lvd1024			done?
ti-sn65dsi86			todo
ti-tfp410			done
ti-tpd12s015			todo

3	done
7	done?
4	ongoing
11	todo

Maybe a little simplistic - but gives some kind of overview.

Drivers marked with "Done?" do not have any hits for ATTACH,
dunno if they need more attantion.

I will try to take a look at a few of the trivial conversions
later this week.

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

Re: [PATCH v1 4/4] drm/bridge: tc358764: make connector creation optional

[Laurent Pinchart]

Hi Sam,

On Tue, Apr 14, 2020 at 03:36:26PM +0200, Sam Ravnborg wrote:
> On Tue, Apr 14, 2020 at 12:38:16PM +0300, Laurent Pinchart wrote:
> > On Tue, Apr 14, 2020 at 10:47:27AM +0200, Sam Ravnborg wrote:
> > > 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>
> > > 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>
> 
> Thanks. I will wait a few days for more feedback before applying.
>  
> > Thank you for the conversion. "Just a few" more bridge drivers to go,
> > and then there will be no more excuse not to use the DRM bridge
> > connector helper :-)
> 
> A quick grep for ATTACH and drm_bridge_attach_flags gave me following list:
> 
> adv7511				ongoing, Laurent
> analogix/analogix-anx6345	todo
> analogix/analogix-anx78xx	todo
> analogix/analogix_dp_core	todo
> cdns-dsi			done?
> display-connector.c		done
> lvds-codec			done?
> megachips-stdpxxxx-ge-b850v3-fw	todo
> nwl-dsi				(ongoing), Guido
> nxp-ptn3460			todo
> panel.c				done
> parade-ps8622			todo
> parade-ps8640			done?
> sii902x				todo
> sil-sii8620			done?
> simple-bridge			ongoing, Laurent
> synopsys/dw-hdmi		todo
> synopsys/dw-mipi-dsi		done?
> tc358764			ongoing, Sam
> tc358767			todo
> tc358768			done?
> thc63lvd1024			done?
> ti-sn65dsi86			todo
> ti-tfp410			done
> ti-tpd12s015			todo
> 
> 3	done
> 7	done?
> 4	ongoing
> 11	todo
> 
> Maybe a little simplistic - but gives some kind of overview.

Please note there are also bridge drivers in individual drivers
directories. For instance I'll convert rcar-du/rcar-lvds.

> Drivers marked with "Done?" do not have any hits for ATTACH,
> dunno if they need more attantion.

No, those drivers never had connector support in the first place, so as
far as I can tell they're good. I haven't checked if some of them need
to move to the DRM bridge panel helper, but if they don't create a
connector in the first place, they should already be using it.

> I will try to take a look at a few of the trivial conversions
> later this week.

Thanks a lot for that !

-- 
Regards,

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