[PATCH v1 1/2] Input: tsc2007 - convert to GPIO descriptors

This converts the driver to use GPIO descriptors.

Signed-off-by: Andy Shevchenko <redacted>
---
 drivers/input/touchscreen/tsc2007.h      |  4 +++-
 drivers/input/touchscreen/tsc2007_core.c | 17 ++++++++---------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007.h b/drivers/input/touchscreen/tsc2007.h
index 91c60bf6dcaf..69b08dd6c8df 100644
--- a/drivers/input/touchscreen/tsc2007.h
+++ b/drivers/input/touchscreen/tsc2007.h

@@ -19,6 +19,8 @@
 #ifndef _TSC2007_H
 #define _TSC2007_H
 
+struct gpio_desc;
+
 #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
 #define TSC2007_MEASURE_AUX		(0x2 << 4)
 #define TSC2007_MEASURE_TEMP1		(0x4 << 4)

@@ -69,7 +71,7 @@ struct tsc2007 {
 	int			fuzzy;
 	int			fuzzz;
 
-	unsigned int		gpio;
+	struct gpio_desc	*gpiod;
 	int			irq;
 
 	wait_queue_head_t	wait;

diff --git a/drivers/input/touchscreen/tsc2007_core.c b/drivers/input/touchscreen/tsc2007_core.c
index 3b80abfc1eca..9b747c7d11f1 100644
--- a/drivers/input/touchscreen/tsc2007_core.c
+++ b/drivers/input/touchscreen/tsc2007_core.c

@@ -19,11 +19,11 @@
 
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/gpio/consumer.h>
 #include <linux/input.h>
 #include <linux/interrupt.h>
 #include <linux/i2c.h>
 #include <linux/of_device.h>
-#include <linux/of_gpio.h>
 #include <linux/platform_data/tsc2007.h>
 #include "tsc2007.h"
 

@@ -226,11 +226,12 @@ static int tsc2007_get_pendown_state_gpio(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct tsc2007 *ts = i2c_get_clientdata(client);
 
-	return !gpio_get_value(ts->gpio);
+	return !gpiod_get_value(ts->gpiod);
 }
 
 static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts)
 {
+	struct device *dev = &client->dev;
 	struct device_node *np = client->dev.of_node;
 	u32 val32;
 	u64 val64;

@@ -266,13 +267,11 @@ static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts)
 		return -EINVAL;
 	}
 
-	ts->gpio = of_get_gpio(np, 0);
-	if (gpio_is_valid(ts->gpio))
-		ts->get_pendown_state = tsc2007_get_pendown_state_gpio;
-	else
-		dev_warn(&client->dev,
-			 "GPIO not specified in DT (of_get_gpio returned %d)\n",
-			 ts->gpio);
+	ts->gpiod = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
+	if (IS_ERR(ts->gpiod))
+		return PTR_ERR(ts->gpiod);
+
+	ts->get_pendown_state = tsc2007_get_pendown_state_gpio;
 
 	return 0;
 }

-- 
2.30.1

Device property API allows to gather device resources from different sources,
such as ACPI. Convert the drivers to unleash the power of device property API.

Signed-off-by: Andy Shevchenko <redacted>
---
 drivers/input/touchscreen/tsc2007_core.c | 35 +++++++-----------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007_core.c b/drivers/input/touchscreen/tsc2007_core.c
index 9b747c7d11f1..2759eb6a9bad 100644
--- a/drivers/input/touchscreen/tsc2007_core.c
+++ b/drivers/input/touchscreen/tsc2007_core.c

@@ -23,7 +23,8 @@
 #include <linux/input.h>
 #include <linux/interrupt.h>
 #include <linux/i2c.h>
-#include <linux/of_device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
 #include <linux/platform_data/tsc2007.h>
 #include "tsc2007.h"
 

@@ -220,7 +221,6 @@ static void tsc2007_close(struct input_dev *input_dev)
 	tsc2007_stop(ts);
 }
 
-#ifdef CONFIG_OF
 static int tsc2007_get_pendown_state_gpio(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);

@@ -232,38 +232,32 @@ static int tsc2007_get_pendown_state_gpio(struct device *dev)
 static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts)
 {
 	struct device *dev = &client->dev;
-	struct device_node *np = client->dev.of_node;
 	u32 val32;
 	u64 val64;
 
-	if (!np) {
-		dev_err(&client->dev, "missing device tree data\n");
-		return -EINVAL;
-	}
-
-	if (!of_property_read_u32(np, "ti,max-rt", &val32))
+	if (!device_property_read_u32(dev, "ti,max-rt", &val32))
 		ts->max_rt = val32;
 	else
 		ts->max_rt = MAX_12BIT;
 
-	if (!of_property_read_u32(np, "ti,fuzzx", &val32))
+	if (!device_property_read_u32(dev, "ti,fuzzx", &val32))
 		ts->fuzzx = val32;
 
-	if (!of_property_read_u32(np, "ti,fuzzy", &val32))
+	if (!device_property_read_u32(dev, "ti,fuzzy", &val32))
 		ts->fuzzy = val32;
 
-	if (!of_property_read_u32(np, "ti,fuzzz", &val32))
+	if (!device_property_read_u32(dev, "ti,fuzzz", &val32))
 		ts->fuzzz = val32;
 
-	if (!of_property_read_u64(np, "ti,poll-period", &val64))
+	if (!device_property_read_u64(dev, "ti,poll-period", &val64))
 		ts->poll_period = msecs_to_jiffies(val64);
 	else
 		ts->poll_period = msecs_to_jiffies(1);
 
-	if (!of_property_read_u32(np, "ti,x-plate-ohms", &val32)) {
+	if (!device_property_read_u32(dev, "ti,x-plate-ohms", &val32)) {
 		ts->x_plate_ohms = val32;
 	} else {
-		dev_err(&client->dev, "missing ti,x-plate-ohms devicetree property.");
+		dev_err(dev, "missing ti,x-plate-ohms devicetree property.");
 		return -EINVAL;
 	}
 

@@ -275,13 +269,6 @@ static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts)
 
 	return 0;
 }
-#else
-static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts)
-{
-	dev_err(&client->dev, "platform data is required!\n");
-	return -EINVAL;
-}
-#endif
 
 static int tsc2007_probe_pdev(struct i2c_client *client, struct tsc2007 *ts,
 			      const struct tsc2007_platform_data *pdata,

@@ -430,18 +417,16 @@ static const struct i2c_device_id tsc2007_idtable[] = {
 
 MODULE_DEVICE_TABLE(i2c, tsc2007_idtable);
 
-#ifdef CONFIG_OF
 static const struct of_device_id tsc2007_of_match[] = {
 	{ .compatible = "ti,tsc2007" },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, tsc2007_of_match);
-#endif
 
 static struct i2c_driver tsc2007_driver = {
 	.driver = {
 		.name	= "tsc2007",
-		.of_match_table = of_match_ptr(tsc2007_of_match),
+		.of_match_table = tsc2007_of_match,
 	},
 	.id_table	= tsc2007_idtable,
 	.probe		= tsc2007_probe,

-- 
2.30.1

Hi Andy,

On Mon, Mar 08, 2021 at 12:05:48AM +0200, Andy Shevchenko wrote:

This is not correct. gpio_get_value() is raw polarity vs
gpiod_get_value() using logical active/inactive, and tsc2007 GPIO lines
are active low. The negation must be dropped after switching to GPIOD
API.

GPIO is definitely not optional in DT case, at least in the way the
driver written right now.

+	if (IS_ERR(ts->gpiod))
+		return PTR_ERR(ts->gpiod);
+
+	ts->get_pendown_state = tsc2007_get_pendown_state_gpio;
 
 	return 0;
 }
-- 
2.30.1

Thanks.

-- 
Dmitry

On Mon, Mar 8, 2021 at 12:57 AM Dmitry Torokhov
[off-list ref] wrote:

On Mon, Mar 08, 2021 at 12:05:48AM +0200, Andy Shevchenko wrote:

...

quoted

-     return !gpio_get_value(ts->gpio);
+     return !gpiod_get_value(ts->gpiod);

This is not correct. gpio_get_value() is raw polarity vs
gpiod_get_value() using logical active/inactive, and tsc2007 GPIO lines
are active low. The negation must be dropped after switching to GPIOD
API.

Ah, indeed, I missed that, thanks!

...

quoted

-     ts->gpio = of_get_gpio(np, 0);
-     if (gpio_is_valid(ts->gpio))
-             ts->get_pendown_state = tsc2007_get_pendown_state_gpio;
-     else
-             dev_warn(&client->dev,
-                      "GPIO not specified in DT (of_get_gpio returned %d)\n",
-                      ts->gpio);
+     ts->gpiod = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);

GPIO is definitely not optional in DT case, at least in the way the
driver written right now.

Can you elaborate this, please? I don't see from the dev_warn() w/o
any error code returned that it's mandatory.
In the bindings one may read:

  Optional properties:
  - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
    The penirq pin goes to low when the panel is touched.
    (see GPIO binding[1] for more details).

Nothing suggested it's mandatory. What have I missed?

quoted

+     if (IS_ERR(ts->gpiod))
+             return PTR_ERR(ts->gpiod);

-- 
With Best Regards,
Andy Shevchenko

On Mon, Mar 08, 2021 at 11:10:38AM +0200, Andy Shevchenko wrote:

On Mon, Mar 8, 2021 at 12:57 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Mon, Mar 08, 2021 at 12:05:48AM +0200, Andy Shevchenko wrote:

...

quoted

quoted

-     return !gpio_get_value(ts->gpio);
+     return !gpiod_get_value(ts->gpiod);

This is not correct. gpio_get_value() is raw polarity vs
gpiod_get_value() using logical active/inactive, and tsc2007 GPIO lines
are active low. The negation must be dropped after switching to GPIOD
API.

Ah, indeed, I missed that, thanks!

...

quoted

quoted

-     ts->gpio = of_get_gpio(np, 0);
-     if (gpio_is_valid(ts->gpio))
-             ts->get_pendown_state = tsc2007_get_pendown_state_gpio;
-     else
-             dev_warn(&client->dev,
-                      "GPIO not specified in DT (of_get_gpio returned %d)\n",
-                      ts->gpio);
+     ts->gpiod = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);

