| Message ID | 20230210223638.12796-4-kaehndan@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| 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.
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c index 27cadadda7c9..23c4518ec016 100644 --- a/drivers/hid/hid-cp2112.c +++ b/drivers/hid/hid-cp2112.c @@ -1234,6 +1234,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) u8 buf[3]; struct cp2112_smbus_config_report config; struct gpio_irq_chip *girq; + struct i2c_timings timings; int ret; dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); @@ -1292,6 +1293,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) goto err_power_normal; } + device_set_node(&dev->adap.dev, device_get_named_child_node(&hdev->dev, "i2c")); + i2c_parse_fw_timings(&dev->adap.dev, &timings, true); + + config.clock_speed = cpu_to_be32(timings.bus_freq_hz); config.retry_time = cpu_to_be16(1); ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config), @@ -1336,6 +1341,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) dev->gc.ngpio = 8; dev->gc.can_sleep = 1; dev->gc.parent = &hdev->dev; + dev->gc.fwnode = device_get_named_child_node(&hdev->dev, "gpio"); dev->irq.name = "cp2112-gpio"; dev->irq.irq_startup = cp2112_gpio_irq_startup;
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. Signed-off-by: Danny Kaehn <kaehndan@gmail.com> --- drivers/hid/hid-cp2112.c | 6 ++++++ 1 file changed, 6 insertions(+)