[EXT] Re: [PATCH v6 3/8] drm: bridge: Cadence: Add MHDP8501 DP driver

[Sandor Yu]

Hi Sam,

Thanks your comments,

For i.MX8MQ, the display driver DCSS had create its own connector.
I will drop the code in the next version review patch set.

Thanks
Sandor

> -----Original Message-----
> From: Sam Ravnborg <sam at ravnborg.org>
> Sent: 2023年6月16日 0:33
> To: Sandor Yu <sandor.yu at nxp.com>
> Cc: andrzej.hajda at intel.com; neil.armstrong at linaro.org;
> robert.foss at linaro.org; Laurent.pinchart at ideasonboard.com;
> jonas at kwiboo.se; jernej.skrabec at gmail.com; airlied at gmail.com;
> daniel at ffwll.ch; robh+dt at kernel.org; krzysztof.kozlowski+dt at linaro.org;
> shawnguo at kernel.org; s.hauer at pengutronix.de; festevam at gmail.com;
> vkoul at kernel.org; dri-devel at lists.freedesktop.org;
> devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org; linux-phy at lists.infradead.org; Oliver Brown
> <oliver.brown at nxp.com>; dl-linux-imx <linux-imx at nxp.com>;
> kernel at pengutronix.de
> Subject: [EXT] Re: [PATCH v6 3/8] drm: bridge: Cadence: Add MHDP8501 DP
> driver
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> Hi Sandor,
> 
> On Thu, Jun 15, 2023 at 09:38:13AM +0800, Sandor Yu wrote:
> > Add a new DRM DisplayPort bridge driver for Candence MHDP8501 used in
> > i.MX8MQ SOC. MHDP8501 could support HDMI or DisplayPort standards
> > according embedded Firmware running in the uCPU.
> >
> > For iMX8MQ SOC, the DisplayPort FW was loaded and activated by SOC
> ROM
> > code. Bootload binary included HDMI FW was required for the driver.
> 
> The bridge driver supports creating a connector, but is this really necessary?
> 
> This part:
> > +static const struct drm_connector_funcs cdns_dp_connector_funcs = {
> > +     .fill_modes = drm_helper_probe_single_connector_modes,
> > +     .destroy = drm_connector_cleanup,
> > +     .reset = drm_atomic_helper_connector_reset,
> > +     .atomic_duplicate_state =
> drm_atomic_helper_connector_duplicate_state,
> > +     .atomic_destroy_state =
> > +drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static const struct drm_connector_helper_funcs
> cdns_dp_connector_helper_funcs = {
> > +     .get_modes = cdns_dp_connector_get_modes, };
> > +
> > +static int cdns_dp_bridge_attach(struct drm_bridge *bridge,
> > +                              enum drm_bridge_attach_flags flags)
> {
> > +     struct cdns_mhdp_device *mhdp = bridge->driver_private;
> > +     struct drm_encoder *encoder = bridge->encoder;
> > +     struct drm_connector *connector = &mhdp->connector;
> > +     int ret;
> > +
> > +     if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> > +             connector->interlace_allowed = 0;
> > +
> > +             connector->polled = DRM_CONNECTOR_POLL_HPD;
> > +
> > +             drm_connector_helper_add(connector,
> > + &cdns_dp_connector_helper_funcs);
> > +
> > +             drm_connector_init(bridge->dev, connector,
> &cdns_dp_connector_funcs,
> > +
> DRM_MODE_CONNECTOR_DisplayPort);
> > +
> > +             drm_connector_attach_encoder(connector, encoder);
> > +     }
> 
> Unless you have a display driver that do not create their own connector then
> drop the above and error out if DRM_BRIDGE_ATTACH_NO_CONNECTOR is
> not set.
> It is encouraged that display drivers create their own connector.
> 
> This was the only detail I looked for in the driver, I hope some else volunteer
> to review it.
> 
>         Sam