| Message ID | 20230210223638.12796-1-kaehndan@gmail.com |
|---|---|
| Headers | show |
| Series | Firmware Support for USB-HID Devices and CP2112 | expand |
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.
Hi Andy, On Sat, Feb 11, 2023 at 6:10 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 > >
On Thu, Feb 16, 2023 at 01:02:40PM -0600, Daniel Kaehn wrote: > On Sat, Feb 11, 2023 at 6:10 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Feb 10, 2023 at 04:36:38PM -0600, Danny Kaehn wrote: ... > > > + 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 :) ). Good catch! > 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. Patch to update the kernel documentation has been just sent.