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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
