[PATCH 1/2] input: st1232: Add reset pin handling

[PATCH 1/2] input: st1232: Add reset pin handling

[Bastian Hecht]

We add the possiblity to hand over a GPIO number for the reset pin.
This way we can remove existing board code that takes care of it and
group this information properly in the platform data or in the device
tree confguration.

The implementation is analogous to the cy8ctmg110 driver, thanks.

Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
 .../bindings/input/touchscreen/sitronix-st1232.txt |   24 ++++++++++
 drivers/input/touchscreen/st1232.c                 |   47 ++++++++++++++++++--
 include/linux/input/st1232_pdata.h                 |   13 ++++++
 3 files changed, 81 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
 create mode 100644 include/linux/input/st1232_pdata.h

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
new file mode 100644
index 0000000..1936969
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
@@ -0,0 +1,24 @@
+* Sitronix st1232 touchscreen controller
+
+Required properties:
+- compatible: must be "sitronix,st1232"
+- reg: I2C address of the chip
+- interrupts: interrupt to which the chip is connected
+
+Optional properties:
+- sitronix,reset-pin: GPIO number of the reset pin
+
+Example:
+
+	i2c@00000000 {
+		/* ... */
+
+		touchscreen@55 {
+			compatible = "sitronix,st1232";
+			reg = <0x55>;
+			interrupts = <2 0>;
+			sitronix,reset-pin = <166>;
+		};
+
+		/* ... */
+	};
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index d9d05e2..6aa5038 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -19,6 +19,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/input.h>
 #include <linux/interrupt.h>
@@ -26,6 +27,7 @@
 #include <linux/pm_qos.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/input/st1232_pdata.h>
 
 #define ST1232_TS_NAME	"st1232-ts"
 
@@ -48,6 +50,7 @@ struct st1232_ts_data {
 	struct input_dev *input_dev;
 	struct st1232_ts_finger finger[MAX_FINGERS];
 	struct dev_pm_qos_request low_latency_req;
+	int reset_pin;
 };
 
 static int st1232_ts_read_data(struct st1232_ts_data *ts)
@@ -139,10 +142,17 @@ end:
 	return IRQ_HANDLED;
 }
 
+static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron)
+{
+	if (ts->reset_pin)
+		gpio_direction_output(ts->reset_pin, poweron);
+}
+
 static int st1232_ts_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
 	struct st1232_ts_data *ts;
+	struct st1232_pdata *pdata = client->dev.platform_data;
 	struct input_dev *input_dev;
 	int error;
 
