Hi Laurent.
Thanks for looking into this, but you will not be happy in a minute...
On Fri, Aug 23, 2019 at 04:40:32AM +0300, Laurent Pinchart wrote:
> Add a type field to the drm_panel structure to report the panel type,
> using DRM_MODE_CONNECTOR_* macros (the values that make sense are LVDS,
> eDP, DSI and DPI).
> This will be used to initialise the corresponding connector type.
Ohh, that explains what this should be used for.
I had missed that we have the panel when we create the drm_connector.
(I had seen we had to use the connector type to match a panel with a
connector or something like that).
> diff --git a/drivers/gpu/drm/panel/panel-arm-versatile.c b/drivers/gpu/drm/panel/panel-arm-versatile.c
> index 5f72c922a04b..5c335fc1632b 100644
> --- a/drivers/gpu/drm/panel/panel-arm-versatile.c
> +++ b/drivers/gpu/drm/panel/panel-arm-versatile.c
> @@ -353,6 +353,7 @@ static int versatile_panel_probe(struct platform_device *pdev)
> drm_panel_init(&vpanel->panel);
> vpanel->panel.dev = dev;
> vpanel->panel.funcs = &versatile_panel_drm_funcs;
> + vpanel->panel.type = DRM_MODE_CONNECTOR_DPI;
The pattern where we call a simple init like here,
and then have a set of mandatory assignments right after
is not a good way to help the driver writer.
We should instead do:
drm_panel_init(&vpanel->panel, dev, &versatile_panel_drm_funcs, DRM_MODE_CONNECTOR_DPI);
Then all drm_panel users are forced to supply the init parameters,
and we do not secretly rely on some defaults for panels that do not have it.
Also new panels will fail to build if they do not specify the new field.
This may also allow us to chatch that:
> diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> index b5b14aa059ea..cac074939e2c 100644
> --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> @@ -428,6 +428,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
>
> ts->base.dev = dev;
> ts->base.funcs = &rpi_touchscreen_funcs;
> + ts->base.type = DRM_MODE_CONNECTOR_DSI;
forgets to call drm_panel_init()....
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 28fa6ba7b767..6d5d0c51e97e 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -94,6 +94,7 @@ struct panel_desc {
>
> u32 bus_format;
> u32 bus_flags;
> + unsigned int type;
> };
>
> struct panel_simple {
> @@ -467,6 +468,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
> drm_panel_init(&panel->base);
> panel->base.dev = dev;
> panel->base.funcs = &panel_simple_funcs;
> + panel->base.type = desc->type ? desc->type : DRM_MODE_CONNECTOR_DPI;
>
> err = drm_panel_add(&panel->base);
> if (err < 0)
> @@ -833,6 +835,7 @@ static const struct panel_desc auo_g133han01 = {
> .unprepare = 1000,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct display_timing auo_g185han01_timings = {
> @@ -862,6 +865,7 @@ static const struct panel_desc auo_g185han01 = {
> .unprepare = 1000,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct display_timing auo_p320hvn03_timings = {
> @@ -890,6 +894,7 @@ static const struct panel_desc auo_p320hvn03 = {
> .unprepare = 500,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct drm_display_mode auo_t215hvn01_mode = {
> @@ -1205,6 +1210,7 @@ static const struct panel_desc dlc_dlc0700yzg_1 = {
> .disable = 200,
> },
> .bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct display_timing dlc_dlc1010gig_timing = {
> @@ -1235,6 +1241,7 @@ static const struct panel_desc dlc_dlc1010gig = {
> .unprepare = 60,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct drm_display_mode edt_et035012dm6_mode = {
> @@ -1501,6 +1508,7 @@ static const struct panel_desc hannstar_hsd070pww1 = {
> .height = 94,
> },
> .bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct display_timing hannstar_hsd100pxn1_timing = {
> @@ -1525,6 +1533,7 @@ static const struct panel_desc hannstar_hsd100pxn1 = {
> .height = 152,
> },
> .bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct drm_display_mode hitachi_tx23d38vm0caa_mode = {
> @@ -1631,6 +1640,7 @@ static const struct panel_desc innolux_g070y2_l01 = {
> .unprepare = 800,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct display_timing innolux_g101ice_l01_timing = {
> @@ -1659,6 +1669,7 @@ static const struct panel_desc innolux_g101ice_l01 = {
> .disable = 200,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct display_timing innolux_g121i1_l01_timing = {
> @@ -1686,6 +1697,7 @@ static const struct panel_desc innolux_g121i1_l01 = {
> .disable = 20,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct drm_display_mode innolux_g121x1_l03_mode = {
> @@ -1869,6 +1881,7 @@ static const struct panel_desc koe_tx31d200vm0baa = {
> .height = 109,
> },
> .bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct display_timing kyo_tcg121xglp_timing = {
> @@ -1893,6 +1906,7 @@ static const struct panel_desc kyo_tcg121xglp = {
> .height = 184,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct drm_display_mode lemaker_bl035_rgb_002_mode = {
> @@ -1941,6 +1955,7 @@ static const struct panel_desc lg_lb070wv8 = {
> .height = 91,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct drm_display_mode lg_lp079qx1_sp0v_mode = {
> @@ -2063,6 +2078,7 @@ static const struct panel_desc mitsubishi_aa070mc01 = {
> .disable = 400,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> };
>
> @@ -2091,6 +2107,7 @@ static const struct panel_desc nec_nl12880bc20_05 = {
> .disable = 50,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct drm_display_mode nec_nl4827hc19_05b_mode = {
> @@ -2193,6 +2210,7 @@ static const struct panel_desc nlt_nl192108ac18_02d = {
> .unprepare = 500,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct drm_display_mode nvd_9128_mode = {
> @@ -2216,6 +2234,7 @@ static const struct panel_desc nvd_9128 = {
> .height = 88,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct display_timing okaya_rs800480t_7x0gp_timing = {
> @@ -2628,6 +2647,7 @@ static const struct panel_desc sharp_lq101k1ly04 = {
> .height = 136,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct display_timing sharp_lq123p1jx31_timing = {
> @@ -2807,6 +2827,7 @@ static const struct panel_desc tianma_tm070jdhg30 = {
> .height = 95,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct display_timing tianma_tm070rvhg71_timing = {
> @@ -2831,6 +2852,7 @@ static const struct panel_desc tianma_tm070rvhg71 = {
> .height = 86,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> static const struct drm_display_mode ti_nspire_cx_lcd_mode[] = {
> @@ -2983,6 +3005,7 @@ static const struct panel_desc urt_umsh_8596md_lvds = {
> .height = 91,
> },
> .bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG,
> + .type = DRM_MODE_CONNECTOR_LVDS,
> };
Not too big a fan that we rely on the default CONNONTOR_Unknown for the
remaining panels.
I think the right connector type would be CONNECTOR_DSI - but I did not
check.
Sam
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 624bd15ecfab..5899a7d6559c 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -139,6 +139,13 @@ struct drm_panel {
> */
> const struct drm_panel_funcs *funcs;
>
> + /**
> + * @type:
> + *
> + * Type of the panel as a DRM_MODE_CONNECTOR_* value.
Maybe here add that the purpose it to init the drm_connector with the
correct connector type?
> + */
> + int type;
In the bikeshedding department - consider connector_type. To make it
more obvious what the type is about.
That it matches the field name in drm_mode_get_connector is likely
a little thing - but my OCD like the same naming all over.
Sam
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
