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(
