[PATCH v2] drm/amd/amdgpu: Remove redundant else branch in amdgpu_encoders.c

SHANMUGAM, SRINIVASAN SRINIVASAN.SHANMUGAM at amd.com
Wed May 17 13:18:41 UTC 2023


[AMD Official Use Only - General]

-----Original Message-----
From: Deucher, Alexander <Alexander.Deucher at amd.com>
Sent: Wednesday, May 17, 2023 6:25 PM
To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>
Cc: amd-gfx at lists.freedesktop.org
Subject: RE: [PATCH v2] drm/amd/amdgpu: Remove redundant else branch in amdgpu_encoders.c

[AMD Official Use Only - General]

> -----Original Message-----
> From: SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM at amd.com>
> Sent: Wednesday, May 17, 2023 6:07 AM
> To: Koenig, Christian <Christian.Koenig at amd.com>; Deucher, Alexander
> <Alexander.Deucher at amd.com>
> Cc: amd-gfx at lists.freedesktop.org; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM at amd.com>
> Subject: [PATCH v2] drm/amd/amdgpu: Remove redundant else branch in
> amdgpu_encoders.c
>
> Adhere to Linux kernel coding style.
>
> Reported by checkpatch:
>
> WARNING: else is not generally useful after a break or return
>
> Cc: Christian König <christian.koenig at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
> ---
>
> v2:
>    - Avoid mulitple return statements in amdgpu_dig_monitor_is_duallink
>      function without affecting functionality
>    - Fix following warnings:
>         WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
>       WARNING: Missing a blank line after declarations
> #74: FILE: drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c:74:
> +                       struct amdgpu_connector *amdgpu_connector =
> to_amdgpu_connector(connector);
> +                       amdgpu_encoder->active_device =
> + amdgpu_encoder->devices & amdgpu_connector->devices;
>
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c | 41 ++++++++++++---
> -----
>  1 file changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c
> index c96e458ed088..8c17a74f001a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c
> @@ -71,6 +71,7 @@ void amdgpu_encoder_set_active_device(struct
> drm_encoder *encoder)
>       drm_for_each_connector_iter(connector, &iter) {
>               if (connector->encoder == encoder) {
>                       struct amdgpu_connector *amdgpu_connector =
> to_amdgpu_connector(connector);
> +
>                       amdgpu_encoder->active_device =
> amdgpu_encoder->devices & amdgpu_connector->devices;
>                       DRM_DEBUG_KMS("setting active device to %08x from %08x %08x for
> encoder %d\n",
>                                 amdgpu_encoder->active_device,
> amdgpu_encoder->devices, @@ -166,12 +167,12 @@ void
> amdgpu_panel_mode_fixup(struct drm_encoder *encoder,  {
>       struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
>       struct drm_display_mode *native_mode = &amdgpu_encoder-
> >native_mode;
> -     unsigned hblank = native_mode->htotal - native_mode->hdisplay;
> -     unsigned vblank = native_mode->vtotal - native_mode->vdisplay;
> -     unsigned hover = native_mode->hsync_start - native_mode-
> >hdisplay;
> -     unsigned vover = native_mode->vsync_start - native_mode-
> >vdisplay;
> -     unsigned hsync_width = native_mode->hsync_end - native_mode-
> >hsync_start;
> -     unsigned vsync_width = native_mode->vsync_end - native_mode-
> >vsync_start;
> +     unsigned int hblank = native_mode->htotal - native_mode-
> >hdisplay;
> +     unsigned int vblank = native_mode->vtotal - native_mode->vdisplay;
> +     unsigned int hover = native_mode->hsync_start - native_mode-
> >hdisplay;
> +     unsigned int vover = native_mode->vsync_start - native_mode-
> >vdisplay;
> +     unsigned int hsync_width = native_mode->hsync_end -
> native_mode->hsync_start;
> +     unsigned int vsync_width = native_mode->vsync_end -
> +native_mode->vsync_start;
>

This part looks fine.

Cool thanks a lot! Could you please help me with your feedbacks - I have sent separate patch https://patchwork.freedesktop.org/patch/537574/?series=117876&rev=1 for the above part , with the below one's dropped.

>       adjusted_mode->clock = native_mode->clock;
>       adjusted_mode->flags = native_mode->flags; @@ -208,6 +209,7 @@ bool
> amdgpu_dig_monitor_is_duallink(struct drm_encoder *encoder,
>       struct drm_connector *connector;
>       struct amdgpu_connector *amdgpu_connector;
>       struct amdgpu_connector_atom_dig *dig_connector;
> +     bool ret;
>
>       connector = amdgpu_get_connector_for_encoder(encoder);
>       /* if we don't have an active device yet, just use one of @@ -224,39
> +226,44 @@ bool amdgpu_dig_monitor_is_duallink(struct drm_encoder
> *encoder,
>                       /* HDMI 1.3 supports up to 340 Mhz over single link */
>                       if (connector->display_info.is_hdmi) {
>                               if (pixel_clock > 340000)
> -                                     return true;
> +                                     ret = true;
>                               else
> -                                     return false;
> +                                     ret = false;
>                       } else {
>                               if (pixel_clock > 165000)
> -                                     return true;
> +                                     ret = true;
>                               else
> -                                     return false;
> +                                     ret =  false;
>                       }
>               } else
> -                     return false;
> +                     ret = false;
> +             break;
>       case DRM_MODE_CONNECTOR_DVID:
>       case DRM_MODE_CONNECTOR_HDMIA:
>       case DRM_MODE_CONNECTOR_DisplayPort:
>               dig_connector = amdgpu_connector->con_priv;
>               if ((dig_connector->dp_sink_type ==
> CONNECTOR_OBJECT_ID_DISPLAYPORT) ||
>                   (dig_connector->dp_sink_type ==
> CONNECTOR_OBJECT_ID_eDP))
> -                     return false;
> +                     ret = false;
>               else {
>                       /* HDMI 1.3 supports up to 340 Mhz over single link */
>                       if (connector->display_info.is_hdmi) {
>                               if (pixel_clock > 340000)
> -                                     return true;
> +                                     ret = true;
>                               else
> -                                     return false;
> +                                     ret = false;
>                       } else {
>                               if (pixel_clock > 165000)
> -                                     return true;
> +                                     ret = true;
>                               else
> -                                     return false;
> +                                     ret = false;
>                       }
>               }
> +             break;
>       default:
> -             return false;
> +             ret = false;
> +             break;
>       }
> +
> +     return ret;

These hunks make the code less readable.  I'd suggest dropping them.

Alex

>  }
> --
> 2.25.1


More information about the amd-gfx mailing list