| Message ID | 1365425547-10391-1-git-send-email-hechtb+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Bastian, Thanks for the patch. On Monday 08 April 2013 14:52:26 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> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > v2: > > - using a gpio phandle now in the DT code > - rename from reset_pin to reset_gpio > - gpio number 0 is allowed now and the absence of the reset gpio is marked > with an error code > - using devm_request_gpio instead of plain request_gpio to simplify things > > .../bindings/input/touchscreen/sitronix-st1232.txt | 24 ++++++++++++ > drivers/input/touchscreen/st1232.c | 41 ++++++++++++++++- > include/linux/input/st1232_pdata.h | 13 +++++++ > 3 files changed, 76 insertions(+), 2 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..64ad48b > --- /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: > +- gpios: a phandle to the reset GPIO > + > +Example: > + > + i2c@00000000 { > + /* ... */ > + > + touchscreen@55 { > + compatible = "sitronix,st1232"; > + reg = <0x55>; > + interrupts = <2 0>; > + gpios = <&gpio1 166 0>; > + }; > + > + /* ... */ > + }; > diff --git a/drivers/input/touchscreen/st1232.c > b/drivers/input/touchscreen/st1232.c index d9d05e2..498d18b 100644 > --- a/drivers/input/touchscreen/st1232.c > +++ b/drivers/input/touchscreen/st1232.c > @@ -19,13 +19,16 @@ > */ > > #include <linux/delay.h> > +#include <linux/gpio.h> > #include <linux/i2c.h> > #include <linux/input.h> > #include <linux/interrupt.h> > #include <linux/module.h> > +#include <linux/of_gpio.h> > #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 +51,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_gpio; > }; > > static int st1232_ts_read_data(struct st1232_ts_data *ts) > @@ -139,10 +143,17 @@ end: > return IRQ_HANDLED; > } > > +static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron) > +{ > + if (ts->reset_gpio >= 0) > + gpio_direction_output(ts->reset_gpio, 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 +178,25 @@ static int st1232_ts_probe(struct i2c_client *client, > ts->client = client; > ts->input_dev = input_dev; > > + if (client->dev.of_node) > + ts->reset_gpio = of_get_gpio(client->dev.of_node, 0); > + else if (pdata) > + ts->reset_gpio = pdata->reset_gpio; > + else > + ts->reset_gpio = -ENODEV; > + > + if (ts->reset_gpio >= 0) { > + error = devm_gpio_request(&client->dev, ts->reset_gpio, NULL); > + if (error) { > + dev_err(&client->dev, > + "Unable to request GPIO pin %d.\n", > + ts->reset_gpio); > + 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; > @@ -210,6 +240,7 @@ 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); > input_unregister_device(ts->input_dev); > @@ -222,11 +253,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 +268,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..cac3e7b > --- /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_gpio; > +}; > + > +#endif
On Mon, Apr 08, 2013 at 04:17:20PM +0200, Laurent Pinchart wrote: > Hi Bastian, > > Thanks for the patch. > > On Monday 08 April 2013 14:52:26 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> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Bastian, I'm unclear on which tree this will go through. Please let me know if it is mine. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Simon, 2013/4/9 Simon Horman <horms@verge.net.au>: > On Mon, Apr 08, 2013 at 04:17:20PM +0200, Laurent Pinchart wrote: >> Hi Bastian, >> >> Thanks for the patch. >> >> On Monday 08 April 2013 14:52:26 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> >> >> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Bastian, I'm unclear on which tree this will go through. > Please let me know if it is mine. To be honest, no idea. If it doesn't have to be a maintainer of linux-input, I would be glad if you could take up that patch in your tree, yes. And for your information, I don't rely on it until the pfc DT version is merged. Then it would be nice to have it for the touchscreen in the arma reference implementation. Thanks, Bastian -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[ Cc Dimitry Torokhov ] On Tue, Apr 09, 2013 at 11:23:51AM +0200, Bastian Hecht wrote: > Hi Simon, > > 2013/4/9 Simon Horman <horms@verge.net.au>: > > On Mon, Apr 08, 2013 at 04:17:20PM +0200, Laurent Pinchart wrote: > >> Hi Bastian, > >> > >> Thanks for the patch. > >> > >> On Monday 08 April 2013 14:52:26 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> > >> > >> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Bastian, I'm unclear on which tree this will go through. > > Please let me know if it is mine. > > To be honest, no idea. If it doesn't have to be a maintainer of > linux-input, I would be glad if you could take up that patch in your > tree, yes. And for your information, I don't rely on it until the pfc > DT version is merged. Then it would be nice to have it for the > touchscreen in the arma reference implementation. Dimitry, you seem to have signed-off on most of the st1232 I see in the git history. Could you take this patch? There is an accompanying patch for the Armaillo800EVA which depends on this change, so it would be good if you could take that too. Alternatively I can take both patches through my renesas tree if that is better for you. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bastian, On Mon, Apr 08, 2013 at 02:52:26PM +0200, Bastian Hecht wrote: > +static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron) > +{ > + if (ts->reset_gpio >= 0) This should be gpio_is_valid() I think. > + gpio_direction_output(ts->reset_gpio, 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 +178,25 @@ static int st1232_ts_probe(struct i2c_client *client, > ts->client = client; > ts->input_dev = input_dev; > > + if (client->dev.of_node) > + ts->reset_gpio = of_get_gpio(client->dev.of_node, 0); > + else if (pdata) > + ts->reset_gpio = pdata->reset_gpio; I believe we should favor platform data, then firmware, so that you can override if necessary. > + else > + ts->reset_gpio = -ENODEV; > + > + if (ts->reset_gpio >= 0) { gpio_is_valid() again? > + error = devm_gpio_request(&client->dev, ts->reset_gpio, NULL); Frankly I do not like mixing managed and noon-managed resources. Maybe the entire driver needs to be converted to managed resources? Thanks.
On Wed, Apr 10, 2013 at 12:01:06AM +0900, Simon Horman wrote: > [ Cc Dimitry Torokhov ] > > On Tue, Apr 09, 2013 at 11:23:51AM +0200, Bastian Hecht wrote: > > Hi Simon, > > > > 2013/4/9 Simon Horman <horms@verge.net.au>: > > > On Mon, Apr 08, 2013 at 04:17:20PM +0200, Laurent Pinchart wrote: > > >> Hi Bastian, > > >> > > >> Thanks for the patch. > > >> > > >> On Monday 08 April 2013 14:52:26 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> > > >> > > >> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > Bastian, I'm unclear on which tree this will go through. > > > Please let me know if it is mine. > > > > To be honest, no idea. If it doesn't have to be a maintainer of > > linux-input, I would be glad if you could take up that patch in your > > tree, yes. And for your information, I don't rely on it until the pfc > > DT version is merged. Then it would be nice to have it for the > > touchscreen in the arma reference implementation. > > Dimitry, > > you seem to have signed-off on most of the st1232 I see in the git history. > Could you take this patch? Yes, I will, once feedback is addressed. Thanks.
On Tue, Apr 09, 2013 at 11:51:57PM -0700, Dmitry Torokhov wrote: > On Wed, Apr 10, 2013 at 12:01:06AM +0900, Simon Horman wrote: > > [ Cc Dimitry Torokhov ] > > > > On Tue, Apr 09, 2013 at 11:23:51AM +0200, Bastian Hecht wrote: > > > Hi Simon, > > > > > > 2013/4/9 Simon Horman <horms@verge.net.au>: > > > > On Mon, Apr 08, 2013 at 04:17:20PM +0200, Laurent Pinchart wrote: > > > >> Hi Bastian, > > > >> > > > >> Thanks for the patch. > > > >> > > > >> On Monday 08 April 2013 14:52:26 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> > > > >> > > > >> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > Bastian, I'm unclear on which tree this will go through. > > > > Please let me know if it is mine. > > > > > > To be honest, no idea. If it doesn't have to be a maintainer of > > > linux-input, I would be glad if you could take up that patch in your > > > tree, yes. And for your information, I don't rely on it until the pfc > > > DT version is merged. Then it would be nice to have it for the > > > touchscreen in the arma reference implementation. > > > > Dimitry, > > > > you seem to have signed-off on most of the st1232 I see in the git history. > > Could you take this patch? > > Yes, I will, once feedback is addressed. Great, thanks. Please let me know if you need anything from me. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dmitry, On Tuesday 09 April 2013 23:51:14 Dmitry Torokhov wrote: > On Mon, Apr 08, 2013 at 02:52:26PM +0200, Bastian Hecht wrote: [snip] > > + error = devm_gpio_request(&client->dev, ts->reset_gpio, NULL); > > Frankly I do not like mixing managed and noon-managed resources. Maybe > the entire driver needs to be converted to managed resources? I've just sent a patch to do so.
Hi all, 2013/4/10 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > Hi Dmitry, > > On Tuesday 09 April 2013 23:51:14 Dmitry Torokhov wrote: >> On Mon, Apr 08, 2013 at 02:52:26PM +0200, Bastian Hecht wrote: > > [snip] > >> > + error = devm_gpio_request(&client->dev, ts->reset_gpio, NULL); >> >> Frankly I do not like mixing managed and noon-managed resources. Maybe >> the entire driver needs to be converted to managed resources? > > I've just sent a patch to do so. Thanks Laurent! I've tested it - works fine. Dimitri, I've addressed your suggestions and use gpio_valid and have reordered the probing for platform and OF data. Thanks for your feedback. I've based my changes on top of it and will post it shortly. Cheers, Bastian -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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..64ad48b --- /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: +- gpios: a phandle to the reset GPIO + +Example: + + i2c@00000000 { + /* ... */ + + touchscreen@55 { + compatible = "sitronix,st1232"; + reg = <0x55>; + interrupts = <2 0>; + gpios = <&gpio1 166 0>; + }; + + /* ... */ + }; diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c index d9d05e2..498d18b 100644 --- a/drivers/input/touchscreen/st1232.c +++ b/drivers/input/touchscreen/st1232.c @@ -19,13 +19,16 @@ */ #include <linux/delay.h> +#include <linux/gpio.h> #include <linux/i2c.h> #include <linux/input.h> #include <linux/interrupt.h> #include <linux/module.h> +#include <linux/of_gpio.h> #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 +51,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_gpio; }; static int st1232_ts_read_data(struct st1232_ts_data *ts) @@ -139,10 +143,17 @@ end: return IRQ_HANDLED; } +static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron) +{ + if (ts->reset_gpio >= 0) + gpio_direction_output(ts->reset_gpio, 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 +178,25 @@ static int st1232_ts_probe(struct i2c_client *client, ts->client = client; ts->input_dev = input_dev; + if (client->dev.of_node) + ts->reset_gpio = of_get_gpio(client->dev.of_node, 0); + else if (pdata) + ts->reset_gpio = pdata->reset_gpio; + else + ts->reset_gpio = -ENODEV; + + if (ts->reset_gpio >= 0) { + error = devm_gpio_request(&client->dev, ts->reset_gpio, NULL); + if (error) { + dev_err(&client->dev, + "Unable to request GPIO pin %d.\n", + ts->reset_gpio); + 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; @@ -210,6 +240,7 @@ 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); input_unregister_device(ts->input_dev); @@ -222,11 +253,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 +268,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..cac3e7b --- /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_gpio; +}; + +#endif
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> --- v2: - using a gpio phandle now in the DT code - rename from reset_pin to reset_gpio - gpio number 0 is allowed now and the absence of the reset gpio is marked with an error code - using devm_request_gpio instead of plain request_gpio to simplify things .../bindings/input/touchscreen/sitronix-st1232.txt | 24 ++++++++++++ drivers/input/touchscreen/st1232.c | 41 +++++++++++++++++++- include/linux/input/st1232_pdata.h | 13 +++++++ 3 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt create mode 100644 include/linux/input/st1232_pdata.h