Re: [PATCH v3 13/21] drm/bridge: megachips: add helper to create connector
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
> > Cc: Peter Senna Tschudin
> > Cc: Martin Donnelly
> > Cc: Martyn Welch
> > Cc: Andrzej Hajda
> > Cc: Neil Armstrong
> > Cc: Laurent Pinchart
> > Cc: Jonas Karlman
> > Cc: Jernej Skrabec
> > ---
> > .../bridge/megachips-stdp-ge-b850v3-fw.c | 47 +++
> > 1 file changed, 27 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> > b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> > index 6200f12a37e6..258e0525cdcc 100644
> > --- a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> > +++ b/drivers/gpu/drm/bridge/megachips-stdp-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
>
> > + 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
Re: [PATCH v3 13/21] drm/bridge: megachips: add helper to create connector
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
> Cc: Peter Senna Tschudin
> Cc: Martin Donnelly
> Cc: Martyn Welch
> Cc: Andrzej Hajda
> Cc: Neil Armstrong
> Cc: Laurent Pinchart
> Cc: Jonas Karlman
> Cc: Jernej Skrabec
> ---
> .../bridge/megachips-stdp-ge-b850v3-fw.c | 47 +++
> 1 file changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> index 6200f12a37e6..258e0525cdcc 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdp-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 ?
Reviewed-by: Laurent Pinchart
> + 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
[PATCH v3 13/21] drm/bridge: megachips: add helper to create connector
Factor out connector creation to a small helper function.
Signed-off-by: Sam Ravnborg
Cc: Peter Senna Tschudin
Cc: Martin Donnelly
Cc: Martyn Welch
Cc: Andrzej Hajda
Cc: Neil Armstrong
Cc: Laurent Pinchart
Cc: Jonas Karlman
Cc: Jernej Skrabec
---
.../bridge/megachips-stdp-ge-b850v3-fw.c | 47 +++
1 file changed, 27 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
index 6200f12a37e6..258e0525cdcc 100644
--- a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdp-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;
+ 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;
--
2.25.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
