Re: [PATCH v3 13/21] drm/bridge: megachips: add helper to create connector

From: Sam Ravnborg <sam@ravnborg.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>,
	kbuild test robot <lkp@intel.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Peter Senna Tschudin <peter.senna@gmail.com>,
	dri-devel@lists.freedesktop.org,
	Martyn Welch <martyn.welch@collabora.co.uk>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	Thierry Reding <thierry.reding@gmail.com>,
	Martin Donnelly <martin.donnelly@ge.com>
Subject: Re: [PATCH v3 13/21] drm/bridge: megachips: add helper to create connector
Date: Sun, 26 Jul 2020 21:57:43 +0200	[thread overview]
Message-ID: <20200726195743.GA3676671@ravnborg.org> (raw)
In-Reply-To: <20200710223430.GQ5964@pendragon.ideasonboard.com>

On Sat, Jul 11, 2020 at 01:34:30AM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> Thank you for the patch.
> 
> On Fri, Jul 03, 2020 at 09:24:09PM +0200, Sam Ravnborg wrote:
> > Factor out connector creation to a small helper function.
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Peter Senna Tschudin <peter.senna@gmail.com>
> > Cc: Martin Donnelly <martin.donnelly@ge.com>
> > Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> >  .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 47 +++++++++++--------
> >  1 file changed, 27 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > index 6200f12a37e6..258e0525cdcc 100644
> > --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > @@ -191,6 +191,32 @@ static const struct drm_connector_funcs ge_b850v3_lvds_connector_funcs = {
> >  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> >  };
> >  
> > +static int ge_b850v3_lvds_create_connector(struct drm_bridge *bridge)
> > +{
> > +	struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector;
> 
> Wow, storing device state in a global variable... :-( How did this go
> past review ?
Took a short look at eliminating this.
But there are two module entries involved so I droppet it again.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +	int ret;
> > +
> > +	if (!bridge->encoder) {
> > +		DRM_ERROR("Parent encoder object not found");
> > +		return -ENODEV;
> > +	}
> > +
> > +	connector->polled = DRM_CONNECTOR_POLL_HPD;
> > +
> > +	drm_connector_helper_add(connector,
> > +				 &ge_b850v3_lvds_connector_helper_funcs);
> > +
> > +	ret = drm_connector_init(bridge->dev, connector,
> > +				 &ge_b850v3_lvds_connector_funcs,
> > +				 DRM_MODE_CONNECTOR_DisplayPort);
> > +	if (ret) {
> > +		DRM_ERROR("Failed to initialize connector with drm\n");
> > +		return ret;
> > +	}
> > +
> > +	return drm_connector_attach_encoder(connector, bridge->encoder);
> > +}
> > +
> >  static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
> >  {
> >  	struct i2c_client *stdp4028_i2c
> > @@ -209,7 +235,6 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
> >  static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
> >  				 enum drm_bridge_attach_flags flags)
> >  {
> > -	struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector;
> >  	struct i2c_client *stdp4028_i2c
> >  			= ge_b850v3_lvds_ptr->stdp4028_i2c;
> >  	int ret;
> > @@ -219,25 +244,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (!bridge->encoder) {
> > -		DRM_ERROR("Parent encoder object not found");
> > -		return -ENODEV;
> > -	}
> > -
> > -	connector->polled = DRM_CONNECTOR_POLL_HPD;
> > -
> > -	drm_connector_helper_add(connector,
> > -				 &ge_b850v3_lvds_connector_helper_funcs);
> > -
> > -	ret = drm_connector_init(bridge->dev, connector,
> > -				 &ge_b850v3_lvds_connector_funcs,
> > -				 DRM_MODE_CONNECTOR_DisplayPort);
> > -	if (ret) {
> > -		DRM_ERROR("Failed to initialize connector with drm\n");
> > -		return ret;
> > -	}
> > -
> > -	ret = drm_connector_attach_encoder(connector, bridge->encoder);
> > +	ret = ge_b850v3_lvds_create_connector(bridge);
> >  	if (ret)
> >  		return ret;
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel