Re: [PATCH 6/7] drm/panel: sitronix-st7789v: Add EDT ET028013DMA panel support

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Maxime,

mripard@kernel.org wrote on Mon, 12 Jun 2023 10:51:09 +0200:

> On Fri, Jun 09, 2023 at 04:59:50PM +0200, Miquel Raynal wrote:
> > This panel from Emerging Display Technologies Corporation features an
> > ST7789V2 panel inside which is almost identical to what the Sitronix
> > panel driver supports.
> > 
> > In practice, the module physical size is specific, and experiments show
> > that the display will malfunction if any of the following situation
> > occurs:
> > * Pixel clock is above 3MHz
> > * Pixel clock is not inverted
> > I could not properly identify the reasons behind these failures, scope
> > captures show valid input signals.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../gpu/drm/panel/panel-sitronix-st7789v.c    | 34 +++++++++++++++++--
> >  1 file changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > index 212bccc31804..7de192a3a8aa 100644
> > --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > @@ -30,7 +30,8 @@
> >  #define ST7789V_RGBCTRL_RCM(n)			(((n) & 3) << 5)
> >  #define ST7789V_RGBCTRL_VSYNC_HIGH		BIT(3)
> >  #define ST7789V_RGBCTRL_HSYNC_HIGH		BIT(2)
> > -#define ST7789V_RGBCTRL_PCLK_HIGH		BIT(1)
> > +#define ST7789V_RGBCTRL_PCLK_FALLING		BIT(1)
> > +#define ST7789V_RGBCTRL_PCLK_RISING		0
> >  #define ST7789V_RGBCTRL_VBP(n)			((n) & 0x7f)
> >  #define ST7789V_RGBCTRL_HBP(n)			((n) & 0x1f)
> >  
> > @@ -117,6 +118,7 @@ struct st7789v_panel_info {
> >  	u16 width_mm;
> >  	u16 height_mm;
> >  	u32 bus_format;
> > +	u32 bus_flags;
> >  };
> >  
> >  struct st7789v {
> > @@ -175,6 +177,18 @@ static const struct drm_display_mode default_mode = {
> >  	.vtotal = 320 + 8 + 4 + 4,
> >  };
> >  
> > +static const struct drm_display_mode slow_mode = {
> > +	.clock = 3000,
> > +	.hdisplay = 240,
> > +	.hsync_start = 240 + 38,
> > +	.hsync_end = 240 + 38 + 10,
> > +	.htotal = 240 + 38 + 10 + 10,
> > +	.vdisplay = 320,
> > +	.vsync_start = 320 + 8,
> > +	.vsync_end = 320 + 8 + 4,
> > +	.vtotal = 320 + 8 + 4 + 4,
> > +};  
> 
> Why is it supposed to be slow (and compared to what)? It looks like a
> fairly normal mode to me? Or is it because it's refreshed at 30Hz?

The initial support was using 60Hz and for a reason that I do not
understand (scope capture look right), the panel I use is highly
unstable at whatever frequency above 30Hz, so for me it was "slow" :-)
But of course I'll use the panel name to qualify the mode.

> Either way, the ST7789V is a panel controller and can be connected to a
> wide range of panels depending on the model, so it would be better to
> just use the model name there.

Sure.

> >  static int st7789v_get_modes(struct drm_panel *panel,
> >  			     struct drm_connector *connector)
> >  {
> > @@ -197,6 +211,7 @@ static int st7789v_get_modes(struct drm_panel *panel,
> >  
> >  	connector->display_info.width_mm = panel_info->width_mm;
> >  	connector->display_info.height_mm = panel_info->height_mm;
> > +	connector->display_info.bus_flags = panel_info->bus_flags;
> >  	drm_display_info_set_bus_formats(&connector->display_info,
> >  					 &panel_info->bus_format, 1);
> >  
> > @@ -206,8 +221,13 @@ static int st7789v_get_modes(struct drm_panel *panel,
> >  static int st7789v_prepare(struct drm_panel *panel)
> >  {
> >  	struct st7789v *ctx = panel_to_st7789v(panel);
> > +	const struct st7789v_panel_info *panel_info = ctx->panel_info;
> > +	u8 pck = ST7789V_RGBCTRL_PCLK_FALLING;
> >  	int ret;
> >  
> > +	if (panel_info->bus_flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE)
> > +		pck = ST7789V_RGBCTRL_PCLK_RISING;
> > +  
> 
> I guess it could be in a separate commit

I will rebase on top of Sebastian's series who already addressed that
point (as well as many others).

[...]

> >  
> > +static const struct st7789v_panel_info et028013dma_info = {
> > +	.display_mode = &slow_mode,
> > +	.width_mm = 43,
> > +	.height_mm = 58,
> > +	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> > +};
> > +
> >  static const struct of_device_id st7789v_of_match[] = {
> >  	{ .compatible = "sitronix,st7789v", .data = &st7789v_info },
> > +	{ .compatible = "edt,et028013dma", .data = &et028013dma_info },  
> 
> We should sort them by alphabetical order
> 
> Maxime

Ok!

Thanks,
Miquèl