Messages in this thread | | | Date | Wed, 24 Jul 2013 18:37:45 +0300 | Subject | Re: [PATCH 1/2] HID: trivial devm conversion for special hid drivers | From | Andy Shevchenko <> |
| |
On Wed, Jul 24, 2013 at 6:29 PM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > It is safe to use devres allocation within the hid subsystem: > - the devres release is called _after_ the call to .remove(), meaning > that no freed pointers will exists while removing the device > - if a .probe() fails, devres releases all the allocated ressources > before going to the next driver: there will not be ghost ressources > attached to a hid device if several drivers are probed. > > Given that, we can clean up a little some of the HID drivers. These ones > are trivial: > - there is only one kzalloc in the driver > - the .remove() callback contains only one kfree on top of hid_hw_stop() > - the error path in the probe is easy enough to be manually checked
Thanks for the patch! I'm sorry I didn't find time to do what I was talking about last time.
Few comments below.
> --- a/drivers/hid/hid-a4tech.c > +++ b/drivers/hid/hid-a4tech.c
> @@ -104,29 +103,16 @@ static int a4_probe(struct hid_device *hdev, const struct hid_device_id *id) > ret = hid_parse(hdev); > if (ret) { > hid_err(hdev, "parse failed\n"); > - goto err_free; > + return ret; > } > > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > - if (ret) { > + if (ret) > hid_err(hdev, "hw start failed\n"); > - goto err_free; > - } > > - return 0;
Isn't it better to leave explicit return 0? I think it would be fool proof in case someone wants to add anything in the middle.
> -err_free: > - kfree(a4); > return ret; > }
> -static void a4_remove(struct hid_device *hdev) > -{ > - struct a4tech_sc *a4 = hid_get_drvdata(hdev); > - > - hid_hw_stop(hdev);
Is it safe to remove this call? This question is the same for all patched drivers.
-- With Best Regards, Andy Shevchenko
| |