Re: [PATCH 2/5] drm/panel: nv3051d: Add Powkiddy RK2023 Panel Support

[Chris Morgan <macromorgan@hotmail.com>]

On Thu, Oct 19, 2023 at 10:22:24AM -0700, Jessica Zhang wrote:
> 
> 
> On 10/18/2023 9:18 AM, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Refactor the driver to add support for the powkiddy,rk2023-panel
> > panel. This panel is extremely similar to the rg353p-panel but
> > requires a smaller vertical back porch and isn't as tolerant of
> > higher speeds.
> > 
> > Tested on my RG351V, RG353P, RG353V, and RK2023.
> > 
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> 
> Hi Chris,
> 
> Thanks for the patch. Just have a minor question below.
> 
> > ---
> >   .../gpu/drm/panel/panel-newvision-nv3051d.c   | 56 +++++++++++++++----
> >   1 file changed, 45 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-newvision-nv3051d.c b/drivers/gpu/drm/panel/panel-newvision-nv3051d.c
> > index 79de6c886292..d24c51503d68 100644
> > --- a/drivers/gpu/drm/panel/panel-newvision-nv3051d.c
> > +++ b/drivers/gpu/drm/panel/panel-newvision-nv3051d.c
> > @@ -28,6 +28,7 @@ struct nv3051d_panel_info {
> >   	unsigned int num_modes;
> >   	u16 width_mm, height_mm;
> >   	u32 bus_flags;
> > +	u32 mode_flags;
> >   };
> >   struct panel_nv3051d {
> > @@ -385,15 +386,7 @@ static int panel_nv3051d_probe(struct mipi_dsi_device *dsi)
> >   	dsi->lanes = 4;
> >   	dsi->format = MIPI_DSI_FMT_RGB888;
> > -	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> > -			  MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
> > -
> > -	/*
> > -	 * The panel in the RG351V is identical to the 353P, except it
> > -	 * requires MIPI_DSI_CLOCK_NON_CONTINUOUS to operate correctly.
> > -	 */
> > -	if (of_device_is_compatible(dev->of_node, "anbernic,rg351v-panel"))
> > -		dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
> > +	dsi->mode_flags = ctx->panel_info->mode_flags;
> >   	drm_panel_init(&ctx->panel, &dsi->dev, &panel_nv3051d_funcs,
> >   		       DRM_MODE_CONNECTOR_DSI);
> > @@ -481,18 +474,59 @@ static const struct drm_display_mode nv3051d_rgxx3_modes[] = {
> >   	},
> >   };
> > -static const struct nv3051d_panel_info nv3051d_rgxx3_info = {
> > +static const struct drm_display_mode nv3051d_rk2023_modes[] = {
> > +	{
> > +		.hdisplay       = 640,
> > +		.hsync_start    = 640 + 40,
> > +		.hsync_end      = 640 + 40 + 2,
> > +		.htotal         = 640 + 40 + 2 + 80,
> > +		.vdisplay       = 480,
> > +		.vsync_start    = 480 + 18,
> > +		.vsync_end      = 480 + 18 + 2,
> > +		.vtotal         = 480 + 18 + 2 + 4,
> > +		.clock          = 24150,
> > +		.flags          = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> > +	},
> > +};
> > +
> > +static const struct nv3051d_panel_info nv3051d_rg351v_info = {
> >   	.display_modes = nv3051d_rgxx3_modes,
> >   	.num_modes = ARRAY_SIZE(nv3051d_rgxx3_modes),
> >   	.width_mm = 70,
> >   	.height_mm = 57,
> >   	.bus_flags = DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
> > +	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> > +		      MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET |
> > +		      MIPI_DSI_CLOCK_NON_CONTINUOUS,
> > +};
> > +
> > +static const struct nv3051d_panel_info nv3051d_rg353p_info = {
> > +	.display_modes = nv3051d_rgxx3_modes,
> > +	.num_modes = ARRAY_SIZE(nv3051d_rgxx3_modes),
> > +	.width_mm = 70,
> > +	.height_mm = 57,
> 
> Will all the panels for this driver be 70x57? If so, would it be better to
> set display_info.[width_mm|height_mm] directly?

They are all so far the same size, but I can't guarantee that going forward.
To my knowledge this is the last of the nv3051d devices I'll be working on
in the foreseeable future though, and so far they're all identical in size.

> 
> > +	.bus_flags = DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
> > +	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> > +		      MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET,
> > +};
> > +
> > +static const struct nv3051d_panel_info nv3051d_rk2023_info = {
> > +	.display_modes = nv3051d_rk2023_modes,
> > +	.num_modes = ARRAY_SIZE(nv3051d_rk2023_modes),
> > +	.width_mm = 70,
> > +	.height_mm = 57,
> > +	.bus_flags = DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
> > +	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> > +		      MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET,
> >   };
> >   static const struct of_device_id newvision_nv3051d_of_match[] = {
> > -	{ .compatible = "newvision,nv3051d", .data = &nv3051d_rgxx3_info },
> > +	{ .compatible = "anbernic,rg351v-panel", .data = &nv3051d_rg351v_info },
> > +	{ .compatible = "anbernic,rg353p-panel", .data = &nv3051d_rg353p_info },
> > +	{ .compatible = "powkiddy,rk2023-panel", .data = &nv3051d_rk2023_info },
> >   	{ /* sentinel */ }
> >   };
> > +

Sorry, will fix that in a V2. Thank you.

> 
> I think you can drop this stray newline.
> 
> Thanks,
> 
> Jessica Zhang
> 
> >   MODULE_DEVICE_TABLE(of, newvision_nv3051d_of_match);
> >   static struct mipi_dsi_driver newvision_nv3051d_driver = {
> > -- 
> > 2.34.1
> >