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

[Markus Schneider-Pargmann <msp@baylibre.com>]

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.
>  
> 

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