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

[Herve Codina <herve.codina@bootlin.com>]

Hi Luca, Kory,

On Wed, 17 Dec 2025 15:23:26 +0100
"Luca Ceresoli" <luca.ceresoli@bootlin.com> (by way of Kory Maincent <kory.maincent@bootlin.com>) wrote:

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

Yes sure.

Here is my global review.

Depending on the discussion on things I have spotted, I will go deeper in
patch details.

...

> > +
> > +static void __init
> > +tilcdc_panel_update_prop(struct device_node *node, char *name,
> > +			 void *val, int length)
> > +{
> > +	struct property *prop;
> > +
> > +	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> > +	if (!prop)
> > +		return;
> > +
> > +	prop->name = kstrdup(name, GFP_KERNEL);
> > +	prop->length = length;
> > +	prop->value = kmemdup(val, length, GFP_KERNEL);
> > +	of_update_property(node, prop);

I would use OF changesets to perform the modification.

OF changesets are kind of atomic. You first prepare all modifications in a
changeset and then you apply the changeset.
If something goes wrong, the changeset is removed.

Also, if something goes wrong during the changeset preparation, you can abort
without any modification on the live device-tree.

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

Here, for instance, you have already modified your live tree and you abort the
operation. Your live tree is somewhat corrupted.

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

As soon as the overlay is applied, the driver handling the panel-dti node
can be probed.

Modifying some properties after applying the overlay could be not seen by the
driver.

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

Agreed, the overlay should not be removed here.

> 
> > +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.
> 
> > +	return ret;
> > +}
> > +
> > +subsys_initcall(tilcdc_panel_legacy_init);

IMHO, the call to tilcdc_panel_legacy_init() will be too late.

subsys initcalls are called after arch initcalls.

During arch initcalls, of_platform_populate_init() is called
https://elixir.bootlin.com/linux/v6.19-rc3/source/drivers/of/platform.c#L599

The root node is populated and handled by the platform bus.

Later at subsys initcall, the tilcdc_panel_legacy_init() function is called.
This function starts by applying the overlay and so a new node (panel-dpi)
is added at the root node.

This trigger an OF_RECONFIG_CHANGE_ADD event handled by the platform bus.
https://elixir.bootlin.com/linux/v6.19-rc3/source/drivers/of/platform.c#L731

If the "panel-dpi" compatible driver is available, its probe() is called but
the panel-dpi DT node is not fully correct. Indeed, tilcdc_panel_copy_props()
has not be called yet.

Also, the legacy compatible string is removed after the of_platform_populate_init()
call. The legacy driver could have been already probed.

Of course, please correct me if I have misunderstood something.

Best regards,
Hervé