[PATCH 1/2] input: egalax_ts: switch to i2c interface before wake up

[PATCH 1/2] input: egalax_ts: switch to i2c interface before wake up

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

For HannStar (HSD100PXN1 Rev: 1-A00C11 F/W:0634) LVDS touch screen,
it has a special request for the EETI touch controller. The host
needs to trigger I2C event to device FW at booting first, and then
the FW can switch to I2C interface. Otherwise, the FW can’t  work
with I2C interface, and can't generate any interrupt when touch
the screen.

This patch send an I2C command before the device wake up, make sure
the device switch to I2C interface first.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/input/touchscreen/egalax_ts.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c
index 83ac8c128192..5e35ca5edc7b 100644
--- a/drivers/input/touchscreen/egalax_ts.c
+++ b/drivers/input/touchscreen/egalax_ts.c
@@ -183,6 +183,20 @@ static int egalax_ts_probe(struct i2c_client *client,
 	ts->client = client;
 	ts->input_dev = input_dev;
 
+	/* HannStar (HSD100PXN1 Rev: 1-A00C11 F/W:0634) LVDS touch
+	 * screen needs to trigger I2C event to device FW at booting
+	 * first, and then the FW can switch to I2C interface.
+	 * Otherwise, the FW can’t  work with I2C interface. So here
+	 * just use the exist function egalax_firmware_version() to
+	 * send a I2C command to the device, make sure the device FW
+	 * switch to I2C interface.
+	 */
+	error = egalax_firmware_version(client);
+	if (error) {
+		dev_err(&client->dev, "Failed to switch to I2C interface\n");
+		return error;
+	}
+
 	/* controller may be in sleep, wake it up. */
 	error = egalax_wake_up_device(client);
 	if (error) {
-- 
2.17.1

[PATCH 2/2] input: egalax_ts: free irq resource before request the line as GPIO

[haibo.chen]

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);
-
+	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);
-- 
2.17.1

RE: [PATCH 1/2] input: egalax_ts: switch to i2c interface before wake up

[BOUGH CHEN]

Ping...

Hi Dmitry,

Can you help review these two patches?

