| Message ID | BD2BA1E2-D61E-4D25-857C-C9F435D41EF3@live.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: remove redundant else across drivers | expand |
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
> 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?
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