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@xxxxxxxxxxxxxxxx>
On nit below, but otherwise:
Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
