| Message ID | 1581410472-3225-2-git-send-email-haibo.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] input: egalax_ts: switch to i2c interface before wake up | expand |
On Tue, Feb 11, 2020 at 04:41:12PM +0800, haibo.chen@nxp.com wrote: > From: Haibo Chen <haibo.chen@nxp.com> > > If GPIO is connected to an IRQ then it should not request it as > GPIO function only when free its IRQ resource. > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > --- > drivers/input/touchscreen/egalax_ts.c | 44 +++++++++++++++++++-------- > 1 file changed, 31 insertions(+), 13 deletions(-) > > diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c > index 5e35ca5edc7b..c7983104a0b9 100644 > --- a/drivers/input/touchscreen/egalax_ts.c > +++ b/drivers/input/touchscreen/egalax_ts.c > @@ -116,6 +116,26 @@ static irqreturn_t egalax_ts_interrupt(int irq, void *dev_id) > return IRQ_HANDLED; > } > > +static int egalax_irq_request(struct egalax_ts *ts) > +{ > + int ret; > + struct i2c_client *client = ts->client; > + > + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > + egalax_ts_interrupt, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > + "egalax_ts", ts); > + if (ret < 0) > + dev_err(&client->dev, "Failed to register interrupt\n"); > + > + return ret; > +} > + > +static void egalax_free_irq(struct egalax_ts *ts) > +{ > + devm_free_irq(&ts->client->dev, ts->client->irq, ts); > +} > + > /* wake up controller by an falling edge of interrupt gpio. */ > static int egalax_wake_up_device(struct i2c_client *client) > { > @@ -225,19 +245,15 @@ static int egalax_ts_probe(struct i2c_client *client, > ABS_MT_POSITION_Y, 0, EGALAX_MAX_Y, 0, 0); > input_mt_init_slots(input_dev, MAX_SUPPORT_POINTS, 0); > > - error = devm_request_threaded_irq(&client->dev, client->irq, NULL, > - egalax_ts_interrupt, > - IRQF_TRIGGER_LOW | IRQF_ONESHOT, > - "egalax_ts", ts); > - if (error < 0) { > - dev_err(&client->dev, "Failed to register interrupt\n"); > + error = egalax_irq_request(ts); > + if (error) > return error; > - } > > error = input_register_device(ts->input_dev); > if (error) > return error; > > + i2c_set_clientdata(client, ts); > return 0; > } > > @@ -253,11 +269,10 @@ static int __maybe_unused egalax_ts_suspend(struct device *dev) > 0x3, 0x6, 0xa, 0x3, 0x36, 0x3f, 0x2, 0, 0, 0 > }; > struct i2c_client *client = to_i2c_client(dev); > + struct egalax_ts *ts = i2c_get_clientdata(client); > int ret; > > - if (device_may_wakeup(dev)) > - return enable_irq_wake(client->irq); > - Why are you removing handling of the interrupt as a wakeup source if device is configured for wakeup? I guess it is a noop and I2C core does it for us on newer kernels anyways, but such cleanup should be in a separate patch. Still, you do not want to put the controller to sleep if it is supposed to be a wakeup source. > + egalax_free_irq(ts); It sounds to me you want simply disable interrupts in suspend. Does not calling disable_irq() here suffice? Thanks.
> -----Original Message----- > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Sent: 2020年3月10日 12:27 > To: BOUGH CHEN <haibo.chen@nxp.com> > Cc: linux-input@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH 2/2] input: egalax_ts: free irq resource before request > the line as GPIO > > On Tue, Feb 11, 2020 at 04:41:12PM +0800, haibo.chen@nxp.com wrote: > > From: Haibo Chen <haibo.chen@nxp.com> > > > > If GPIO is connected to an IRQ then it should not request it as GPIO > > function only when free its IRQ resource. > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > --- > > drivers/input/touchscreen/egalax_ts.c | 44 > > +++++++++++++++++++-------- > > 1 file changed, 31 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/input/touchscreen/egalax_ts.c > > b/drivers/input/touchscreen/egalax_ts.c > > index 5e35ca5edc7b..c7983104a0b9 100644 > > --- a/drivers/input/touchscreen/egalax_ts.c > > +++ b/drivers/input/touchscreen/egalax_ts.c > > @@ -116,6 +116,26 @@ static irqreturn_t egalax_ts_interrupt(int irq, void > *dev_id) > > return IRQ_HANDLED; > > } > > > > +static int egalax_irq_request(struct egalax_ts *ts) { > > + int ret; > > + struct i2c_client *client = ts->client; > > + > > + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > > + egalax_ts_interrupt, > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > + "egalax_ts", ts); > > + if (ret < 0) > > + dev_err(&client->dev, "Failed to register interrupt\n"); > > + > > + return ret; > > +} > > + > > +static void egalax_free_irq(struct egalax_ts *ts) { > > + devm_free_irq(&ts->client->dev, ts->client->irq, ts); } > > + > > /* wake up controller by an falling edge of interrupt gpio. */ > > static int egalax_wake_up_device(struct i2c_client *client) { @@ > > -225,19 +245,15 @@ static int egalax_ts_probe(struct i2c_client *client, > > ABS_MT_POSITION_Y, 0, EGALAX_MAX_Y, 0, 0); > > input_mt_init_slots(input_dev, MAX_SUPPORT_POINTS, 0); > > > > - error = devm_request_threaded_irq(&client->dev, client->irq, NULL, > > - egalax_ts_interrupt, > > - IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > - "egalax_ts", ts); > > - if (error < 0) { > > - dev_err(&client->dev, "Failed to register interrupt\n"); > > + error = egalax_irq_request(ts); > > + if (error) > > return error; > > - } > > > > error = input_register_device(ts->input_dev); > > if (error) > > return error; > > > > + i2c_set_clientdata(client, ts); > > return 0; > > } > > > > @@ -253,11 +269,10 @@ static int __maybe_unused > egalax_ts_suspend(struct device *dev) > > 0x3, 0x6, 0xa, 0x3, 0x36, 0x3f, 0x2, 0, 0, 0 > > }; > > struct i2c_client *client = to_i2c_client(dev); > > + struct egalax_ts *ts = i2c_get_clientdata(client); > > int ret; > > > > - if (device_may_wakeup(dev)) > > - return enable_irq_wake(client->irq); > > - > > Why are you removing handling of the interrupt as a wakeup source if device > is configured for wakeup? I guess it is a noop and I2C core does it for us on > newer kernels anyways, but such cleanup should be in a separate patch. > Still, you do not want to put the controller to sleep if it is supposed to be a > wakeup source. Agree, I will keep the code there. I will add a code in the egalax_ts_resume(), if the device my wakeup, skip egalax_wake_up_device(), return directly. > > > + egalax_free_irq(ts); > > It sounds to me you want simply disable interrupts in suspend. Does not > calling disable_irq() here suffice? Here why I want to disable interrupts here is because in the newest gpio system, if the gpio is request as an irq, it can't be request as a gpio anymore. In the function egalax_wake_up_device(), we need to request the irq pin as a gpio for a while, generate a signal to wake up the device. So before request the pad as gpio, need first free irq resource. Seems there are two methods, one is free irq in egalax_ts_suspend(), anther is just free irq in egalax_ts_suspend(), after the gpio operation done, request the irq resource back, which one do you prefer? Best Regards Haibo Chen > > Thanks. > > -- > Dmitry
On Tue, Mar 10, 2020 at 07:15:35AM +0000, BOUGH CHEN wrote: > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Sent: 2020年3月10日 12:27 > > To: BOUGH CHEN <haibo.chen@nxp.com> > > Cc: linux-input@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > Subject: Re: [PATCH 2/2] input: egalax_ts: free irq resource before request > > the line as GPIO > > > > On Tue, Feb 11, 2020 at 04:41:12PM +0800, haibo.chen@nxp.com wrote: > > > From: Haibo Chen <haibo.chen@nxp.com> > > > > > > + egalax_free_irq(ts); > > > > It sounds to me you want simply disable interrupts in suspend. Does not > > calling disable_irq() here suffice? > > Here why I want to disable interrupts here is because in the newest > gpio system, if the gpio is request as an irq, it can't be request as > a gpio anymore. In the function egalax_wake_up_device(), we need to > request the irq pin as a gpio for a while, generate a signal to wake > up the device. So before request the pad as gpio, need first free irq > resource. This seems like a fairly common pattern and I wonder if our GPIO overlords can help us here. Linus, Mika, Andy, would it be possible to have an API that would allow driver to temporarily "take over" GPIO that is used for interrupts and drive it as output without resorting to freeing and re-acquiring irq? I.e. something like gpiod_irq_drive_output_start() and gpiod_irq_drive_output_end()? Thanks.
On Sun, Apr 19, 2020 at 9:10 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Tue, Mar 10, 2020 at 07:15:35AM +0000, BOUGH CHEN wrote: > > > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > Sent: 2020年3月10日 12:27 > > > To: BOUGH CHEN <haibo.chen@nxp.com> > > > Cc: linux-input@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > > Subject: Re: [PATCH 2/2] input: egalax_ts: free irq resource before request > > > the line as GPIO > > > > > > On Tue, Feb 11, 2020 at 04:41:12PM +0800, haibo.chen@nxp.com wrote: > > > > From: Haibo Chen <haibo.chen@nxp.com> > > > > > > > > + egalax_free_irq(ts); > > > > > > It sounds to me you want simply disable interrupts in suspend. Does not > > > calling disable_irq() here suffice? > > > > > Here why I want to disable interrupts here is because in the newest > > gpio system, if the gpio is request as an irq, it can't be request as > > a gpio anymore. In the function egalax_wake_up_device(), we need to > > request the irq pin as a gpio for a while, generate a signal to wake > > up the device. So before request the pad as gpio, need first free irq > > resource. > > This seems like a fairly common pattern and I wonder if our GPIO > overlords can help us here. > > Linus, Mika, Andy, would it be possible to have an API that would allow > driver to temporarily "take over" GPIO that is used for interrupts and > drive it as output without resorting to freeing and re-acquiring irq? > I.e. something like gpiod_irq_drive_output_start() and > gpiod_irq_drive_output_end()? Shouldn't it be handled by pinctrl states? sleep state / default state / etc?
On Sun, Apr 19, 2020 at 8:10 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Tue, Mar 10, 2020 at 07:15:35AM +0000, BOUGH CHEN wrote: > > > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > Sent: 2020年3月10日 12:27 > > > To: BOUGH CHEN <haibo.chen@nxp.com> > > > Cc: linux-input@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > > > Subject: Re: [PATCH 2/2] input: egalax_ts: free irq resource before request > > > the line as GPIO > > > > > > On Tue, Feb 11, 2020 at 04:41:12PM +0800, haibo.chen@nxp.com wrote: > > > > From: Haibo Chen <haibo.chen@nxp.com> > > > > > > > > + egalax_free_irq(ts); > > > > > > It sounds to me you want simply disable interrupts in suspend. Does not > > > calling disable_irq() here suffice? > > > > > Here why I want to disable interrupts here is because in the newest > > gpio system, if the gpio is request as an irq, it can't be request as > > a gpio anymore. In the function egalax_wake_up_device(), we need to > > request the irq pin as a gpio for a while, generate a signal to wake > > up the device. So before request the pad as gpio, need first free irq > > resource. > > This seems like a fairly common pattern and I wonder if our GPIO > overlords can help us here. > > Linus, Mika, Andy, would it be possible to have an API that would allow > driver to temporarily "take over" GPIO that is used for interrupts and > drive it as output without resorting to freeing and re-acquiring irq? > I.e. something like gpiod_irq_drive_output_start() and > gpiod_irq_drive_output_end()? Hans Verkuil had exactly this problem in the CEC GPIO driver, drivers/media/platform/cec-gpio/cec-gpio.c If you call disable_irq() on an irqchip implemented in a GPIO driver using the generic GPIOLIB_IRQCHIP helper library (which I really recommend) then the line can be switched to output, used like such, then switched to input and enable_irq() be called again. See these commits: commit 4e9439ddacea06f35acce4d374bf6bd0acf99bc8 "gpiolib: add flag to indicate if the irq is disabled" commit 461c1a7d4733d1dfd5c47b040cf32a5e7eefbc6c "gpiolib: override irq_enable/disable" If your corresponding GPIO driver does not use GPIOLIB_IRQCHIP then you have a bigger problem and we need to look into that. Yours, Linus Walleij
diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c index 5e35ca5edc7b..c7983104a0b9 100644 --- a/drivers/input/touchscreen/egalax_ts.c +++ b/drivers/input/touchscreen/egalax_ts.c @@ -116,6 +116,26 @@ static irqreturn_t egalax_ts_interrupt(int irq, void *dev_id) return IRQ_HANDLED; } +static int egalax_irq_request(struct egalax_ts *ts) +{ + int ret; + struct i2c_client *client = ts->client; + + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, + egalax_ts_interrupt, + IRQF_TRIGGER_LOW | IRQF_ONESHOT, + "egalax_ts", ts); + if (ret < 0) + dev_err(&client->dev, "Failed to register interrupt\n"); + + return ret; +} + +static void egalax_free_irq(struct egalax_ts *ts) +{ + devm_free_irq(&ts->client->dev, ts->client->irq, ts); +} + /* wake up controller by an falling edge of interrupt gpio. */ static int egalax_wake_up_device(struct i2c_client *client) { @@ -225,19 +245,15 @@ static int egalax_ts_probe(struct i2c_client *client, ABS_MT_POSITION_Y, 0, EGALAX_MAX_Y, 0, 0); input_mt_init_slots(input_dev, MAX_SUPPORT_POINTS, 0); - error = devm_request_threaded_irq(&client->dev, client->irq, NULL, - egalax_ts_interrupt, - IRQF_TRIGGER_LOW | IRQF_ONESHOT, - "egalax_ts", ts); - if (error < 0) { - dev_err(&client->dev, "Failed to register interrupt\n"); + error = egalax_irq_request(ts); + if (error) return error; - } error = input_register_device(ts->input_dev); if (error) return error; + i2c_set_clientdata(client, ts); return 0; } @@ -253,11 +269,10 @@ static int __maybe_unused egalax_ts_suspend(struct device *dev) 0x3, 0x6, 0xa, 0x3, 0x36, 0x3f, 0x2, 0, 0, 0 }; struct i2c_client *client = to_i2c_client(dev); + struct egalax_ts *ts = i2c_get_clientdata(client); int ret; - if (device_may_wakeup(dev)) - return enable_irq_wake(client->irq); - + egalax_free_irq(ts); ret = i2c_master_send(client, suspend_cmd, MAX_I2C_DATA_LEN); return ret > 0 ? 0 : ret; } @@ -265,11 +280,14 @@ 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); + int ret; - if (device_may_wakeup(dev)) - return disable_irq_wake(client->irq); + ret = egalax_wake_up_device(client); + if (!ret) + ret = egalax_irq_request(ts); - return egalax_wake_up_device(client); + return ret; } static SIMPLE_DEV_PM_OPS(egalax_ts_pm_ops, egalax_ts_suspend, egalax_ts_resume);