Hi Andy, On Sat, Feb 11, 2023 at 6:10 AM Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > On Fri, Feb 10, 2023 at 04:36:38PM -0600, Danny Kaehn wrote: > > 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. > > ... > > > + 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 > >
