Hi Marco,

On 5/4/20 9:34 AM, Marco Felsch wrote:

thanks for your patch :)

On 20-04-30 19:53, Daniel Mack wrote:

quoted

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.

My concern was that the IRQ handler starts firing as soon as the
interrupt is requested. I however just learned that this is not a
problem though. So I'll drop that change to keep the patch smaller.
Thanks for noting!

quoted

---
 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?

Right. Will drop it.

quoted

 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.

Yes, we should. But I'll keep that out of this patch set.

quoted

+	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?

Okay.

quoted

 	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.

Agreed.

quoted

+	err = input_register_device(input_dev);
+	if (err)
+		goto err_cleanup_filter;

It seems quite common to register the device on the end.

Yes, as stated above. Reverted that change in ordering.

quoted

 	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.

Good point. Added.

Will send a v2 soon. Thanks for the review!


Daniel