Hi Sam,
Thank you for the patch.
On Sun, Feb 16, 2020 at 07:15:13PM +0100, Sam Ravnborg wrote:
> RFC only - not tested yet!
>
> The panel-dpi compatible is a fallback that
> allows the DT to specify the timing.
>
> When matching panel-dpi expect the device tree to include the
> timing information for the display-panel.
>
> Background for this change:
> There are a lot of panels and new models hits the market very often.
> It is a lost cause trying to chase them all and users of new panels
> will often find them in situations that the panel they ues are not
s/ues/use/
> supported by the kernel.
> On top of this a lot of panels are customized based on customer
> specifications.
>
> Including the panel timing in the device tree allows for a simple
> way to describe the actual HW and use this description in a generic
> way in the kernel.
> This allows uses of proprietary panels, or panels which are not
> included in the kernel, to specify the timing in the device tree
> together with all the other HW descriptions.
> And thus, using the device tree it is then easy to add support
> for an otherwise unknown panel.
>
> The current support expect panels that do not require any
> delays for prepare/enable/disable/unprepare.
I've proposed something similar a few times in the past, so I can't
disagree :-)
> Signed-off-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> Cc: Oleksandr Suvorov <oleksandr.suvorov@xxxxxxxxxxx>
> ---
> drivers/gpu/drm/panel/panel-simple.c | 74 +++++++++++++++++++++++++++-
> 1 file changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 82363d05bad4..188526637398 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -351,6 +351,65 @@ static const struct drm_panel_funcs panel_simple_funcs = {
> .get_timings = panel_simple_get_timings,
> };
>
> +static struct panel_desc panel_dpi;
Do you need this, can't you set data to 0 in the panel-dpi entry ?
> +
> +static int panel_dpi_probe(struct device *dev,
> + struct panel_simple *panel)
> +{
> + struct display_timing *timing;
> + const struct device_node *np;
> + struct panel_desc *desc;
> + unsigned int bus_flags;
> + struct videomode vm;
> + const char *mapping;
> + int ret;
> +
> + np = dev->of_node;
> + desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
> + if (!desc)
> + return -ENOMEM;
> +
> + timing = devm_kzalloc(dev, sizeof(*timing), GFP_KERNEL);
> + if (!timing)
> + return -ENOMEM;
> +
> + ret = of_get_display_timing(np, "panel-timing", timing);
> + if (ret < 0) {
> + dev_err(dev, "%pOF: no panel-timing node found for \"panel-dpi\" binding\n",
> + np);
> + return ret;
> + }
> +
> + desc->timings = timing;
> + desc->num_timings = 1;
> +
> + of_property_read_u32(np, "width-mm", &desc->size.width);
> + of_property_read_u32(np, "height-mm", &desc->size.height);
> +
> + of_property_read_string(np, "data-mapping", &mapping);
> + if (!strcmp(mapping, "rgb24"))
> + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> + else if (!strcmp(mapping, "rgb565"))
> + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> + else if (!strcmp(mapping, "bgr666"))
> + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> + else if (!strcmp(mapping, "lvds666"))
> + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
I think you've dropped lvds666 in the DT bindings, so this should be
dropped too. Furthermore, I think the data-mapping property should be
reconsidered (see my review of the bindings), so this would need to
change too.
> +
> + /* Extract bus_flags from display_timing */
> + bus_flags = 0;
> + vm.flags = timing->flags;
> + drm_bus_flags_from_videomode(&vm, &bus_flags);
> + desc->bus_flags = bus_flags;
> +
> + /* We do not know the connector for the DT node, so guess it */
> + desc->connector_type = DRM_MODE_CONNECTOR_DPI;
> +
> + panel->desc = desc;
> +
> + return 0;
> +}
> +
> #define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \
> (to_check->field.typ >= bounds->field.min && \
> to_check->field.typ <= bounds->field.max)
> @@ -437,8 +496,15 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
> return -EPROBE_DEFER;
> }
>
> - if (!of_get_display_timing(dev->of_node, "panel-timing", &dt))
> - panel_simple_parse_panel_timing_node(dev, panel, &dt);
> + if (desc == &panel_dpi) {
> + /* Handle the generic panel-dpi binding */
> + err = panel_dpi_probe(dev, panel);
> + if (err)
> + goto free_ddc;
> + } else {
> + if (!of_get_display_timing(dev->of_node, "panel-timing", &dt))
> + panel_simple_parse_panel_timing_node(dev, panel, &dt);
> + }
>
> drm_panel_init(&panel->base, dev, &panel_simple_funcs,
> desc->connector_type);
> @@ -3688,6 +3754,10 @@ static const struct of_device_id platform_of_match[] = {
> }, {
> .compatible = "winstar,wf35ltiacd",
> .data = &winstar_wf35ltiacd,
> + }, {
> + /* Must be the last entry */
> + .compatible = "panel-dpi",
> + .data = &panel_dpi,
> }, {
> /* sentinel */
> }
--
Regards,
Laurent Pinchart