Haibo Chen
> -----Original Message-----
> From: haibo.chen@nxp.com <haibo.chen@nxp.com>
> Sent: 2020年2月11日 16:41
> To: dmitry.torokhov@gmail.com; linux-input@vger.kernel.org
> Cc: BOUGH CHEN <haibo.chen@nxp.com>; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: [PATCH 1/2] input: egalax_ts: switch to i2c interface before wake up
> 
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> For HannStar (HSD100PXN1 Rev: 1-A00C11 F/W:0634) LVDS touch screen, it
> has a special request for the EETI touch controller. The host needs to trigger
> I2C event to device FW at booting first, and then the FW can switch to I2C
> interface. Otherwise, the FW can’t  work with I2C interface, and can't
> generate any interrupt when touch the screen.
> 
> This patch send an I2C command before the device wake up, make sure the
> device switch to I2C interface first.
> 
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/input/touchscreen/egalax_ts.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/egalax_ts.c
> b/drivers/input/touchscreen/egalax_ts.c
> index 83ac8c128192..5e35ca5edc7b 100644
> --- a/drivers/input/touchscreen/egalax_ts.c
> +++ b/drivers/input/touchscreen/egalax_ts.c
> @@ -183,6 +183,20 @@ static int egalax_ts_probe(struct i2c_client *client,
>  	ts->client = client;
>  	ts->input_dev = input_dev;
> 
> +	/* HannStar (HSD100PXN1 Rev: 1-A00C11 F/W:0634) LVDS touch
> +	 * screen needs to trigger I2C event to device FW at booting
> +	 * first, and then the FW can switch to I2C interface.
> +	 * Otherwise, the FW can’t  work with I2C interface. So here
> +	 * just use the exist function egalax_firmware_version() to
> +	 * send a I2C command to the device, make sure the device FW
> +	 * switch to I2C interface.
> +	 */
> +	error = egalax_firmware_version(client);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to switch to I2C interface\n");
> +		return error;
> +	}
> +
>  	/* controller may be in sleep, wake it up. */
>  	error = egalax_wake_up_device(client);
>  	if (error) {
> --
> 2.17.1

Re: [PATCH 1/2] input: egalax_ts: switch to i2c interface before wake up

[Dmitry Torokhov]

Hi Haibo,

On Tue, Feb 11, 2020 at 04:41:11PM +0800, haibo.chen@nxp.com wrote:
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> For HannStar (HSD100PXN1 Rev: 1-A00C11 F/W:0634) LVDS touch screen,
> it has a special request for the EETI touch controller. The host
> needs to trigger I2C event to device FW at booting first, and then
> the FW can switch to I2C interface. Otherwise, the FW can’t  work
> with I2C interface, and can't generate any interrupt when touch
> the screen.
> 
> This patch send an I2C command before the device wake up, make sure
> the device switch to I2C interface first.
> 
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/input/touchscreen/egalax_ts.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c
> index 83ac8c128192..5e35ca5edc7b 100644
> --- a/drivers/input/touchscreen/egalax_ts.c
> +++ b/drivers/input/touchscreen/egalax_ts.c
> @@ -183,6 +183,20 @@ static int egalax_ts_probe(struct i2c_client *client,
>  	ts->client = client;
>  	ts->input_dev = input_dev;
>  
> +	/* HannStar (HSD100PXN1 Rev: 1-A00C11 F/W:0634) LVDS touch
> +	 * screen needs to trigger I2C event to device FW at booting
> +	 * first, and then the FW can switch to I2C interface.
> +	 * Otherwise, the FW can’t  work with I2C interface. So here
> +	 * just use the exist function egalax_firmware_version() to
> +	 * send a I2C command to the device, make sure the device FW
> +	 * switch to I2C interface.
> +	 */
> +	error = egalax_firmware_version(client);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to switch to I2C interface\n");
> +		return error;
> +	}
> +
>  	/* controller may be in sleep, wake it up. */
>  	error = egalax_wake_up_device(client);
>  	if (error) {

I think this should be part of egalax_wake_up_device(), in case where we
do not have dedicated reset gpio. Also, instead of abusing firmware
read, would a dummy byte read be enough (i2c_probe_func_quick_read)?

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] input: egalax_ts: free irq resource before request the line as GPIO

[Dmitry Torokhov]

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.

-- 
Dmitry

RE: [PATCH 1/2] input: egalax_ts: switch to i2c interface before wake up

[BOUGH CHEN]

> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: 2020年3月10日 12:22
> To: BOUGH CHEN <haibo.chen@nxp.com>
> Cc: linux-input@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 1/2] input: egalax_ts: switch to i2c interface before
> wake up
> 
> Hi Haibo,
> 
> On Tue, Feb 11, 2020 at 04:41:11PM +0800, haibo.chen@nxp.com wrote:
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > For HannStar (HSD100PXN1 Rev: 1-A00C11 F/W:0634) LVDS touch screen,
> it
> > has a special request for the EETI touch controller. The host needs to
> > trigger I2C event to device FW at booting first, and then the FW can
> > switch to I2C interface. Otherwise, the FW can’t  work with I2C
> > interface, and can't generate any interrupt when touch the screen.
> >
> > This patch send an I2C command before the device wake up, make sure
> > the device switch to I2C interface first.
> >
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > ---
> >  drivers/input/touchscreen/egalax_ts.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/input/touchscreen/egalax_ts.c
> > b/drivers/input/touchscreen/egalax_ts.c
> > index 83ac8c128192..5e35ca5edc7b 100644
> > --- a/drivers/input/touchscreen/egalax_ts.c
> > +++ b/drivers/input/touchscreen/egalax_ts.c
> > @@ -183,6 +183,20 @@ static int egalax_ts_probe(struct i2c_client
> *client,
> >  	ts->client = client;
> >  	ts->input_dev = input_dev;
> >
> > +	/* HannStar (HSD100PXN1 Rev: 1-A00C11 F/W:0634) LVDS touch
> > +	 * screen needs to trigger I2C event to device FW at booting
> > +	 * first, and then the FW can switch to I2C interface.
> > +	 * Otherwise, the FW can’t  work with I2C interface. So here
> > +	 * just use the exist function egalax_firmware_version() to
> > +	 * send a I2C command to the device, make sure the device FW
> > +	 * switch to I2C interface.
> > +	 */
> > +	error = egalax_firmware_version(client);
> > +	if (error) {
> > +		dev_err(&client->dev, "Failed to switch to I2C interface\n");
> > +		return error;
> > +	}
> > +
> >  	/* controller may be in sleep, wake it up. */
> >  	error = egalax_wake_up_device(client);
> >  	if (error) {
> 
> I think this should be part of egalax_wake_up_device(), in case where we do
> not have dedicated reset gpio. Also, instead of abusing firmware read, would
> a dummy byte read be enough (i2c_probe_func_quick_read)?

That's reasonable, I will move the change to egalax_wake_up_device(). I will have a try whether a dummy byte read can work.
Really thanks for your suggestion!

Regards,
Haibo Chen
> Thanks.
> 
> --
> Dmitry

RE: [PATCH 2/2] input: egalax_ts: free irq resource before request the line as GPIO

[BOUGH CHEN]

> -----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

Re: [PATCH 2/2] input: egalax_ts: free irq resource before request the line as GPIO

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 2/2] input: egalax_ts: free irq resource before request the line as GPIO

[Andy Shevchenko]

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?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] input: egalax_ts: free irq resource before request the line as GPIO

[Linus Walleij]

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