Re: [PATCH v2 05/20] drm/tilcdc: Convert legacy panel binding via DT overlay at boot time

Hi,

Cc: Hervé, can you review the DT overlay aspects?

On Thu Dec 11, 2025 at 5:38 PM CET, Kory Maincent (TI.com) wrote:

To maintain backward compatibility while removing the deprecated
tilcdc_panel driver, add a tilcdc_panel_legacy subdriver that converts
the legacy "ti,tilcdc,panel" devicetree binding to the standard
panel-dpi binding at early boot.

The conversion uses an embedded device tree overlay that is applied and
modified during subsys_initcall. The process:

I was a bit skeptical about the overlay idea initially. But after having
seen the code and thought about any alternative solution, I think you
probably took the simplest approach.

- Apply embedded overlay to create a panel-dpi node with placeholder
  timing properties and port/endpoint connections to the LCDC
- Copy all properties from the legacy panel node to the new panel-dpi
- Copy display-timings from the legacy panel
- Convert legacy panel-info properties (invert-pxl-clk, sync-edge) to
  standard display timing properties (pixelclk-active, syncclk-active)
- Disable the legacy panel by removing its compatible property to
  prevent the deprecated driver from binding

The result is a standard panel-dpi node with proper endpoints and
timing properties, allowing the DRM panel infrastructure to work with
legacy devicetrees without modification.

Other legacy panel-info properties are not migrated as they consistently
use default values across all mainline devicetrees and can be hardcoded
in the tilcdc driver.

This feature is optional via CONFIG_DRM_TILCDC_PANEL_LEGACY and should
only be enabled for systems with legacy devicetrees containing
"ti,tilcdc,panel" nodes.

Signed-off-by: Kory Maincent (TI.com) <kory.maincent@bootlin.com>

It's fair to add before your SoB line:
 Suggested-by: Tomi Valkeinen [off-list ref]
 Link: https://lore.kernel.org/all/1d9a9269-bfda-4d43-938b-2df6b82b9369@ideasonboard.com/ (local)

'default' defaults to 'n', you can drop this line.

However I think it should instead be enabled by default. You propose to
entirely remove the tilcdc panel driver in the next patch, so any users
without DRM_TILCDC_PANEL_LEGACY in their defconfig would be broken. For
this reason, I propose to enable DRM_TILCDC_PANEL_LEGACY in all cases where
the tilcdc_panel was compiled in, which I guess means:

    default DRM_TILCDC

Except I think if DRM_TILCDC=m, DRM_TILCDC_PANEL_LEGACY should be =y. I
don't know how to do that in Kconfig. But I'm not really sure about this
last topic.

+	depends on DRM_TILCDC
+        depends on OF
+        depends on BACKLIGHT_CLASS_DEVICE
+        depends on PM
+	select OF_OVERLAY
+	select DRM_PANEL_SIMPLE
+	help
+	  Choose this option if you need a kernel that is compatible
+	  with device tree blobs using the obsolete "ti,tilcdc,panel"
+	  binding. If you find "ti,tilcdc,panel"-string from your DTB,
+	  you probably need this. Otherwise you do not.

Maybe mention here what it does?

For example, rewording your commit message:

  Modifies the live device tree at early boot to convert the legacy
  "ti,tilcdc,panel" devicetree node to the standard panel-dpi node.  This
  allows to maintain backward compatibility for boards which were using the
  deprecated tilcdc_panel driver.

There's some housekeeping code in this function to ensure you put all the
device_node refs. It would be simpler and less error prone to use a cleanup
action. E.g.:

-	struct device_node *child, *old_timing, *new_timing, *panel_info;

-	child = of_get_child_by_name(old_panel, "display-timings");
+	struct device_node *child __free(device_node) = of_get_child_by_name(old_panel, "display-timings");

