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 21:43:17 EET Frank Rowand wrote:
> On 02/23/18 01:00, Laurent Pinchart wrote:
> > On Friday, 23 February 2018 04:38:06 EET Frank Rowand wrote:
> >> On 02/22/18 14:10, 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
> >>>> 
> >>>> diff --git a/drivers/gpu/drm/rcar-du/Kconfig
> >>>> b/drivers/gpu/drm/rcar-du/Kconfig index 5d0b4b7119af..3f83352a7313
> >>>> 100644
> >>>> --- a/drivers/gpu/drm/rcar-du/Kconfig
> >>>> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> >> 
> >> < snip >
> >> 
> >>> becomes:
> >>> 	ret = of_overlay_fdt_apply(dtb->begin, &ovcs_id);
> >>> 
> >>> If my overlay patches do not exist, there are other small errors
> >>> in the code block above.  I'll ignore them for the moment.

If you have time to not ignore them I'd appreciate your comments :-) I'd like 
to get this patch series merged in v4.17, and for that I want to be prepared 
to submit it on top of your overlay patches if they make it to mainline, or 
without them if they don't.

> >>> 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.
> >> 
> >> < snip >
> >> 
> >> I was hoping to be able to convert the .dts files to use sugar syntax
> >> instead of hand coding the fragment nodes, but for this specific set
> >> of files I failed, since the labels that would have been required do
> >> not already exist in the base .dts files that that overlays would be
> >> applied against.
> > 
> > And even if they existed in the base .dts in the kernel sources, there's
> > no guarantee that the .dtb on the systems we want to patch would contain
> > symbol, so that wouldn't have been an option anyway, would it ?
> 
> Correct.  And even more troublesome is that some of the fragments are
> targeted at node "/", which dtc does not even allow a label on (at the
> moment).
> 
> >> Oh well.  It was an interesting exercise anyway, trying to be crafty.

-- 
Regards,

Laurent Pinchart

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