Hi Sam, On Fri, Aug 23, 2019 at 06:49:49AM +0200, Sam Ravnborg wrote: > Hi Laurent. > > Thanks for looking into this, but you will not be happy in a minute...
Could be worse :-) > 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. I'll fix that. > 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. I think it would be CONNECTOR_DPI. I could set the explicitly, I wanted to keep the diff small(er), but I can update it. > > 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. OK, I'll update that. -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel