[PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found

Subsystems: drm drivers, drm drivers and misc gpu patches, drm drivers for allwinner a10, the rest

STALE3651d

8 messages, 3 authors, 2016-08-31 · open the first message on its own page

[PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found

From: Chen-Yu Tsai <hidden>
Date: 2016-08-30 12:22:34

The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass
encoder->bridge directly to drm_bridge_mode_fixup, which expects a
valid pointer, or NULL (in which case it just returns).

Clear encoder->bridge if a bridge is not found, instead of keeping
the ERR_PTR value.

Since other drm_bridge functions also follow this pattern of checking
for a non-NULL pointer, we can drop the ifs around the calls and just
pass the pointer directly.

Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support")
Signed-off-by: Chen-Yu Tsai <redacted>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index d4e52522ec53..ee0795152a33 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c

@@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
 	if (!IS_ERR(tcon->panel))
 		drm_panel_enable(tcon->panel);
 
-	if (!IS_ERR(encoder->bridge))
-		drm_bridge_enable(encoder->bridge);
+	drm_bridge_enable(encoder->bridge);
 
 	sun4i_tcon_channel_enable(tcon, 0);
 }

@@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
 
 	sun4i_tcon_channel_disable(tcon, 0);
 
-	if (!IS_ERR(encoder->bridge))
-		drm_bridge_disable(encoder->bridge);
+	drm_bridge_disable(encoder->bridge);
 
 	if (!IS_ERR(tcon->panel))
 		drm_panel_disable(tcon->panel);

@@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm)
 		return 0;
 	}
 
+	if (IS_ERR(encoder->bridge))
+		encoder->bridge = NULL;
+
 	drm_encoder_helper_add(&rgb->encoder,
 			       &sun4i_rgb_enc_helper_funcs);
 	ret = drm_encoder_init(drm,

@@ -266,7 +267,7 @@ int sun4i_rgb_init(struct drm_device *drm)
 		}
 	}
 
-	if (!IS_ERR(encoder->bridge)) {
+	if (encoder->bridge) {
 		encoder->bridge->encoder = &rgb->encoder;
 
 		ret = drm_bridge_attach(drm, encoder->bridge);

-- 
2.9.3

Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found

From: Maxime Ripard <hidden>
Date: 2016-08-30 12:56:26

Hi,

On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote:

quoted hunk

The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass
encoder->bridge directly to drm_bridge_mode_fixup, which expects a
valid pointer, or NULL (in which case it just returns).

Clear encoder->bridge if a bridge is not found, instead of keeping
the ERR_PTR value.

Since other drm_bridge functions also follow this pattern of checking
for a non-NULL pointer, we can drop the ifs around the calls and just
pass the pointer directly.

Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support")
Signed-off-by: Chen-Yu Tsai <redacted>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index d4e52522ec53..ee0795152a33 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c

@@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
 	if (!IS_ERR(tcon->panel))
 		drm_panel_enable(tcon->panel);
 
-	if (!IS_ERR(encoder->bridge))
-		drm_bridge_enable(encoder->bridge);
+	drm_bridge_enable(encoder->bridge);
 
 	sun4i_tcon_channel_enable(tcon, 0);
 }

@@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
 
 	sun4i_tcon_channel_disable(tcon, 0);
 
-	if (!IS_ERR(encoder->bridge))
-		drm_bridge_disable(encoder->bridge);
+	drm_bridge_disable(encoder->bridge);

I'd rather keep those changes, it makes it obvious that it's something
optionnal, that can be set to NULL.

quoted hunk

 	if (!IS_ERR(tcon->panel))
 		drm_panel_disable(tcon->panel);

@@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm)
 		return 0;
 	}
 
+	if (IS_ERR(encoder->bridge))
+		encoder->bridge = NULL;
+

And that could be the else condition of the if statement below.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160830/9deb77a5/attachment.sig>

Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found

From: Chen-Yu Tsai <hidden>
Date: 2016-08-30 15:51:54

On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard
[off-list ref] wrote:

Hi,

On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote:

quoted

The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass
encoder->bridge directly to drm_bridge_mode_fixup, which expects a
valid pointer, or NULL (in which case it just returns).

Clear encoder->bridge if a bridge is not found, instead of keeping
the ERR_PTR value.

