[PATCH v3 0/3] Input: ads7846: pdata cleanups and devm init

[PATCH v3 0/3] Input: ads7846: pdata cleanups and devm init

[Daniel Mack]

Hi,

This is v3 of the patches to bring devm initialization to the ads7846
driver. I left the gpiod conversion patch out for now as it needs more
work, and it's also independent of the other changes.

v3:

* Added a patch to remove custom filter handling from pdata
* Added devm_add_action_or_reset() for regulator state maintaining
* Addressed minor nits pointed out by Marco Felsch

Daniel Mack (3):
  Input: ads7846: Add short-hand for spi->dev in probe() function
  Input: ads7846: Remove custom filter handling functions from pdata
  Input: ads7846: Switch to devm initialization

 drivers/input/touchscreen/ads7846.c | 185 +++++++++++-----------------
 include/linux/spi/ads7846.h         |  15 ---
 2 files changed, 72 insertions(+), 128 deletions(-)

-- 
2.26.2

[PATCH v3 1/3] Input: ads7846: Add short-hand for spi->dev in probe() function

[Daniel Mack]

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..0fd0037ef226 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);
 		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

[PATCH v3 2/3] Input: ads7846: remove custom filter handling functions from pdata

[Daniel Mack]

The functions in the platform data struct to initialize, cleanup and
apply custom filters are not in use by any mainline board.

Remove support for them to pave the road for more cleanups to come.

The enum was moved as it has no users outside of the driver code
itself.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/input/touchscreen/ads7846.c | 25 ++++++++-----------------
 include/linux/spi/ads7846.h         | 15 ---------------
 2 files changed, 8 insertions(+), 32 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 0fd0037ef226..4635e8867d10 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -139,13 +139,18 @@ struct ads7846 {
 
 	int			(*filter)(void *data, int data_idx, int *val);
 	void			*filter_data;
-	void			(*filter_cleanup)(void *data);
 	int			(*get_pendown_state)(void);
 	int			gpio_pendown;
 
 	void			(*wait_for_sync)(void);
 };
 
+enum ads7846_filter {
+	ADS7846_FILTER_OK,
+	ADS7846_FILTER_REPEAT,
+	ADS7846_FILTER_IGNORE,
+};
+
 /* leave chip selected when we're done, for quicker re-select? */
 #if	0
 #define	CS_CHANGE(xfer)	((xfer).cs_change = 1)
@@ -1325,15 +1330,7 @@ static int ads7846_probe(struct spi_device *spi)
 	ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
 	ts->vref_mv = pdata->vref_mv;
 
-	if (pdata->filter != NULL) {
-		if (pdata->filter_init != NULL) {
-			err = pdata->filter_init(pdata, &ts->filter_data);
-			if (err < 0)
-				goto err_free_mem;
-		}
-		ts->filter = pdata->filter;
-		ts->filter_cleanup = pdata->filter_cleanup;
-	} else if (pdata->debounce_max) {
+	if (pdata->debounce_max) {
 		ts->debounce_max = pdata->debounce_max;
 		if (ts->debounce_max < 2)
 			ts->debounce_max = 2;
@@ -1347,7 +1344,7 @@ static int ads7846_probe(struct spi_device *spi)
 
 	err = ads7846_setup_pendown(spi, ts, pdata);
 	if (err)
-		goto err_cleanup_filter;
+		goto err_free_mem;
 
 	if (pdata->penirq_recheck_delay_usecs)
 		ts->penirq_recheck_delay_usecs =
@@ -1473,9 +1470,6 @@ static int ads7846_probe(struct spi_device *spi)
  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);
@@ -1506,9 +1500,6 @@ static int ads7846_remove(struct spi_device *spi)
 		gpio_free(ts->gpio_pendown);
 	}
 
-	if (ts->filter_cleanup)
-		ts->filter_cleanup(ts->filter_data);
-
 	kfree(ts->packet);
 	kfree(ts);
 
diff --git a/include/linux/spi/ads7846.h b/include/linux/spi/ads7846.h
index 1a5eaef3b7f2..d424c1aadf38 100644
--- a/include/linux/spi/ads7846.h
+++ b/include/linux/spi/ads7846.h
@@ -1,17 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* linux/spi/ads7846.h */
 
