Re: [PATCH v2 4/4] drm/bridge: tfp410: Fix setup and hold time calculation

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Ricardo,

Thank you for the patch.

On Thu, May 14, 2020 at 04:36:12PM +0200, Ricardo Cañuelo wrote:
> The tfp410 has a data de-skew feature that allows the user to compensate
> the skew between IDCK and the pixel data and control signals.
> 
> In the driver, the setup and hold times are calculated from the de-skew
> value. This retrieves the deskew value from the DT using the proper
> datatype and range check as described by the binding (u32 from 0 to 7)
> and fixes the calculation of the setup and hold times.

How about mentioning that the fix results from a change in the DT
bindings ? Otherwise it sounds it was a bug in the driver.

I would also mention that the patch fixes the min() calculation, which
was clearly wrong. I think I would have split this in two patches.

With this addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
> ---
>  drivers/gpu/drm/bridge/ti-tfp410.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index e3eb6364c0f7..21d99b4ea0c9 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -220,7 +220,7 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>  	struct device_node *ep;
>  	u32 pclk_sample = 0;
>  	u32 bus_width = 24;
> -	s32 deskew = 0;
> +	u32 deskew = 0;
>  
>  	/* Start with defaults. */
>  	*timings = tfp410_default_timings;
> @@ -274,12 +274,12 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>  	}
>  
>  	/* Get the setup and hold time from vendor-specific properties. */
> -	of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew);
> -	if (deskew < -4 || deskew > 3)
> +	of_property_read_u32(dvi->dev->of_node, "ti,deskew", &deskew);
> +	if (deskew > 7)
>  		return -EINVAL;
>  
> -	timings->setup_time_ps = min(0, 1200 - 350 * deskew);
> -	timings->hold_time_ps = min(0, 1300 + 350 * deskew);
> +	timings->setup_time_ps = 1200 - 350 * ((s32)deskew - 4);
> +	timings->hold_time_ps = max(0, 1300 + 350 * ((s32)deskew - 4));
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart