On Sat, Feb 04, 2017 at 04:58:33PM +0200, Andy Shevchenko wrote:
> On Sat, Feb 4, 2017 at 3:14 AM, Darren Hart <dvhart@xxxxxxxxxxxxx> wrote:
> > On Thu, Jan 26, 2017 at 03:33:01PM +0800, Alex Hung wrote:
> >> New firmwares include a feature called 5 button array that supports
> >> super key, volume up/down, rotation lock and power button. Especially,
> >> support for this feature is required to fix power button on some recent
> >> systems.
>
> >> +static int intel_button_array_input_setup(struct platform_device *device)
> >> +{
> >> + struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
> >> + int ret;
> >> +
> >> + /* Setup input device for 5 button array */
> >> + priv->array = input_allocate_device();
> >> + if (!priv->array)
> >> + return -ENOMEM;
> >> +
> >> + ret = sparse_keymap_setup(priv->array, intel_array_keymap, NULL);
> >> + if (ret)
> >> + goto err_free_array_device;
> >> +
> >> + priv->array->dev.parent = &device->dev;
> >> + priv->array->name = "Intel HID 5 button array";
> >> + priv->array->id.bustype = BUS_HOST;
> >> +
> >> + ret = input_register_device(priv->array);
> >> + if (ret)
> >> + goto err_free_array_device;
> >> +
> >> + return 0;
> >> +
> >> +err_free_array_device:
> >> + input_free_device(priv->array);
> >> + return ret;
> >
> > This return path is more complex than it could be, since you test for ret before
> > return anyway:
> >
> > out:
> > if (ret)
> > input_free_device(priv->array);
> > return ret;
> >
> > There is no need for a second return point that I can see. Same for the
> > hid_input_setup function. We can remove 8 lines.
>
> Darren, if I didn't miss anything the function is purely used in
> ->probe() path and thus devm_ version of input_allocate_device() would
> make this error path gone.
Hi Andy,
These are optional input devices for the driver, so if I understand the
semantics of devm_ correctly, the input device would remain allocated until such
time as the driver is unloaded or if it fails to bind - which for systems with
intel-hid, but no 5 button array, the unused input device would remain allocated
until system shutdown.
Alex, is that correct?
--
Darren Hart
Intel Open Source Technology Center
