Hello Luca,

Thank you for your full review on this series!

On Wed, 17 Dec 2025 15:23:26 +0100
"Luca Ceresoli" [off-list ref] wrote:

quoted

+config DRM_TILCDC_PANEL_LEGACY
+	bool "Support device tree blobs using TI LCDC Panel binding"
+	default n  

'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.

Just setting default to 'y' works for both cases TILCDC as a module or builtin. 

quoted

+	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.

Ack, I will update it.

...

quoted

+static int __init tilcdc_panel_copy_props(struct device_node *old_panel,
+					  struct device_node *new_panel)
+{
+	struct device_node *child, *old_timing, *new_timing, *panel_info;
+	u32 invert_pxl_clk = 0, sync_edge = 0;
+	struct property *prop;
+
+	/* Copy all panel properties to the new panel node */
+	for_each_property_of_node(old_panel, prop) {
+		if (!strncmp(prop->name, "compatible",
sizeof("compatible")))
+			continue;
+
+		tilcdc_panel_update_prop(new_panel, prop->name,
+					 prop->value, prop->length);
+	}
+
+	child = of_get_child_by_name(old_panel, "display-timings");  

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");

I am not used to this __free() macro and even some subsystem (net) are avoiding
it but ok I will move to it. I don't know what are the pros and cons.

...

quoted

+	/* 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.

Ok.

...

quoted

+	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?

Indeed this should be only in the error path. That's weird that it was still
working during my tests. 

quoted

+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.

Ack.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com