Re: [PATCH 04/11] drm/bridge: tc358768: Use struct videomode

[Péter Ujfalusi]

On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The TC358768 documentation uses HFP, HBP, etc. values to deal with the
> video mode, while the driver currently uses the DRM display mode
> (htotal, hsync_start, etc).
> 
> Change the driver to convert the DRM display mode to struct videomode,
> which then allows us to use the same units the documentation uses. This
> makes it much easier to work on the code when using the TC358768
> documentation as a reference.

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@xxxxxxxxx>

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 45 +++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index d5831a1236e9..9b633038af33 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -650,6 +650,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	u32 dsiclk, dsibclk, video_start;
>  	const u32 internal_delay = 40;
>  	int ret, i;
> +	struct videomode vm;
>  
>  	if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
>  		dev_warn_once(priv->dev, "Non-continuous mode unimplemented, falling back to continuous\n");
> @@ -673,6 +674,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  		return;
>  	}
>  
> +	drm_display_mode_to_videomode(mode, &vm);
> +
>  	dsiclk = priv->dsiclk;
>  	dsibclk = dsiclk / 4;
>  
> @@ -681,28 +684,28 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	switch (dsi_dev->format) {
>  	case MIPI_DSI_FMT_RGB888:
>  		val |= (0x3 << 4);
> -		hact = mode->hdisplay * 3;
> -		video_start = (mode->htotal - mode->hsync_start) * 3;
> +		hact = vm.hactive * 3;
> +		video_start = (vm.hsync_len + vm.hback_porch) * 3;
>  		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
>  		break;
>  	case MIPI_DSI_FMT_RGB666:
>  		val |= (0x4 << 4);
> -		hact = mode->hdisplay * 3;
> -		video_start = (mode->htotal - mode->hsync_start) * 3;
> +		hact = vm.hactive * 3;
> +		video_start = (vm.hsync_len + vm.hback_porch) * 3;
>  		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
>  		break;
>  
>  	case MIPI_DSI_FMT_RGB666_PACKED:
>  		val |= (0x4 << 4) | BIT(3);
> -		hact = mode->hdisplay * 18 / 8;
> -		video_start = (mode->htotal - mode->hsync_start) * 18 / 8;
> +		hact = vm.hactive * 18 / 8;
> +		video_start = (vm.hsync_len + vm.hback_porch) * 18 / 8;
>  		data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
>  		break;
>  
>  	case MIPI_DSI_FMT_RGB565:
>  		val |= (0x5 << 4);
> -		hact = mode->hdisplay * 2;
> -		video_start = (mode->htotal - mode->hsync_start) * 2;
> +		hact = vm.hactive * 2;
> +		video_start = (vm.hsync_len + vm.hback_porch) * 2;
>  		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
>  		break;
>  	default:
> @@ -814,43 +817,43 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  		tc358768_write(priv, TC358768_DSI_EVENT, 0);
>  
>  		/* vact */
> -		tc358768_write(priv, TC358768_DSI_VACT, mode->vdisplay);
> +		tc358768_write(priv, TC358768_DSI_VACT, vm.vactive);
>  
>  		/* vsw */
> -		tc358768_write(priv, TC358768_DSI_VSW,
> -			       mode->vsync_end - mode->vsync_start);
> +		tc358768_write(priv, TC358768_DSI_VSW, vm.vsync_len);
> +
>  		/* vbp */
> -		tc358768_write(priv, TC358768_DSI_VBPR,
> -			       mode->vtotal - mode->vsync_end);
> +		tc358768_write(priv, TC358768_DSI_VBPR, vm.vback_porch);
>  
>  		/* hsw * byteclk * ndl / pclk */
> -		val = (u32)div_u64((mode->hsync_end - mode->hsync_start) *
> +		val = (u32)div_u64(vm.hsync_len *
>  				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
> -				   mode->clock * 1000);
> +				   vm.pixelclock);
>  		tc358768_write(priv, TC358768_DSI_HSW, val);
>  
>  		/* hbp * byteclk * ndl / pclk */
> -		val = (u32)div_u64((mode->htotal - mode->hsync_end) *
> +		val = (u32)div_u64(vm.hback_porch *
>  				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
> -				   mode->clock * 1000);
> +				   vm.pixelclock);
>  		tc358768_write(priv, TC358768_DSI_HBPR, val);
>  	} else {
>  		/* Set event mode */
>  		tc358768_write(priv, TC358768_DSI_EVENT, 1);
>  
>  		/* vact */
> -		tc358768_write(priv, TC358768_DSI_VACT, mode->vdisplay);
> +		tc358768_write(priv, TC358768_DSI_VACT, vm.vactive);
>  
>  		/* vsw (+ vbp) */
>  		tc358768_write(priv, TC358768_DSI_VSW,
> -			       mode->vtotal - mode->vsync_start);
> +			       vm.vsync_len + vm.vback_porch);
> +
>  		/* vbp (not used in event mode) */
>  		tc358768_write(priv, TC358768_DSI_VBPR, 0);
>  
>  		/* (hsw + hbp) * byteclk * ndl / pclk */
> -		val = (u32)div_u64((mode->htotal - mode->hsync_start) *
> +		val = (u32)div_u64((vm.hsync_len + vm.hback_porch) *
>  				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
> -				   mode->clock * 1000);
> +				   vm.pixelclock);
>  		tc358768_write(priv, TC358768_DSI_HSW, val);
>  
>  		/* hbp (not used in event mode) */
> 

-- 
Péter