Since other drm_bridge functions also follow this pattern of checking
for a non-NULL pointer, we can drop the ifs around the calls and just
pass the pointer directly.

Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support")
Signed-off-by: Chen-Yu Tsai <redacted>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index d4e52522ec53..ee0795152a33 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c

@@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
      if (!IS_ERR(tcon->panel))
              drm_panel_enable(tcon->panel);

-     if (!IS_ERR(encoder->bridge))
-             drm_bridge_enable(encoder->bridge);
+     drm_bridge_enable(encoder->bridge);

      sun4i_tcon_channel_enable(tcon, 0);
 }

@@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)

      sun4i_tcon_channel_disable(tcon, 0);

-     if (!IS_ERR(encoder->bridge))
-             drm_bridge_disable(encoder->bridge);
+     drm_bridge_disable(encoder->bridge);

I'd rather keep those changes, it makes it obvious that it's something
optionnal, that can be set to NULL.

OK.

quoted

      if (!IS_ERR(tcon->panel))
              drm_panel_disable(tcon->panel);

@@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm)
              return 0;
      }

+     if (IS_ERR(encoder->bridge))
+             encoder->bridge = NULL;
+

And that could be the else condition of the if statement below.

That would be a bit confusing, changing it after calling drm_encoder_init.
The code says it ok to do though.

ChenYu

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found

From: Chen-Yu Tsai <hidden>
Date: 2016-08-31 11:10:04

Hi Maxime,

On Tue, Aug 30, 2016 at 11:51 PM, Chen-Yu Tsai [off-list ref] wrote:

On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard
[off-list ref] wrote:

quoted

Hi,

On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote:

quoted

The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass
encoder->bridge directly to drm_bridge_mode_fixup, which expects a
valid pointer, or NULL (in which case it just returns).

Clear encoder->bridge if a bridge is not found, instead of keeping
the ERR_PTR value.

Since other drm_bridge functions also follow this pattern of checking
for a non-NULL pointer, we can drop the ifs around the calls and just
pass the pointer directly.

Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support")
Signed-off-by: Chen-Yu Tsai <redacted>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index d4e52522ec53..ee0795152a33 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c

@@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
      if (!IS_ERR(tcon->panel))
              drm_panel_enable(tcon->panel);

-     if (!IS_ERR(encoder->bridge))
-             drm_bridge_enable(encoder->bridge);
+     drm_bridge_enable(encoder->bridge);

      sun4i_tcon_channel_enable(tcon, 0);
 }

@@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)

      sun4i_tcon_channel_disable(tcon, 0);

-     if (!IS_ERR(encoder->bridge))
-             drm_bridge_disable(encoder->bridge);
+     drm_bridge_disable(encoder->bridge);

I'd rather keep those changes, it makes it obvious that it's something
optionnal, that can be set to NULL.

OK.

What about having a comment instead? Saves an extra branch condition,
while still showing that it's optional.

ChenYu

quoted

      if (!IS_ERR(tcon->panel))
              drm_panel_disable(tcon->panel);

@@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm)
              return 0;
      }

+     if (IS_ERR(encoder->bridge))
+             encoder->bridge = NULL;
+

And that could be the else condition of the if statement below.

That would be a bit confusing, changing it after calling drm_encoder_init.
The code says it ok to do though.

ChenYu

quoted

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found

From: Maxime Ripard <hidden>
Date: 2016-08-31 15:40:14

On Tue, Aug 30, 2016 at 11:51:26PM +0800, Chen-Yu Tsai wrote:

On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard
[off-list ref] wrote:

quoted

Hi,

On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote:

quoted

The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass
encoder->bridge directly to drm_bridge_mode_fixup, which expects a
valid pointer, or NULL (in which case it just returns).

Clear encoder->bridge if a bridge is not found, instead of keeping
the ERR_PTR value.

Since other drm_bridge functions also follow this pattern of checking
for a non-NULL pointer, we can drop the ifs around the calls and just
pass the pointer directly.

Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support")
Signed-off-by: Chen-Yu Tsai <redacted>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index d4e52522ec53..ee0795152a33 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c

@@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
      if (!IS_ERR(tcon->panel))
              drm_panel_enable(tcon->panel);

-     if (!IS_ERR(encoder->bridge))
-             drm_bridge_enable(encoder->bridge);
+     drm_bridge_enable(encoder->bridge);

      sun4i_tcon_channel_enable(tcon, 0);
 }

@@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)

      sun4i_tcon_channel_disable(tcon, 0);

