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 wens@csie.org --- 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);
Hi,
On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote:
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 wens@csie.org
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.
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
On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Hi,
On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote:
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 wens@csie.org
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.
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
Hi Maxime,
On Tue, Aug 30, 2016 at 11:51 PM, Chen-Yu Tsai wens@csie.org wrote:
On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Hi,
On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote:
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 wens@csie.org
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
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
On Wed, Aug 31, 2016 at 07:09:23PM +0800, Chen-Yu Tsai wrote:
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
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 maxime.ripard@free-electrons.com wrote:
Hi,
On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote:
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 wens@csie.org
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.
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
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:
On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Hi,
On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote:
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 wens@csie.org
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.
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
On Wed, Aug 31, 2016 at 06:27:08PM +0200, Daniel Vetter wrote:
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
dri-devel@lists.freedesktop.org
-
Chen-Yu Tsai -
Daniel Vetter -
Maxime Ripard