lkml.org 
[lkml]   [2024]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v4 02/10] drm/bridge: add common api for inno hdmi
Date
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
\
 
 \ /
  Last update: 2024-05-22 08:31    [W:0.053 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site