Hi Andy,

On Sat, Feb 11, 2023 at 6:10 AM Andy Shevchenko
[off-list ref] wrote:

On Fri, Feb 10, 2023 at 04:36:38PM -0600, Danny Kaehn wrote:

quoted

Bind i2c and gpio interfaces to subnodes with names
"i2c" and "gpio" if they exist, respectively. This
allows the gpio and i2c controllers to be described
in firmware as usual. Additionally, support configuring the
i2c bus speed from the clock-frequency device property.

Entire series (code-wise, w/o DT bindings, not an expert there) looks good to
me, but one thing to address.

...

quoted

+     dev->gc.fwnode                  = device_get_named_child_node(&hdev->dev, "gpio");

Using like this bumps a reference count IIRC, so one need to drop it after use.
But please double check this.

Thanks for bringing this up -- I should have explicitly called this
out as something I was looking for feedback on, as I was unsure on
this.

I noticed that many of the users of device_get_named_child_node didn't
explicitly call fwnode_handle_put, and was unsure about the mechanics
of when this is needed.

The underlying call to device_get_named_child_node for an of_node is
of_fwnode_get_named_child_node, which does call
for_each_available_child_of_node and returns from within the loop, so
I _think_ you're right that the return will have its refcount
incremented (of_get_next_available_child calls of_node_get on the next
node, and doesn't call put until the next iteration).

However, I also noticed that many other functions in
drivers/base/property.c contain a message like the following in their
header comment:
"The caller is responsible for calling fwnode_handle_put() for the
returned node."
and this isn't present for device_get_named_child_node, which is
defined in that same file, which made me suspicious that this is
somehow done elsewhere internally (although I should know better than
to trust documenting comments :) ).

I'll wait a while longer to see if someone with a better grasp than me
on dynamic DT/firmware weighs in, otherwise, I'll assume I'll need to
call fwnode_handle_put both on error paths in _probe as well as in
_remove, since that appeared to be the case with the DT-specific
of_get_child_by_name path.

Thanks,

Danny Kaehn

--
With Best Regards,
Andy Shevchenko