Re: [PATCH V6 5/8] drm/exynos: dp: Modify driver to support drm_panel

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

On Sat, Jul 26, 2014 at 12:52:07AM +0530, Ajay Kumar wrote:
[...]
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
[...]
> @@ -887,6 +885,12 @@ static void exynos_dp_commit(struct exynos_drm_display *display)
>  	struct exynos_dp_device *dp = display->ctx;
>  	int ret;
>  
> +	/* Keep backlight disabled while we configure video */
> +	if (dp->panel) {
> +		if (drm_panel_disable(dp->panel))
> +			DRM_ERROR("failed to disable panel backlight\n");
> +	}

drm_panel_disable() already gracefully handles the dp->panel == NULL
case, so no need to check for it explicitly. But perhaps you do that
only because panel support is optional and you want to avoid the
error message if it isn't set up. In that case it's probably fine to
leave this as-is.

But you should change the comment and error message, since you don't
know what exactly drm_panel_disable() does.

And it might be useful to print the error code while at it, it might
help save some debugging time in the future.

The same comments apply to most of the remainder of the file, but it
looks good to me otherwise.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]