[PATCH 0/6] drm: remove redundant else across drivers

[PATCH 0/6] drm: remove redundant else across drivers

[Aditya Garg]

In primary_plane_helper_atomic_check of various drm drivers, a redundant
else statement is there. All these statements match this pattern:

if (foo)
	return foo;
else if (bar)
	return bar;

Which is equivalent to:

if (foo)
	return foo;

if (bar)
	return bar;

This error was observed by Andy Shevchenko [1] while reviewing the
appletbdrm driver, which has been submitted upstream.

A simple grep was used to find such errors.

Most likely a common code was copied and pasted among these drivers.
Some of them also seem to have been ported to use atomic helpers instead
of simple_display_pipe.

[1]: https://lore.kernel.org/dri-devel/Z7igVXqvRYTVFpXU@smile.fi.intel.com/

Aditya Garg (6):
  drm/ast: Remove redundant else in atomic_check
  drm/cirrus-qemu: Remove redundant else in atomic_check
  drm/offdrm: Remove redundant else in atomic_check
  drm/bochs: Remove redundant else in atomic_check
  drm/simpledrm: Remove redundant else in atomic_check
  drm/mgag200: Remove redundant else in atomic_check

 drivers/gpu/drm/ast/ast_mode.c         | 5 +++--
 drivers/gpu/drm/mgag200/mgag200_mode.c | 3 ++-
 drivers/gpu/drm/tiny/bochs.c           | 3 ++-
 drivers/gpu/drm/tiny/cirrus-qemu.c     | 6 ++++--
 drivers/gpu/drm/tiny/ofdrm.c           | 3 ++-
 drivers/gpu/drm/tiny/simpledrm.c       | 3 ++-
 6 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH 1/6] drm/ast: Remove redundant else in atomic_check

[Aditya Garg]

From: Aditya Garg <gargaditya08@live.com>

Remove the redundant else statement from atomic_check since the previous if
statement was returning if true.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/gpu/drm/ast/ast_mode.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 9d5321c81..3817d1e4c 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -610,9 +610,10 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
 						  DRM_PLANE_NO_SCALING,
 						  DRM_PLANE_NO_SCALING,
 						  false, true);
