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) <[email protected]> It's fair to add before your SoB line: Suggested-by: Tomi Valkeinen <[email protected]> Link: https://lore.kernel.org/all/[email protected]/ > --- > > Using the approach of applying an overlay and then modifying the live > device tree is the solution I found that requires no modification of the > OF core. Dealing entirely with overlay changesets would bring additional > requirements such as phandle resolution management, which is internal to > the OF framework. I intend to avoid OF core change to support this legacy > binding. > > Change in v2: > - New patch. > --- > drivers/gpu/drm/tilcdc/Kconfig | 14 +++ > drivers/gpu/drm/tilcdc/Makefile | 2 + > drivers/gpu/drm/tilcdc/tilcdc_panel_legacy.c | 159 > ++++++++++++++++++++++++ > drivers/gpu/drm/tilcdc/tilcdc_panel_legacy.dtso | 44 +++++++ > 4 files changed, 219 insertions(+) > > diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig > index 24f9a245ba593..4dcae0a06b819 100644 > --- a/drivers/gpu/drm/tilcdc/Kconfig > +++ b/drivers/gpu/drm/tilcdc/Kconfig > @@ -14,3 +14,17 @@ config DRM_TILCDC > controller, for example AM33xx in beagle-bone, DA8xx, or > OMAP-L1xx. This driver replaces the FB_DA8XX fbdev driver. > > +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. > + 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. > diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile > index f5190477de721..6d6a08b5adf40 100644 > --- a/drivers/gpu/drm/tilcdc/Makefile > +++ b/drivers/gpu/drm/tilcdc/Makefile > @@ -11,3 +11,5 @@ tilcdc-y := \ > tilcdc_drv.o > > obj-$(CONFIG_DRM_TILCDC) += tilcdc.o > +obj-$(CONFIG_DRM_TILCDC_PANEL_LEGACY) += tilcdc_panel_legacy.o \ > + tilcdc_panel_legacy.dtbo.o > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel_legacy.c > b/drivers/gpu/drm/tilcdc/tilcdc_panel_legacy.c > new file mode 100644 > index 0000000000000..a9651dd9f9935 > --- /dev/null > +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel_legacy.c > @@ -0,0 +1,159 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2025 Kory Maincent <[email protected]> > + * > + * To support the legacy "ti,tilcdc,panel" binding, the devicetree has to > + * be transformed to the new panel-dpi binding with the endpoint associated. > + */ > + > +#include <linux/kernel.h> > +#include <linux/of.h> > +#include <linux/of_fdt.h> > +#include <linux/slab.h> > + > +/* Embedded dtbo symbols created by cmd_wrap_S_dtb in scripts/Makefile.lib */ > +extern char __dtbo_tilcdc_panel_legacy_begin[]; > +extern char __dtbo_tilcdc_panel_legacy_end[]; > + > +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); > +} > + > +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; > + > + /* 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. > + return ret; > +} > + > +subsys_initcall(tilcdc_panel_legacy_init); > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel_legacy.dtso > b/drivers/gpu/drm/tilcdc/tilcdc_panel_legacy.dtso > new file mode 100644 > index 0000000000000..77f3ec9391d55 > --- /dev/null > +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel_legacy.dtso > @@ -0,0 +1,44 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * DTS overlay for converting ti,tilcdc,panel binding to new binding. > + * > + * Copyright (C) Kory Maincent <[email protected]> > + */ > + > +/dts-v1/; > +/plugin/; > + > +&{/} { > + panel-dpi { > + compatible = "panel-dpi"; > + port { > + panel_in: endpoint@0 { > + remote-endpoint = <&lcd_0>; > + }; > + }; > + panel-timing { > + clock-frequency = <0>; > + hactive = <0>; > + vactive = <0>; > + hfront-porch = <0>; > + hback-porch = <0>; > + hsync-len = <0>; > + vfront-porch = <0>; > + vback-porch = <0>; > + vsync-len = <0>; > + hsync-active = <0>; > + vsync-active = <0>; > + de-active = <0>; > + pixelclk-active = <0>; > + syncclk-active = <0>; > + }; > + }; > +}; > + > +&lcdc { > + port { > + lcd_0: endpoint@0 { > + remote-endpoint = <&panel_in>; > + }; > + }; > +}; -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
