Re: [PATCH] drm/omap: change default signal polarities and drives

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

Hi Tomi,

Thank you for the patch.

On Fri, Apr 17, 2020 at 02:41:51PM +0300, Tomi Valkeinen wrote:
> If the given videomode does not specify DISPLAY_FLAG_* for the specific
> signal property, the driver used a default value. These defaults were
> never thought through, as the expectation was that all the DISPLAY_FLAGS
> are always set explicitly.
> 
> With DRM bridge and panel drivers this is not the case, and while that
> issue should be resolved in the future, it's still good to have sane
> signal defaults.
> 
> This patch changes the defaults to what the hardware has as reset
> defaults. Also, based on my experience, I think they make sense and are
> more likely correct than the defaults without this patch.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 33 ++++++-----------------------
>  1 file changed, 6 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index dbb90f2d2ccd..6639ee9b05d3 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -3137,33 +3137,12 @@ static void _dispc_mgr_set_lcd_timings(struct dispc_device *dispc,
>  	dispc_write_reg(dispc, DISPC_TIMING_H(channel), timing_h);
>  	dispc_write_reg(dispc, DISPC_TIMING_V(channel), timing_v);
>  
> -	if (vm->flags & DISPLAY_FLAGS_VSYNC_HIGH)
> -		vs = false;
> -	else
> -		vs = true;
> -
> -	if (vm->flags & DISPLAY_FLAGS_HSYNC_HIGH)
> -		hs = false;
> -	else
> -		hs = true;
> -
> -	if (vm->flags & DISPLAY_FLAGS_DE_HIGH)
> -		de = false;
> -	else
> -		de = true;
> -
> -	if (vm->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
> -		ipc = false;
> -	else
> -		ipc = true;
> -
> -	/* always use the 'rf' setting */
> -	onoff = true;
> -
> -	if (vm->flags & DISPLAY_FLAGS_SYNC_POSEDGE)
> -		rf = true;
> -	else
> -		rf = false;
> +	vs = !!(vm->flags & DISPLAY_FLAGS_VSYNC_LOW);
> +	hs = !!(vm->flags & DISPLAY_FLAGS_HSYNC_LOW);
> +	de = !!(vm->flags & DISPLAY_FLAGS_DE_LOW);
> +	ipc = !!(vm->flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE);
> +	onoff = true; /* always use the 'rf' setting */
> +	rf = !!(vm->flags & DISPLAY_FLAGS_SYNC_POSEDGE);

Would it make sense to WARN() if flags are not set, to catch offenders
and fix them ? Apart from that,

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

>  
>  	l = FLD_VAL(onoff, 17, 17) |
>  		FLD_VAL(rf, 16, 16) |

-- 
Regards,

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