linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] drm: bridge: adv7511: Enable usage with DRM bridge connector helper
@ 2020-04-09  0:46 Laurent Pinchart
  2020-04-09  0:46 ` [PATCH 1/4] drm: bridge: adv7511: Split EDID read to a separate function Laurent Pinchart
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Laurent Pinchart @ 2020-04-09  0:46 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-renesas-soc, Andrzej Hajda, Neil Armstrong, Jonas Karlman,
	Jernej Skrabec, Rob Clark, Sean Paul

Hello,

This patch series enables usage of the adv7511 driver with the DRM
bridge connector helper (drm_bridge_connector.c).

Patch 1/4 and 2/4 start by splitting EDID read and connector creation to
separate functions to ease review of 3/4 and 4/4. Patch 3/4 performs the
bulk of the work by implementing the DRM bridge connector-related
operations, and patch 4/4 then makes connector creation optional.

I've had trouble wrapping my head around the HPD handling code in the
av7511 driver (this is why I've CC'ed Rob and Sean who last touched
this). The split of the code between the .detect() operation and the HPD
IRQ seems a bit weird to me, and I haven't dared touching it as it also
appears fragile.

In particular, I'm not sure why we need to retrieve modes in the
.detect() operation. git blame didn't help, as the code has been there
since the beginning. I'd like to remove that completely, but to avoid
breakages, patch 3/4 only does so when the adv7511 is used without
creating a DRM connector.

Usage of both adv7511->status and adv7511->connector.status in the two
operations also seems awkward, and I would like to drop usage of the
latter in the new code path, but I also haven't dared refactoring that
yet. Feedback would be welcome.

Laurent Pinchart (4):
  drm: bridge: adv7511: Split EDID read to a separate function
  drm: bridge: adv7511: Split connector creation to a separate function
  drm: bridge: adv7511: Implement bridge connector operations
  drm: bridge: adv7511: Make connector creation optional

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 137 +++++++++++++------
 1 file changed, 98 insertions(+), 39 deletions(-)

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/4] drm: bridge: adv7511: Split EDID read to a separate function
  2020-04-09  0:46 [PATCH 0/4] drm: bridge: adv7511: Enable usage with DRM bridge connector helper Laurent Pinchart
@ 2020-04-09  0:46 ` Laurent Pinchart
  2020-04-13  5:54   ` Sam Ravnborg
  2020-04-09  0:46 ` [PATCH 2/4] drm: bridge: adv7511: Split connector creation " Laurent Pinchart
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2020-04-09  0:46 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-renesas-soc, Andrzej Hajda, Neil Armstrong, Jonas Karlman,
	Jernej Skrabec, Rob Clark, Sean Paul

To prepare for the implementation of the DRM bridge connector
operations, move EDID read out of adv7511_get_modes() to a separate
function.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 23 ++++++++++++++------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 87b58c1acff4..58d02e92b6b9 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -589,11 +589,10 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
  * ADV75xx helpers
  */
 
-static int adv7511_get_modes(struct adv7511 *adv7511,
-			     struct drm_connector *connector)
+static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
+				     struct drm_connector *connector)
 {
 	struct edid *edid;
-	unsigned int count;
 
 	/* Reading the EDID only works if the device is powered */
 	if (!adv7511->powered) {
@@ -612,15 +611,25 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
 	if (!adv7511->powered)
 		__adv7511_power_off(adv7511);
 
-
-	drm_connector_update_edid_property(connector, edid);
-	count = drm_add_edid_modes(connector, edid);
-
 	adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
 			       drm_detect_hdmi_monitor(edid));
 
 	cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
 
+	return edid;
+}
+
+static int adv7511_get_modes(struct adv7511 *adv7511,
+			     struct drm_connector *connector)
+{
+	struct edid *edid;
+	unsigned int count;
+
+	edid = adv7511_get_edid(adv7511, connector);
+
+	drm_connector_update_edid_property(connector, edid);
+	count = drm_add_edid_modes(connector, edid);
+
 	kfree(edid);
 
 	return count;
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/4] drm: bridge: adv7511: Split connector creation to a separate function
  2020-04-09  0:46 [PATCH 0/4] drm: bridge: adv7511: Enable usage with DRM bridge connector helper Laurent Pinchart
  2020-04-09  0:46 ` [PATCH 1/4] drm: bridge: adv7511: Split EDID read to a separate function Laurent Pinchart
@ 2020-04-09  0:46 ` Laurent Pinchart
  2020-04-13  5:58   ` Sam Ravnborg
  2020-04-09  0:46 ` [PATCH 3/4] drm: bridge: adv7511: Implement bridge connector operations Laurent Pinchart
  2020-04-09  0:46 ` [PATCH 4/4] drm: bridge: adv7511: Make connector creation optional Laurent Pinchart
  3 siblings, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2020-04-09  0:46 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-renesas-soc, Andrzej Hajda, Neil Armstrong, Jonas Karlman,
	Jernej Skrabec, Rob Clark, Sean Paul

To prepare for making the connector creation optional, move the related
code out of adv7511_bridge_attach() to a separate function.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 62 +++++++++++++-------
 1 file changed, 40 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 58d02e92b6b9..e3b62ad95389 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -783,7 +783,10 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
 	adv7511->f_tmds = mode->clock;
 }
 
-/* Connector funcs */
+/* -----------------------------------------------------------------------------
+ * DRM Connector Operations
+ */
+
 static struct adv7511 *connector_to_adv7511(struct drm_connector *connector)
 {
 	return container_of(connector, struct adv7511, connector);
@@ -827,7 +830,40 @@ static const struct drm_connector_funcs adv7511_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-/* Bridge funcs */
+static int adv7511_connector_init(struct adv7511 *adv)
+{
+	struct drm_bridge *bridge = &adv->bridge;
+	int ret;
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found");
+		return -ENODEV;
+	}
+
+	if (adv->i2c_main->irq)
+		adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
+	else
+		adv->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
+				DRM_CONNECTOR_POLL_DISCONNECT;
+
+	ret = drm_connector_init(bridge->dev, &adv->connector,
+				 &adv7511_connector_funcs,
+				 DRM_MODE_CONNECTOR_HDMIA);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+	drm_connector_helper_add(&adv->connector,
+				 &adv7511_connector_helper_funcs);
+	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
+
+	return 0;
+}
+
+/* -----------------------------------------------------------------------------
+ * DRM Bridge Operations
+ */
+
 static struct adv7511 *bridge_to_adv7511(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct adv7511, bridge);
@@ -867,27 +903,9 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge,
 		return -EINVAL;
 	}
 
-	if (!bridge->encoder) {
-		DRM_ERROR("Parent encoder object not found");
-		return -ENODEV;
-	}
-
-	if (adv->i2c_main->irq)
-		adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
-	else
-		adv->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
-				DRM_CONNECTOR_POLL_DISCONNECT;
-
-	ret = drm_connector_init(bridge->dev, &adv->connector,
-				 &adv7511_connector_funcs,
-				 DRM_MODE_CONNECTOR_HDMIA);
-	if (ret) {
-		DRM_ERROR("Failed to initialize connector with drm\n");
+	ret = adv7511_connector_init(adv);
+	if (ret < 0)
 		return ret;
-	}
-	drm_connector_helper_add(&adv->connector,
-				 &adv7511_connector_helper_funcs);
-	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
 
 	if (adv->type == ADV7533 || adv->type == ADV7535)
 		ret = adv7533_attach_dsi(adv);
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/4] drm: bridge: adv7511: Implement bridge connector operations
  2020-04-09  0:46 [PATCH 0/4] drm: bridge: adv7511: Enable usage with DRM bridge connector helper Laurent Pinchart
  2020-04-09  0:46 ` [PATCH 1/4] drm: bridge: adv7511: Split EDID read to a separate function Laurent Pinchart
  2020-04-09  0:46 ` [PATCH 2/4] drm: bridge: adv7511: Split connector creation " Laurent Pinchart
@ 2020-04-09  0:46 ` Laurent Pinchart
  2020-04-09  0:46 ` [PATCH 4/4] drm: bridge: adv7511: Make connector creation optional Laurent Pinchart
  3 siblings, 0 replies; 8+ messages in thread
From: Laurent Pinchart @ 2020-04-09  0:46 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-renesas-soc, Andrzej Hajda, Neil Armstrong, Jonas Karlman,
	Jernej Skrabec, Rob Clark, Sean Paul

Implement the bridge connector-related .get_edid(), .detect() and
.hpd_notify() operations, and report the related bridge capabilities.

Output status detection is implemented using the same backend as for the
DRM connector, but requires making mode retrieval at detection time
optional as no pointer to the connector is available to the bridge
.detect() operation. The reason for the need to retrieve modes at
detection time is unclear to me, and this may benefit from further
refactoring of hot plug handling code.

Hot plug detection is notified through the bridge HPD notification
framework when the bridge is used without creating a connector, and
falls back to the existing implementation otherwise. CEC handling of
disconnection is handled in the new .hpd_notify() operation in the new
code path.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 43 ++++++++++++++++++--
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index e3b62ad95389..723560b36ee0 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -443,9 +443,14 @@ static void adv7511_hpd_work(struct work_struct *work)
 
 	if (adv7511->connector.status != status) {
 		adv7511->connector.status = status;
-		if (status == connector_status_disconnected)
-			cec_phys_addr_invalidate(adv7511->cec_adap);
-		drm_kms_helper_hotplug_event(adv7511->connector.dev);
+
+		if (adv7511->connector.dev) {
+			if (status == connector_status_disconnected)
+				cec_phys_addr_invalidate(adv7511->cec_adap);
+			drm_kms_helper_hotplug_event(adv7511->connector.dev);
+		} else {
+			drm_bridge_hpd_notify(&adv7511->bridge, status);
+		}
 	}
 }
 
@@ -661,7 +666,8 @@ adv7511_detect(struct adv7511 *adv7511, struct drm_connector *connector)
 	if (status == connector_status_connected && hpd && adv7511->powered) {
 		regcache_mark_dirty(adv7511->regmap);
 		adv7511_power_on(adv7511);
-		adv7511_get_modes(adv7511, connector);
+		if (connector)
+			adv7511_get_modes(adv7511, connector);
 		if (adv7511->status == connector_status_connected)
 			status = connector_status_disconnected;
 	} else {
@@ -917,11 +923,38 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge,
 	return ret;
 }
 
+static enum drm_connector_status adv7511_bridge_detect(struct drm_bridge *bridge)
+{
+	struct adv7511 *adv = bridge_to_adv7511(bridge);
+
+	return adv7511_detect(adv, NULL);
+}
+
+static struct edid *adv7511_bridge_get_edid(struct drm_bridge *bridge,
+					    struct drm_connector *connector)
+{
+	struct adv7511 *adv = bridge_to_adv7511(bridge);
+
+	return adv7511_get_edid(adv, connector);
+}
+
+static void adv7511_bridge_hpd_notify(struct drm_bridge *bridge,
+				      enum drm_connector_status status)
+{
+	struct adv7511 *adv = bridge_to_adv7511(bridge);
+
+	if (status == connector_status_disconnected)
+		cec_phys_addr_invalidate(adv->cec_adap);
+}
+
 static const struct drm_bridge_funcs adv7511_bridge_funcs = {
 	.enable = adv7511_bridge_enable,
 	.disable = adv7511_bridge_disable,
 	.mode_set = adv7511_bridge_mode_set,
 	.attach = adv7511_bridge_attach,
+	.detect = adv7511_bridge_detect,
+	.get_edid = adv7511_bridge_get_edid,
+	.hpd_notify = adv7511_bridge_hpd_notify,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1250,6 +1283,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 		goto err_unregister_cec;
 
 	adv7511->bridge.funcs = &adv7511_bridge_funcs;
+	adv7511->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID
+			    | DRM_BRIDGE_OP_HPD;
 	adv7511->bridge.of_node = dev->of_node;
 
 	drm_bridge_add(&adv7511->bridge);
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/4] drm: bridge: adv7511: Make connector creation optional
  2020-04-09  0:46 [PATCH 0/4] drm: bridge: adv7511: Enable usage with DRM bridge connector helper Laurent Pinchart
                   ` (2 preceding siblings ...)
  2020-04-09  0:46 ` [PATCH 3/4] drm: bridge: adv7511: Implement bridge connector operations Laurent Pinchart
@ 2020-04-09  0:46 ` Laurent Pinchart
  2020-04-13  6:03   ` Sam Ravnborg
  3 siblings, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2020-04-09  0:46 UTC (permalink / raw)
  To: dri-devel
  Cc: linux-renesas-soc, Andrzej Hajda, Neil Armstrong, Jonas Karlman,
	Jernej Skrabec, Rob Clark, Sean Paul

Now that the driver supports all the connector-related bridge
operations, make the connector creation optional. This enables usage of
the adv7511 with the DRM bridge connector helper.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 723560b36ee0..60efd19fa4df 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -902,17 +902,14 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge,
 				 enum drm_bridge_attach_flags flags)
 {
 	struct adv7511 *adv = bridge_to_adv7511(bridge);
-	int ret;
+	int ret = 0;
 
-	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)) {
+		ret = adv7511_connector_init(adv);
+		if (ret < 0)
+			return ret;
 	}
 
-	ret = adv7511_connector_init(adv);
-	if (ret < 0)
-		return ret;
-
 	if (adv->type == ADV7533 || adv->type == ADV7535)
 		ret = adv7533_attach_dsi(adv);
 
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/4] drm: bridge: adv7511: Split EDID read to a separate function
  2020-04-09  0:46 ` [PATCH 1/4] drm: bridge: adv7511: Split EDID read to a separate function Laurent Pinchart