-	if (ret) {
+	if (ret)
 		return ret;
-	} else if (!new_plane_state->visible) {
+
+	if (!new_plane_state->visible) {
 		if (drm_WARN_ON(dev, new_plane_state->crtc)) /* cannot legally happen */
 			return -EINVAL;
 		else
-- 
2.43.0

[PATCH 2/6] drm/cirrus-qemu: Remove redundant else in atomic_check

[Aditya Garg]

From: Aditya Garg <gargaditya08@live.com>

Remove the redundant else statement from atomic_check since the previous if
statement was returning if true.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/gpu/drm/tiny/cirrus-qemu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tiny/cirrus-qemu.c b/drivers/gpu/drm/tiny/cirrus-qemu.c
index 52ec1e4ea..e696531b6 100644
--- a/drivers/gpu/drm/tiny/cirrus-qemu.c
+++ b/drivers/gpu/drm/tiny/cirrus-qemu.c
@@ -359,7 +359,8 @@ static int cirrus_primary_plane_helper_atomic_check(struct drm_plane *plane,
 						  false, false);
 	if (ret)
 		return ret;
-	else if (!new_plane_state->visible)
+
+	if (!new_plane_state->visible)
 		return 0;
 
 	pitch = cirrus_pitch(fb);
@@ -367,7 +368,8 @@ static int cirrus_primary_plane_helper_atomic_check(struct drm_plane *plane,
 	/* validate size constraints */
 	if (pitch > CIRRUS_MAX_PITCH)
 		return -EINVAL;
-	else if (pitch * fb->height > CIRRUS_VRAM_SIZE)
+
+	if (pitch * fb->height > CIRRUS_VRAM_SIZE)
 		return -EINVAL;
 
 	new_primary_plane_state->format = cirrus_format(fb);
-- 
2.43.0

[PATCH 3/6] drm/offdrm: Remove redundant else in atomic_check

[Aditya Garg]

From: Aditya Garg <gargaditya08@live.com>

Remove the redundant else statement from atomic_check since the previous if
statement was returning if true.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/gpu/drm/tiny/ofdrm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
index 13491c0e7..fc669ab1d 100644
--- a/drivers/gpu/drm/tiny/ofdrm.c
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -777,7 +777,8 @@ static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
 						  false, false);
 	if (ret)
 		return ret;
-	else if (!new_plane_state->visible)
+
+	if (!new_plane_state->visible)
 		return 0;
 
 	if (new_fb->format != odev->format) {
-- 
2.43.0

[PATCH 4/6] drm/bochs: Remove redundant else in atomic_check

[Aditya Garg]

From: Aditya Garg <gargaditya08@live.com>

Remove the redundant else statement from atomic_check since the previous if
statement was returning if true.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/gpu/drm/tiny/bochs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
index c67e1f906..9ed5f70a0 100644
--- a/drivers/gpu/drm/tiny/bochs.c
+++ b/drivers/gpu/drm/tiny/bochs.c
@@ -433,7 +433,8 @@ static int bochs_primary_plane_helper_atomic_check(struct drm_plane *plane,
 						  false, false);
 	if (ret)
 		return ret;
-	else if (!new_plane_state->visible)
+
+	if (!new_plane_state->visible)
 		return 0;
 
 	return 0;
-- 
2.43.0

[PATCH 5/6] drm/simpledrm: Remove redundant else in atomic_check

[Aditya Garg]

From: Aditya Garg <gargaditya08@live.com>

Remove the redundant else statement from atomic_check since the previous if
statement was returning if true.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/gpu/drm/tiny/simpledrm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 5d9ab8adf..1c20d4c49 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -602,7 +602,8 @@ static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
 						  false, false);
 	if (ret)
 		return ret;
-	else if (!new_plane_state->visible)
+
+	if (!new_plane_state->visible)
 		return 0;
 
 	if (new_fb->format != sdev->format) {
-- 
2.43.0

[PATCH 6/6] drm/mgag200: Remove redundant else in atomic_check

[Aditya Garg]

From: Aditya Garg <gargaditya08@live.com>

Remove the redundant else statement from atomic_check since the previous if
statement was returning if true.

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index fb71658c3..e3e102374 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -483,7 +483,8 @@ int mgag200_primary_plane_helper_atomic_check(struct drm_plane *plane,
 						  false, true);
 	if (ret)
 		return ret;
-	else if (!new_plane_state->visible)
+
+	if (!new_plane_state->visible)
 		return 0;
 
 	if (plane->state)
-- 
2.43.0

Re: [PATCH 1/6] drm/ast: Remove redundant else in atomic_check

[Thomas Zimmermann]

Hi

Am 25.02.25 um 12:06 schrieb Aditya Garg:
> From: Aditya Garg <gargaditya08@live.com>
>
> Remove the redundant else statement from atomic_check since the previous if
> statement was returning if true.
>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
>   drivers/gpu/drm/ast/ast_mode.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 9d5321c81..3817d1e4c 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -610,9 +610,10 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>   						  DRM_PLANE_NO_SCALING,
>   						  DRM_PLANE_NO_SCALING,
>   						  false, true);
> -	if (ret) {
> +	if (ret)
>   		return ret;
> -	} else if (!new_plane_state->visible) {
> +
> +	if (!new_plane_state->visible) {

I've seen this posted before.

The reason why there is an 'else' branch here is that both branches 
handle the state returned by the function call above, 
drm_atomic_helper_check_plane_state(). First it does an error check, and 
then it tests for >visible. In both cases, the plane's atomic_check 
should return. And only if we have a valid and visible plane, we do the 
actual checks on the plane. Conceptually, these if-else cases belong 
together and signal an early-out from the call.

I'd prefer to keep the drivers as they are.

Best regards
Thomas


>   		if (drm_WARN_ON(dev, new_plane_state->crtc)) /* cannot legally happen */
>   			return -EINVAL;
>   		else

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 1/6] drm/ast: Remove redundant else in atomic_check

[Aditya Garg]

> On 25 Feb 2025, at 5:19 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
> Hi
> 
>> Am 25.02.25 um 12:06 schrieb Aditya Garg:
>> From: Aditya Garg <gargaditya08@live.com>
>> 
>> Remove the redundant else statement from atomic_check since the previous if
>> statement was returning if true.
>> 
>> Signed-off-by: Aditya Garg <gargaditya08@live.com>
>> ---
>>  drivers/gpu/drm/ast/ast_mode.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index 9d5321c81..3817d1e4c 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -610,9 +610,10 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>                            DRM_PLANE_NO_SCALING,
>>                            DRM_PLANE_NO_SCALING,
>>                            false, true);
>> -    if (ret) {
>> +    if (ret)
>>          return ret;
>> -    } else if (!new_plane_state->visible) {
>> +
>> +    if (!new_plane_state->visible) {
> 
> I've seen this posted before.
> 
> The reason why there is an 'else' branch here is that both branches handle the state returned by the function call above, drm_atomic_helper_check_plane_state(). First it does an error check, and then it tests for >visible. In both cases, the plane's atomic_check should return. And only if we have a valid and visible plane, we do the actual checks on the plane. Conceptually, these if-else cases belong together and signal an early-out from the call.
> 
> I'd prefer to keep the drivers as they are.

Alright, should this be kept in the appletbdrm driver as well then?