-     if (!IS_ERR(encoder->bridge))
-             drm_bridge_disable(encoder->bridge);
+     drm_bridge_disable(encoder->bridge);

I'd rather keep those changes, it makes it obvious that it's something
optionnal, that can be set to NULL.

OK.

quoted

      if (!IS_ERR(tcon->panel))
              drm_panel_disable(tcon->panel);

@@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm)
              return 0;
      }

+     if (IS_ERR(encoder->bridge))
+             encoder->bridge = NULL;
+

And that could be the else condition of the if statement below.

That would be a bit confusing, changing it after calling drm_encoder_init.
The code says it ok to do though.

The magic really happens only after the encoder has been attached to
something, so it's really safe.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160831/6f45c0e7/attachment.sig>

Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found

From: Daniel Vetter <hidden>
Date: 2016-08-31 16:28:10

On Wed, Aug 31, 2016 at 05:40:02PM +0200, Maxime Ripard wrote:

On Tue, Aug 30, 2016 at 11:51:26PM +0800, Chen-Yu Tsai wrote:

quoted

On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard
[off-list ref] wrote:

quoted

Hi,

On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote:

quoted

The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass
encoder->bridge directly to drm_bridge_mode_fixup, which expects a
valid pointer, or NULL (in which case it just returns).

Clear encoder->bridge if a bridge is not found, instead of keeping
the ERR_PTR value.

Since other drm_bridge functions also follow this pattern of checking
for a non-NULL pointer, we can drop the ifs around the calls and just
pass the pointer directly.

Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support")
Signed-off-by: Chen-Yu Tsai <redacted>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index d4e52522ec53..ee0795152a33 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c

@@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
      if (!IS_ERR(tcon->panel))
              drm_panel_enable(tcon->panel);

-     if (!IS_ERR(encoder->bridge))
-             drm_bridge_enable(encoder->bridge);
+     drm_bridge_enable(encoder->bridge);

      sun4i_tcon_channel_enable(tcon, 0);
 }

@@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)

      sun4i_tcon_channel_disable(tcon, 0);

-     if (!IS_ERR(encoder->bridge))
-             drm_bridge_disable(encoder->bridge);
+     drm_bridge_disable(encoder->bridge);

I'd rather keep those changes, it makes it obvious that it's something
optionnal, that can be set to NULL.

OK.

quoted

      if (!IS_ERR(tcon->panel))
              drm_panel_disable(tcon->panel);

@@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm)
              return 0;
      }

+     if (IS_ERR(encoder->bridge))
+             encoder->bridge = NULL;
+

And that could be the else condition of the if statement below.

That would be a bit confusing, changing it after calling drm_encoder_init.
The code says it ok to do though.

The magic really happens only after the encoder has been attached to
something, so it's really safe.

s/attached/registered using drm_dev_register(). Which should
happen _way_ later for all drivers which have gotten rid of their ->load
callback and implemented the recommend driver load sequence.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found

From: Maxime Ripard <hidden>
Date: 2016-08-31 16:42:02

On Wed, Aug 31, 2016 at 07:09:23PM +0800, Chen-Yu Tsai wrote:

quoted

      sun4i_tcon_channel_disable(tcon, 0);

-     if (!IS_ERR(encoder->bridge))
-             drm_bridge_disable(encoder->bridge);
+     drm_bridge_disable(encoder->bridge);

I'd rather keep those changes, it makes it obvious that it's something
optionnal, that can be set to NULL.

OK.

What about having a comment instead? Saves an extra branch condition,
while still showing that it's optional.

I'm not sure we have to worry about an extra branch condition, but
yeah, that works for me.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160831/05fc9ca6/attachment.sig>

Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found

From: Maxime Ripard <hidden>
Date: 2016-08-31 16:43:05

On Wed, Aug 31, 2016 at 06:27:08PM +0200, Daniel Vetter wrote:

quoted

+     if (IS_ERR(encoder->bridge))
+             encoder->bridge = NULL;
+

And that could be the else condition of the if statement below.

That would be a bit confusing, changing it after calling drm_encoder_init.
The code says it ok to do though.

The magic really happens only after the encoder has been attached to
something, so it's really safe.

s/attached/registered using drm_dev_register(). Which should
happen _way_ later for all drivers which have gotten rid of their ->load
callback and implemented the recommend driver load sequence.

My bad, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160831/39904c8d/attachment.sig>

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help