| Message ID | 20170111172841.9825-1-gary.bisson@boundarydevices.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Gary, On Wed, Jan 11, 2017 at 06:28:41PM +0100, Gary Bisson wrote: > Thus preventing anyone to later modify the interrupt GPIO direction > and/or state without the driver knowing. I am afraid not releasing gpio after waking up the controller will cause request_irq to fail if we are using the same pin for interrupt and wakeup (as majority of current DTSes do: see arch/arm/boot/dts/imx53-tx53-x13x.dts for example). You'll need to figure out if irq is backed by the same gpio as wakeup and act accordingly. What setup did you test this on? Was it shared pin or dedicated wakeup? Thanks. > > Also checking if device is present before allocating the input device. > > Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com> > --- > drivers/input/touchscreen/egalax_ts.c | 56 ++++++++++++++++------------------- > 1 file changed, 25 insertions(+), 31 deletions(-) > > diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c > index 1afc08b08155..f6b94bb19bd8 100644 > --- a/drivers/input/touchscreen/egalax_ts.c > +++ b/drivers/input/touchscreen/egalax_ts.c > @@ -62,6 +62,7 @@ > struct egalax_ts { > struct i2c_client *client; > struct input_dev *input_dev; > + struct gpio_desc *wakeup_gpio; > }; > > static irqreturn_t egalax_ts_interrupt(int irq, void *dev_id) > @@ -120,36 +121,21 @@ static irqreturn_t egalax_ts_interrupt(int irq, void *dev_id) > } > > /* wake up controller by an falling edge of interrupt gpio. */ > -static int egalax_wake_up_device(struct i2c_client *client) > +static int egalax_wake_up_device(struct gpio_desc *wakeup_gpio) > { > - struct device_node *np = client->dev.of_node; > - int gpio; > int ret; > > - if (!np) > - return -ENODEV; > - > - gpio = of_get_named_gpio(np, "wakeup-gpios", 0); > - if (!gpio_is_valid(gpio)) > - return -ENODEV; > - > - ret = gpio_request(gpio, "egalax_irq"); > - if (ret < 0) { > - dev_err(&client->dev, > - "request gpio failed, cannot wake up controller: %d\n", > - ret); > + /* wake up controller via an falling edge on IRQ gpio. */ > + ret = gpiod_direction_output(wakeup_gpio, 0); > + if (ret < 0) > return ret; > - } > > - /* wake up controller via an falling edge on IRQ gpio. */ > - gpio_direction_output(gpio, 0); > - gpio_set_value(gpio, 1); > + gpiod_set_value(wakeup_gpio, 1); > > /* controller should be waken up, return irq. */ > - gpio_direction_input(gpio); > - gpio_free(gpio); > + ret = gpiod_direction_input(wakeup_gpio); > > - return 0; > + return ret; > } > > static int egalax_firmware_version(struct i2c_client *client) > @@ -177,17 +163,15 @@ static int egalax_ts_probe(struct i2c_client *client, > return -ENOMEM; > } > > - input_dev = devm_input_allocate_device(&client->dev); > - if (!input_dev) { > - dev_err(&client->dev, "Failed to allocate memory\n"); > - return -ENOMEM; > + ts->wakeup_gpio = devm_gpiod_get_index(&client->dev, "wakeup", > + 0, GPIOD_ASIS); > + if (IS_ERR(ts->wakeup_gpio)) { > + dev_err(&client->dev, "Failed to get wakeup gpio"); > + return -ENODEV; > } > > - ts->client = client; > - ts->input_dev = input_dev; > - > /* controller may be in sleep, wake it up. */ > - error = egalax_wake_up_device(client); > + error = egalax_wake_up_device(ts->wakeup_gpio); > if (error) { > dev_err(&client->dev, "Failed to wake up the controller\n"); > return error; > @@ -199,6 +183,15 @@ static int egalax_ts_probe(struct i2c_client *client, > return error; > } > > + input_dev = devm_input_allocate_device(&client->dev); > + if (!input_dev) { > + dev_err(&client->dev, "Failed to allocate memory\n"); > + return -ENOMEM; > + } > + > + ts->client = client; > + ts->input_dev = input_dev; > + > input_dev->name = "EETI eGalax Touch Screen"; > input_dev->id.bustype = BUS_I2C; > > @@ -254,8 +247,9 @@ static int __maybe_unused egalax_ts_suspend(struct device *dev) > static int __maybe_unused egalax_ts_resume(struct device *dev) > { > struct i2c_client *client = to_i2c_client(dev); > + struct egalax_ts *ts = i2c_get_clientdata(client); > > - return egalax_wake_up_device(client); > + return egalax_wake_up_device(ts->wakeup_gpio); > } > > static SIMPLE_DEV_PM_OPS(egalax_ts_pm_ops, egalax_ts_suspend, egalax_ts_resume); > -- > 2.11.0 >
Hi Dmitri, On Thu, Jan 12, 2017 at 10:30:19AM -0800, Dmitry Torokhov wrote: > Hi Gary, > > On Wed, Jan 11, 2017 at 06:28:41PM +0100, Gary Bisson wrote: > > Thus preventing anyone to later modify the interrupt GPIO direction > > and/or state without the driver knowing. > > I am afraid not releasing gpio after waking up the controller will cause > request_irq to fail if we are using the same pin for interrupt and > wakeup (as majority of current DTSes do: see > arch/arm/boot/dts/imx53-tx53-x13x.dts for example). No, keeping the GPIO doesn't prevent from requesting the IRQ. However it keeps other drivers/users from changing the GPIO as output later on. > You'll need to figure out if irq is backed by the same gpio as wakeup > and act accordingly. > > What setup did you test this on? Was it shared pin or dedicated wakeup? I've tested on i.MX6Q Nitrogen6x [1] with Hannstar 10" display [2]. Regards, Gary [1] https://boundarydevices.com/product/nitrogen6x-board-imx6-arm-cortex-a9-sbc/ [2] https://boundarydevices.com/product/nit6x_10-1hannstar/ -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 13, 2017 at 12:03:43AM +0100, Gary Bisson wrote: > Hi Dmitri, > > On Thu, Jan 12, 2017 at 10:30:19AM -0800, Dmitry Torokhov wrote: > > Hi Gary, > > > > On Wed, Jan 11, 2017 at 06:28:41PM +0100, Gary Bisson wrote: > > > Thus preventing anyone to later modify the interrupt GPIO direction > > > and/or state without the driver knowing. > > > > I am afraid not releasing gpio after waking up the controller will cause > > request_irq to fail if we are using the same pin for interrupt and > > wakeup (as majority of current DTSes do: see > > arch/arm/boot/dts/imx53-tx53-x13x.dts for example). > > No, keeping the GPIO doesn't prevent from requesting the IRQ. > > However it keeps other drivers/users from changing the GPIO as output > later on. Hmm, I think _gpiod_direction_output_raw() will not let them as it checks FLAG_USED_AS_IRQ, so as long as it's the same line it's OK. But I guess if they are separate some other component might try to grab it and mess with it. OK, in this case please: - use devm_gpiod_get - request with GPIOD_OUT_LOW - do not override the return value with -ENODEV (especially important with probe deferrals) - lose gpiod_direction_output() call - move everything into egalax_ts_probe() as egalax_wake_up_device() is no longer self-contained. Thanks!
Hi Dmitri, On Fri, Jan 13, 2017 at 12:40 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Fri, Jan 13, 2017 at 12:03:43AM +0100, Gary Bisson wrote: >> Hi Dmitri, >> >> On Thu, Jan 12, 2017 at 10:30:19AM -0800, Dmitry Torokhov wrote: >> > Hi Gary, >> > >> > On Wed, Jan 11, 2017 at 06:28:41PM +0100, Gary Bisson wrote: >> > > Thus preventing anyone to later modify the interrupt GPIO direction >> > > and/or state without the driver knowing. >> > >> > I am afraid not releasing gpio after waking up the controller will cause >> > request_irq to fail if we are using the same pin for interrupt and >> > wakeup (as majority of current DTSes do: see >> > arch/arm/boot/dts/imx53-tx53-x13x.dts for example). >> >> No, keeping the GPIO doesn't prevent from requesting the IRQ. >> >> However it keeps other drivers/users from changing the GPIO as output >> later on. > > > Hmm, I think _gpiod_direction_output_raw() will not let them as it > checks FLAG_USED_AS_IRQ, so as long as it's the same line it's OK. But I > guess if they are separate some other component might try to grab it and > mess with it. > > OK, in this case please: > > - use devm_gpiod_get > - request with GPIOD_OUT_LOW > - do not override the return value with -ENODEV (especially important > with probe deferrals) > - lose gpiod_direction_output() call > - move everything into egalax_ts_probe() as egalax_wake_up_device() is > no longer self-contained. Thanks for reviewing. I'll try to do the v2 this week. Regards, Gary -- To unsubscribe from this list: send the line "unsubscribe linux-input" 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/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c index 1afc08b08155..f6b94bb19bd8 100644 --- a/drivers/input/touchscreen/egalax_ts.c +++ b/drivers/input/touchscreen/egalax_ts.c @@ -62,6 +62,7 @@ struct egalax_ts { struct i2c_client *client; struct input_dev *input_dev; + struct gpio_desc *wakeup_gpio; }; static irqreturn_t egalax_ts_interrupt(int irq, void *dev_id) @@ -120,36 +121,21 @@ static irqreturn_t egalax_ts_interrupt(int irq, void *dev_id) } /* wake up controller by an falling edge of interrupt gpio. */ -static int egalax_wake_up_device(struct i2c_client *client) +static int egalax_wake_up_device(struct gpio_desc *wakeup_gpio) { - struct device_node *np = client->dev.of_node; - int gpio; int ret; - if (!np) - return -ENODEV; - - gpio = of_get_named_gpio(np, "wakeup-gpios", 0); - if (!gpio_is_valid(gpio)) - return -ENODEV; - - ret = gpio_request(gpio, "egalax_irq"); - if (ret < 0) { - dev_err(&client->dev, - "request gpio failed, cannot wake up controller: %d\n", - ret); + /* wake up controller via an falling edge on IRQ gpio. */ + ret = gpiod_direction_output(wakeup_gpio, 0); + if (ret < 0) return ret; - } - /* wake up controller via an falling edge on IRQ gpio. */ - gpio_direction_output(gpio, 0); - gpio_set_value(gpio, 1); + gpiod_set_value(wakeup_gpio, 1); /* controller should be waken up, return irq. */ - gpio_direction_input(gpio); - gpio_free(gpio); + ret = gpiod_direction_input(wakeup_gpio); - return 0; + return ret; } static int egalax_firmware_version(struct i2c_client *client) @@ -177,17 +163,15 @@ static int egalax_ts_probe(struct i2c_client *client, return -ENOMEM; } - input_dev = devm_input_allocate_device(&client->dev); - if (!input_dev) { - dev_err(&client->dev, "Failed to allocate memory\n"); - return -ENOMEM; + ts->wakeup_gpio = devm_gpiod_get_index(&client->dev, "wakeup", + 0, GPIOD_ASIS); + if (IS_ERR(ts->wakeup_gpio)) { + dev_err(&client->dev, "Failed to get wakeup gpio"); + return -ENODEV; } - ts->client = client; - ts->input_dev = input_dev; - /* controller may be in sleep, wake it up. */ - error = egalax_wake_up_device(client); + error = egalax_wake_up_device(ts->wakeup_gpio); if (error) { dev_err(&client->dev, "Failed to wake up the controller\n"); return error; @@ -199,6 +183,15 @@ static int egalax_ts_probe(struct i2c_client *client, return error; } + input_dev = devm_input_allocate_device(&client->dev); + if (!input_dev) { + dev_err(&client->dev, "Failed to allocate memory\n"); + return -ENOMEM; + } + + ts->client = client; + ts->input_dev = input_dev; + input_dev->name = "EETI eGalax Touch Screen"; input_dev->id.bustype = BUS_I2C; @@ -254,8 +247,9 @@ static int __maybe_unused egalax_ts_suspend(struct device *dev) static int __maybe_unused egalax_ts_resume(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); + struct egalax_ts *ts = i2c_get_clientdata(client); - return egalax_wake_up_device(client); + return egalax_wake_up_device(ts->wakeup_gpio); } static SIMPLE_DEV_PM_OPS(egalax_ts_pm_ops, egalax_ts_suspend, egalax_ts_resume);
Thus preventing anyone to later modify the interrupt GPIO direction and/or state without the driver knowing. Also checking if device is present before allocating the input device. Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com> --- drivers/input/touchscreen/egalax_ts.c | 56 ++++++++++++++++------------------- 1 file changed, 25 insertions(+), 31 deletions(-)