Re: [PATCH V13 4/5] of: overlay: Extend of_overlay_fdt_apply() to specify the target node

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

Thanks for reviewing the patch. I add my comment in-line.

On 8/24/23 01:31, Geert Uytterhoeven wrote:
    Hi Lizhi,

On Tue, 15 Aug 2023, Lizhi Hou wrote:
Currently, in an overlay fdt fragment, it needs to specify the exact
location in base DT. In another word, when the fdt fragment is generated,
the base DT location for the fragment is already known.

There is new use case that the base DT location is unknown when fdt
fragment is generated. For example, the add-on device provide a fdt
overlay with its firmware to describe its downstream devices. Because it
is add-on device which can be plugged to different systems, its firmware
will not be able to know the overlay location in base DT. Instead, the
device driver will load the overlay fdt and apply it to base DT at runtime.
In this case, of_overlay_fdt_apply() needs to be extended to specify
the target node for device driver to apply overlay fdt.
   int overlay_fdt_apply(..., struct device_node *base);

Signed-off-by: Lizhi Hou <lizhi.hou@xxxxxxx>

Thanks for your patch, which is now commit 47284862bfc7fd56 ("of:
overlay: Extend of_overlay_fdt_apply() in dt-rh/for-next.

--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -715,6 +730,7 @@ static struct device_node *find_target(struct device_node *info_node)
/**
 * init_overlay_changeset() - initialize overlay changeset from overlay tree
 * @ovcs:        Overlay changeset to build
+ * @target_base:    Point to the target node to apply overlay
 *
 * Initialize @ovcs.  Populate @ovcs->fragments with node information from
 * the top level of @overlay_root.  The relevant top level nodes are the

As an overlay can contain one or more fragments, this means the
base (when specified) will be applied to all fragments, and will thus
override the target-path properties in all fragments.

However, for the use case of an overlay that you can plug into
a random location (and of which there can be multiple instances),
there can really be only a single fragment.  Even nodes that typically
live at the root level (e.g. gpio-leds or gpio-keys) must be inserted
below the specified location, to avoid conflicts.

Hence:
  1. Should init_overlay_changeset() return -EINVAL if target_base is
     specified, and there is more than one fragment?

Maybe allowing more than one fragment make the interface more generic?  For example, it could support the use case that multiple fragments share the same base node.

Currently, the fragment overlay path is "base node path" + "fragment target path". Thus, for the structure:

/a/b/c/fragment0

/a/b/d/fagment1

It can be two fragments in one fdt by using

  base node path = /a/b

  fragment0 target path = /c

  fragment1 target path = /d

I am not sure if there will be this kind of use case or not. And I think it would not be hurt to allow that.


  2. Should there be a convention about the target-path property's
     contents in the original overlay?
     drivers/of/unittest-data/overlay_pci_node.dtso in "[PATCH V13 5/5]
     of: unittest: Add pci_dt_testdrv pci driver" uses

         target-path="";

     which cannot be represented when using sugar syntax.
     "/" should work fine, though.

Because the fragment overlay path is "base node path" + "fragment target path", I may add code to check if "fragment target patch is '/' and ignore it. I think that would support sugar syntax with only '/' specified.


Thanks,

Lizhi


Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux