Re: [PATCH] input: touch: ads7846: switch to devm initialization
From: Marco Felsch <hidden>
Date: 2020-05-04 07:34:20
Hi Daniel, thanks for your patch :) On 20-04-30 19:53, Daniel Mack wrote:
This simplies the code a lot and fixes some potential resource leaks in the error return paths. It also ensures the input device is registered before the interrupt is requested, as the IRQ handler will commit events when it fires.
Why is it necessary to get those events during probe()? Pls, see also my inline comments.
quoted hunk ↗ jump to hunk
Signed-off-by: Daniel Mack <daniel@zonque.org> --- drivers/input/touchscreen/ads7846.c | 137 +++++++++------------------- 1 file changed, 45 insertions(+), 92 deletions(-)diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c index 8fd7fc39c4fd..acf736f5ddab 100644 --- a/drivers/input/touchscreen/ads7846.c +++ b/drivers/input/touchscreen/ads7846.c@@ -528,30 +528,19 @@ static int ads784x_hwmon_register(struct spi_device *spi, struct ads7846 *ts) break; } - ts->hwmon = hwmon_device_register_with_groups(&spi->dev, spi->modalias, - ts, ads7846_attr_groups); + ts->hwmon = devm_hwmon_device_register_with_groups(&spi->dev, + spi->modalias, ts, + ads7846_attr_groups);
We don't need the hwmon member anymore if we are switching to the devres intializer. Pls, can you drop it completely?
quoted hunk ↗ jump to hunk
return PTR_ERR_OR_ZERO(ts->hwmon); } -static void ads784x_hwmon_unregister(struct spi_device *spi, - struct ads7846 *ts) -{ - if (ts->hwmon) - hwmon_device_unregister(ts->hwmon); -} - #else static inline int ads784x_hwmon_register(struct spi_device *spi, struct ads7846 *ts) { return 0; } - -static inline void ads784x_hwmon_unregister(struct spi_device *spi, - struct ads7846 *ts) -{ -} #endif static ssize_t ads7846_pen_down_show(struct device *dev,@@ -944,8 +933,8 @@ static int ads7846_setup_pendown(struct spi_device *spi, ts->get_pendown_state = pdata->get_pendown_state; } else if (gpio_is_valid(pdata->gpio_pendown)) { - err = gpio_request_one(pdata->gpio_pendown, GPIOF_IN, - "ads7846_pendown"); + err = devm_gpio_request_one(&spi->dev, pdata->gpio_pendown, + GPIOF_IN, "ads7846_pendown");
I see that you want to keep the changes minimal and I'm fine with this but we should considering to move the driver to gpio_desc.
quoted hunk ↗ jump to hunk
if (err) { dev_err(&spi->dev, "failed to request/setup pendown GPIO%d: %d\n",@@ -1264,6 +1253,7 @@ static const struct ads7846_platform_data *ads7846_probe_dt(struct device *dev) static int ads7846_probe(struct spi_device *spi) { const struct ads7846_platform_data *pdata; + struct device *dev = &spi->dev; struct ads7846 *ts; struct ads7846_packet *packet; struct input_dev *input_dev;@@ -1293,13 +1283,11 @@ static int ads7846_probe(struct spi_device *spi) if (err < 0) return err; - ts = kzalloc(sizeof(struct ads7846), GFP_KERNEL); - packet = kzalloc(sizeof(struct ads7846_packet), GFP_KERNEL); - input_dev = input_allocate_device(); - if (!ts || !packet || !input_dev) { - err = -ENOMEM; - goto err_free_mem; - } + ts = devm_kzalloc(dev, sizeof(struct ads7846), GFP_KERNEL); + packet = devm_kzalloc(dev, sizeof(struct ads7846_packet), GFP_KERNEL); + input_dev = devm_input_allocate_device(dev); + if (!ts || !packet || !input_dev) + return -ENOMEM;
Pls, can we split that so each alloc get its own error check?
quoted hunk ↗ jump to hunk
spi_set_drvdata(spi, ts);@@ -1313,10 +1301,8 @@ static int ads7846_probe(struct spi_device *spi) pdata = dev_get_platdata(&spi->dev); if (!pdata) { pdata = ads7846_probe_dt(&spi->dev); - if (IS_ERR(pdata)) { - err = PTR_ERR(pdata); - goto err_free_mem; - } + if (IS_ERR(pdata)) + return PTR_ERR(pdata); } ts->model = pdata->model ? : 7846;@@ -1328,7 +1314,7 @@ static int ads7846_probe(struct spi_device *spi) if (pdata->filter_init != NULL) { err = pdata->filter_init(pdata, &ts->filter_data); if (err < 0) - goto err_free_mem; + return err; } ts->filter = pdata->filter; ts->filter_cleanup = pdata->filter_cleanup;@@ -1354,12 +1340,12 @@ static int ads7846_probe(struct spi_device *spi) ts->wait_for_sync = pdata->wait_for_sync ? : null_wait_for_sync; - snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi->dev)); + snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(dev)); snprintf(ts->name, sizeof(ts->name), "ADS%d Touchscreen", ts->model); input_dev->name = ts->name; input_dev->phys = ts->phys; - input_dev->dev.parent = &spi->dev; + input_dev->dev.parent = dev;
I would split the dev usage into another patch since it is unrelated to the change you wanna make and keeps the diff smaller.
quoted hunk ↗ jump to hunk
input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS); input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);@@ -1391,94 +1377,80 @@ static int ads7846_probe(struct spi_device *spi) ts->core_prop.swap_x_y = true; } + err = input_register_device(input_dev); + if (err) + goto err_cleanup_filter;
It seems quite common to register the device on the end.
+
ads7846_setup_spi_msg(ts, pdata);
- ts->reg = regulator_get(&spi->dev, "vcc");
+ ts->reg = devm_regulator_get(dev, "vcc");
if (IS_ERR(ts->reg)) {
err = PTR_ERR(ts->reg);
- dev_err(&spi->dev, "unable to get regulator: %d\n", err);
- goto err_free_gpio;
+ dev_err(dev, "unable to get regulator: %d\n", err);
+ goto err_cleanup_filter;
}
err = regulator_enable(ts->reg);
if (err) {
- dev_err(&spi->dev, "unable to enable regulator: %d\n", err);
- goto err_put_regulator;
+ dev_err(dev, "unable to enable regulator: %d\n", err);
+ goto err_cleanup_filter;
}From now on the regulator is on an keeps on since you never turn it of. So you need to add a devm_add_action_or_reset() here.
quoted hunk ↗ jump to hunk
irq_flags = pdata->irq_flags ? : IRQF_TRIGGER_FALLING; irq_flags |= IRQF_ONESHOT; - err = request_threaded_irq(spi->irq, ads7846_hard_irq, ads7846_irq, - irq_flags, spi->dev.driver->name, ts); + err = devm_request_threaded_irq(dev, spi->irq, + ads7846_hard_irq, ads7846_irq, + irq_flags, dev->driver->name, ts); if (err && !pdata->irq_flags) { - dev_info(&spi->dev, + dev_info(dev, "trying pin change workaround on irq %d\n", spi->irq); irq_flags |= IRQF_TRIGGER_RISING; - err = request_threaded_irq(spi->irq, - ads7846_hard_irq, ads7846_irq, - irq_flags, spi->dev.driver->name, ts); + err = devm_request_threaded_irq(dev, spi->irq, + ads7846_hard_irq, ads7846_irq, + irq_flags, dev->driver->name, + ts); } if (err) { - dev_dbg(&spi->dev, "irq %d busy?\n", spi->irq); - goto err_disable_regulator; + dev_dbg(dev, "irq %d busy?\n", spi->irq); + goto err_cleanup_filter; } err = ads784x_hwmon_register(spi, ts); if (err) - goto err_free_irq; + goto err_cleanup_filter; - dev_info(&spi->dev, "touchscreen, irq %d\n", spi->irq); + dev_info(dev, "touchscreen, irq %d\n", spi->irq); /* * Take a first sample, leaving nPENIRQ active and vREF off; avoid * the touchscreen, in case it's not connected. */ if (ts->model == 7845) - ads7845_read12_ser(&spi->dev, PWRDOWN); + ads7845_read12_ser(dev, PWRDOWN); else - (void) ads7846_read12_ser(&spi->dev, READ_12BIT_SER(vaux)); + (void) ads7846_read12_ser(dev, READ_12BIT_SER(vaux)); - err = sysfs_create_group(&spi->dev.kobj, &ads784x_attr_group); + err = sysfs_create_group(&dev->kobj, &ads784x_attr_group); if (err) - goto err_remove_hwmon; - - err = input_register_device(input_dev); - if (err) - goto err_remove_attr_group; + goto err_cleanup_filter; - device_init_wakeup(&spi->dev, pdata->wakeup); + device_init_wakeup(dev, pdata->wakeup); /* * If device does not carry platform data we must have allocated it * when parsing DT data. */ - if (!dev_get_platdata(&spi->dev)) - devm_kfree(&spi->dev, (void *)pdata); + if (!dev_get_platdata(dev)) + devm_kfree(dev, (void *)pdata); return 0; - err_remove_attr_group: - sysfs_remove_group(&spi->dev.kobj, &ads784x_attr_group); - err_remove_hwmon: - ads784x_hwmon_unregister(spi, ts); - err_free_irq: - free_irq(spi->irq, ts); - err_disable_regulator: - regulator_disable(ts->reg); - err_put_regulator: - regulator_put(ts->reg); - err_free_gpio: - if (!ts->get_pendown_state) - gpio_free(ts->gpio_pendown); err_cleanup_filter: if (ts->filter_cleanup) ts->filter_cleanup(ts->filter_data); - err_free_mem: - input_free_device(input_dev); - kfree(packet); - kfree(ts); + return err; }@@ -1487,30 +1459,11 @@ static int ads7846_remove(struct spi_device *spi) struct ads7846 *ts = spi_get_drvdata(spi); sysfs_remove_group(&spi->dev.kobj, &ads784x_attr_group); - ads7846_disable(ts); - free_irq(ts->spi->irq, ts); - - input_unregister_device(ts->input); - - ads784x_hwmon_unregister(spi, ts); - - regulator_put(ts->reg);
Hm.. The current driver code seems a bit strange. This should throw an error if no one en-/disables the device and suspend/resume (PM) is disabled. The above devm_add_action_or_reset() will fix this. Also it seems a bit odd to me to diable the regualtor and the device while it can act as wakeup source but I'm not familar with this touchcontroller. Regards, Marco
-
- if (!ts->get_pendown_state) {
- /*
- * If we are not using specialized pendown method we must
- * have been relying on gpio we set up ourselves.
- */
- gpio_free(ts->gpio_pendown);
- }
if (ts->filter_cleanup)
ts->filter_cleanup(ts->filter_data);
- kfree(ts->packet);
- kfree(ts);
-
dev_dbg(&spi->dev, "unregistered touchscreen\n");
return 0;
--
2.25.2