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

Jessica Zhang
2023-10-19T17:22:43Z

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?

> + .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 */ }
> };
> +

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
>