@@ -167,6 +177,25 @@ static int st1232_ts_probe(struct i2c_client *client,
 	ts->client = client;
 	ts->input_dev = input_dev;
 
+	if (client->dev.of_node)
+		of_property_read_u32(client->dev.of_node,
+				"sitronix,reset-pin", &ts->reset_pin);
+	else if (pdata)
+		ts->reset_pin = pdata->reset_pin;
+
+
+	if (ts->reset_pin) {
+		error = gpio_request(ts->reset_pin, NULL);
+		if (error) {
+			dev_err(&client->dev,
+				"Unable to request GPIO pin %d.\n",
+				ts->reset_pin);
+				goto err_free_mem;
+		}
+	}
+
+	st1232_ts_power(ts, true);
+
 	input_dev->name = "st1232-touchscreen";
 	input_dev->id.bustype = BUS_I2C;
 	input_dev->dev.parent = &client->dev;
@@ -183,7 +212,7 @@ static int st1232_ts_probe(struct i2c_client *client,
 				     IRQF_ONESHOT, client->name, ts);
 	if (error) {
 		dev_err(&client->dev, "Failed to register interrupt\n");
-		goto err_free_mem;
+		goto err_free_reset_gpio;
 	}
 
 	error = input_register_device(ts->input_dev);
@@ -200,6 +229,9 @@ static int st1232_ts_probe(struct i2c_client *client,
 
 err_free_irq:
 	free_irq(client->irq, ts);
+err_free_reset_gpio:
+	if (ts->reset_pin)
+		gpio_free(ts->reset_pin);
 err_free_mem:
 	input_free_device(input_dev);
 	kfree(ts);
@@ -210,8 +242,11 @@ static int st1232_ts_remove(struct i2c_client *client)
 {
 	struct st1232_ts_data *ts = i2c_get_clientdata(client);
 
+	st1232_ts_power(ts, false);
 	device_init_wakeup(&client->dev, 0);
 	free_irq(client->irq, ts);
+	if (ts->reset_pin)
+		gpio_free(ts->reset_pin);
 	input_unregister_device(ts->input_dev);
 	kfree(ts);
 
@@ -222,11 +257,14 @@ static int st1232_ts_remove(struct i2c_client *client)
 static int st1232_ts_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct st1232_ts_data *ts = i2c_get_clientdata(client);
 
 	if (device_may_wakeup(&client->dev))
 		enable_irq_wake(client->irq);
-	else
+	else {
 		disable_irq(client->irq);
+		st1232_ts_power(ts, false);
+	}
 
 	return 0;
 }
@@ -234,11 +272,14 @@ static int st1232_ts_suspend(struct device *dev)
 static int st1232_ts_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct st1232_ts_data *ts = i2c_get_clientdata(client);
 
 	if (device_may_wakeup(&client->dev))
 		disable_irq_wake(client->irq);
-	else
+	else {
+		st1232_ts_power(ts, true);
 		enable_irq(client->irq);
+	}
 
 	return 0;
 }
diff --git a/include/linux/input/st1232_pdata.h b/include/linux/input/st1232_pdata.h
new file mode 100644
index 0000000..ef99354
--- /dev/null
+++ b/include/linux/input/st1232_pdata.h
@@ -0,0 +1,13 @@
+#ifndef _LINUX_ST1232_PDATA_H
+#define _LINUX_ST1232_PDATA_H
+
+/*
+ * Optional platform data
+ *
+ * Use this if you want the driver to drive the reset pin.
+ */
+struct st1232_pdata {
+	int reset_pin;
+};
+
+#endif
-- 
1.7.9.5

[PATCH 2/2] ARM: shmobile: Armadillo800EVA: Move st1232 reset pin handling

[Bastian Hecht]

We no longer need to set up the reset pin for the st1232 in the board
code, but can pass the GPIO number via the platform data to the driver.
This results in a cleaner grouping of the device setup.

Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
 arch/arm/mach-shmobile/board-armadillo800eva.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c b/arch/arm/mach-shmobile/board-armadillo800eva.c
index bf4c3d2..d82e67a 100644
--- a/arch/arm/mach-shmobile/board-armadillo800eva.c
+++ b/arch/arm/mach-shmobile/board-armadillo800eva.c
@@ -24,6 +24,7 @@
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/input.h>
+#include <linux/input/st1232_pdata.h>
 #include <linux/irq.h>
 #include <linux/platform_device.h>
 #include <linux/gpio.h>
@@ -997,10 +998,15 @@ static struct platform_device i2c_gpio_device = {
 };
 
 /* I2C */
+static struct st1232_pdata st1232_i2c0_pdata = {
+	.reset_pin = 166,
+};
+
 static struct i2c_board_info i2c0_devices[] = {
 	{
 		I2C_BOARD_INFO("st1232-ts", 0x55),
 		.irq = irq_pin(10),
+		.platform_data = &st1232_i2c0_pdata,
 	},
 	{
 		I2C_BOARD_INFO("wm8978", 0x1a),
@@ -1120,7 +1126,6 @@ static void __init eva_init(void)
 
 	/* Touchscreen */
 	gpio_request(GPIO_FN_IRQ10,	NULL); /* TP_INT */
-	gpio_request_one(166, GPIOF_OUT_INIT_HIGH, NULL); /* TP_RST_B */
 
 	/* GETHER */
 	gpio_request(GPIO_FN_ET_CRS,		NULL);
-- 
1.7.9.5

Re: [PATCH 1/2] input: st1232: Add reset pin handling

[Laurent Pinchart]

Hi Bastian,

Thanks for the patch.

On Saturday 06 April 2013 15:18:45 Bastian Hecht wrote:
> We add the possiblity to hand over a GPIO number for the reset pin.
> This way we can remove existing board code that takes care of it and
> group this information properly in the platform data or in the device
> tree confguration.
> 
> The implementation is analogous to the cy8ctmg110 driver, thanks.
> 
> Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
> ---
>  .../bindings/input/touchscreen/sitronix-st1232.txt |   24 ++++++++++
>  drivers/input/touchscreen/st1232.c                 |   47 +++++++++++++++--
>  include/linux/input/st1232_pdata.h                 |   13 ++++++
>  3 files changed, 81 insertions(+), 3 deletions(-) 
>  create mode 100644
> Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
> create mode 100644 include/linux/input/st1232_pdata.h
> 
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
> b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
> new file mode 100644
> index 0000000..1936969
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
> @@ -0,0 +1,24 @@
> +* Sitronix st1232 touchscreen controller
> +
> +Required properties:
> +- compatible: must be "sitronix,st1232"
> +- reg: I2C address of the chip
> +- interrupts: interrupt to which the chip is connected
> +
> +Optional properties:
> +- sitronix,reset-pin: GPIO number of the reset pin

GPIO bindings use phandles, please see 
Documentation/devicetree/bindings/gpio/gpio.txt.

> +Example:
> +
> +	i2c@00000000 {
> +		/* ... */
> +
> +		touchscreen@55 {
> +			compatible = "sitronix,st1232";
> +			reg = <0x55>;
> +			interrupts = <2 0>;
> +			sitronix,reset-pin = <166>;
> +		};
> +
> +		/* ... */
> +	};
> diff --git a/drivers/input/touchscreen/st1232.c
> b/drivers/input/touchscreen/st1232.c index d9d05e2..6aa5038 100644
> --- a/drivers/input/touchscreen/st1232.c
> +++ b/drivers/input/touchscreen/st1232.c

[snip]

> @@ -48,6 +50,7 @@ struct st1232_ts_data {
>  	struct input_dev *input_dev;
>  	struct st1232_ts_finger finger[MAX_FINGERS];
>  	struct dev_pm_qos_request low_latency_req;
> +	int reset_pin;

Nitpicking, I would call it reset_gpio.

>  };
> 
>  static int st1232_ts_read_data(struct st1232_ts_data *ts)
> @@ -139,10 +142,17 @@ end:
>  	return IRQ_HANDLED;
>  }
> 
> +static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron)
> +{
> +	if (ts->reset_pin)
> +		gpio_direction_output(ts->reset_pin, poweron);
> +}
> +
>  static int st1232_ts_probe(struct i2c_client *client,
>  					const struct i2c_device_id *id)
>  {
>  	struct st1232_ts_data *ts;
> +	struct st1232_pdata *pdata = client->dev.platform_data;
>  	struct input_dev *input_dev;
>  	int error;
> 
> @@ -167,6 +177,25 @@ static int st1232_ts_probe(struct i2c_client *client,
>  	ts->client = client;
>  	ts->input_dev = input_dev;
> 
> +	if (client->dev.of_node)
> +		of_property_read_u32(client->dev.of_node,
> +				"sitronix,reset-pin", &ts->reset_pin);
> +	else if (pdata)
> +		ts->reset_pin = pdata->reset_pin;
> +
> +
> +	if (ts->reset_pin) {
> +		error = gpio_request(ts->reset_pin, NULL);

You should use devm_gpio_request(), it will simplify the error path and the 
remove function.

> +		if (error) {
> +			dev_err(&client->dev,
> +				"Unable to request GPIO pin %d.\n",
> +				ts->reset_pin);
> +				goto err_free_mem;
> +		}
> +	}
> +
> +	st1232_ts_power(ts, true);
> +
>  	input_dev->name = "st1232-touchscreen";
>  	input_dev->id.bustype = BUS_I2C;
>  	input_dev->dev.parent = &client->dev;

[snip]

> diff --git a/include/linux/input/st1232_pdata.h
> b/include/linux/input/st1232_pdata.h new file mode 100644
> index 0000000..ef99354
> --- /dev/null
> +++ b/include/linux/input/st1232_pdata.h
> @@ -0,0 +1,13 @@
> +#ifndef _LINUX_ST1232_PDATA_H
> +#define _LINUX_ST1232_PDATA_H
> +
> +/*
> + * Optional platform data
> + *
> + * Use this if you want the driver to drive the reset pin.
> + */
> +struct st1232_pdata {
> +	int reset_pin;

I would call this reset_gpio as well.

> +};
> +
> +#endif
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] input: st1232: Add reset pin handling

[Simon Horman]

On Sat, Apr 06, 2013 at 03:18:45PM +0200, Bastian Hecht wrote:
> We add the possiblity to hand over a GPIO number for the reset pin.
> This way we can remove existing board code that takes care of it and
> group this information properly in the platform data or in the device
> tree confguration.
> 
> The implementation is analogous to the cy8ctmg110 driver, thanks.

Laurent, could I get a review from you of this and the following patch?

Re: [PATCH 1/2] input: st1232: Add reset pin handling

[Simon Horman]

On Mon, Apr 08, 2013 at 10:39:02AM +0900, Simon Horman wrote:
> On Sat, Apr 06, 2013 at 03:18:45PM +0200, Bastian Hecht wrote:
> > We add the possiblity to hand over a GPIO number for the reset pin.
> > This way we can remove existing board code that takes care of it and
> > group this information properly in the platform data or in the device
> > tree confguration.
> > 
> > The implementation is analogous to the cy8ctmg110 driver, thanks.
> 
> Laurent, could I get a review from you of this and the following patch?

Sorry, I missed that you had already provided one.

Re: [PATCH 1/2] input: st1232: Add reset pin handling

[Bastian Hecht]

Hi Laurent,

2013/4/7 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Bastian,
>
> Thanks for the patch.

Thanks for the review.

> On Saturday 06 April 2013 15:18:45 Bastian Hecht wrote:
>> We add the possiblity to hand over a GPIO number for the reset pin.
>> This way we can remove existing board code that takes care of it and
>> group this information properly in the platform data or in the device
>> tree confguration.
>>
>> The implementation is analogous to the cy8ctmg110 driver, thanks.
>>
>> Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
>> ---
>>  .../bindings/input/touchscreen/sitronix-st1232.txt |   24 ++++++++++
>>  drivers/input/touchscreen/st1232.c                 |   47 +++++++++++++++--
>>  include/linux/input/st1232_pdata.h                 |   13 ++++++
>>  3 files changed, 81 insertions(+), 3 deletions(-)
>>  create mode 100644
>> Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
>> create mode 100644 include/linux/input/st1232_pdata.h
>>
>> diff --git
>> a/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
>> b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
>> new file mode 100644
>> index 0000000..1936969
>> --- /dev/null
>> +++
>> b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
>> @@ -0,0 +1,24 @@
>> +* Sitronix st1232 touchscreen controller
>> +
>> +Required properties:
>> +- compatible: must be "sitronix,st1232"
>> +- reg: I2C address of the chip
>> +- interrupts: interrupt to which the chip is connected
>> +
>> +Optional properties:
>> +- sitronix,reset-pin: GPIO number of the reset pin
>
> GPIO bindings use phandles, please see
> Documentation/devicetree/bindings/gpio/gpio.txt.

Ok done. I haven't tested that part yet as the pfc is not set up via
the DT. I saw in topic/pinmux-dt that you added OF support for the
pfc, but I couldn't apply the patch without conflicts. Do you think
this part is going to be merged to next any time soon?

>> +Example:
>> +
>> +     i2c@00000000 {
>> +             /* ... */
>> +
>> +             touchscreen@55 {
>> +                     compatible = "sitronix,st1232";
>> +                     reg = <0x55>;
>> +                     interrupts = <2 0>;
>> +                     sitronix,reset-pin = <166>;
>> +             };
>> +
>> +             /* ... */
>> +     };
>> diff --git a/drivers/input/touchscreen/st1232.c
>> b/drivers/input/touchscreen/st1232.c index d9d05e2..6aa5038 100644
>> --- a/drivers/input/touchscreen/st1232.c
>> +++ b/drivers/input/touchscreen/st1232.c
>
> [snip]
>
>> @@ -48,6 +50,7 @@ struct st1232_ts_data {
>>       struct input_dev *input_dev;
>>       struct st1232_ts_finger finger[MAX_FINGERS];
>>       struct dev_pm_qos_request low_latency_req;
>> +     int reset_pin;
>
> Nitpicking, I would call it reset_gpio.

Consistency is a nice thing, so thanks! Changed.

>>  };
>>
>>  static int st1232_ts_read_data(struct st1232_ts_data *ts)
>> @@ -139,10 +142,17 @@ end:
>>       return IRQ_HANDLED;
>>  }
>>
>> +static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron)
>> +{
>> +     if (ts->reset_pin)
>> +             gpio_direction_output(ts->reset_pin, poweron);
>> +}
>> +
>>  static int st1232_ts_probe(struct i2c_client *client,
>>                                       const struct i2c_device_id *id)
>>  {
>>       struct st1232_ts_data *ts;
>> +     struct st1232_pdata *pdata = client->dev.platform_data;
>>       struct input_dev *input_dev;
>>       int error;
>>
>> @@ -167,6 +177,25 @@ static int st1232_ts_probe(struct i2c_client *client,
>>       ts->client = client;
>>       ts->input_dev = input_dev;
>>
>> +     if (client->dev.of_node)
>> +             of_property_read_u32(client->dev.of_node,
>> +                             "sitronix,reset-pin", &ts->reset_pin);
>> +     else if (pdata)
>> +             ts->reset_pin = pdata->reset_pin;
>> +
>> +
>> +     if (ts->reset_pin) {
>> +             error = gpio_request(ts->reset_pin, NULL);
>
> You should use devm_gpio_request(), it will simplify the error path and the
> remove function.

Ok.

>> +             if (error) {
>> +                     dev_err(&client->dev,
>> +                             "Unable to request GPIO pin %d.\n",
>> +                             ts->reset_pin);
>> +                             goto err_free_mem;
>> +             }
>> +     }
>> +
>> +     st1232_ts_power(ts, true);
>> +
>>       input_dev->name = "st1232-touchscreen";
>>       input_dev->id.bustype = BUS_I2C;
>>       input_dev->dev.parent = &client->dev;
>
> [snip]
>
>> diff --git a/include/linux/input/st1232_pdata.h
>> b/include/linux/input/st1232_pdata.h new file mode 100644
>> index 0000000..ef99354
>> --- /dev/null
>> +++ b/include/linux/input/st1232_pdata.h
>> @@ -0,0 +1,13 @@
>> +#ifndef _LINUX_ST1232_PDATA_H
>> +#define _LINUX_ST1232_PDATA_H
>> +
>> +/*
>> + * Optional platform data
>> + *
>> + * Use this if you want the driver to drive the reset pin.
>> + */
>> +struct st1232_pdata {
>> +     int reset_pin;
>
> I would call this reset_gpio as well.

Done.

>> +};
>> +
>> +#endif
> --

Thanks,

 Bastian

Re: [PATCH 1/2] input: st1232: Add reset pin handling

[Laurent Pinchart]

Hi Bastian,

On Monday 08 April 2013 14:43:26 Bastian Hecht wrote:
> 2013/4/7 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> > On Saturday 06 April 2013 15:18:45 Bastian Hecht wrote:
> >> We add the possiblity to hand over a GPIO number for the reset pin.
> >> This way we can remove existing board code that takes care of it and
> >> group this information properly in the platform data or in the device
> >> tree confguration.
> >> 
> >> The implementation is analogous to the cy8ctmg110 driver, thanks.
> >> 
> >> Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
> >> ---
> >> 
> >>  .../bindings/input/touchscreen/sitronix-st1232.txt |   24 ++++++++++
> >>  drivers/input/touchscreen/st1232.c                 |   47 ++++++++++++--
> >>  include/linux/input/st1232_pdata.h                 |   13 ++++++
> >>  3 files changed, 81 insertions(+), 3 deletions(-)
> >>  create mode 100644
> >> 
> >> Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
> >> create mode 100644 include/linux/input/st1232_pdata.h
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
> >> b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
> >> new file mode 100644
> >> index 0000000..1936969
> >> --- /dev/null
> >> +++
> >> b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
> >> @@ -0,0 +1,24 @@
> >> +* Sitronix st1232 touchscreen controller
> >> +
> >> +Required properties:
> >> +- compatible: must be "sitronix,st1232"
> >> +- reg: I2C address of the chip
> >> +- interrupts: interrupt to which the chip is connected
> >> +
> >> +Optional properties:
> >> +- sitronix,reset-pin: GPIO number of the reset pin
> > 
> > GPIO bindings use phandles, please see
> > Documentation/devicetree/bindings/gpio/gpio.txt.
> 
> Ok done. I haven't tested that part yet as the pfc is not set up via the DT.
> I saw in topic/pinmux-dt that you added OF support for the pfc, but I
> couldn't apply the patch without conflicts. Do you think this part is going
> to be merged to next any time soon?

I need to rework the patches, I hope to get them merged in v3.11

> >> +Example:
> >> +
> >> +     i2c@00000000 {
> >> +             /* ... */
> >> +
> >> +             touchscreen@55 {
> >> +                     compatible = "sitronix,st1232";
> >> +                     reg = <0x55>;
> >> +                     interrupts = <2 0>;
> >> +                     sitronix,reset-pin = <166>;
> >> +             };
> >> +
> >> +             /* ... */
> >> +     };

-- 
Regards,

Laurent Pinchart