Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links
On Thu, Nov 5, 2020 at 11:51 PM Greg Kroah-Hartman
wrote:
>
> On Thu, Nov 05, 2020 at 11:41:20PM -0800, Saravana Kannan wrote:
> > On Thu, Nov 5, 2020 at 11:22 PM Greg Kroah-Hartman
> > wrote:
> > >
> > > On Thu, Nov 05, 2020 at 03:25:56PM -0800, Saravana Kannan wrote:
> > > > On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
> > > > wrote:
> > > > >
> > > > > On Wed, Nov 04, 2020 at 03:23:52PM -0800, Saravana Kannan wrote:
> > > > > > The semantics of add_links() has changed from creating device link
> > > > > > between devices to creating fwnode links between fwnodes. So,
> > > > > > update the
> > > > > > implementation of add_links() to match the new semantics.
> > > > > >
> > > > > > Signed-off-by: Saravana Kannan
> > > > > > ---
> > > > > > drivers/of/property.c | 150
> > > > > > --
> > > > > > 1 file changed, 41 insertions(+), 109 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > > > index 408a7b5f06a9..86303803f1b3 100644
> > > > > > --- a/drivers/of/property.c
> > > > > > +++ b/drivers/of/property.c
> > > > > > @@ -1038,33 +1038,9 @@ static bool of_is_ancestor_of(struct
> > > > > > device_node *test_ancestor,
> > > > > > }
> > > > > >
> > > > > > /**
> > > > > > - * of_get_next_parent_dev - Add device link to supplier from
> > > > > > supplier phandle
> > > > > > - * @np: device tree node
> > > > > > - *
> > > > > > - * Given a device tree node (@np), this function finds its closest
> > > > > > ancestor
> > > > > > - * device tree node that has a corresponding struct device.
> > > > > > - *
> > > > > > - * The caller of this function is expected to call put_device() on
> > > > > > the returned
> > > > > > - * device when they are done.
> > > > > > - */
> > > > > > -static struct device *of_get_next_parent_dev(struct device_node
> > > > > > *np)
> > > > > > -{
> > > > > > - struct device *dev = NULL;
> > > > > > -
> > > > > > - of_node_get(np);
> > > > > > - do {
> > > > > > - np = of_get_next_parent(np);
> > > > > > - if (np)
> > > > > > - dev = get_dev_from_fwnode(>fwnode);
> > > > > > - } while (np && !dev);
> > > > > > - of_node_put(np);
> > > > > > - return dev;
> > > > > > -}
> > > > > > -
> > > > > > -/**
> > > > > > - * of_link_to_phandle - Add device link to supplier from supplier
> > > > > > phandle
> > > > > > - * @dev: consumer device
> > > > > > - * @sup_np: phandle to supplier device tree 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
> > > > > > @@ -1074,16 +1050,14 @@ static struct device
> > > > > > *of_get_next_parent_dev(struct device_node *np)
> > > > > > * cases, it returns an error.
> > > > > > *
> > > > > > * Returns:
> > > > > > - * - 0 if link successfully created to supplier
> > > > > > - * - -EAGAIN if linking to the supplier should be reattempted
> > > > > > + * - 0 if fwnode link successfully created to supplier
> > > > > > * - -EINVAL if the supplier link is invalid and should not be
> > > > > > created
> > > > > > - * - -ENODEV if there is no device that corresponds to the
> > > > > > supplier phandle
> > > > > > + * - -ENODEV if struct device will never be create for supplier
> > > > > > */
> > > > > > -static int of_link_to_phandle(struct device *dev, struct
> > > > > > device_node *sup_np,
> > > > > > - u32 dl_flags)
> > > > > > +static int of_link_to_phandle(struct device_node *con_np,
> > > > > > + struct device_node *sup_np)
> > > > > > {
> > > > > > - struct device *sup_dev, *sup_par_dev;
> > > > > > - int ret = 0;
> > > > > > + struct device *sup_dev;
> > > > > > struct device_node *tmp_np = sup_np;
> > > > > >
> > > > > > of_node_get(sup_np);
> > > > > > @@ -1106,7 +1080,8 @@ static int of_link_to_phandle(struct device
> > > > > > *dev, struct device_node *sup_np,
> > > > > > }
> > > > > >
> > > > > > if (!sup_np) {
> > > > > > - dev_dbg(dev, "Not linking to %pOFP - No device\n",
> > > > > > tmp_np);
> > > > > > + pr_debug("Not linking %pOFP to %pOFP - No device\n",
> > > > > > + con_np, tmp_np);
> > > > >
> > > > > Who is calling this function without a valid dev pointer?
> > > >
> > > > Sorry, I plan to delete the "dev" parameter as it's not really used
> > > > anywhere. I'm trying to do that without causing build time errors and
> > > > making the series into digestible small patches.
> > > >
> > > > I can do the deletion of the parameter as a Patch 19/19. Will
Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links
On Thu, Nov 05, 2020 at 11:41:20PM -0800, Saravana Kannan wrote:
> On Thu, Nov 5, 2020 at 11:22 PM Greg Kroah-Hartman
> wrote:
> >
> > On Thu, Nov 05, 2020 at 03:25:56PM -0800, Saravana Kannan wrote:
> > > On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
> > > wrote:
> > > >
> > > > On Wed, Nov 04, 2020 at 03:23:52PM -0800, Saravana Kannan wrote:
> > > > > The semantics of add_links() has changed from creating device link
> > > > > between devices to creating fwnode links between fwnodes. So, update
> > > > > the
> > > > > implementation of add_links() to match the new semantics.
> > > > >
> > > > > Signed-off-by: Saravana Kannan
> > > > > ---
> > > > > drivers/of/property.c | 150
> > > > > --
> > > > > 1 file changed, 41 insertions(+), 109 deletions(-)
> > > > >
> > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > > index 408a7b5f06a9..86303803f1b3 100644
> > > > > --- a/drivers/of/property.c
> > > > > +++ b/drivers/of/property.c
> > > > > @@ -1038,33 +1038,9 @@ static bool of_is_ancestor_of(struct
> > > > > device_node *test_ancestor,
> > > > > }
> > > > >
> > > > > /**
> > > > > - * of_get_next_parent_dev - Add device link to supplier from
> > > > > supplier phandle
> > > > > - * @np: device tree node
> > > > > - *
> > > > > - * Given a device tree node (@np), this function finds its closest
> > > > > ancestor
> > > > > - * device tree node that has a corresponding struct device.
> > > > > - *
> > > > > - * The caller of this function is expected to call put_device() on
> > > > > the returned
> > > > > - * device when they are done.
> > > > > - */
> > > > > -static struct device *of_get_next_parent_dev(struct device_node *np)
> > > > > -{
> > > > > - struct device *dev = NULL;
> > > > > -
> > > > > - of_node_get(np);
> > > > > - do {
> > > > > - np = of_get_next_parent(np);
> > > > > - if (np)
> > > > > - dev = get_dev_from_fwnode(>fwnode);
> > > > > - } while (np && !dev);
> > > > > - of_node_put(np);
> > > > > - return dev;
> > > > > -}
> > > > > -
> > > > > -/**
> > > > > - * of_link_to_phandle - Add device link to supplier from supplier
> > > > > phandle
> > > > > - * @dev: consumer device
> > > > > - * @sup_np: phandle to supplier device tree 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
> > > > > @@ -1074,16 +1050,14 @@ static struct device
> > > > > *of_get_next_parent_dev(struct device_node *np)
> > > > > * cases, it returns an error.
> > > > > *
> > > > > * Returns:
> > > > > - * - 0 if link successfully created to supplier
> > > > > - * - -EAGAIN if linking to the supplier should be reattempted
> > > > > + * - 0 if fwnode link successfully created to supplier
> > > > > * - -EINVAL if the supplier link is invalid and should not be
> > > > > created
> > > > > - * - -ENODEV if there is no device that corresponds to the supplier
> > > > > phandle
> > > > > + * - -ENODEV if struct device will never be create for supplier
> > > > > */
> > > > > -static int of_link_to_phandle(struct device *dev, struct device_node
> > > > > *sup_np,
> > > > > - u32 dl_flags)
> > > > > +static int of_link_to_phandle(struct device_node *con_np,
> > > > > + struct device_node *sup_np)
> > > > > {
> > > > > - struct device *sup_dev, *sup_par_dev;
> > > > > - int ret = 0;
> > > > > + struct device *sup_dev;
> > > > > struct device_node *tmp_np = sup_np;
> > > > >
> > > > > of_node_get(sup_np);
> > > > > @@ -1106,7 +1080,8 @@ static int of_link_to_phandle(struct device
> > > > > *dev, struct device_node *sup_np,
> > > > > }
> > > > >
> > > > > if (!sup_np) {
> > > > > - dev_dbg(dev, "Not linking to %pOFP - No device\n",
> > > > > tmp_np);
> > > > > + pr_debug("Not linking %pOFP to %pOFP - No device\n",
> > > > > + con_np, tmp_np);
> > > >
> > > > Who is calling this function without a valid dev pointer?
> > >
> > > Sorry, I plan to delete the "dev" parameter as it's not really used
> > > anywhere. I'm trying to do that without causing build time errors and
> > > making the series into digestible small patches.
> > >
> > > I can do the deletion of the parameter as a Patch 19/19. Will that work?
> >
> > That's fine, but why get rid of dev? The driver core works on these
> > things, and we want errors/messages/warnings to spit out what device is
> > causing those issues. It is fine to drag around a struct device pointer
> > just for messages, that's to be expected, and is good.
Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links
On Thu, Nov 5, 2020 at 11:22 PM Greg Kroah-Hartman
wrote:
>
> On Thu, Nov 05, 2020 at 03:25:56PM -0800, Saravana Kannan wrote:
> > On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
> > wrote:
> > >
> > > On Wed, Nov 04, 2020 at 03:23:52PM -0800, Saravana Kannan wrote:
> > > > The semantics of add_links() has changed from creating device link
> > > > between devices to creating fwnode links between fwnodes. So, update the
> > > > implementation of add_links() to match the new semantics.
> > > >
> > > > Signed-off-by: Saravana Kannan
> > > > ---
> > > > drivers/of/property.c | 150 --
> > > > 1 file changed, 41 insertions(+), 109 deletions(-)
> > > >
> > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > index 408a7b5f06a9..86303803f1b3 100644
> > > > --- a/drivers/of/property.c
> > > > +++ b/drivers/of/property.c
> > > > @@ -1038,33 +1038,9 @@ static bool of_is_ancestor_of(struct device_node
> > > > *test_ancestor,
> > > > }
> > > >
> > > > /**
> > > > - * of_get_next_parent_dev - Add device link to supplier from supplier
> > > > phandle
> > > > - * @np: device tree node
> > > > - *
> > > > - * Given a device tree node (@np), this function finds its closest
> > > > ancestor
> > > > - * device tree node that has a corresponding struct device.
> > > > - *
> > > > - * The caller of this function is expected to call put_device() on the
> > > > returned
> > > > - * device when they are done.
> > > > - */
> > > > -static struct device *of_get_next_parent_dev(struct device_node *np)
> > > > -{
> > > > - struct device *dev = NULL;
> > > > -
> > > > - of_node_get(np);
> > > > - do {
> > > > - np = of_get_next_parent(np);
> > > > - if (np)
> > > > - dev = get_dev_from_fwnode(>fwnode);
> > > > - } while (np && !dev);
> > > > - of_node_put(np);
> > > > - return dev;
> > > > -}
> > > > -
> > > > -/**
> > > > - * of_link_to_phandle - Add device link to supplier from supplier
> > > > phandle
> > > > - * @dev: consumer device
> > > > - * @sup_np: phandle to supplier device tree 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
> > > > @@ -1074,16 +1050,14 @@ static struct device
> > > > *of_get_next_parent_dev(struct device_node *np)
> > > > * cases, it returns an error.
> > > > *
> > > > * Returns:
> > > > - * - 0 if link successfully created to supplier
> > > > - * - -EAGAIN if linking to the supplier should be reattempted
> > > > + * - 0 if fwnode link successfully created to supplier
> > > > * - -EINVAL if the supplier link is invalid and should not be created
> > > > - * - -ENODEV if there is no device that corresponds to the supplier
> > > > phandle
> > > > + * - -ENODEV if struct device will never be create for supplier
> > > > */
> > > > -static int of_link_to_phandle(struct device *dev, struct device_node
> > > > *sup_np,
> > > > - u32 dl_flags)
> > > > +static int of_link_to_phandle(struct device_node *con_np,
> > > > + struct device_node *sup_np)
> > > > {
> > > > - struct device *sup_dev, *sup_par_dev;
> > > > - int ret = 0;
> > > > + struct device *sup_dev;
> > > > struct device_node *tmp_np = sup_np;
> > > >
> > > > of_node_get(sup_np);
> > > > @@ -1106,7 +1080,8 @@ static int of_link_to_phandle(struct device *dev,
> > > > struct device_node *sup_np,
> > > > }
> > > >
> > > > if (!sup_np) {
> > > > - dev_dbg(dev, "Not linking to %pOFP - No device\n",
> > > > tmp_np);
> > > > + pr_debug("Not linking %pOFP to %pOFP - No device\n",
> > > > + con_np, tmp_np);
> > >
> > > Who is calling this function without a valid dev pointer?
> >
> > Sorry, I plan to delete the "dev" parameter as it's not really used
> > anywhere. I'm trying to do that without causing build time errors and
> > making the series into digestible small patches.
> >
> > I can do the deletion of the parameter as a Patch 19/19. Will that work?
>
> That's fine, but why get rid of dev? The driver core works on these
> things, and we want errors/messages/warnings to spit out what device is
> causing those issues. It is fine to drag around a struct device pointer
> just for messages, that's to be expected, and is good.
In general I agree. If the fwnode being parsed is related to the dev,
it's nice to have the dev name in the logs.
But in this instance I feel it's analogous to printing the PID that's
parsing the fwnode -- kinda irrelevant. The device in question can
appear very random and it'll just cause more confusion than be of help
Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links
On Thu, Nov 05, 2020 at 03:25:56PM -0800, Saravana Kannan wrote:
> On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
> wrote:
> >
> > On Wed, Nov 04, 2020 at 03:23:52PM -0800, Saravana Kannan wrote:
> > > The semantics of add_links() has changed from creating device link
> > > between devices to creating fwnode links between fwnodes. So, update the
> > > implementation of add_links() to match the new semantics.
> > >
> > > Signed-off-by: Saravana Kannan
> > > ---
> > > drivers/of/property.c | 150 --
> > > 1 file changed, 41 insertions(+), 109 deletions(-)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 408a7b5f06a9..86303803f1b3 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1038,33 +1038,9 @@ static bool of_is_ancestor_of(struct device_node
> > > *test_ancestor,
> > > }
> > >
> > > /**
> > > - * of_get_next_parent_dev - Add device link to supplier from supplier
> > > phandle
> > > - * @np: device tree node
> > > - *
> > > - * Given a device tree node (@np), this function finds its closest
> > > ancestor
> > > - * device tree node that has a corresponding struct device.
> > > - *
> > > - * The caller of this function is expected to call put_device() on the
> > > returned
> > > - * device when they are done.
> > > - */
> > > -static struct device *of_get_next_parent_dev(struct device_node *np)
> > > -{
> > > - struct device *dev = NULL;
> > > -
> > > - of_node_get(np);
> > > - do {
> > > - np = of_get_next_parent(np);
> > > - if (np)
> > > - dev = get_dev_from_fwnode(>fwnode);
> > > - } while (np && !dev);
> > > - of_node_put(np);
> > > - return dev;
> > > -}
> > > -
> > > -/**
> > > - * of_link_to_phandle - Add device link to supplier from supplier phandle
> > > - * @dev: consumer device
> > > - * @sup_np: phandle to supplier device tree 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
> > > @@ -1074,16 +1050,14 @@ static struct device
> > > *of_get_next_parent_dev(struct device_node *np)
> > > * cases, it returns an error.
> > > *
> > > * Returns:
> > > - * - 0 if link successfully created to supplier
> > > - * - -EAGAIN if linking to the supplier should be reattempted
> > > + * - 0 if fwnode link successfully created to supplier
> > > * - -EINVAL if the supplier link is invalid and should not be created
> > > - * - -ENODEV if there is no device that corresponds to the supplier
> > > phandle
> > > + * - -ENODEV if struct device will never be create for supplier
> > > */
> > > -static int of_link_to_phandle(struct device *dev, struct device_node
> > > *sup_np,
> > > - u32 dl_flags)
> > > +static int of_link_to_phandle(struct device_node *con_np,
> > > + struct device_node *sup_np)
> > > {
> > > - struct device *sup_dev, *sup_par_dev;
> > > - int ret = 0;
> > > + struct device *sup_dev;
> > > struct device_node *tmp_np = sup_np;
> > >
> > > of_node_get(sup_np);
> > > @@ -1106,7 +1080,8 @@ static int of_link_to_phandle(struct device *dev,
> > > struct device_node *sup_np,
> > > }
> > >
> > > if (!sup_np) {
> > > - dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
> > > + pr_debug("Not linking %pOFP to %pOFP - No device\n",
> > > + con_np, tmp_np);
> >
> > Who is calling this function without a valid dev pointer?
>
> Sorry, I plan to delete the "dev" parameter as it's not really used
> anywhere. I'm trying to do that without causing build time errors and
> making the series into digestible small patches.
>
> I can do the deletion of the parameter as a Patch 19/19. Will that work?
That's fine, but why get rid of dev? The driver core works on these
things, and we want errors/messages/warnings to spit out what device is
causing those issues. It is fine to drag around a struct device pointer
just for messages, that's to be expected, and is good.
> > And the only way it can be NULL is if fwnode is NULL, and as you control
> > the callers to it, how can that be the case?
>
> fwnode represents a generic firmware node. The to_of_node() returns
> NULL if fwnode is not a DT node. So con_np can be NULL if that
> happens. That's why we need a NULL check here. With the current code,
> that can never happen, bit I think it doesn't hurt to check in case
> there's a buggy caller. I don't have a strong opinion - so I can do it
> whichever way.
If it can't happen, no need to check for it :)
thanks,
greg k-h
Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links
On Thu, Nov 5, 2020 at 3:25 PM Saravana Kannan wrote:
>
> On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
> wrote:
> >
> > On Wed, Nov 04, 2020 at 03:23:52PM -0800, Saravana Kannan wrote:
> > > The semantics of add_links() has changed from creating device link
> > > between devices to creating fwnode links between fwnodes. So, update the
> > > implementation of add_links() to match the new semantics.
> > >
> > > Signed-off-by: Saravana Kannan
> > > ---
> > > drivers/of/property.c | 150 --
> > > 1 file changed, 41 insertions(+), 109 deletions(-)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 408a7b5f06a9..86303803f1b3 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1038,33 +1038,9 @@ static bool of_is_ancestor_of(struct device_node
> > > *test_ancestor,
> > > }
> > >
> > > /**
> > > - * of_get_next_parent_dev - Add device link to supplier from supplier
> > > phandle
> > > - * @np: device tree node
> > > - *
> > > - * Given a device tree node (@np), this function finds its closest
> > > ancestor
> > > - * device tree node that has a corresponding struct device.
> > > - *
> > > - * The caller of this function is expected to call put_device() on the
> > > returned
> > > - * device when they are done.
> > > - */
> > > -static struct device *of_get_next_parent_dev(struct device_node *np)
> > > -{
> > > - struct device *dev = NULL;
> > > -
> > > - of_node_get(np);
> > > - do {
> > > - np = of_get_next_parent(np);
> > > - if (np)
> > > - dev = get_dev_from_fwnode(>fwnode);
> > > - } while (np && !dev);
> > > - of_node_put(np);
> > > - return dev;
> > > -}
> > > -
> > > -/**
> > > - * of_link_to_phandle - Add device link to supplier from supplier phandle
> > > - * @dev: consumer device
> > > - * @sup_np: phandle to supplier device tree 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
> > > @@ -1074,16 +1050,14 @@ static struct device
> > > *of_get_next_parent_dev(struct device_node *np)
> > > * cases, it returns an error.
> > > *
> > > * Returns:
> > > - * - 0 if link successfully created to supplier
> > > - * - -EAGAIN if linking to the supplier should be reattempted
> > > + * - 0 if fwnode link successfully created to supplier
> > > * - -EINVAL if the supplier link is invalid and should not be created
> > > - * - -ENODEV if there is no device that corresponds to the supplier
> > > phandle
> > > + * - -ENODEV if struct device will never be create for supplier
> > > */
> > > -static int of_link_to_phandle(struct device *dev, struct device_node
> > > *sup_np,
> > > - u32 dl_flags)
> > > +static int of_link_to_phandle(struct device_node *con_np,
> > > + struct device_node *sup_np)
> > > {
> > > - struct device *sup_dev, *sup_par_dev;
> > > - int ret = 0;
> > > + struct device *sup_dev;
> > > struct device_node *tmp_np = sup_np;
> > >
> > > of_node_get(sup_np);
> > > @@ -1106,7 +1080,8 @@ static int of_link_to_phandle(struct device *dev,
> > > struct device_node *sup_np,
> > > }
> > >
> > > if (!sup_np) {
> > > - dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
> > > + pr_debug("Not linking %pOFP to %pOFP - No device\n",
> > > + con_np, tmp_np);
> >
> > Who is calling this function without a valid dev pointer?
>
> Sorry, I plan to delete the "dev" parameter as it's not really used
> anywhere. I'm trying to do that without causing build time errors and
> making the series into digestible small patches.
*facepalm* for my earlier response. You'll notice that I've already
deleted the "dev" input param to this function. That's why I can't use
it here :)
-Saravana
Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links
On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
wrote:
>
> On Wed, Nov 04, 2020 at 03:23:52PM -0800, Saravana Kannan wrote:
> > The semantics of add_links() has changed from creating device link
> > between devices to creating fwnode links between fwnodes. So, update the
> > implementation of add_links() to match the new semantics.
> >
> > Signed-off-by: Saravana Kannan
> > ---
> > drivers/of/property.c | 150 --
> > 1 file changed, 41 insertions(+), 109 deletions(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 408a7b5f06a9..86303803f1b3 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1038,33 +1038,9 @@ static bool of_is_ancestor_of(struct device_node
> > *test_ancestor,
> > }
> >
> > /**
> > - * of_get_next_parent_dev - Add device link to supplier from supplier
> > phandle
> > - * @np: device tree node
> > - *
> > - * Given a device tree node (@np), this function finds its closest ancestor
> > - * device tree node that has a corresponding struct device.
> > - *
> > - * The caller of this function is expected to call put_device() on the
> > returned
> > - * device when they are done.
> > - */
> > -static struct device *of_get_next_parent_dev(struct device_node *np)
> > -{
> > - struct device *dev = NULL;
> > -
> > - of_node_get(np);
> > - do {
> > - np = of_get_next_parent(np);
> > - if (np)
> > - dev = get_dev_from_fwnode(>fwnode);
> > - } while (np && !dev);
> > - of_node_put(np);
> > - return dev;
> > -}
> > -
> > -/**
> > - * of_link_to_phandle - Add device link to supplier from supplier phandle
> > - * @dev: consumer device
> > - * @sup_np: phandle to supplier device tree 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
> > @@ -1074,16 +1050,14 @@ static struct device *of_get_next_parent_dev(struct
> > device_node *np)
> > * cases, it returns an error.
> > *
> > * Returns:
> > - * - 0 if link successfully created to supplier
> > - * - -EAGAIN if linking to the supplier should be reattempted
> > + * - 0 if fwnode link successfully created to supplier
> > * - -EINVAL if the supplier link is invalid and should not be created
> > - * - -ENODEV if there is no device that corresponds to the supplier phandle
> > + * - -ENODEV if struct device will never be create for supplier
> > */
> > -static int of_link_to_phandle(struct device *dev, struct device_node
> > *sup_np,
> > - u32 dl_flags)
> > +static int of_link_to_phandle(struct device_node *con_np,
> > + struct device_node *sup_np)
> > {
> > - struct device *sup_dev, *sup_par_dev;
> > - int ret = 0;
> > + struct device *sup_dev;
> > struct device_node *tmp_np = sup_np;
> >
> > of_node_get(sup_np);
> > @@ -1106,7 +1080,8 @@ static int of_link_to_phandle(struct device *dev,
> > struct device_node *sup_np,
> > }
> >
> > if (!sup_np) {
> > - dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
> > + pr_debug("Not linking %pOFP to %pOFP - No device\n",
> > + con_np, tmp_np);
>
> Who is calling this function without a valid dev pointer?
Sorry, I plan to delete the "dev" parameter as it's not really used
anywhere. I'm trying to do that without causing build time errors and
making the series into digestible small patches.
I can do the deletion of the parameter as a Patch 19/19. Will that work?
> > return -ENODEV;
> > }
> >
> > @@ -1115,53 +1090,30 @@ static int of_link_to_phandle(struct device *dev,
> > struct device_node *sup_np,
> >* descendant nodes. By definition, a child node can't be a functional
> >* dependency for the parent node.
> >*/
> > - if (of_is_ancestor_of(dev->of_node, sup_np)) {
> > - dev_dbg(dev, "Not linking to %pOFP - is descendant\n",
> > sup_np);
> > + if (of_is_ancestor_of(con_np, sup_np)) {
> > + pr_debug("Not linking %pOFP to %pOFP - is descendant\n",
> > + con_np, sup_np);
>
> Why not dev_dbg() here? dev should be valid, correct?
Responded above.
> > of_node_put(sup_np);
> > return -EINVAL;
> > }
> > +
> > + /*
> > + * Don't create links to "early devices" that won't have struct
> > devices
> > + * created for them.
> > + */
> > sup_dev = get_dev_from_fwnode(_np->fwnode);
> > if (!sup_dev && of_node_check_flag(sup_np, OF_POPULATED)) {
> > - /* Early device without struct device. */
> > - dev_dbg(dev, "Not linking to %pOFP - No struct
Re: [PATCH v1 15/18] of: property: Update implementation of add_links() to create fwnode links
On Wed, Nov 04, 2020 at 03:23:52PM -0800, Saravana Kannan wrote:
> The semantics of add_links() has changed from creating device link
> between devices to creating fwnode links between fwnodes. So, update the
> implementation of add_links() to match the new semantics.
>
> Signed-off-by: Saravana Kannan
> ---
> drivers/of/property.c | 150 --
> 1 file changed, 41 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 408a7b5f06a9..86303803f1b3 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1038,33 +1038,9 @@ static bool of_is_ancestor_of(struct device_node
> *test_ancestor,
> }
>
> /**
> - * of_get_next_parent_dev - Add device link to supplier from supplier phandle
> - * @np: device tree node
> - *
> - * Given a device tree node (@np), this function finds its closest ancestor
> - * device tree node that has a corresponding struct device.
> - *
> - * The caller of this function is expected to call put_device() on the
> returned
> - * device when they are done.
> - */
> -static struct device *of_get_next_parent_dev(struct device_node *np)
> -{
> - struct device *dev = NULL;
> -
> - of_node_get(np);
> - do {
> - np = of_get_next_parent(np);
> - if (np)
> - dev = get_dev_from_fwnode(>fwnode);
> - } while (np && !dev);
> - of_node_put(np);
> - return dev;
> -}
> -
> -/**
> - * of_link_to_phandle - Add device link to supplier from supplier phandle
> - * @dev: consumer device
> - * @sup_np: phandle to supplier device tree 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
> @@ -1074,16 +1050,14 @@ static struct device *of_get_next_parent_dev(struct
> device_node *np)
> * cases, it returns an error.
> *
> * Returns:
> - * - 0 if link successfully created to supplier
> - * - -EAGAIN if linking to the supplier should be reattempted
> + * - 0 if fwnode link successfully created to supplier
> * - -EINVAL if the supplier link is invalid and should not be created
> - * - -ENODEV if there is no device that corresponds to the supplier phandle
> + * - -ENODEV if struct device will never be create for supplier
> */
> -static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> - u32 dl_flags)
> +static int of_link_to_phandle(struct device_node *con_np,
> + struct device_node *sup_np)
> {
> - struct device *sup_dev, *sup_par_dev;
> - int ret = 0;
> + struct device *sup_dev;
> struct device_node *tmp_np = sup_np;
>
> of_node_get(sup_np);
> @@ -1106,7 +1080,8 @@ static int of_link_to_phandle(struct device *dev,
> struct device_node *sup_np,
> }
>
> if (!sup_np) {
> - dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
> + pr_debug("Not linking %pOFP to %pOFP - No device\n",
> + con_np, tmp_np);
Who is calling this function without a valid dev pointer?
> return -ENODEV;
> }
>
> @@ -1115,53 +1090,30 @@ static int of_link_to_phandle(struct device *dev,
> struct device_node *sup_np,
>* descendant nodes. By definition, a child node can't be a functional
>* dependency for the parent node.
>*/
> - if (of_is_ancestor_of(dev->of_node, sup_np)) {
> - dev_dbg(dev, "Not linking to %pOFP - is descendant\n", sup_np);
> + if (of_is_ancestor_of(con_np, sup_np)) {
> + pr_debug("Not linking %pOFP to %pOFP - is descendant\n",
> + con_np, sup_np);
Why not dev_dbg() here? dev should be valid, correct?
> of_node_put(sup_np);
> return -EINVAL;
> }
> +
> + /*
> + * Don't create links to "early devices" that won't have struct devices
> + * created for them.
> + */
> sup_dev = get_dev_from_fwnode(_np->fwnode);
> if (!sup_dev && of_node_check_flag(sup_np, OF_POPULATED)) {
> - /* Early device without struct device. */
> - dev_dbg(dev, "Not linking to %pOFP - No struct device\n",
> - sup_np);
> + pr_debug("Not linking %pOFP to %pOFP - No struct device\n",
> + con_np, sup_np);
How is dev not valid here? sup_dev might not be, but dev should be.
> of_node_put(sup_np);
> return -ENODEV;
> - } else if (!sup_dev) {
> - /*
> - * DL_FLAG_SYNC_STATE_ONLY doesn't block probing and supports
> - * cycles. So cycle detection isn't necessary and shouldn't be
> - * done.
> - */
> - if
