Re: [PATCH 01/41] drm/panel: simple: Change mode for Sharp lq123p1jx31

From: Thierry Reding <thierry.reding@gmail.com>
To: Sean Paul <seanpaul@chromium.org>
Cc: "Chris Zhong" <zyw@rock-chips.com>,
	linux-rockchip@lists.infradead.org,
	dri-devel@lists.freedesktop.org,
	"Stéphane Marchesin" <marcheu@chromium.org>
Subject: Re: [PATCH 01/41] drm/panel: simple: Change mode for Sharp lq123p1jx31
Date: Mon, 20 Mar 2017 14:59:45 +0100	[thread overview]
Message-ID: <20170320135945.GQ22463@ulmo.ba.sec> (raw)
In-Reply-To: <20170310043305.17216-2-seanpaul@chromium.org>

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

On Thu, Mar 09, 2017 at 11:32:16PM -0500, Sean Paul wrote:
> Change the mode for Sharp lq123p1jx31 panel to something more
> rockchip-friendly such that we can use the fixed PLLs to
> generate the pixel clock
> 
> Cc: Chris Zhong <zyw@rock-chips.com>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

That's really not how you should be doing this. If you want to support
this panel on more than one type of hardware, especially if they have
different restrictions on what clocks and timings they can generate,
the driver should specify the timings using a struct display_timing and
drivers should use that data to generate a mode that matches their
requirements but is still within the valid ranges specified in the .min
and .max values.

That said, in this particular case nobody seems to be using the panel
in the upstream kernel.

One minor nit below...

> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 89eb0422821c..787b4d143220 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1598,17 +1598,18 @@ static const struct panel_desc sharp_lq101k1ly04 = {
>  };
>  
>  static const struct drm_display_mode sharp_lq123p1jx31_mode = {
> -	.clock = 252750,
> +	.clock = 266667,
>  	.hdisplay = 2400,
>  	.hsync_start = 2400 + 48,
>  	.hsync_end = 2400 + 48 + 32,
> -	.htotal = 2400 + 48 + 32 + 80,
> +	.htotal = 2400 + 48 + 32 + 139,
>  	.vdisplay = 1600,
>  	.vsync_start = 1600 + 3,
>  	.vsync_end = 1600 + 3 + 10,
> -	.vtotal = 1600 + 3 + 10 + 33,
> +	.vtotal = 1600 + 3 + 10 + 84,
>  	.vrefresh = 60,
>  	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> +	.type = DRM_MODE_TYPE_PREFERRED | DRM_MODE_TYPE_DRIVER,

The panel-simple driver will set these two flags itself, no need to
explicitly specify them.

Thierry

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

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

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