[AMD Official Use Only - General]
-----Original Message-----
From: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
Sent: Wednesday, May 17, 2023 6:25 PM
To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@xxxxxxx>; Koenig, Christian <Christian.Koenig@xxxxxxx>
Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
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@xxxxxxx>
> Sent: Wednesday, May 17, 2023 6:07 AM
> To: Koenig, Christian <Christian.Koenig@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>
> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM@xxxxxxx>
> 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@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> ---
>
> 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
