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@xxxxxxxxxxxx>
> Sent: 2023年6月16日 0:33
> To: Sandor Yu <sandor.yu@xxxxxxx>
> Cc: andrzej.hajda@xxxxxxxxx; neil.armstrong@xxxxxxxxxx;
> robert.foss@xxxxxxxxxx; Laurent.pinchart@xxxxxxxxxxxxxxxx;
> jonas@xxxxxxxxx; jernej.skrabec@xxxxxxxxx; airlied@xxxxxxxxx;
> daniel@xxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> vkoul@xxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-phy@xxxxxxxxxxxxxxxxxxx; Oliver Brown
> <oliver.brown@xxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>;
> kernel@xxxxxxxxxxxxxx
> 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