@ 2020-04-13  5:54   ` Sam Ravnborg
  0 siblings, 0 replies; 8+ messages in thread
From: Sam Ravnborg @ 2020-04-13  5:54 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: dri-devel, Rob Clark, Jernej Skrabec, Jonas Karlman,
	Neil Armstrong, linux-renesas-soc, Andrzej Hajda, Sean Paul

Hi Laurent.

On Thu, Apr 09, 2020 at 03:46:07AM +0300, Laurent Pinchart wrote:
> To prepare for the implementation of the DRM bridge connector
> operations, move EDID read out of adv7511_get_modes() to a separate
> function.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

The code do not check if drm_do_get_edid() return NULL.
But all functions called with the edid seems to handle a NULL edid well.
So thats OK.

Acked-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 23 ++++++++++++++------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 87b58c1acff4..58d02e92b6b9 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -589,11 +589,10 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
>   * ADV75xx helpers
>   */
>  
> -static int adv7511_get_modes(struct adv7511 *adv7511,
> -			     struct drm_connector *connector)
> +static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
> +				     struct drm_connector *connector)
>  {
>  	struct edid *edid;
> -	unsigned int count;
>  
>  	/* Reading the EDID only works if the device is powered */
>  	if (!adv7511->powered) {
> @@ -612,15 +611,25 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>  	if (!adv7511->powered)
>  		__adv7511_power_off(adv7511);
>  
> -
> -	drm_connector_update_edid_property(connector, edid);
> -	count = drm_add_edid_modes(connector, edid);
> -
>  	adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
>  			       drm_detect_hdmi_monitor(edid));
>  
>  	cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
>  
> +	return edid;
> +}
> +
> +static int adv7511_get_modes(struct adv7511 *adv7511,
> +			     struct drm_connector *connector)
> +{
> +	struct edid *edid;
> +	unsigned int count;
> +
> +	edid = adv7511_get_edid(adv7511, connector);
> +
> +	drm_connector_update_edid_property(connector, edid);
> +	count = drm_add_edid_modes(connector, edid);
> +
>  	kfree(edid);
>  
>  	return count;
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/4] drm: bridge: adv7511: Split connector creation to a separate function
  2020-04-09  0:46 ` [PATCH 2/4] drm: bridge: adv7511: Split connector creation " Laurent Pinchart
