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 <s...@ravnborg.org>
> > Cc: Peter Senna Tschudin <peter.se...@gmail.com>
> > Cc: Martin Donnelly <martin.donne...@ge.com>
> > Cc: Martyn Welch <martyn.we...@collabora.co.uk>
> > Cc: Andrzej Hajda <a.ha...@samsung.com>
> > Cc: Neil Armstrong <narmstr...@baylibre.com>
> > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> > Cc: Jonas Karlman <jo...@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skra...@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.pinch...@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
