Re: [PATCH v6 3/4] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Frank,

On Friday, 23 February 2018 00:10:17 EET Frank Rowand wrote:
> Hi Laurent, Rob,
> 
> Thanks for the prompt spin to address my concerns.  There are some small
> technical issues.
> 
> I did not read the v3 patch until today.  v3 through v6 are still using the
> old overlay apply method which uses an expanded device tree as input.
> 
> Rob, I don't see my overlay patches in you for-next branch, and I have
> not seen an "Applied" message from you.  What is the status of the
> overlay patches?
> 
> Comments in the patch below.
> 
> On 02/22/18 05:13, Laurent Pinchart wrote:
> > The internal LVDS encoders now have their own DT bindings. Before
> > switching the driver infrastructure to those new bindings, implement
> > backward-compatibility through live DT patching.
> > 
> > Patching is disabled and will be enabled along with support for the new
> > DT bindings in the DU driver.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > Changes since v5:
> > 
> > - Use a private copy of rcar_du_of_changeset_add_property()
> > 
> > Changes since v3:
> > 
> > - Use the OF changeset API
> > - Use of_graph_get_endpoint_by_regs()
> > - Replace hardcoded constants by sizeof()
> > 
> > Changes since v2:
> > 
> > - Update the SPDX headers to use C-style comments in header files
> > - Removed the manually created __local_fixups__ node
> > - Perform manual fixups on live DT instead of overlay
> > 
> > Changes since v1:
> > 
> > - Select OF_FLATTREE
> > - Compile LVDS DT bindings patch code when DRM_RCAR_LVDS is selected
> > - Update the SPDX headers to use GPL-2.0 instead of GPL-2.0-only
> > - Turn __dtb_rcar_du_of_lvds_(begin|end) from u8 to char
> > - Pass void begin and end pointers to rcar_du_of_get_overlay()
> > - Use of_get_parent() instead of accessing the parent pointer directly
> > - Find the LVDS endpoints nodes based on the LVDS node instead of the
> > 
> >   root of the overlay
> > 
> > - Update to the <soc>-lvds compatible string format
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/Kconfig                    |   2 +
> >  drivers/gpu/drm/rcar-du/Makefile                   |   7 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_of.c               | 342 ++++++++++++++++
> >  drivers/gpu/drm/rcar-du/rcar_du_of.h               |  20 ++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts    |  79 +++++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts    |  53 ++++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts    |  53 ++++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts    |  53 ++++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts    |  53 ++++
> >  9 files changed, 661 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.c
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.h
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts

[snip]

> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_of.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_of.c new file mode 100644
> > index 000000000000..ac442ddfed16
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_of.c

[snip]

> > +static int __init rcar_du_of_apply_overlay(const struct
> > rcar_du_of_overlay *dtbs,
> > +					   const char *compatible)
> > +{
> > +	const struct rcar_du_of_overlay *dtb = NULL;
> > +	struct device_node *node = NULL;
> > +	unsigned int i;
> > +	int ovcs_id;
> > +	void *data;
> > +	void *mem;
> > +	int ret;
> > +
> > +	for (i = 0; dtbs[i].compatible; ++i) {
> > +		if (!strcmp(dtbs[i].compatible, compatible)) {
> > +			dtb = &dtbs[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!dtb)
> > +		return -ENODEV;
> 
> __If__ my overlay patches are accepted, this block:
> 
> > +
> > +	data = kmemdup(dtb->begin, dtb->end - dtb->begin, GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	mem = of_fdt_unflatten_tree(data, NULL, &node);
> > +	if (!mem) {
> > +		ret = -ENOMEM;
> > +		goto done;
> > +	}
> > +
> > +	ovcs_id = 0;
> > +	ret = of_overlay_apply(node, &ovcs_id);
> > +
> > +done:
> > +	of_node_put(node);
> > +	kfree(data);
> > +	kfree(mem);
> 
> becomes:
> 
> 	ret = of_overlay_fdt_apply(dtb->begin, &ovcs_id);

I tried to rework this patch in a way that would make switching to FDT 
overlays easy, and I'm glad to hear I haven't done a too bad job :-)

Are your patches scheduled for merge in v4.17 ? If so, is it possible to get 
apply them in a stable branch on top of v4.16-rc1 that can be merged as a 
dependency for this series ? There are changes to the Renesas DT queued for 
merge in v4.17 that would make delaying this patch series to v4.18 quite 
painful.

> If my overlay patches do not exist, there are other small errors
> in the code block above.  I'll ignore them for the moment.
> 
> A quick scan of the rest of the code looks OK.  I'll read through it
> more carefully, but wanted to get this reply out without further
> delay.

Thank you.

> > +
> > +	return ret;
> > +}

[snip]

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel