Re: [PATCH 1/2] drm/panel: Add and fill drm_panel type field

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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