Re: [RFC PATCH 5/5] drm/mediatek: Add mt8195 DisplayPort driver

[Sam Ravnborg <sam@ravnborg.org>]

Hi Markus,

A few general things in the following. This is what I look for first in
a bridge driver - and I had no time today to review the driver in full.
Please address these, then cc: me on next revision where I hopefully
have more time.

	Sam

> +static int mtk_dp_bridge_attach(struct drm_bridge *bridge,
> +                               enum drm_bridge_attach_flags flags)
> +{
> +       struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
> +       int ret;
> +
> +       mtk_dp_poweron(mtk_dp);
> +
> +       if (mtk_dp->next_bridge) {
> +               ret = drm_bridge_attach(bridge->encoder, mtk_dp->next_bridge,
> +                                       &mtk_dp->bridge, flags);
> +               if (ret) {
> +                       drm_warn(mtk_dp->drm_dev,
> +                                "Failed to attach external bridge: %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> +               drm_err(mtk_dp->drm_dev,
> +                       "Fix bridge driver to make connector optional!");
> +               return 0;
> +       }

This driver is only used by mediatek, and I thought all of mediatek is
converted so the display driver creates the connector.

It would be better to migrate mediatek over to always let the display
driver create the connector and drop the connector support in this
driver.


> + struct drm_bridge_funcs mtk_dp_bridge_funcs = {
> +	.attach = mtk_dp_bridge_attach,
> +	.mode_fixup = mtk_dp_bridge_mode_fixup,
> +	.disable = mtk_dp_bridge_disable,
> +	.post_disable = mtk_dp_bridge_post_disable,
> +	.mode_set = mtk_dp_bridge_mode_set,
> +	.pre_enable = mtk_dp_bridge_pre_enable,
> +	.enable = mtk_dp_bridge_enable,
> +	.get_edid = mtk_get_edid,
> +	.detect = mtk_dp_bdg_detect,
> +};


For new drivers please avoid the recently deprecated functions.

- Use the atomic versions of pre_enable, enable, disable and post_disable.

- Merge mode_set with atomic_enable - as there is no need for the mode_Set
  operation.

- Use atomic_check in favour of mode_fixup, albeit the rules for
  atomic_check is at best vauge at the moment.
 


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek