| Message ID | 20230127001141.407071-10-saravanak@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | fw_devlink improvements | expand |
Hi Saravana, On Thu, Jan 26, 2023 at 04:11:36PM -0800, Saravana Kannan wrote: > The driver core now: > - Has the parent device of a supplier pick up the consumers if the > supplier never has a device created for it. > - Ignores a supplier if the supplier has no parent device and will never > be probed by a driver > > And already prevents creating a device link with the consumer as a > supplier of a parent. > > So, we no longer need to find the "compatible" node of the supplier or > do any other checks in of_link_to_phandle(). We simply need to make sure > that the supplier is available in DT. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/of/property.c | 84 +++++++------------------------------------ > 1 file changed, 13 insertions(+), 71 deletions(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 134cfc980b70..c651aad6f34b 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1062,20 +1062,6 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, > return of_device_get_match_data(dev); > } > > -static bool of_is_ancestor_of(struct device_node *test_ancestor, > - struct device_node *child) > -{ > - of_node_get(child); > - while (child) { > - if (child == test_ancestor) { > - of_node_put(child); > - return true; > - } > - child = of_get_next_parent(child); > - } > - return false; > -} > - > static struct device_node *of_get_compat_node(struct device_node *np) > { > of_node_get(np); > @@ -1106,71 +1092,27 @@ static struct device_node *of_get_compat_node_parent(struct device_node *np) > return node; > } > > -/** > - * of_link_to_phandle - Add fwnode link to supplier from supplier phandle > - * @con_np: consumer device tree node > - * @sup_np: supplier device tree node > - * > - * Given a phandle to a supplier device tree node (@sup_np), this function > - * finds the device that owns the supplier device tree node and creates a > - * device link from @dev consumer device to the supplier device. This function > - * doesn't create device links for invalid scenarios such as trying to create a > - * link with a parent device as the consumer of its child device. In such > - * cases, it returns an error. > - * > - * Returns: > - * - 0 if fwnode link successfully created to supplier > - * - -EINVAL if the supplier link is invalid and should not be created > - * - -ENODEV if struct device will never be create for supplier > - */ > -static int of_link_to_phandle(struct device_node *con_np, > +static void of_link_to_phandle(struct device_node *con_np, > struct device_node *sup_np) > { > - struct device *sup_dev; > - struct device_node *tmp_np = sup_np; > + struct device_node *tmp_np = of_node_get(sup_np); > > - /* > - * Find the device node that contains the supplier phandle. It may be > - * @sup_np or it may be an ancestor of @sup_np. > - */ > - sup_np = of_get_compat_node(sup_np); > - if (!sup_np) { > - pr_debug("Not linking %pOFP to %pOFP - No device\n", > - con_np, tmp_np); > - return -ENODEV; > - } > + /* Check that sup_np and its ancestors are available. */ > + while (tmp_np) { > + if (of_fwnode_handle(tmp_np)->dev) { > + of_node_put(tmp_np); > + break; > + } > > - /* > - * Don't allow linking a device node as a consumer of one of its > - * descendant nodes. By definition, a child node can't be a functional > - * dependency for the parent node. > - */ > - if (of_is_ancestor_of(con_np, sup_np)) { > - pr_debug("Not linking %pOFP to %pOFP - is descendant\n", > - con_np, sup_np); > - of_node_put(sup_np); > - return -EINVAL; > - } > + if (!of_device_is_available(tmp_np)) { > + of_node_put(tmp_np); > + return; > + } > > - /* > - * Don't create links to "early devices" that won't have struct devices > - * created for them. > - */ > - sup_dev = get_dev_from_fwnode(&sup_np->fwnode); > - if (!sup_dev && > - (of_node_check_flag(sup_np, OF_POPULATED) || > - sup_np->fwnode.flags & FWNODE_FLAG_NOT_DEVICE)) { > - pr_debug("Not linking %pOFP to %pOFP - No struct device\n", > - con_np, sup_np); > - of_node_put(sup_np); > - return -ENODEV; > + tmp_np = of_get_next_parent(tmp_np); > } > - put_device(sup_dev); > > fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np)); fwnode_link_add() returns int. Why is the return type of this function changed to void? > - of_node_put(sup_np); > - > - return 0; > } > > /**
On Mon, Jan 30, 2023 at 10:15 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Saravana, > > On Thu, Jan 26, 2023 at 04:11:36PM -0800, Saravana Kannan wrote: > > The driver core now: > > - Has the parent device of a supplier pick up the consumers if the > > supplier never has a device created for it. > > - Ignores a supplier if the supplier has no parent device and will never > > be probed by a driver > > > > And already prevents creating a device link with the consumer as a > > supplier of a parent. > > > > So, we no longer need to find the "compatible" node of the supplier or > > do any other checks in of_link_to_phandle(). We simply need to make sure > > that the supplier is available in DT. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > drivers/of/property.c | 84 +++++++------------------------------------ > > 1 file changed, 13 insertions(+), 71 deletions(-) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index 134cfc980b70..c651aad6f34b 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -1062,20 +1062,6 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, > > return of_device_get_match_data(dev); > > } > > > > -static bool of_is_ancestor_of(struct device_node *test_ancestor, > > - struct device_node *child) > > -{ > > - of_node_get(child); > > - while (child) { > > - if (child == test_ancestor) { > > - of_node_put(child); > > - return true; > > - } > > - child = of_get_next_parent(child); > > - } > > - return false; > > -} > > - > > static struct device_node *of_get_compat_node(struct device_node *np) > > { > > of_node_get(np); > > @@ -1106,71 +1092,27 @@ static struct device_node *of_get_compat_node_parent(struct device_node *np) > > return node; > > } > > > > -/** > > - * of_link_to_phandle - Add fwnode link to supplier from supplier phandle > > - * @con_np: consumer device tree node > > - * @sup_np: supplier device tree node > > - * > > - * Given a phandle to a supplier device tree node (@sup_np), this function > > - * finds the device that owns the supplier device tree node and creates a > > - * device link from @dev consumer device to the supplier device. This function > > - * doesn't create device links for invalid scenarios such as trying to create a > > - * link with a parent device as the consumer of its child device. In such > > - * cases, it returns an error. > > - * > > - * Returns: > > - * - 0 if fwnode link successfully created to supplier > > - * - -EINVAL if the supplier link is invalid and should not be created > > - * - -ENODEV if struct device will never be create for supplier > > - */ > > -static int of_link_to_phandle(struct device_node *con_np, > > +static void of_link_to_phandle(struct device_node *con_np, > > struct device_node *sup_np) > > { > > - struct device *sup_dev; > > - struct device_node *tmp_np = sup_np; > > + struct device_node *tmp_np = of_node_get(sup_np); > > > > - /* > > - * Find the device node that contains the supplier phandle. It may be > > - * @sup_np or it may be an ancestor of @sup_np. > > - */ > > - sup_np = of_get_compat_node(sup_np); > > - if (!sup_np) { > > - pr_debug("Not linking %pOFP to %pOFP - No device\n", > > - con_np, tmp_np); > > - return -ENODEV; > > - } > > + /* Check that sup_np and its ancestors are available. */ > > + while (tmp_np) { > > + if (of_fwnode_handle(tmp_np)->dev) { > > + of_node_put(tmp_np); > > + break; > > + } > > > > - /* > > - * Don't allow linking a device node as a consumer of one of its > > - * descendant nodes. By definition, a child node can't be a functional > > - * dependency for the parent node. > > - */ > > - if (of_is_ancestor_of(con_np, sup_np)) { > > - pr_debug("Not linking %pOFP to %pOFP - is descendant\n", > > - con_np, sup_np); > > - of_node_put(sup_np); > > - return -EINVAL; > > - } > > + if (!of_device_is_available(tmp_np)) { > > + of_node_put(tmp_np); > > + return; > > + } > > > > - /* > > - * Don't create links to "early devices" that won't have struct devices > > - * created for them. > > - */ > > - sup_dev = get_dev_from_fwnode(&sup_np->fwnode); > > - if (!sup_dev && > > - (of_node_check_flag(sup_np, OF_POPULATED) || > > - sup_np->fwnode.flags & FWNODE_FLAG_NOT_DEVICE)) { > > - pr_debug("Not linking %pOFP to %pOFP - No struct device\n", > > - con_np, sup_np); > > - of_node_put(sup_np); > > - return -ENODEV; > > + tmp_np = of_get_next_parent(tmp_np); > > } > > - put_device(sup_dev); > > > > fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np)); > > fwnode_link_add() returns int. Why is the return type of this function > changed to void? The return value of fwnode_link_add() was ignored even before this patch. Since all other reasons for of_link_to_phandle() to fail are gone, I'm switching it to void. fwnode_link_add() is ignored because it can only fail due to -ENOMEM. Not much to do in that case. We do our best and move on. -Saravana
diff --git a/drivers/of/property.c b/drivers/of/property.c index 134cfc980b70..c651aad6f34b 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1062,20 +1062,6 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, return of_device_get_match_data(dev); } -static bool of_is_ancestor_of(struct device_node *test_ancestor, - struct device_node *child) -{ - of_node_get(child); - while (child) { - if (child == test_ancestor) { - of_node_put(child); - return true; - } - child = of_get_next_parent(child); - } - return false; -} - static struct device_node *of_get_compat_node(struct device_node *np) { of_node_get(np); @@ -1106,71 +1092,27 @@ static struct device_node *of_get_compat_node_parent(struct device_node *np) return node; } -/** - * of_link_to_phandle - Add fwnode link to supplier from supplier phandle - * @con_np: consumer device tree node - * @sup_np: supplier device tree node - * - * Given a phandle to a supplier device tree node (@sup_np), this function - * finds the device that owns the supplier device tree node and creates a - * device link from @dev consumer device to the supplier device. This function - * doesn't create device links for invalid scenarios such as trying to create a - * link with a parent device as the consumer of its child device. In such - * cases, it returns an error. - * - * Returns: - * - 0 if fwnode link successfully created to supplier - * - -EINVAL if the supplier link is invalid and should not be created - * - -ENODEV if struct device will never be create for supplier - */ -static int of_link_to_phandle(struct device_node *con_np, +static void of_link_to_phandle(struct device_node *con_np, struct device_node *sup_np) { - struct device *sup_dev; - struct device_node *tmp_np = sup_np; + struct device_node *tmp_np = of_node_get(sup_np); - /* - * Find the device node that contains the supplier phandle. It may be - * @sup_np or it may be an ancestor of @sup_np. - */ - sup_np = of_get_compat_node(sup_np); - if (!sup_np) { - pr_debug("Not linking %pOFP to %pOFP - No device\n", - con_np, tmp_np); - return -ENODEV; - } + /* Check that sup_np and its ancestors are available. */ + while (tmp_np) { + if (of_fwnode_handle(tmp_np)->dev) { + of_node_put(tmp_np); + break; + } - /* - * Don't allow linking a device node as a consumer of one of its - * descendant nodes. By definition, a child node can't be a functional - * dependency for the parent node. - */ - if (of_is_ancestor_of(con_np, sup_np)) { - pr_debug("Not linking %pOFP to %pOFP - is descendant\n", - con_np, sup_np); - of_node_put(sup_np); - return -EINVAL; - } + if (!of_device_is_available(tmp_np)) { + of_node_put(tmp_np); + return; + } - /* - * Don't create links to "early devices" that won't have struct devices - * created for them. - */ - sup_dev = get_dev_from_fwnode(&sup_np->fwnode); - if (!sup_dev && - (of_node_check_flag(sup_np, OF_POPULATED) || - sup_np->fwnode.flags & FWNODE_FLAG_NOT_DEVICE)) { - pr_debug("Not linking %pOFP to %pOFP - No struct device\n", - con_np, sup_np); - of_node_put(sup_np); - return -ENODEV; + tmp_np = of_get_next_parent(tmp_np); } - put_device(sup_dev); fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np)); - of_node_put(sup_np); - - return 0; } /**
The driver core now: - Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it. - Ignores a supplier if the supplier has no parent device and will never be probed by a driver And already prevents creating a device link with the consumer as a supplier of a parent. So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/of/property.c | 84 +++++++------------------------------------ 1 file changed, 13 insertions(+), 71 deletions(-)