Hello Luca, Thank you for your full review on this series!
On Wed, 17 Dec 2025 15:23:26 +0100 "Luca Ceresoli" <[email protected]> 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 https://bootlin.com
