diff mbox series

[2/4] drm: bridge: adv7511: Split connector creation to a separate function

Message ID 20200409004610.12346-3-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series drm: bridge: adv7511: Enable usage with DRM bridge connector helper | expand

Commit Message

Laurent Pinchart April 9, 2020, 12:46 a.m. UTC
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(-)

Comments

Sam Ravnborg April 13, 2020, 5:58 a.m. UTC | #1
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
diff mbox series

Patch

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