Message-ID: <20260105152939.49642d0a@kmaincent-XPS-13-7390>
Date: Mon, 5 Jan 2026 15:29:39 +0100
From: Kory Maincent <kory.maincent@...tlin.com>
To: "Luca Ceresoli" <luca.ceresoli@...tlin.com>
Cc: "Jyri Sarha" <jyri.sarha@....fi>, "Tomi Valkeinen"
 <tomi.valkeinen@...asonboard.com>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "David Airlie"
 <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Rob Herring"
 <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor
 Dooley" <conor+dt@...nel.org>, "Russell King" <linux@...linux.org.uk>,
 "Bartosz Golaszewski" <brgl@...ev.pl>, "Tony Lindgren" <tony@...mide.com>,
 "Andrzej Hajda" <andrzej.hajda@...el.com>, "Neil Armstrong"
 <neil.armstrong@...aro.org>, "Robert Foss" <rfoss@...nel.org>, "Laurent
 Pinchart" <Laurent.pinchart@...asonboard.com>, "Jonas Karlman"
 <jonas@...boo.se>, "Jernej Skrabec" <jernej.skrabec@...il.com>, "Markus
 Schneider-Pargmann" <msp@...libre.com>, "Bajjuri Praneeth"
 <praneeth@...com>, "Louis Chauvet" <louis.chauvet@...tlin.com>, "Thomas
 Petazzoni" <thomas.petazzoni@...tlin.com>, "Miguel Gazquez"
 <miguel.gazquez@...tlin.com>, <dri-devel@...ts.freedesktop.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-arm-kernel@...ts.infradead.org>, <linux-omap@...r.kernel.org>
Subject: Re: [PATCH v2 05/20] drm/tilcdc: Convert legacy panel binding via
 DT overlay at boot time

Hello Luca,

Thank you for your full review on this series!

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

Powered by blists - more mailing lists