[PATCH] drm/bridge: analogix-anx78xx: Fix drm_dp_link helper removal

[PATCH] drm/bridge: analogix-anx78xx: Fix drm_dp_link helper removal

[Torsten Duwe]

drm_dp_link_rate_to_bw_code and ...bw_code_to_link_rate simply divide by
and multiply with 27000, respectively. Avoid an overflow in the u8 dpcd[0]
and the multiply+divide alltogether.

fixes: ff1e8fb68ea06027 ("analogix-anx78xx: Avoid drm_dp_link helpers")
Signed-off-by: Torsten Duwe <duwe@suse.de>
---
Has anybody actually tested ff1e8fb68ea06027 ? I copied that code in good
faith for the anx6345 and changed a few other things simultaneously, and
spent some time wondering why the panel stayed dark.

---
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
index 41867be03751..864423f59d66 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
@@ -722,10 +722,9 @@ static int anx78xx_dp_link_training(struct anx78xx *anx78xx)
 	if (err)
 		return err;
 
-	dpcd[0] = drm_dp_max_link_rate(anx78xx->dpcd);
-	dpcd[0] = drm_dp_link_rate_to_bw_code(dpcd[0]);
 	err = regmap_write(anx78xx->map[I2C_IDX_TX_P0],
-			   SP_DP_MAIN_LINK_BW_SET_REG, dpcd[0]);
+			   SP_DP_MAIN_LINK_BW_SET_REG,
+			   anx78xx->dpcd[DP_MAX_LINK_RATE]);
 	if (err)
 		return err;
 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/bridge: analogix-anx78xx: Fix drm_dp_link helper removal

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2022 bytes --]

On Sat, Nov 30, 2019 at 03:51:55PM +0100, Torsten Duwe wrote:
> drm_dp_link_rate_to_bw_code and ...bw_code_to_link_rate simply divide by
> and multiply with 27000, respectively. Avoid an overflow in the u8 dpcd[0]
> and the multiply+divide alltogether.

This was supposed to be a 1:1 conversion of what the helpers did, but
then I tried to be clever and reuse variables rather than add an
intermediate one and failed to realize that dpcd[0] was u8 and would
therefore overflow with the multiplication.

Personally I think it's slightly nicer to go through the multiply/divide
for consistency with how these helpers are used. MAX_LINK_RATE is a bit
of an exception in that the field occupies the whole register, but most
other registers need to have the fields extracted properly.

In this case it doesn't really matter, so the below is probably good
enough.

Good catch!

Reviewed-by: Thierry Reding <treding@nvidia.com>

> fixes: ff1e8fb68ea06027 ("analogix-anx78xx: Avoid drm_dp_link helpers")
> Signed-off-by: Torsten Duwe <duwe@suse.de>
> ---
> Has anybody actually tested ff1e8fb68ea06027 ? I copied that code in good
> faith for the anx6345 and changed a few other things simultaneously, and
> spent some time wondering why the panel stayed dark.
> 
> ---
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> index 41867be03751..864423f59d66 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> @@ -722,10 +722,9 @@ static int anx78xx_dp_link_training(struct anx78xx *anx78xx)
>  	if (err)
>  		return err;
>  
> -	dpcd[0] = drm_dp_max_link_rate(anx78xx->dpcd);
> -	dpcd[0] = drm_dp_link_rate_to_bw_code(dpcd[0]);
>  	err = regmap_write(anx78xx->map[I2C_IDX_TX_P0],
> -			   SP_DP_MAIN_LINK_BW_SET_REG, dpcd[0]);
> +			   SP_DP_MAIN_LINK_BW_SET_REG,
> +			   anx78xx->dpcd[DP_MAX_LINK_RATE]);
>  	if (err)
>  		return err;
>  

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel