Re: [PATCH] of: property: fw_devlink: Fix support for nvmem-cells

[Saravana Kannan <saravanak@google.com>]

On Thu, Oct 27, 2022 at 12:20 PM Michael Pratt <mcpratt@pm.me> wrote:
>
> Device linking to an OF node specified by the "nvmem-cells" property
> as a supplier device causes probing to hangup on the consumer device.
> A previous discussion is linked below.
>
> NVMEM framework does not create standalone devices (i.e. no probing,
> not in the hardware layer). Specifically, nvmem devices
> are dependent on other subsystem devices and are created when
> another driver (mtd, thunderbolt, etc.) calls nvmem_register() or
> devm_nvmem_register() during struct device creation in that driver.
>
> The "compatible" property nvmem-cells serves as a trigger
> for parsers in other drivers, not for the nvmem driver itself.
> An example for MTD devices is commit 658c4448bbbf
> ("mtd: core: add nvmem-cells compatible to parse mtd as nvmem cells").
>
> This commit introduces function of_get_next_compat_node_parent()
> in order to select the next parent node with a "compatible" property,
> and the condition sup_not_dev in order to trigger the function,
> so that an ancestor node is selected as the supplier device node instead.
>
> Link: https://lore.kernel.org/linux-devicetree/696cb2da-20b9-b3dd-46d9-de4bf91a1506@gmail.com/
> Fixes: 53e6a671f70a ("of: property: Add device link support for multiple DT bindings")
> Signed-off-by: Michael Pratt <mcpratt@pm.me>

Hi Michael,

Thanks a lot for submitting a patch to fix issues in fw_devlink. I
think my other patch series[1] should fix this in a generic way for
all such cases where the phandle doesn't actually point to the
supplier struct device. The series itself has some bugs, but there are
"try this if it fixes it" code snippets in the thread that I need to
roll into a v2.

Give it a shot if you can. I'll try to get back to the series soon.

[1] - https://lore.kernel.org/lkml/20220810060040.321697-1-saravanak@google.com/
-Saravana

> ---
>  drivers/of/property.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 967f79b59016..4b7ee036f3e8 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1104,6 +1104,18 @@ static struct device_node *of_get_compat_node_parent(struct device_node *np)
>         return node;
>  }
>
> +static struct device_node *of_get_next_compat_node_parent(struct device_node *np)
> +{
> +       struct device_node *compat, *node;
> +
> +       compat = of_get_compat_node(np);
> +       node = of_get_compat_node_parent(compat);
> +       of_node_put(compat);
> +       of_node_put(np);
> +
> +       return node;
> +}
> +
>  /**
>   * of_link_to_phandle - Add fwnode link to supplier from supplier phandle
>   * @con_np: consumer device tree node
> @@ -1281,6 +1293,9 @@ static struct device_node *parse_##fname(struct device_node *np,       \
>   * @node_not_dev: The consumer node containing the property is never converted
>   *               to a struct device. Instead, parse ancestor nodes for the
>   *               compatible property to find a node corresponding to a device.
> + * @sup_not_dev: The supplier node and nearest parent node with a "compatible"
> + *              property are not a struct device. Instead, parse ancestor nodes for the
> + *              next "compatible" property to find a node corresponding to the device.
>   *
>   * Returns:
>   * parse_prop() return values are
> @@ -1293,6 +1308,7 @@ struct supplier_bindings {
>                                           const char *prop_name, int index);
>         bool optional;
>         bool node_not_dev;
> +       bool sup_not_dev;
>  };
>
>  DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
> @@ -1393,7 +1409,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_power_domains, },
>         { .parse_prop = parse_hwlocks, },
>         { .parse_prop = parse_extcon, },
> -       { .parse_prop = parse_nvmem_cells, },
> +       { .parse_prop = parse_nvmem_cells, .sup_not_dev = true, },
>         { .parse_prop = parse_phys, },
>         { .parse_prop = parse_wakeup_parent, },
>         { .parse_prop = parse_pinctrl0, },
> @@ -1457,6 +1473,10 @@ static int of_link_property(struct device_node *con_np, const char *prop_name)
>                         con_dev_np = s->node_not_dev
>                                         ? of_get_compat_node_parent(con_np)
>                                         : of_node_get(con_np);
> +
> +                       if (s->sup_not_dev)
> +                               phandle = of_get_next_compat_node_parent(phandle);
> +
>                         matched = true;
>                         i++;
>                         of_link_to_phandle(con_dev_np, phandle);
> --
> 2.30.2
>
>