On Tue, May 21, 2024 at 05:36:43PM +0200, Alex Bee wrote: > Hi Keith, > > thanks a lot for working on this. See some general remarks below > > Am 21.05.24 um 12:58 schrieb keith: > > Add INNO common api so that it can be used by vendor > > drivers which implement vendor specific extensions to Innosilicon HDMI. > > > > Signed-off-by: keith <keith.z...@starfivetech.com> > > --- > > MAINTAINERS | 2 + > > drivers/gpu/drm/bridge/Kconfig | 2 + > > drivers/gpu/drm/bridge/Makefile | 1 + > > drivers/gpu/drm/bridge/innosilicon/Kconfig | 6 + > > drivers/gpu/drm/bridge/innosilicon/Makefile | 2 + > > .../gpu/drm/bridge/innosilicon/inno-hdmi.c | 587 ++++++++++++++++++ > > .../gpu/drm/bridge/innosilicon/inno-hdmi.h | 97 +++ > > include/drm/bridge/inno_hdmi.h | 69 ++ > > 8 files changed, 766 insertions(+) > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/Kconfig > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/Makefile > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/inno-hdmi.c > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/inno-hdmi.h > > create mode 100644 include/drm/bridge/inno_hdmi.h > > > .... > > > + drm_encoder_helper_add(encoder, pdata->helper_private); > > + > > + hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD; > > + > > + drm_connector_helper_add(&hdmi->connector, > > + &inno_hdmi_connector_helper_funcs); > > + > > + drmm_connector_init(drm, &hdmi->connector, > > + &inno_hdmi_connector_funcs, > > + DRM_MODE_CONNECTOR_HDMIA, > > + hdmi->ddc); > > + > > I really don't want to anticipate bridge maintainer's feedback, but new > bridge drivers must not contain connector creation. That must happen > somewhere else.
You're absolutely right :-) Connector creation should be handled by the drm_bridge_connector helper. The HDMI bridge driver should focus on the HDMI bridge itself. > Also I'm neither seeing any drm_brige struct nor drm_bridge_funcs, which > are both essential for a bridge driver. I don't think moving a part of a > driver to .../drm/bridge/ makes it a bridge driver. > > > + drm_connector_attach_encoder(&hdmi->connector, encoder); > > + > > + return 0; > > +} > > + > .... > -- Regards, Laurent Pinchart
