Re: [PATCH v1 1/1] driver core: Move fw_devlink stuff to where it belongs

[Saravana Kannan]

On Tue, Feb 20, 2024 at 8:10 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> A few APIs that belong specifically to the fw_devlink APIs
> - are exposed to others without need
> - prevents device property code to be cleaned up in the future
>
> Resolve this mess by moving fw_devlink code to where it belongs
> and hide from others.
>
> Fixes: b5d3e2fbcb10 ("device property: Add fwnode_is_ancestor_of() and fwnode_get_next_parent_dev()")
> Fixes: 372a67c0c5ef ("driver core: Add fwnode_to_dev() to look up device from fwnode")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  drivers/base/core.c      | 58 ++++++++++++++++++++++++++++++++++++++++
>  drivers/base/property.c  | 56 --------------------------------------
>  include/linux/fwnode.h   |  1 -
>  include/linux/property.h |  2 --
>  4 files changed, 58 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 9828da9b933c..35ccd8bb2c9b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1871,6 +1871,7 @@ static void fw_devlink_unblock_consumers(struct device *dev)
>         device_links_write_unlock();
>  }
>
> +#define get_dev_from_fwnode(fwnode)    get_device((fwnode)->dev)
>
>  static bool fwnode_init_without_drv(struct fwnode_handle *fwnode)
>  {
> @@ -1901,6 +1902,63 @@ static bool fwnode_ancestor_init_without_drv(struct fwnode_handle *fwnode)
>         return false;
>  }
>
> +/**
> + * fwnode_is_ancestor_of - Test if @ancestor is ancestor of @child
> + * @ancestor: Firmware which is tested for being an ancestor
> + * @child: Firmware which is tested for being the child
> + *
> + * A node is considered an ancestor of itself too.
> + *
> + * Return: true if @ancestor is an ancestor of @child. Otherwise, returns false.
> + */
> +static bool fwnode_is_ancestor_of(const struct fwnode_handle *ancestor,
> +                                 const struct fwnode_handle *child)
> +{
> +       struct fwnode_handle *parent;
> +
> +       if (IS_ERR_OR_NULL(ancestor))
> +               return false;
> +
> +       if (child == ancestor)
> +               return true;
> +
> +       fwnode_for_each_parent_node(child, parent) {
> +               if (parent == ancestor) {
> +                       fwnode_handle_put(parent);
> +                       return true;
> +               }
> +       }
> +       return false;
> +}
> +
> +/**
> + * fwnode_get_next_parent_dev - Find device of closest ancestor fwnode
> + * @fwnode: firmware node
> + *
> + * Given a firmware node (@fwnode), this function finds its closest ancestor
> + * firmware node that has a corresponding struct device and returns that struct
> + * device.
> + *
> + * The caller is responsible for calling put_device() on the returned device
> + * pointer.
> + *
> + * Return: a pointer to the device of the @fwnode's closest ancestor.
> + */
> +static struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwnode)
> +{
> +       struct fwnode_handle *parent;
> +       struct device *dev;
> +
> +       fwnode_for_each_parent_node(fwnode, parent) {
> +               dev = get_dev_from_fwnode(parent);
> +               if (dev) {
> +                       fwnode_handle_put(parent);
> +                       return dev;
> +               }
> +       }
> +       return NULL;
> +}
> +
>  /**
>   * __fw_devlink_relax_cycles - Relax and mark dependency cycles.
>   * @con: Potential consumer device.
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index a1b01ab42052..afa1bf2b3c5a 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -699,34 +699,6 @@ struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode)
>  }
>  EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
>
> -/**
> - * fwnode_get_next_parent_dev - Find device of closest ancestor fwnode
> - * @fwnode: firmware node
> - *
> - * Given a firmware node (@fwnode), this function finds its closest ancestor
> - * firmware node that has a corresponding struct device and returns that struct
> - * device.
> - *
> - * The caller is responsible for calling put_device() on the returned device
> - * pointer.
> - *
> - * Return: a pointer to the device of the @fwnode's closest ancestor.
> - */
> -struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwnode)
> -{
> -       struct fwnode_handle *parent;
> -       struct device *dev;
> -
> -       fwnode_for_each_parent_node(fwnode, parent) {
> -               dev = get_dev_from_fwnode(parent);
> -               if (dev) {
> -                       fwnode_handle_put(parent);
> -                       return dev;
> -               }
> -       }
> -       return NULL;
> -}
> -
>  /**
>   * fwnode_count_parents - Return the number of parents a node has
>   * @fwnode: The node the parents of which are to be counted
> @@ -773,34 +745,6 @@ struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
>  }
>  EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
>
> -/**
> - * fwnode_is_ancestor_of - Test if @ancestor is ancestor of @child
> - * @ancestor: Firmware which is tested for being an ancestor
> - * @child: Firmware which is tested for being the child
> - *
> - * A node is considered an ancestor of itself too.
> - *
> - * Return: true if @ancestor is an ancestor of @child. Otherwise, returns false.
> - */
> -bool fwnode_is_ancestor_of(const struct fwnode_handle *ancestor, const struct fwnode_handle *child)
> -{
> -       struct fwnode_handle *parent;
> -
> -       if (IS_ERR_OR_NULL(ancestor))
> -               return false;
> -
> -       if (child == ancestor)
> -               return true;
> -
> -       fwnode_for_each_parent_node(child, parent) {
> -               if (parent == ancestor) {
> -                       fwnode_handle_put(parent);
> -                       return true;
> -               }
> -       }
> -       return false;
> -}
> -
>  /**
>   * fwnode_get_next_child_node - Return the next child node handle for a node
>   * @fwnode: Firmware node to find the next child node for.
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 0428942093a7..4228c45d5ccc 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -193,7 +193,6 @@ struct fwnode_operations {
>                 if (fwnode_has_op(fwnode, op))                          \
>                         (fwnode)->ops->op(fwnode, ## __VA_ARGS__);      \
>         } while (false)
> -#define get_dev_from_fwnode(fwnode)    get_device((fwnode)->dev)
>
>  static inline void fwnode_init(struct fwnode_handle *fwnode,
>                                const struct fwnode_operations *ops)
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 07fbebc73243..1f0135e24d00 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -150,11 +150,9 @@ struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode);
>         for (parent = fwnode_get_parent(fwnode); parent;        \
>              parent = fwnode_get_next_parent(parent))
>
> -struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwnode);
>  unsigned int fwnode_count_parents(const struct fwnode_handle *fwn);
>  struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn,
>                                             unsigned int depth);
> -bool fwnode_is_ancestor_of(const struct fwnode_handle *ancestor, const struct fwnode_handle *child);

The rest of the functions here are related to parents and children of
a fwnode. So, why is this function considered to be in the wrong
place?

-Saravana

>  struct fwnode_handle *fwnode_get_next_child_node(
>         const struct fwnode_handle *fwnode, struct fwnode_handle *child);
>  struct fwnode_handle *fwnode_get_next_available_child_node(