| Message ID | 20230714080611.81302-7-frank.li@vivo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/8] Input: lm8333 - convert to use devm_* api | expand |
Hi Yangtao, On Fri, Jul 14, 2023 at 04:06:10PM +0800, Yangtao Li wrote: > Use devm_* api to simplify code, this makes it unnecessary to explicitly > release resources. > > Signed-off-by: Yangtao Li <frank.li@vivo.com> > --- > drivers/input/keyboard/qt2160.c | 30 +++++++++++------------------- > 1 file changed, 11 insertions(+), 19 deletions(-) > > diff --git a/drivers/input/keyboard/qt2160.c b/drivers/input/keyboard/qt2160.c > index 599ea85cfd30..218ef92b8c2b 100644 > --- a/drivers/input/keyboard/qt2160.c > +++ b/drivers/input/keyboard/qt2160.c > @@ -340,6 +340,7 @@ static bool qt2160_identify(struct i2c_client *client) > > static int qt2160_probe(struct i2c_client *client) > { > + struct device *dev = &client->dev; You create a temporary, but half of the code does not use it. Please if you introduce a temporary make sure everything is using it. > struct qt2160_data *qt2160; > struct input_dev *input; > int i; > @@ -358,12 +359,11 @@ static int qt2160_probe(struct i2c_client *client) > return -ENODEV; > > /* Chip is valid and active. Allocate structure */ > - qt2160 = kzalloc(sizeof(struct qt2160_data), GFP_KERNEL); > - input = input_allocate_device(); > + qt2160 = devm_kzalloc(dev, sizeof(struct qt2160_data), GFP_KERNEL); > + input = devm_input_allocate_device(dev); > if (!qt2160 || !input) { This double check was a cheat when resources did not free automatically, it makes no sense to carry with devm. > dev_err(&client->dev, "insufficient memory\n"); > - error = -ENOMEM; > - goto err_free_mem; > + return -ENOMEM; > } > > qt2160->client = client; > @@ -389,23 +389,23 @@ static int qt2160_probe(struct i2c_client *client) > error = qt2160_write(client, QT2160_CMD_CALIBRATE, 1); > if (error) { > dev_err(&client->dev, "failed to calibrate device\n"); > - goto err_free_mem; > + return error; > } > > if (client->irq) { > - error = request_irq(client->irq, qt2160_irq, > - IRQF_TRIGGER_FALLING, "qt2160", qt2160); > + error = devm_request_irq(dev, client->irq, qt2160_irq, > + IRQF_TRIGGER_FALLING, "qt2160", qt2160); > if (error) { > dev_err(&client->dev, > "failed to allocate irq %d\n", client->irq); > - goto err_free_mem; > + return error; > } > } > > error = qt2160_register_leds(qt2160); > if (error) { > dev_err(&client->dev, "Failed to register leds\n"); > - goto err_free_irq; > + return error; > } > > error = input_register_device(qt2160->input); > @@ -422,29 +422,21 @@ static int qt2160_probe(struct i2c_client *client) > > err_unregister_leds: > qt2160_unregister_leds(qt2160); LEDs can also be registered with devm. > -err_free_irq: > - if (client->irq) > - free_irq(client->irq, qt2160); > -err_free_mem: > - input_free_device(input); > - kfree(qt2160); > return error; > } > > static void qt2160_remove(struct i2c_client *client) > { > struct qt2160_data *qt2160 = i2c_get_clientdata(client); > + struct device *dev = &client->dev; > > qt2160_unregister_leds(qt2160); > > /* Release IRQ so no queue will be scheduled */ > if (client->irq) > - free_irq(client->irq, qt2160); > + devm_free_irq(dev, client->irq, qt2160); > > cancel_delayed_work_sync(&qt2160->dwork); It is not great that we are left with non-empty qt2160_remove(). The driver should be converted away from using work item, and then entirety of qt2160_remove() can be deleted. I posted a series that cleans up the driver and incorporates an updated version of your patch. Thanks.
diff --git a/drivers/input/keyboard/qt2160.c b/drivers/input/keyboard/qt2160.c index 599ea85cfd30..218ef92b8c2b 100644 --- a/drivers/input/keyboard/qt2160.c +++ b/drivers/input/keyboard/qt2160.c @@ -340,6 +340,7 @@ static bool qt2160_identify(struct i2c_client *client) static int qt2160_probe(struct i2c_client *client) { + struct device *dev = &client->dev; struct qt2160_data *qt2160; struct input_dev *input; int i; @@ -358,12 +359,11 @@ static int qt2160_probe(struct i2c_client *client) return -ENODEV; /* Chip is valid and active. Allocate structure */ - qt2160 = kzalloc(sizeof(struct qt2160_data), GFP_KERNEL); - input = input_allocate_device(); + qt2160 = devm_kzalloc(dev, sizeof(struct qt2160_data), GFP_KERNEL); + input = devm_input_allocate_device(dev); if (!qt2160 || !input) { dev_err(&client->dev, "insufficient memory\n"); - error = -ENOMEM; - goto err_free_mem; + return -ENOMEM; } qt2160->client = client; @@ -389,23 +389,23 @@ static int qt2160_probe(struct i2c_client *client) error = qt2160_write(client, QT2160_CMD_CALIBRATE, 1); if (error) { dev_err(&client->dev, "failed to calibrate device\n"); - goto err_free_mem; + return error; } if (client->irq) { - error = request_irq(client->irq, qt2160_irq, - IRQF_TRIGGER_FALLING, "qt2160", qt2160); + error = devm_request_irq(dev, client->irq, qt2160_irq, + IRQF_TRIGGER_FALLING, "qt2160", qt2160); if (error) { dev_err(&client->dev, "failed to allocate irq %d\n", client->irq); - goto err_free_mem; + return error; } } error = qt2160_register_leds(qt2160); if (error) { dev_err(&client->dev, "Failed to register leds\n"); - goto err_free_irq; + return error; } error = input_register_device(qt2160->input); @@ -422,29 +422,21 @@ static int qt2160_probe(struct i2c_client *client) err_unregister_leds: qt2160_unregister_leds(qt2160); -err_free_irq: - if (client->irq) - free_irq(client->irq, qt2160); -err_free_mem: - input_free_device(input); - kfree(qt2160); return error; } static void qt2160_remove(struct i2c_client *client) { struct qt2160_data *qt2160 = i2c_get_clientdata(client); + struct device *dev = &client->dev; qt2160_unregister_leds(qt2160); /* Release IRQ so no queue will be scheduled */ if (client->irq) - free_irq(client->irq, qt2160); + devm_free_irq(dev, client->irq, qt2160); cancel_delayed_work_sync(&qt2160->dwork); - - input_unregister_device(qt2160->input); - kfree(qt2160); } static const struct i2c_device_id qt2160_idtable[] = {
Use devm_* api to simplify code, this makes it unnecessary to explicitly release resources. Signed-off-by: Yangtao Li <frank.li@vivo.com> --- drivers/input/keyboard/qt2160.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-)