GPIO is definitely not optional in DT case, at least in the way the
driver written right now.

Can you elaborate this, please? I don't see from the dev_warn() w/o
any error code returned that it's mandatory.
In the bindings one may read:

  Optional properties:
  - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
    The penirq pin goes to low when the panel is touched.
    (see GPIO binding[1] for more details).

Nothing suggested it's mandatory. What have I missed?

Ah, indeed, I misread the code and thought we'd abort if there is no
pendown GPIO. I wonder if we should remove the warning since we seem to
support operations without it.

Thanks.

-- 
Dmitry

On Mon, Mar 8, 2021 at 9:29 PM Dmitry Torokhov
[off-list ref] wrote:

On Mon, Mar 08, 2021 at 11:10:38AM +0200, Andy Shevchenko wrote:

quoted

On Mon, Mar 8, 2021 at 12:57 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Mon, Mar 08, 2021 at 12:05:48AM +0200, Andy Shevchenko wrote:

...

quoted

quoted

-     return !gpio_get_value(ts->gpio);
+     return !gpiod_get_value(ts->gpiod);

This is not correct. gpio_get_value() is raw polarity vs
gpiod_get_value() using logical active/inactive, and tsc2007 GPIO lines
are active low. The negation must be dropped after switching to GPIOD
API.

Ah, indeed, I missed that, thanks!

...

quoted

quoted

-     ts->gpio = of_get_gpio(np, 0);
-     if (gpio_is_valid(ts->gpio))
-             ts->get_pendown_state = tsc2007_get_pendown_state_gpio;
-     else
-             dev_warn(&client->dev,
-                      "GPIO not specified in DT (of_get_gpio returned %d)\n",
-                      ts->gpio);
+     ts->gpiod = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);

GPIO is definitely not optional in DT case, at least in the way the
driver written right now.

Can you elaborate this, please? I don't see from the dev_warn() w/o
any error code returned that it's mandatory.
In the bindings one may read:

  Optional properties:
  - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
    The penirq pin goes to low when the panel is touched.
    (see GPIO binding[1] for more details).

Nothing suggested it's mandatory. What have I missed?

Ah, indeed, I misread the code and thought we'd abort if there is no
pendown GPIO. I wonder if we should remove the warning since we seem to
support operations without it.

But that's what I have done, i.e. removed the warning as well.

So, if there are no other concerns than inverted value, I'll send v2.

-- 
With Best Regards,
Andy Shevchenko

On Mon, Mar 08, 2021 at 11:04:59PM +0200, Andy Shevchenko wrote:

On Mon, Mar 8, 2021 at 9:29 PM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Mon, Mar 08, 2021 at 11:10:38AM +0200, Andy Shevchenko wrote:

quoted

On Mon, Mar 8, 2021 at 12:57 AM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Mon, Mar 08, 2021 at 12:05:48AM +0200, Andy Shevchenko wrote:

...

quoted

quoted

-     return !gpio_get_value(ts->gpio);
+     return !gpiod_get_value(ts->gpiod);

This is not correct. gpio_get_value() is raw polarity vs
gpiod_get_value() using logical active/inactive, and tsc2007 GPIO lines
are active low. The negation must be dropped after switching to GPIOD
API.

Ah, indeed, I missed that, thanks!

...

quoted

quoted

-     ts->gpio = of_get_gpio(np, 0);
-     if (gpio_is_valid(ts->gpio))
-             ts->get_pendown_state = tsc2007_get_pendown_state_gpio;
-     else
-             dev_warn(&client->dev,
-                      "GPIO not specified in DT (of_get_gpio returned %d)\n",
-                      ts->gpio);
+     ts->gpiod = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);

GPIO is definitely not optional in DT case, at least in the way the
driver written right now.

Can you elaborate this, please? I don't see from the dev_warn() w/o
any error code returned that it's mandatory.
In the bindings one may read:

  Optional properties:
  - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
    The penirq pin goes to low when the panel is touched.
    (see GPIO binding[1] for more details).

Nothing suggested it's mandatory. What have I missed?

Ah, indeed, I misread the code and thought we'd abort if there is no
pendown GPIO. I wonder if we should remove the warning since we seem to
support operations without it.

But that's what I have done, i.e. removed the warning as well.

Well, what can I say, -ENOCOFFEE.

So, if there are no other concerns than inverted value, I'll send v2.

Yes, please.

-- 
Dmitry