@ 2020-04-13  5:58   ` Sam Ravnborg
  0 siblings, 0 replies; 8+ messages in thread
From: Sam Ravnborg @ 2020-04-13  5:58 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: dri-devel, Rob Clark, Jernej Skrabec, Jonas Karlman,
	Neil Armstrong, linux-renesas-soc, Andrzej Hajda, Sean Paul

Hi Laurent.

On Thu, Apr 09, 2020 at 03:46:08AM +0300, Laurent Pinchart wrote:
> To prepare for making the connector creation optional, move the related
> code out of adv7511_bridge_attach() to a separate function.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

On nit below, but otherwise:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 62 +++++++++++++-------
>  1 file changed, 40 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 58d02e92b6b9..e3b62ad95389 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -783,7 +783,10 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
>  	adv7511->f_tmds = mode->clock;
>  }
>  
> -/* Connector funcs */
> +/* -----------------------------------------------------------------------------
> + * DRM Connector Operations
> + */
> +
>  static struct adv7511 *connector_to_adv7511(struct drm_connector *connector)
>  {
>  	return container_of(connector, struct adv7511, connector);
> @@ -827,7 +830,40 @@ static const struct drm_connector_funcs adv7511_connector_funcs = {
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
> -/* Bridge funcs */
> +static int adv7511_connector_init(struct adv7511 *adv)
> +{
> +	struct drm_bridge *bridge = &adv->bridge;
> +	int ret;
> +
> +	if (!bridge->encoder) {
> +		DRM_ERROR("Parent encoder object not found");
> +		return -ENODEV;
> +	}
> +
> +	if (adv->i2c_main->irq)
> +		adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
> +	else
> +		adv->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
> +				DRM_CONNECTOR_POLL_DISCONNECT;
> +
> +	ret = drm_connector_init(bridge->dev, &adv->connector,
> +				 &adv7511_connector_funcs,
> +				 DRM_MODE_CONNECTOR_HDMIA);
> +	if (ret) {

Here we test for ret != 0

> +		DRM_ERROR("Failed to initialize connector with drm\n");
> +		return ret;
> +	}
> +	drm_connector_helper_add(&adv->connector,
> +				 &adv7511_connector_helper_funcs);
> +	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
> +
> +	return 0;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * DRM Bridge Operations
> + */
> +
>  static struct adv7511 *bridge_to_adv7511(struct drm_bridge *bridge)
>  {
>  	return container_of(bridge, struct adv7511, bridge);
> @@ -867,27 +903,9 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge,
>  		return -EINVAL;
>  	}
>  
> -	if (!bridge->encoder) {
> -		DRM_ERROR("Parent encoder object not found");
> -		return -ENODEV;
> -	}
> -
> -	if (adv->i2c_main->irq)
> -		adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
> -	else
> -		adv->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
> -				DRM_CONNECTOR_POLL_DISCONNECT;
> -
> -	ret = drm_connector_init(bridge->dev, &adv->connector,
> -				 &adv7511_connector_funcs,
> -				 DRM_MODE_CONNECTOR_HDMIA);
> -	if (ret) {
> -		DRM_ERROR("Failed to initialize connector with drm\n");
> +	ret = adv7511_connector_init(adv);
> +	if (ret < 0)
Here we test for ret < 0

The code works - but it is inconsistent.
drm_connector_init() is documented to:
"Zero on success, error code on failure."

>  		return ret;
> -	}
> -	drm_connector_helper_add(&adv->connector,
> -				 &adv7511_connector_helper_funcs);
> -	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
>  
>  	if (adv->type == ADV7533 || adv->type == ADV7535)
>  		ret = adv7533_attach_dsi(adv);
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 4/4] drm: bridge: adv7511: Make connector creation optional
  2020-04-09  0:46 ` [PATCH 4/4] drm: bridge: adv7511: Make connector creation optional Laurent Pinchart
