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

Kory Maincent
2026-01-05T14:29:51Z

Hello Luca,

Thank you for your full review on this series!

On Wed, 17 Dec 2025 15:23:26 +0100
"Luca Ceresoli" <luca.ceresoli@bootlin.com> wrote:
> > +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.

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

...

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

...

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

...

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

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