* [PATCH v2 2/3] input: touch: ads7846: switch to devm initialization
2020-05-04 17:37 [PATCH v2 1/3] input: touch: ads7846: add short-hand for spi->dev in probe() function Daniel Mack
@ 2020-05-04 17:37 ` Daniel Mack
2020-05-05 8:37 ` Marco Felsch
2020-05-04 17:37 ` [PATCH v2 3/3] input: touchscreen: ads7846: switch to gpiod API Daniel Mack
2020-05-05 8:19 ` [PATCH v2 1/3] input: touch: ads7846: add short-hand for spi->dev in probe() function Marco Felsch
2 siblings, 1 reply; 9+ messages in thread
From: Daniel Mack @ 2020-05-04 17:37 UTC (permalink / raw)
To: linux-input; +Cc: dmitry.torokhov, m.felsch, Daniel Mack
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.
Signed-off-by: Daniel Mack <daniel@zonque.org>
---
drivers/input/touchscreen/ads7846.c | 127 ++++++++++------------------
1 file changed, 46 insertions(+), 81 deletions(-)
diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index a1033b06f031..7f4ead542a73 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -98,10 +98,6 @@ struct ads7846 {
struct spi_device *spi;
struct regulator *reg;
-#if IS_ENABLED(CONFIG_HWMON)
- struct device *hwmon;
-#endif
-
u16 model;
u16 vref_mv;
u16 vref_delay_usecs;
@@ -508,6 +504,8 @@ __ATTRIBUTE_GROUPS(ads7846_attr);
static int ads784x_hwmon_register(struct spi_device *spi, struct ads7846 *ts)
{
+ struct device *hwmon;
+
/* hwmon sensors need a reference voltage */
switch (ts->model) {
case 7846:
@@ -528,17 +526,11 @@ 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);
-
- return PTR_ERR_OR_ZERO(ts->hwmon);
-}
+ hwmon = devm_hwmon_device_register_with_groups(&spi->dev,
+ spi->modalias, ts,
+ ads7846_attr_groups);
-static void ads784x_hwmon_unregister(struct spi_device *spi,
- struct ads7846 *ts)
-{
- if (ts->hwmon)
- hwmon_device_unregister(ts->hwmon);
+ return PTR_ERR_OR_ZERO(hwmon);
}
#else
@@ -547,11 +539,6 @@ static inline int ads784x_hwmon_register(struct spi_device *spi,
{
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 +931,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");
if (err) {
dev_err(&spi->dev,
"failed to request/setup pendown GPIO%d: %d\n",
@@ -1261,6 +1248,11 @@ static const struct ads7846_platform_data *ads7846_probe_dt(struct device *dev)
}
#endif
+static void ads7846_regulator_disable(void *regulator)
+{
+ regulator_disable(regulator);
+}
+
static int ads7846_probe(struct spi_device *spi)
{
const struct ads7846_platform_data *pdata;
@@ -1294,13 +1286,17 @@ 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);
+ if (!ts)
+ return -ENOMEM;
+
+ packet = devm_kzalloc(dev, sizeof(struct ads7846_packet), GFP_KERNEL);
+ if (!packet)
+ return -ENOMEM;
+
+ input_dev = devm_input_allocate_device(dev);
+ if (!input_dev)
+ return -ENOMEM;
spi_set_drvdata(spi, ts);
@@ -1314,10 +1310,8 @@ static int ads7846_probe(struct spi_device *spi)
pdata = dev_get_platdata(dev);
if (!pdata) {
pdata = ads7846_probe_dt(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;
@@ -1329,7 +1323,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;
@@ -1394,41 +1388,47 @@ static int ads7846_probe(struct spi_device *spi)
ads7846_setup_spi_msg(ts, pdata);
- ts->reg = regulator_get(dev, "vcc");
+ ts->reg = devm_regulator_get(dev, "vcc");
if (IS_ERR(ts->reg)) {
err = PTR_ERR(ts->reg);
dev_err(dev, "unable to get regulator: %d\n", err);
- goto err_free_gpio;
+ goto err_cleanup_filter;
}
err = regulator_enable(ts->reg);
if (err) {
dev_err(dev, "unable to enable regulator: %d\n", err);
- goto err_put_regulator;
+ goto err_cleanup_filter;
}
+ err = devm_add_action_or_reset(dev, ads7846_regulator_disable, ts->reg);
+ if (err)
+ return err;
+
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, 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(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, 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(dev, "irq %d busy?\n", spi->irq);
- goto err_disable_regulator;
+ goto err_cleanup_filter;
}
err = ads784x_hwmon_register(spi, ts);
if (err)
- goto err_free_irq;
+ goto err_cleanup_filter;
dev_info(dev, "touchscreen, irq %d\n", spi->irq);
@@ -1443,11 +1443,11 @@ static int ads7846_probe(struct spi_device *spi)
err = sysfs_create_group(&dev->kobj, &ads784x_attr_group);
if (err)
- goto err_remove_hwmon;
+ goto err_cleanup_filter;
err = input_register_device(input_dev);
if (err)
- goto err_remove_attr_group;
+ goto err_cleanup_filter;
device_init_wakeup(dev, pdata->wakeup);
@@ -1460,26 +1460,10 @@ static int ads7846_probe(struct spi_device *spi)
return 0;
- err_remove_attr_group:
- sysfs_remove_group(&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;
}
@@ -1488,30 +1472,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);
-
- 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.26.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/3] input: touch: ads7846: switch to devm initialization
2020-05-04 17:37 ` [PATCH v2 2/3] input: touch: ads7846: switch to devm initialization Daniel Mack
@ 2020-05-05 8:37 ` Marco Felsch
2020-05-06 0:05 ` Dmitry Torokhov
2020-05-06 6:54 ` Daniel Mack
0 siblings, 2 replies; 9+ messages in thread
From: Marco Felsch @ 2020-05-05 8:37 UTC (permalink / raw)
To: Daniel Mack; +Cc: linux-input, dmitry.torokhov
Hi Daniel,
also here, pls align the patch subject with the existing ones, so the
prefix should be "Input: ads7846 - ".
On 20-05-04 19:37, 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.
Pls adapt the commit message too.
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
It is common to add a changelog here so reviewers can see what you have
done in this version.
> drivers/input/touchscreen/ads7846.c | 127 ++++++++++------------------
> 1 file changed, 46 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index a1033b06f031..7f4ead542a73 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -98,10 +98,6 @@ struct ads7846 {
> struct spi_device *spi;
> struct regulator *reg;
>
> -#if IS_ENABLED(CONFIG_HWMON)
> - struct device *hwmon;
> -#endif
> -
> u16 model;
> u16 vref_mv;
> u16 vref_delay_usecs;
> @@ -508,6 +504,8 @@ __ATTRIBUTE_GROUPS(ads7846_attr);
>
> static int ads784x_hwmon_register(struct spi_device *spi, struct ads7846 *ts)
> {
> + struct device *hwmon;
> +
> /* hwmon sensors need a reference voltage */
> switch (ts->model) {
> case 7846:
> @@ -528,17 +526,11 @@ 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);
> -
> - return PTR_ERR_OR_ZERO(ts->hwmon);
> -}
> + hwmon = devm_hwmon_device_register_with_groups(&spi->dev,
> + spi->modalias, ts,
> + ads7846_attr_groups);
>
> -static void ads784x_hwmon_unregister(struct spi_device *spi,
> - struct ads7846 *ts)
> -{
> - if (ts->hwmon)
> - hwmon_device_unregister(ts->hwmon);
> + return PTR_ERR_OR_ZERO(hwmon);
> }
Nit:
return PTR_ERR_OR_ZERO(devm_hwmon_device_register_with_groups())
> #else
> @@ -547,11 +539,6 @@ static inline int ads784x_hwmon_register(struct spi_device *spi,
> {
> 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 +931,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");
> if (err) {
> dev_err(&spi->dev,
> "failed to request/setup pendown GPIO%d: %d\n",
> @@ -1261,6 +1248,11 @@ static const struct ads7846_platform_data *ads7846_probe_dt(struct device *dev)
> }
> #endif
>
> +static void ads7846_regulator_disable(void *regulator)
> +{
> + regulator_disable(regulator);
> +}
> +
> static int ads7846_probe(struct spi_device *spi)
> {
> const struct ads7846_platform_data *pdata;
> @@ -1294,13 +1286,17 @@ 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);
> + if (!ts)
> + return -ENOMEM;
> +
> + packet = devm_kzalloc(dev, sizeof(struct ads7846_packet), GFP_KERNEL);
> + if (!packet)
> + return -ENOMEM;
> +
> + input_dev = devm_input_allocate_device(dev);
> + if (!input_dev)
> + return -ENOMEM;
>
> spi_set_drvdata(spi, ts);
>
> @@ -1314,10 +1310,8 @@ static int ads7846_probe(struct spi_device *spi)
> pdata = dev_get_platdata(dev);
> if (!pdata) {
> pdata = ads7846_probe_dt(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;
> @@ -1329,7 +1323,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;
> @@ -1394,41 +1388,47 @@ static int ads7846_probe(struct spi_device *spi)
>
> ads7846_setup_spi_msg(ts, pdata);
>
> - ts->reg = regulator_get(dev, "vcc");
> + ts->reg = devm_regulator_get(dev, "vcc");
> if (IS_ERR(ts->reg)) {
> err = PTR_ERR(ts->reg);
> dev_err(dev, "unable to get regulator: %d\n", err);
> - goto err_free_gpio;
> + goto err_cleanup_filter;
> }
>
> err = regulator_enable(ts->reg);
> if (err) {
> dev_err(dev, "unable to enable regulator: %d\n", err);
> - goto err_put_regulator;
> + goto err_cleanup_filter;
> }
>
> + err = devm_add_action_or_reset(dev, ads7846_regulator_disable, ts->reg);
> + if (err)
> + return err;
Nope, either you use the "goto err_cleanup_filter" here or you add a 2nd
action for the filter cleanup.
> +
> 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, 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(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, 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(dev, "irq %d busy?\n", spi->irq);
> - goto err_disable_regulator;
> + goto err_cleanup_filter;
> }
>
> err = ads784x_hwmon_register(spi, ts);
> if (err)
> - goto err_free_irq;
> + goto err_cleanup_filter;
>
> dev_info(dev, "touchscreen, irq %d\n", spi->irq);
>
> @@ -1443,11 +1443,11 @@ static int ads7846_probe(struct spi_device *spi)
>
> err = sysfs_create_group(&dev->kobj, &ads784x_attr_group);
> if (err)
> - goto err_remove_hwmon;
> + goto err_cleanup_filter;
>
> err = input_register_device(input_dev);
> if (err)
> - goto err_remove_attr_group;
> + goto err_cleanup_filter;
Nope..
>
> device_init_wakeup(dev, pdata->wakeup);
>
> @@ -1460,26 +1460,10 @@ static int ads7846_probe(struct spi_device *spi)
>
> return 0;
>
> - err_remove_attr_group:
> - sysfs_remove_group(&dev->kobj, &ads784x_attr_group);
this must still be handled.
Regards,
Marco
> - 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;
> }
>
> @@ -1488,30 +1472,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);
> -
> - 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.26.2
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/3] input: touch: ads7846: switch to devm initialization
2020-05-05 8:37 ` Marco Felsch
@ 2020-05-06 0:05 ` Dmitry Torokhov
2020-05-06 6:38 ` Daniel Mack
2020-05-06 6:54 ` Daniel Mack
1 sibling, 1 reply; 9+ messages in thread
From: Dmitry Torokhov @ 2020-05-06 0:05 UTC (permalink / raw)
To: Marco Felsch; +Cc: Daniel Mack, linux-input
On Tue, May 05, 2020 at 10:37:01AM +0200, Marco Felsch wrote:
> On 20-05-04 19:37, Daniel Mack wrote:
> > @@ -1488,30 +1472,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);
> > -
> > - 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);
This makes filter_cleanup() be called much earlier now, before we free
interrupt, unregister input device, etc.
I am very concerned with mixing manual unwinding and devm and would
very much prefer if everything would be converted to devm.
> >
> > - kfree(ts->packet);
> > - kfree(ts);
> > -
> > dev_dbg(&spi->dev, "unregistered touchscreen\n");
> >
> > return 0;
> > --
> > 2.26.2
> >
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/3] input: touch: ads7846: switch to devm initialization
2020-05-06 0:05 ` Dmitry Torokhov
@ 2020-05-06 6:38 ` Daniel Mack
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Mack @ 2020-05-06 6:38 UTC (permalink / raw)
To: Dmitry Torokhov, Marco Felsch; +Cc: linux-input
Hi Dmitry,
Thanks for having a look!
On 5/6/20 2:05 AM, Dmitry Torokhov wrote:
> On Tue, May 05, 2020 at 10:37:01AM +0200, Marco Felsch wrote:
>> On 20-05-04 19:37, Daniel Mack wrote:
>>> @@ -1488,30 +1472,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);
>>> -
>>> - 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);
>
> This makes filter_cleanup() be called much earlier now, before we free
> interrupt, unregister input device, etc.
>
> I am very concerned with mixing manual unwinding and devm and would
> very much prefer if everything would be converted to devm.
Yes, I see. These filter_init()/filter_cleanup() functions can
potentially be set by pdata users, hence I thought it's easiest if we
keep it as-is.
However, turns out this pdata logic has no active user in mainline, so I
can just add a patch to remove it completely and simplify the code further.
Will pick up yours and Marco's comments and put them in a v3.
Thanks!
Daniel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] input: touch: ads7846: switch to devm initialization
2020-05-05 8:37 ` Marco Felsch
2020-05-06 0:05 ` Dmitry Torokhov
@ 2020-05-06 6:54 ` Daniel Mack
1 sibling, 0 replies; 9+ messages in thread
From: Daniel Mack @ 2020-05-06 6:54 UTC (permalink / raw)
To: Marco Felsch; +Cc: linux-input, dmitry.torokhov
Hi Marco,
On 5/5/20 10:37 AM, Marco Felsch wrote:
> also here, pls align the patch subject with the existing ones, so the
> prefix should be "Input: ads7846 - ".
>
> On 20-05-04 19:37, 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.
>
> Pls adapt the commit message too.
>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> -static void ads784x_hwmon_unregister(struct spi_device *spi,
>> - struct ads7846 *ts)
>> -{
>> - if (ts->hwmon)
>> - hwmon_device_unregister(ts->hwmon);
>> + return PTR_ERR_OR_ZERO(hwmon);
>> }
>
> Nit:
> return PTR_ERR_OR_ZERO(devm_hwmon_device_register_with_groups())
I tried that before, but it looks ugly and breaks the coding style
rules. So I'll leave that one out.
The rest of your comments I agree with. Thanks again!
Daniel
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] input: touchscreen: ads7846: switch to gpiod API
2020-05-04 17:37 [PATCH v2 1/3] input: touch: ads7846: add short-hand for spi->dev in probe() function Daniel Mack
2020-05-04 17:37 ` [PATCH v2 2/3] input: touch: ads7846: switch to devm initialization Daniel Mack
@ 2020-05-04 17:37 ` Daniel Mack
2020-05-05 9:16 ` Marco Felsch
2020-05-05 8:19 ` [PATCH v2 1/3] input: touch: ads7846: add short-hand for spi->dev in probe() function Marco Felsch
2 siblings, 1 reply; 9+ messages in thread
From: Daniel Mack @ 2020-05-04 17:37 UTC (permalink / raw)
To: linux-input; +Cc: dmitry.torokhov, m.felsch, Daniel Mack
Use gpiod_* function to access the pendown GPIO line.
Signed-off-by: Daniel Mack <daniel@zonque.org>
---
drivers/input/touchscreen/ads7846.c | 53 ++++++++++++++++-------------
1 file changed, 30 insertions(+), 23 deletions(-)
diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 7f4ead542a73..b3e17ee4e499 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -27,7 +27,7 @@
#include <linux/of.h>
#include <linux/of_gpio.h>
#include <linux/of_device.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/spi/spi.h>
#include <linux/spi/ads7846.h>
#include <linux/regulator/consumer.h>
@@ -137,7 +137,7 @@ struct ads7846 {
void *filter_data;
void (*filter_cleanup)(void *data);
int (*get_pendown_state)(void);
- int gpio_pendown;
+ struct gpio_desc *gpio_pendown;
void (*wait_for_sync)(void);
};
@@ -598,7 +598,7 @@ static int get_pendown_state(struct ads7846 *ts)
if (ts->get_pendown_state)
return ts->get_pendown_state();
- return !gpio_get_value(ts->gpio_pendown);
+ return !gpiod_get_value(ts->gpio_pendown);
}
static void null_wait_for_sync(void)
@@ -919,6 +919,7 @@ static int ads7846_setup_pendown(struct spi_device *spi,
struct ads7846 *ts,
const struct ads7846_platform_data *pdata)
{
+ struct device *dev = &spi->dev;
int err;
/*
@@ -929,27 +930,33 @@ static int ads7846_setup_pendown(struct spi_device *spi,
if (pdata->get_pendown_state) {
ts->get_pendown_state = pdata->get_pendown_state;
- } else if (gpio_is_valid(pdata->gpio_pendown)) {
-
- err = devm_gpio_request_one(&spi->dev, pdata->gpio_pendown,
- GPIOF_IN, "ads7846_pendown");
- if (err) {
- dev_err(&spi->dev,
- "failed to request/setup pendown GPIO%d: %d\n",
- pdata->gpio_pendown, err);
- return err;
- }
+ return 0;
+ }
- ts->gpio_pendown = pdata->gpio_pendown;
+ ts->gpio_pendown = devm_gpiod_get(dev, "pendown", GPIOD_IN);
+ if (IS_ERR(ts->gpio_pendown)) {
+ err = PTR_ERR(ts->gpio_pendown);
- if (pdata->gpio_pendown_debounce)
- gpio_set_debounce(pdata->gpio_pendown,
- pdata->gpio_pendown_debounce);
- } else {
- dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
- return -EINVAL;
+ if (gpio_is_valid(pdata->gpio_pendown)) {
+ err = devm_gpio_request_one(dev, pdata->gpio_pendown,
+ GPIOF_IN,
+ "ads7846_pendown");
+ if (err < 0)
+ return err;
+
+ ts->gpio_pendown = gpio_to_desc(pdata->gpio_pendown);
+ if (!ts->gpio_pendown)
+ return -EINVAL;
+ }
+
+ if (err < 0)
+ return err;
}
+ if (pdata->gpio_pendown_debounce)
+ gpiod_set_debounce(ts->gpio_pendown,
+ pdata->gpio_pendown_debounce);
+
return 0;
}
@@ -1236,8 +1243,6 @@ static const struct ads7846_platform_data *ads7846_probe_dt(struct device *dev)
pdata->wakeup = of_property_read_bool(node, "wakeup-source") ||
of_property_read_bool(node, "linux,wakeup");
- pdata->gpio_pendown = of_get_named_gpio(dev->of_node, "pendown-gpio", 0);
-
return pdata;
}
#else
@@ -1340,8 +1345,10 @@ static int ads7846_probe(struct spi_device *spi)
}
err = ads7846_setup_pendown(spi, ts, pdata);
- if (err)
+ if (err) {
+ dev_err(dev, "Unable to request pendown GPIO: %d", err);
goto err_cleanup_filter;
+ }
if (pdata->penirq_recheck_delay_usecs)
ts->penirq_recheck_delay_usecs =
--
2.26.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 3/3] input: touchscreen: ads7846: switch to gpiod API
2020-05-04 17:37 ` [PATCH v2 3/3] input: touchscreen: ads7846: switch to gpiod API Daniel Mack
@ 2020-05-05 9:16 ` Marco Felsch
0 siblings, 0 replies; 9+ messages in thread
From: Marco Felsch @ 2020-05-05 9:16 UTC (permalink / raw)
To: Daniel Mack; +Cc: linux-input, dmitry.torokhov
Hi Daniel,
thanks for picking my suggestion and converting the driver :)
Unfortunately it is not that easy since we need to care about the
existing users. You will get the legacy users if you grep for
ads7846_platform_data. Those platforms needs to converted to.
Again, pls align the commit subject prefix with the existing patches.
On 20-05-04 19:37, Daniel Mack wrote:
> Use gpiod_* function to access the pendown GPIO line.
Maybe this is better:
Input: ads7846 - Convert to GPIO descriptors
This converts the ads7846 touchscreen driver to use GPIO
descriptors and switches all platform data boards over
to passing descriptors instead of global GPIO numbers.
We use the "pendown-gpio" name as found in the device
tree bindings for this touchscreen driver.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
> drivers/input/touchscreen/ads7846.c | 53 ++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index 7f4ead542a73..b3e17ee4e499 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -27,7 +27,7 @@
> #include <linux/of.h>
> #include <linux/of_gpio.h>
> #include <linux/of_device.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/spi/spi.h>
> #include <linux/spi/ads7846.h>
> #include <linux/regulator/consumer.h>
> @@ -137,7 +137,7 @@ struct ads7846 {
> void *filter_data;
> void (*filter_cleanup)(void *data);
> int (*get_pendown_state)(void);
> - int gpio_pendown;
> + struct gpio_desc *gpio_pendown;
>
> void (*wait_for_sync)(void);
> };
> @@ -598,7 +598,7 @@ static int get_pendown_state(struct ads7846 *ts)
> if (ts->get_pendown_state)
> return ts->get_pendown_state();
>
> - return !gpio_get_value(ts->gpio_pendown);
> + return !gpiod_get_value(ts->gpio_pendown);
The gpio_get_value() function return the raw value and the ! before
inverts the logic. Thanks to gpiod we don't need to care about this
inverting logic anymore.
> }
>
> static void null_wait_for_sync(void)
> @@ -919,6 +919,7 @@ static int ads7846_setup_pendown(struct spi_device *spi,
> struct ads7846 *ts,
> const struct ads7846_platform_data *pdata)
> {
> + struct device *dev = &spi->dev;
> int err;
>
> /*
> @@ -929,27 +930,33 @@ static int ads7846_setup_pendown(struct spi_device *spi,
>
> if (pdata->get_pendown_state) {
> ts->get_pendown_state = pdata->get_pendown_state;
> - } else if (gpio_is_valid(pdata->gpio_pendown)) {
> -
> - err = devm_gpio_request_one(&spi->dev, pdata->gpio_pendown,
> - GPIOF_IN, "ads7846_pendown");
> - if (err) {
> - dev_err(&spi->dev,
> - "failed to request/setup pendown GPIO%d: %d\n",
> - pdata->gpio_pendown, err);
> - return err;
> - }
> + return 0;
> + }
>
> - ts->gpio_pendown = pdata->gpio_pendown;
> + ts->gpio_pendown = devm_gpiod_get(dev, "pendown", GPIOD_IN);
> + if (IS_ERR(ts->gpio_pendown)) {
> + err = PTR_ERR(ts->gpio_pendown);
>
> - if (pdata->gpio_pendown_debounce)
> - gpio_set_debounce(pdata->gpio_pendown,
> - pdata->gpio_pendown_debounce);
> - } else {
> - dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
> - return -EINVAL;
> + if (gpio_is_valid(pdata->gpio_pendown)) {
> + err = devm_gpio_request_one(dev, pdata->gpio_pendown,
> + GPIOF_IN,
> + "ads7846_pendown");
> + if (err < 0)
> + return err;
> +
> + ts->gpio_pendown = gpio_to_desc(pdata->gpio_pendown);
> + if (!ts->gpio_pendown)
> + return -EINVAL;
> + }
> +
> + if (err < 0)
> + return err;
> }
>
> + if (pdata->gpio_pendown_debounce)
> + gpiod_set_debounce(ts->gpio_pendown,
> + pdata->gpio_pendown_debounce);
> +
> return 0;
> }
>
> @@ -1236,8 +1243,6 @@ static const struct ads7846_platform_data *ads7846_probe_dt(struct device *dev)
> pdata->wakeup = of_property_read_bool(node, "wakeup-source") ||
> of_property_read_bool(node, "linux,wakeup");
>
> - pdata->gpio_pendown = of_get_named_gpio(dev->of_node, "pendown-gpio", 0);
> -
> return pdata;
> }
It's a bit hard to follow the changes within ads7846_setup_pendown(). To
make it easier and IMHO cleaner to everyone we should convert the
ads7846_platform_data to gpio_desc too and request the gpio within the
ads7846_probe_dt() function. Pls take a look on this patchset [1] from
Linus. There you will get all informations who platform data based
machines can be changed usign gpiod_lookup_table's.
[1] https://patches.linaro.org/patch/185481/
Thanks for working on this =)
Regards,
Marco
> #else
> @@ -1340,8 +1345,10 @@ static int ads7846_probe(struct spi_device *spi)
> }
>
> err = ads7846_setup_pendown(spi, ts, pdata);
> - if (err)
> + if (err) {
> + dev_err(dev, "Unable to request pendown GPIO: %d", err);
> goto err_cleanup_filter;
> + }
>
> if (pdata->penirq_recheck_delay_usecs)
> ts->penirq_recheck_delay_usecs =
> --
> 2.26.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] input: touch: ads7846: add short-hand for spi->dev in probe() function
2020-05-04 17:37 [PATCH v2 1/3] input: touch: ads7846: add short-hand for spi->dev in probe() function Daniel Mack
2020-05-04 17:37 ` [PATCH v2 2/3] input: touch: ads7846: switch to devm initialization Daniel Mack
2020-05-04 17:37 ` [PATCH v2 3/3] input: touchscreen: ads7846: switch to gpiod API Daniel Mack
@ 2020-05-05 8:19 ` Marco Felsch
2 siblings, 0 replies; 9+ messages in thread
From: Marco Felsch @ 2020-05-05 8:19 UTC (permalink / raw)
To: Daniel Mack; +Cc: linux-input, dmitry.torokhov
Hi Daniel,
thanks for splitting it up :) The patch subject should start with:
"Input: ads7846 - " to align it with the current patches.
On 20-05-04 19:37, Daniel Mack wrote:
> This will make the code a bit more terse.
> No functional change intended.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
> drivers/input/touchscreen/ads7846.c | 45 +++++++++++++++--------------
> 1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index 8fd7fc39c4fd..a1033b06f031 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -1265,20 +1265,21 @@ static int ads7846_probe(struct spi_device *spi)
> {
> const struct ads7846_platform_data *pdata;
> struct ads7846 *ts;
> + struct device *dev = &spi->dev;
> struct ads7846_packet *packet;
> struct input_dev *input_dev;
> unsigned long irq_flags;
> int err;
>
> if (!spi->irq) {
> - dev_dbg(&spi->dev, "no IRQ?\n");
> + dev_dbg(dev, "no IRQ?\n");
> return -EINVAL;
> }
>
> /* don't exceed max specified sample rate */
> if (spi->max_speed_hz > (125000 * SAMPLE_BITS)) {
> - dev_err(&spi->dev, "f(sample) %d KHz?\n",
> - (spi->max_speed_hz/SAMPLE_BITS)/1000);
> + dev_err(dev, "f(sample) %d KHz?\n",
> + (spi->max_speed_hz/SAMPLE_BITS)/1000);
This should be aligned correctly.
Appart of these two nits feel free to add my:
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
Regards,
Marco
> return -EINVAL;
> }
>
> @@ -1310,9 +1311,9 @@ static int ads7846_probe(struct spi_device *spi)
> mutex_init(&ts->lock);
> init_waitqueue_head(&ts->wait);
>
> - pdata = dev_get_platdata(&spi->dev);
> + pdata = dev_get_platdata(dev);
> if (!pdata) {
> - pdata = ads7846_probe_dt(&spi->dev);
> + pdata = ads7846_probe_dt(dev);
> if (IS_ERR(pdata)) {
> err = PTR_ERR(pdata);
> goto err_free_mem;
> @@ -1354,12 +1355,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;
>
> input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> @@ -1393,16 +1394,16 @@ static int ads7846_probe(struct spi_device *spi)
>
> ads7846_setup_spi_msg(ts, pdata);
>
> - ts->reg = regulator_get(&spi->dev, "vcc");
> + ts->reg = 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);
> + dev_err(dev, "unable to get regulator: %d\n", err);
> goto err_free_gpio;
> }
>
> err = regulator_enable(ts->reg);
> if (err) {
> - dev_err(&spi->dev, "unable to enable regulator: %d\n", err);
> + dev_err(dev, "unable to enable regulator: %d\n", err);
> goto err_put_regulator;
> }
>
> @@ -1410,18 +1411,18 @@ static int ads7846_probe(struct spi_device *spi)
> irq_flags |= IRQF_ONESHOT;
>
> err = request_threaded_irq(spi->irq, ads7846_hard_irq, ads7846_irq,
> - irq_flags, spi->dev.driver->name, ts);
> + 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);
> + irq_flags, dev->driver->name, ts);
> }
>
> if (err) {
> - dev_dbg(&spi->dev, "irq %d busy?\n", spi->irq);
> + dev_dbg(dev, "irq %d busy?\n", spi->irq);
> goto err_disable_regulator;
> }
>
> @@ -1429,18 +1430,18 @@ static int ads7846_probe(struct spi_device *spi)
> if (err)
> goto err_free_irq;
>
> - 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;
>
> @@ -1448,19 +1449,19 @@ static int ads7846_probe(struct spi_device *spi)
> if (err)
> goto err_remove_attr_group;
>
> - 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);
> + sysfs_remove_group(&dev->kobj, &ads784x_attr_group);
> err_remove_hwmon:
> ads784x_hwmon_unregister(spi, ts);
> err_free_irq:
> --
> 2.26.2
^ permalink raw reply [flat|nested] 9+ messages in thread