-/* Touchscreen characteristics vary between boards and models.  The
- * platform_data for the device's "struct device" holds this information.
- *
- * It's OK if the min/max values are zero.
- */
-enum ads7846_filter {
-	ADS7846_FILTER_OK,
-	ADS7846_FILTER_REPEAT,
-	ADS7846_FILTER_IGNORE,
-};
-
 struct ads7846_platform_data {
 	u16	model;			/* 7843, 7845, 7846, 7873. */
 	u16	vref_delay_usecs;	/* 0 for external vref; etc */
@@ -51,10 +40,6 @@ struct ads7846_platform_data {
 	int	gpio_pendown_debounce;	/* platform specific debounce time for
 					 * the gpio_pendown */
 	int	(*get_pendown_state)(void);
-	int	(*filter_init)	(const struct ads7846_platform_data *pdata,
-				 void **filter_data);
-	int	(*filter)	(void *filter_data, int data_idx, int *val);
-	void	(*filter_cleanup)(void *filter_data);
 	void	(*wait_for_sync)(void);
 	bool	wakeup;
 	unsigned long irq_flags;
-- 
2.26.2

[PATCH v3 2/3] Input: ads7846: Remove custom filter handling functions from pdata

[Daniel Mack]

The functions in the platform data struct to initialize, cleanup and
apply custom filters are not in use by any mainline board.

Remove support for them to pave the road for more cleanups to come.

The enum was moved as it has no users outside of the driver code
itself.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/input/touchscreen/ads7846.c | 25 ++++++++-----------------
 include/linux/spi/ads7846.h         | 15 ---------------
 2 files changed, 8 insertions(+), 32 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 0fd0037ef226..4635e8867d10 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -139,13 +139,18 @@ struct ads7846 {
 
 	int			(*filter)(void *data, int data_idx, int *val);
 	void			*filter_data;
-	void			(*filter_cleanup)(void *data);
 	int			(*get_pendown_state)(void);
 	int			gpio_pendown;
 
 	void			(*wait_for_sync)(void);
 };
 
+enum ads7846_filter {
+	ADS7846_FILTER_OK,
+	ADS7846_FILTER_REPEAT,
+	ADS7846_FILTER_IGNORE,
+};
+
 /* leave chip selected when we're done, for quicker re-select? */
 #if	0
 #define	CS_CHANGE(xfer)	((xfer).cs_change = 1)
@@ -1325,15 +1330,7 @@ static int ads7846_probe(struct spi_device *spi)
 	ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
 	ts->vref_mv = pdata->vref_mv;
 
-	if (pdata->filter != NULL) {
-		if (pdata->filter_init != NULL) {
-			err = pdata->filter_init(pdata, &ts->filter_data);
-			if (err < 0)
-				goto err_free_mem;
-		}
-		ts->filter = pdata->filter;
-		ts->filter_cleanup = pdata->filter_cleanup;
-	} else if (pdata->debounce_max) {
+	if (pdata->debounce_max) {
 		ts->debounce_max = pdata->debounce_max;
 		if (ts->debounce_max < 2)
 			ts->debounce_max = 2;
@@ -1347,7 +1344,7 @@ static int ads7846_probe(struct spi_device *spi)
 
 	err = ads7846_setup_pendown(spi, ts, pdata);
 	if (err)
-		goto err_cleanup_filter;
+		goto err_free_mem;
 
 	if (pdata->penirq_recheck_delay_usecs)
 		ts->penirq_recheck_delay_usecs =
@@ -1473,9 +1470,6 @@ static int ads7846_probe(struct spi_device *spi)
  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);
@@ -1506,9 +1500,6 @@ static int ads7846_remove(struct spi_device *spi)
 		gpio_free(ts->gpio_pendown);
 	}
 
-	if (ts->filter_cleanup)
-		ts->filter_cleanup(ts->filter_data);
-
 	kfree(ts->packet);
 	kfree(ts);
 
diff --git a/include/linux/spi/ads7846.h b/include/linux/spi/ads7846.h
index 1a5eaef3b7f2..d424c1aadf38 100644
--- a/include/linux/spi/ads7846.h
+++ b/include/linux/spi/ads7846.h
@@ -1,17 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* linux/spi/ads7846.h */
 
-/* Touchscreen characteristics vary between boards and models.  The
- * platform_data for the device's "struct device" holds this information.
- *
- * It's OK if the min/max values are zero.
- */
-enum ads7846_filter {
-	ADS7846_FILTER_OK,
-	ADS7846_FILTER_REPEAT,
-	ADS7846_FILTER_IGNORE,
-};
-
 struct ads7846_platform_data {
 	u16	model;			/* 7843, 7845, 7846, 7873. */
 	u16	vref_delay_usecs;	/* 0 for external vref; etc */
@@ -51,10 +40,6 @@ struct ads7846_platform_data {
 	int	gpio_pendown_debounce;	/* platform specific debounce time for
 					 * the gpio_pendown */
 	int	(*get_pendown_state)(void);
-	int	(*filter_init)	(const struct ads7846_platform_data *pdata,
-				 void **filter_data);
-	int	(*filter)	(void *filter_data, int data_idx, int *val);
-	void	(*filter_cleanup)(void *filter_data);
 	void	(*wait_for_sync)(void);
 	bool	wakeup;
 	unsigned long irq_flags;
-- 
2.26.2

[PATCH v3 3/3] Input: ads7846: Switch to devm initialization

[Daniel Mack]

This simplies the code a lot and fixes some potential resource leaks in
the error return paths.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/input/touchscreen/ads7846.c | 123 ++++++++++------------------
 1 file changed, 45 insertions(+), 78 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 4635e8867d10..1f496c4ca6ad 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;
@@ -513,6 +509,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:
@@ -533,17 +531,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
@@ -552,11 +544,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,
@@ -949,8 +936,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",
@@ -1266,6 +1253,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;
@@ -1299,13 +1291,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);
 
@@ -1319,10 +1315,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;
@@ -1344,7 +1338,7 @@ static int ads7846_probe(struct spi_device *spi)
 
 	err = ads7846_setup_pendown(spi, ts, pdata);
 	if (err)
-		goto err_free_mem;
+		return err;
 
 	if (pdata->penirq_recheck_delay_usecs)
 		ts->penirq_recheck_delay_usecs =
@@ -1391,41 +1385,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;
+		return err;
 	}
 
 	err = regulator_enable(ts->reg);
 	if (err) {
 		dev_err(dev, "unable to enable regulator: %d\n", err);
-		goto err_put_regulator;
+		return err;
 	}
 
+	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;
+		return err;
 	}
 
 	err = ads784x_hwmon_register(spi, ts);
 	if (err)
-		goto err_free_irq;
+		return err;
 
 	dev_info(dev, "touchscreen, irq %d\n", spi->irq);
 
@@ -1440,7 +1440,7 @@ static int ads7846_probe(struct spi_device *spi)
 
 	err = sysfs_create_group(&dev->kobj, &ads784x_attr_group);
 	if (err)
-		goto err_remove_hwmon;
+		return err;
 
 	err = input_register_device(input_dev);
 	if (err)
@@ -1459,21 +1459,7 @@ static int ads7846_probe(struct spi_device *spi)
 
  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_free_mem:
-	input_free_device(input_dev);
-	kfree(packet);
-	kfree(ts);
+
 	return err;
 }
 
