Re: [PATCH] drm/panel-simple: Add Vivax TPC-9150 panel v3

From: Sam Ravnborg <sam@ravnborg.org>
To: Nikola Pavlica <pavlica.nikola@gmail.com>
Cc: dri-devel@lists.freedesktop.org, thierry.reding@gmail.com,
	airlied@linux.ie, daniel@ffwll.ch,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@googlegroups.com, linux-sunxi@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/panel-simple: Add Vivax TPC-9150 panel v3
Date: Tue, 17 Aug 2021 19:41:57 +0200	[thread overview]
Message-ID: <YRv05QbPz4lBQG8E@ravnborg.org> (raw)
In-Reply-To: <20210817163605.1077257-1-pavlica.nikola@gmail.com>

Hi Nikola,

thanks for the quick re-spin. There is still a few things that needs to
be addressed though. Sorry for not catching these the first time.

On Tue, Aug 17, 2021 at 06:36:05PM +0200, Nikola Pavlica wrote:
> The model and make of the LCD panel of the Vivax TPC-9150 is unknown,
> hence the panel settings that were retrieved with a FEX dump are named
HEX dump?

> after the device NOT the actual panel.
> 
> The LCD in question is a 50 pin MISO TFT LCD panel of the resolution
> 1024x600 used by the aforementioned device.
> 
> Version 2, as Thierry kindly suggested that I fix the order in which the
> panel was ordered compared to others.
> 
> Version 3, filling in the required info suggested by Sam. Plus some
> factual issues that I've corrected myself (tested working)
> 
> Thanks,
> Nikola
> 
> Signed-off-by: Nikola Pavlica <pavlica.nikola@gmail.com>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 29 ++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 4e2dad314c79..f6b3e58c162b 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -3989,6 +3989,32 @@ static const struct panel_desc urt_umsh_8596md_parallel = {
>  	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
>  };
>  
> +static const struct drm_display_mode vivax_tpc9150_panel_mode = {
> +	.clock = 60000,
> +	.hdisplay = 1024,
> +	.hsync_start = 1024 + 160,
> +	.hsync_end = 1024 + 160 + 100,
> +	.htotal = 1024 + 160 + 100 + 60,
> +	.vdisplay = 600,
> +	.vsync_start = 600 + 12,
> +	.vsync_end = 600 + 12 + 10,
> +	.vtotal = 600 + 12 + 10 + 13,
> +};
> +
> +static const struct panel_desc vivax_tpc9150_panel = {
> +	.modes = &vivax_tpc9150_panel_mode,
> +	.num_modes = 1,
Most panels put .bpc right above .size, so they follow the order in the
struct. This is bikeshedding but my OCD triggered here.

> +	.size = {
> +		.width = 200,
> +		.height = 115,
> +	},
> +	.bpc = 6,
> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X24,
This does not build - I have no MEDIA_BUS_FMT_RGB666_1X24 in my kernel
(drm-misc-next).

With an LVDS connector and bpc equals 6 my bet is on: MEDIA_BUS_FMT_RGB666_1X7X3_SPWG
This is from looking at similar panels.

> +	.bus_flags = DRM_BUS_FLAG_DE_HIGH,
> +	.connector_type = DRM_MODE_CONNECTOR_LVDS,
> +};
> +
> +
>  static const struct drm_display_mode vl050_8048nt_c01_mode = {
>  	.clock = 33333,
>  	.hdisplay = 800,
> @@ -4490,6 +4516,9 @@ static const struct of_device_id platform_of_match[] = {
>  	}, {
>  		.compatible = "urt,umsh-8596md-20t",
>  		.data = &urt_umsh_8596md_parallel,
> +	}, {
> +		.compatible = "vivax,tpc9150-panel",
vivax is an unknown vendor, needs to be added to
Documentation/devicetree/bindings/vendor-prefixes.yaml in a separate
patch.

tpc9150-panel should be added to
Documentation/devicetree/bindings/display/panel/panel-simple.yaml
or at least I assume this is the file to add it to.
Again as a separate patch.

For the two binding related patches see
Documentation/devicetree/bindings/submitting-patches.rst

Sorry for making this difficult, but we need it done right.

	Sam