linux-mediatek.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Markus Schneider-Pargmann <msp@baylibre.com>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Chun-Kuang Hu <chunkuang.hu@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	dri-devel@lists.freedesktop.org,
	linux-mediatek@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH 5/5] drm/mediatek: Add mt8195 DisplayPort driver
Date: Tue, 17 Aug 2021 09:31:20 +0200	[thread overview]
Message-ID: <20210817073120.fqa2wybvdgbn55a4@blmsp> (raw)
In-Reply-To: <YRraTWcY4yVuJANQ@ravnborg.org>

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

  reply	other threads:[~2021-08-17  7:31 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-16 19:25 [RFC PATCH 0/5] " Markus Schneider-Pargmann
2021-08-16 19:25 ` [RFC PATCH 1/5] dt-bindings: mediatek,dpi: Add mt8195 dpintf Markus Schneider-Pargmann
2021-08-18  4:45   ` CK Hu
2021-08-18  7:30     ` Markus Schneider-Pargmann
2021-08-16 19:25 ` [RFC PATCH 2/5] drm/mediatek: dpi: Add dpintf support Markus Schneider-Pargmann
2021-08-17  9:50   ` CK Hu
2021-08-18  7:26     ` Markus Schneider-Pargmann
2021-08-16 19:25 ` [RFC PATCH 3/5] drm/edid: Add cea_sad helpers for freq/length Markus Schneider-Pargmann
2021-08-16 19:25 ` [RFC PATCH 4/5] video/hdmi: Add audio_infoframe packing for DP Markus Schneider-Pargmann
2021-08-16 19:25 ` [RFC PATCH 5/5] drm/mediatek: Add mt8195 DisplayPort driver Markus Schneider-Pargmann
2021-08-16 21:36   ` Sam Ravnborg
2021-08-17  7:31     ` Markus Schneider-Pargmann [this message]
2021-08-17  5:36   ` CK Hu
2021-08-17  7:35     ` Markus Schneider-Pargmann
2021-08-18  4:42   ` CK Hu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210817073120.fqa2wybvdgbn55a4@blmsp \
    --to=msp@baylibre.com \
    --cc=chunkuang.hu@kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=p.zabel@pengutronix.de \
    --cc=sam@ravnborg.org \
    --subject='Re: [RFC PATCH 5/5] drm/mediatek: Add mt8195 DisplayPort driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).