Hi Sam,
On Mon, Aug 16, 2021 at 11:36:13PM +0200, Sam Ravnborg wrote:
> 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.
Thanks for taking the time and giving me the tips, will fix it and send
a new version.
Best,
Markus
>
> 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.
>
>
