Messages in this thread | | | From | Keith Zhao <> | Subject | RE: [PATCH v4 02/10] drm/bridge: add common api for inno hdmi | Date | Wed, 22 May 2024 05:58:00 +0000 |
| |
Hi Alex, Laurent:
I want to make as few changes as possible on the current basis, and add bridge_fun,
> -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 2024年5月21日 23:42 > To: Alex Bee <knaerzche@gmail.com> > Cc: Keith Zhao <keith.zhao@starfivetech.com>; andrzej.hajda@intel.com; > neil.armstrong@linaro.org; rfoss@kernel.org; jonas@kwiboo.se; > jernej.skrabec@gmail.com; maarten.lankhorst@linux.intel.com; > mripard@kernel.org; tzimmermann@suse.de; airlied@gmail.com; > daniel@ffwll.ch; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; > hjc@rock-chips.com; heiko@sntech.de; andy.yan@rock-chips.com; Xingyu Wu > <xingyu.wu@starfivetech.com>; p.zabel@pengutronix.de; Jack Zhu > <jack.zhu@starfivetech.com>; Shengyang Chen > <shengyang.chen@starfivetech.com>; dri-devel@lists.freedesktop.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v4 02/10] drm/bridge: add common api for inno hdmi > > 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.zhao@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.
static int inno_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct inno_hdmi *hdmi = bridge_to_inno(bridge);
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { DRM_ERROR("Fix bridge driver to make connector optional!"); return -EINVAL; } 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);
drm_connector_attach_encoder(&hdmi->connector, encoder); }
static const struct drm_bridge_funcs inno_bridge_attach = { .attach = inno_bridge_attach, };
Connector creation is handled by the drm_bridge_funcs ->attach. Is it ok?
Regards Keith > > > 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
| |