+	if (!child)
+		return -EINVAL;
+
+	/* The default display timing is the one specified as native-mode.
+	 * If no native-mode is specified then the first node is assumed
+	 * to be the native mode.
+	 */
+	old_timing = of_parse_phandle(child, "native-mode", 0);
+	if (!old_timing) {
+		old_timing = of_get_next_child(child, NULL);
+		if (!old_timing) {
+			of_node_put(child);
+			return -EINVAL;
+		}
+	}
+	of_node_put(child);
+
+	new_timing = of_get_child_by_name(new_panel, "panel-timing");
+	if (!new_timing)
+		return -EINVAL;
+
+	/* Copy all panel timing property to the new panel node */
+	for_each_property_of_node(old_timing, prop)
+		tilcdc_panel_update_prop(new_timing, prop->name,
+					 prop->value, prop->length);
+
+	panel_info = of_get_child_by_name(old_panel, "panel-info");
+	if (!panel_info)
+		return -EINVAL;

tilcdc_panel_update_prop() has previously done various allocations which
will not be freed if you return here. You shoudl probably do all the
of_get_*() at the top, and if they all succeed start copying data along
with with the needed allocations.

+	/* Looked only for these two parameter as all the other are always
+	 * set to default and not related to common DRM properties.
+	 */
+	of_property_read_u32(panel_info, "invert-pxl-clk", &invert_pxl_clk);
+	of_property_read_u32(panel_info, "sync-edge", &sync_edge);
+
+	if (!invert_pxl_clk)
+		tilcdc_panel_update_prop(new_timing, "pixelclk-active",
+					 &(int){1}, sizeof(int));
+
+	if (!sync_edge)
+		tilcdc_panel_update_prop(new_timing, "syncclk-active",
+					 &(int){1}, sizeof(int));
+
+	of_node_put(panel_info);
+	of_node_put(old_timing);
+	of_node_put(new_timing);
+	return 0;
+}
+
+static const struct of_device_id tilcdc_panel_of_match[] __initconst = {
+	{ .compatible = "ti,tilcdc,panel", },
+	{},
+};
+
+static const struct of_device_id tilcdc_of_match[] __initconst = {
+	{ .compatible = "ti,am33xx-tilcdc", },
+	{ .compatible = "ti,da850-tilcdc", },
+	{},
+};
+
+static int __init tilcdc_panel_legacy_init(void)
+{
+	struct device_node *panel, *lcdc, *new_panel;
+	void *dtbo_start;
+	u32 dtbo_size;
+	int ovcs_id;
+	int ret;
+
+	lcdc = of_find_matching_node(NULL, tilcdc_of_match);
+	panel = of_find_matching_node(NULL, tilcdc_panel_of_match);
+
+	if (!of_device_is_available(panel) ||
+	    !of_device_is_available(lcdc)) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	dtbo_start = __dtbo_tilcdc_panel_legacy_begin;
+	dtbo_size = __dtbo_tilcdc_panel_legacy_end -
+		    __dtbo_tilcdc_panel_legacy_begin;
+
+	ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &ovcs_id, NULL);
+	if (ret)
+		goto out;
+
+	new_panel = of_find_node_by_name(NULL, "panel-dpi");
+	if (!new_panel) {
+		ret = -ENODEV;
+		goto overlay_remove;
+	}
+
+	ret = tilcdc_panel_copy_props(panel, new_panel);
+	if (ret)
+		goto overlay_remove;
+
+	/* Remove compatible property to avoid any driver compatible match */
+	of_remove_property(panel, of_find_property(panel, "compatible",
+						   NULL));
+overlay_remove:
+	of_overlay_remove(&ovcs_id);

Is it correct to remove the overlay here? Won't it remove what you have
just added?

+out:
+	of_node_put(new_panel);
+	of_node_put(panel);
+	of_node_put(lcdc);

Here too you can use cleanup actions, even though the current code is
slightly simpler than tilcdc_panel_copy_props as far as of_node_put() is
concerned.

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com