@@ -1482,26 +1468,7 @@ 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);
-	}
-
-	kfree(ts->packet);
-	kfree(ts);
 
 	dev_dbg(&spi->dev, "unregistered touchscreen\n");
 
-- 
2.26.2

Re: [PATCH v3 2/3] Input: ads7846: remove custom filter handling functions from pdata

[Daniel Mack]

On 5/7/20 8:20 AM, Daniel Mack wrote:
> The functions in the platform data struct to initialize, cleanup and
> apply custom filters are not in use by any mainline board.
> 
> Remove support for them to pave the road for more cleanups to come.
> 
> The enum was moved as it has no users outside of the driver code
> itself.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>

Ah damn, a stray file in the folder. Ignore this one please and use the
other one. Sorry.


Daniel

> ---
>  drivers/input/touchscreen/ads7846.c | 25 ++++++++-----------------
>  include/linux/spi/ads7846.h         | 15 ---------------
>  2 files changed, 8 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index 0fd0037ef226..4635e8867d10 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -139,13 +139,18 @@ struct ads7846 {
>  
>  	int			(*filter)(void *data, int data_idx, int *val);
>  	void			*filter_data;
> -	void			(*filter_cleanup)(void *data);
>  	int			(*get_pendown_state)(void);
>  	int			gpio_pendown;
>  
>  	void			(*wait_for_sync)(void);
>  };
>  
> +enum ads7846_filter {
> +	ADS7846_FILTER_OK,
> +	ADS7846_FILTER_REPEAT,
> +	ADS7846_FILTER_IGNORE,
> +};
> +
>  /* leave chip selected when we're done, for quicker re-select? */
>  #if	0
>  #define	CS_CHANGE(xfer)	((xfer).cs_change = 1)
> @@ -1325,15 +1330,7 @@ static int ads7846_probe(struct spi_device *spi)
>  	ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
>  	ts->vref_mv = pdata->vref_mv;
>  
> -	if (pdata->filter != NULL) {
> -		if (pdata->filter_init != NULL) {
> -			err = pdata->filter_init(pdata, &ts->filter_data);
> -			if (err < 0)
> -				goto err_free_mem;
> -		}
> -		ts->filter = pdata->filter;
> -		ts->filter_cleanup = pdata->filter_cleanup;
> -	} else if (pdata->debounce_max) {
> +	if (pdata->debounce_max) {
>  		ts->debounce_max = pdata->debounce_max;
>  		if (ts->debounce_max < 2)
>  			ts->debounce_max = 2;
> @@ -1347,7 +1344,7 @@ static int ads7846_probe(struct spi_device *spi)
>  
>  	err = ads7846_setup_pendown(spi, ts, pdata);
>  	if (err)
> -		goto err_cleanup_filter;
> +		goto err_free_mem;
>  
>  	if (pdata->penirq_recheck_delay_usecs)
>  		ts->penirq_recheck_delay_usecs =
> @@ -1473,9 +1470,6 @@ static int ads7846_probe(struct spi_device *spi)
>   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);
> @@ -1506,9 +1500,6 @@ static int ads7846_remove(struct spi_device *spi)
>  		gpio_free(ts->gpio_pendown);
>  	}
>  
> -	if (ts->filter_cleanup)
> -		ts->filter_cleanup(ts->filter_data);
> -
>  	kfree(ts->packet);
>  	kfree(ts);
>  
> diff --git a/include/linux/spi/ads7846.h b/include/linux/spi/ads7846.h
> index 1a5eaef3b7f2..d424c1aadf38 100644
> --- a/include/linux/spi/ads7846.h
> +++ b/include/linux/spi/ads7846.h
> @@ -1,17 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /* linux/spi/ads7846.h */
>  
> -/* Touchscreen characteristics vary between boards and models.  The
> - * platform_data for the device's "struct device" holds this information.
> - *
> - * It's OK if the min/max values are zero.
> - */
> -enum ads7846_filter {
> -	ADS7846_FILTER_OK,
> -	ADS7846_FILTER_REPEAT,
> -	ADS7846_FILTER_IGNORE,
> -};
> -
>  struct ads7846_platform_data {
>  	u16	model;			/* 7843, 7845, 7846, 7873. */
>  	u16	vref_delay_usecs;	/* 0 for external vref; etc */
> @@ -51,10 +40,6 @@ struct ads7846_platform_data {
>  	int	gpio_pendown_debounce;	/* platform specific debounce time for
>  					 * the gpio_pendown */
>  	int	(*get_pendown_state)(void);
> -	int	(*filter_init)	(const struct ads7846_platform_data *pdata,
> -				 void **filter_data);
> -	int	(*filter)	(void *filter_data, int data_idx, int *val);
> -	void	(*filter_cleanup)(void *filter_data);
>  	void	(*wait_for_sync)(void);
>  	bool	wakeup;
>  	unsigned long irq_flags;
> 

Re: [PATCH v3 1/3] Input: ads7846: Add short-hand for spi->dev in probe() function

[Marco Felsch]

On 20-05-07 08:20, Daniel Mack wrote:
> This will make the code a bit more terse.
> No functional change intended.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

Re: [PATCH v3 2/3] Input: ads7846: Remove custom filter handling functions from pdata

[Marco Felsch]

On 20-05-07 08:20, Daniel Mack wrote:
> The functions in the platform data struct to initialize, cleanup and
> apply custom filters are not in use by any mainline board.
> 
> Remove support for them to pave the road for more cleanups to come.
> 
> The enum was moved as it has no users outside of the driver code
> itself.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

Re: [PATCH v3 0/3] Input: ads7846: pdata cleanups and devm init

[Daniel Mack]

Dmirty,

Do you think this is good to go in?


Thanks,
Daniel

On 5/7/20 8:20 AM, Daniel Mack wrote:
> Hi,
> 
> This is v3 of the patches to bring devm initialization to the ads7846
> driver. I left the gpiod conversion patch out for now as it needs more
> work, and it's also independent of the other changes.
> 
> v3:
> 
> * Added a patch to remove custom filter handling from pdata
> * Added devm_add_action_or_reset() for regulator state maintaining
> * Addressed minor nits pointed out by Marco Felsch
> 
> Daniel Mack (3):
>   Input: ads7846: Add short-hand for spi->dev in probe() function
>   Input: ads7846: Remove custom filter handling functions from pdata
>   Input: ads7846: Switch to devm initialization
> 
>  drivers/input/touchscreen/ads7846.c | 185 +++++++++++-----------------
>  include/linux/spi/ads7846.h         |  15 ---
>  2 files changed, 72 insertions(+), 128 deletions(-)
> 

Re: [PATCH v3 3/3] Input: ads7846: Switch to devm initialization

[Marco Felsch]

Hi Daniel,

sry for my late response.

On 20-05-07 08:20, Daniel Mack wrote:
> This simplies the code a lot and fixes some potential resource leaks in
> the error return paths.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  drivers/input/touchscreen/ads7846.c | 123 ++++++++++------------------
>  1 file changed, 45 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index 4635e8867d10..1f496c4ca6ad 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;
> @@ -513,6 +509,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:
> @@ -533,17 +531,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
> @@ -552,11 +544,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,
> @@ -949,8 +936,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",
> @@ -1266,6 +1253,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;
> @@ -1299,13 +1291,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);
>  
> @@ -1319,10 +1315,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;
> @@ -1344,7 +1338,7 @@ static int ads7846_probe(struct spi_device *spi)
>  
>  	err = ads7846_setup_pendown(spi, ts, pdata);
>  	if (err)
> -		goto err_free_mem;
> +		return err;
>  
>  	if (pdata->penirq_recheck_delay_usecs)
>  		ts->penirq_recheck_delay_usecs =
> @@ -1391,41 +1385,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;
> +		return err;
>  	}
>  
>  	err = regulator_enable(ts->reg);
>  	if (err) {
>  		dev_err(dev, "unable to enable regulator: %d\n", err);
> -		goto err_put_regulator;
> +		return err;
>  	}
>  
> +	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;
> +		return err;
>  	}
>  
>  	err = ads784x_hwmon_register(spi, ts);
>  	if (err)
> -		goto err_free_irq;
> +		return err;
>  
>  	dev_info(dev, "touchscreen, irq %d\n", spi->irq);
>  
> @@ -1440,7 +1440,7 @@ static int ads7846_probe(struct spi_device *spi)
>  
>  	err = sysfs_create_group(&dev->kobj, &ads784x_attr_group);
>  	if (err)
> -		goto err_remove_hwmon;
> +		return err;
>  
>  	err = input_register_device(input_dev);
>  	if (err)
> @@ -1459,21 +1459,7 @@ static int ads7846_probe(struct spi_device *spi)
>  
>   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_free_mem:
> -	input_free_device(input_dev);
> -	kfree(packet);
> -	kfree(ts);
> +
>  	return err;
>  }
>  
> @@ -1482,26 +1468,7 @@ 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);

