linux-mediatek.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Sam Ravnborg <sam@ravnborg.org>
To: Markus Schneider-Pargmann <msp@baylibre.com>
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: Mon, 16 Aug 2021 23:36:13 +0200	[thread overview]
Message-ID: <YRraTWcY4yVuJANQ@ravnborg.org> (raw)
In-Reply-To: <20210816192523.1739365-6-msp@baylibre.com>

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

  reply	other threads:[~2021-08-16 21:36 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 [this message]
2021-08-17  7:31     ` Markus Schneider-Pargmann
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=YRraTWcY4yVuJANQ@ravnborg.org \
    --to=sam@ravnborg.org \
    --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=msp@baylibre.com \
    --cc=p.zabel@pengutronix.de \
    --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).