From mboxrd@z Thu Jan  1 00:00:00 1970
From: Sam Ravnborg <sam@ravnborg.org>
Subject: Re: [PATCH] drm/panel: simple: Add Ampire am800480b3tmqw
Date: Fri, 2 Nov 2018 17:34:49 +0100
Message-ID: <20181102163449.GA25035@ravnborg.org>
References: <20181101125138.27848-1-aford173@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Return-path: <linux-kernel-owner@vger.kernel.org>
Content-Disposition: inline
In-Reply-To: <20181101125138.27848-1-aford173@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
To: Adam Ford <aford173@gmail.com>
Cc: linux-kernel@vger.kernel.org, airlied@linux.ie, thierry.reding@gmail.com, adam.ford@logicpd.com, dri-devel@lists.freedesktop.org
List-Id: dri-devel@lists.freedesktop.org

Hi Adam

On Thu, Nov 01, 2018 at 07:51:38AM -0500, Adam Ford wrote:
> This adds support for the Ampire am800480b3tmqw display,
> a 7" 24-bit RGB panel wtih 800x480 resolution.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 97964f7f2ace..71e878f63c5b 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -436,6 +436,31 @@ static const struct panel_desc ampire_am800480r3tmqwa1h = {
>  	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
>  };
>  
> +static const struct drm_display_mode ampire_am800480b3tmqw_mode = {
> +	.clock = 30000,
> +	.hdisplay = 800,
> +	.hsync_start = 800 + 210,
> +	.hsync_end = 800 + 210 + 46,
> +	.htotal = 800 + 210 + 46 + 0,
> +	.vdisplay = 480,
> +	.vsync_start = 480 + 22,
> +	.vsync_end = 480 + 22 + 23,
> +	.vtotal = 480 + 22 + 23 + 0,
> +	.vrefresh = 60,
> +	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
> +};
+1 to see .flags specified


> +
> +static const struct panel_desc ampire_am800480b3tmqw = {
> +	.modes = &ampire_am800480b3tmqw_mode,
> +	.num_modes = 1,
> +	.bpc = 6,
> +	.size = {
> +		.width = 152,
> +		.height = 91,
> +	},
> +	.bus_format = MEDIA_BUS_FMT_RBG888_1X24,
> +};
Likewise good to see .bus_format specified.

But .bus_flags are not specified. 

>>From the header file:

<<<<<<<<<<<<<<<<<<<<
#define DRM_BUS_FLAG_DE_LOW             (1<<0)
#define DRM_BUS_FLAG_DE_HIGH            (1<<1)
/* drive data on pos. edge */
#define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
/* drive data on neg. edge */
#define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
/* data is transmitted MSB to LSB on the bus */
#define DRM_BUS_FLAG_DATA_MSB_TO_LSB    (1<<4)
/* data is transmitted LSB to MSB on the bus */
#define DRM_BUS_FLAG_DATA_LSB_TO_MSB    (1<<5)
/* drive sync on pos. edge */
#define DRM_BUS_FLAG_SYNC_POSEDGE       (1<<6)
/* drive sync on neg. edge */
#define DRM_BUS_FLAG_SYNC_NEGEDGE       (1<<7)

        /**
         * @bus_flags: Additional information (like pixel signal polarity) for
         * the pixel data on the bus, using DRM_BUS_FLAGS\_ defines.
         */
        u32 bus_flags;
>>>>>>>>>>>>>>>><

Many panels leave out .bus_flags - and I wonder if this is
because default is OK or because most other panels does so.

I had problems with my display that the text looked blurred
when bus_flags was no specified (using defaults).
This was one issue I had when migrating from 4.4 kernel
to a recent kernel.

So therefore it would good to have .bus_flags specified too
if for nothing else then for documentation purposes.

	Sam