Re: [PATCH v5 3/3] HID: cp2112: Fwnode Support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>
>



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux