[RFC,v3,5/5] drm/panel: simple: add panel-dpi support
diff mbox series

Message ID 20200216181513.28109-6-sam@ravnborg.org
State New
Headers show
Series
  • dt-bindings: convert timing + panel-dpi to DT schema
Related show

Commit Message

Sam Ravnborg Feb. 16, 2020, 6:15 p.m. UTC
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
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.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 74 +++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 2 deletions(-)

Comments

Maxime Ripard Feb. 17, 2020, 12:13 p.m. UTC | #1
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
> 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.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>

I submitted a similar patch a year ago:
https://lists.freedesktop.org/archives/dri-devel/2019-March/209682.html

So I'm all for it :)

Regardless of the outcome of this discussion, for all the patches:
Acked-by: Maxime Ripard <mripard@kernel.org>

Maxime
Oleksandr Suvorov Feb. 17, 2020, 1:29 p.m. UTC | #2
On Sun, Feb 16, 2020 at 8:15 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> RFC only - not tested yet!

I've just tested this patch on Apalis iMX6Q and Colibri iMX7D using
panel settings from the following patch:
https://lore.kernel.org/linux-arm-kernel/20200115123401.2264293-4-oleksandr.suvorov@toradex.com/

It works for me, thanks!

Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>

>
> 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
> 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.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> ---
>  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;
> +
> +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;
> +
> +       /* 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 */
>         }
> --
> 2.20.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



--
Best regards
Oleksandr Suvorov

Toradex AG
Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500
4800 (main line)
Laurent Pinchart March 3, 2020, 7 p.m. UTC | #3
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@ravnborg.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> ---
>  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 */
>  	}

Patch
diff mbox series

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;
+
+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;
+
+	/* 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 */
 	}