Did you tested the bind/unbind path? I think we are getting troubles
here because ads7846_disable calls ads7846_stop() and
regulator_disable(). Since we are using the devm_action the regualtor
gets disabled by this action and ads7846_disable(). This causes a
refcount problem.

Regards,
  Marco

> -	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);
> -	}
> -
> -	kfree(ts->packet);
> -	kfree(ts);
>  
>  	dev_dbg(&spi->dev, "unregistered touchscreen\n");
>  
> -- 
> 2.26.2
> 
> 

Re: [PATCH v3 3/3] Input: ads7846: Switch to devm initialization

[Daniel Mack]

Hi Marco!

On 5/19/20 11:18 AM, Marco Felsch wrote:
> On 20-05-07 08:20, Daniel Mack wrote:
>> This simplies the code a lot and fixes some potential resource leaks in
>> the error return paths.
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> ---
>>  drivers/input/touchscreen/ads7846.c | 123 ++++++++++------------------
>>  1 file changed, 45 insertions(+), 78 deletions(-)
>>

>> @@ -1482,26 +1468,7 @@ 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);
> 
> Did you tested the bind/unbind path? I think we are getting troubles
> here because ads7846_disable calls ads7846_stop() and
> regulator_disable(). Since we are using the devm_action the regualtor
> gets disabled by this action and ads7846_disable(). This causes a
> refcount problem.

Ah, nice catch. Yes, we just need to call ads7846_stop() here. Will post
a v4 then.


Thanks,
Daniel