@ 2020-04-13  6:03   ` Sam Ravnborg
  0 siblings, 0 replies; 8+ messages in thread
From: Sam Ravnborg @ 2020-04-13  6:03 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: dri-devel, Rob Clark, Jernej Skrabec, Jonas Karlman,
	Neil Armstrong, linux-renesas-soc, Andrzej Hajda, Sean Paul

On Thu, Apr 09, 2020 at 03:46:10AM +0300, Laurent Pinchart wrote:
> Now that the driver supports all the connector-related bridge
> operations, make the connector creation optional. This enables usage of
> the adv7511 with the DRM bridge connector helper.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Acked-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 723560b36ee0..60efd19fa4df 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -902,17 +902,14 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge,
>  				 enum drm_bridge_attach_flags flags)
>  {
>  	struct adv7511 *adv = bridge_to_adv7511(bridge);
> -	int ret;
> +	int ret = 0;
>  
> -	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)) {
> +		ret = adv7511_connector_init(adv);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
> -	ret = adv7511_connector_init(adv);
> -	if (ret < 0)
> -		return ret;
> -
>  	if (adv->type == ADV7533 || adv->type == ADV7535)
>  		ret = adv7533_attach_dsi(adv);
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-04-13  6:03 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-09  0:46 [PATCH 0/4] drm: bridge: adv7511: Enable usage with DRM bridge connector helper Laurent Pinchart
2020-04-09  0:46 ` [PATCH 1/4] drm: bridge: adv7511: Split EDID read to a separate function Laurent Pinchart
2020-04-13  5:54   ` Sam Ravnborg
2020-04-09  0:46 ` [PATCH 2/4] drm: bridge: adv7511: Split connector creation " Laurent Pinchart
2020-04-13  5:58   ` Sam Ravnborg
2020-04-09  0:46 ` [PATCH 3/4] drm: bridge: adv7511: Implement bridge connector operations Laurent Pinchart
2020-04-09  0:46 ` [PATCH 4/4] drm: bridge: adv7511: Make connector creation optional Laurent Pinchart
2020-04-13  6:03   ` Sam Ravnborg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).