usb: typec: tps6598x: Add support for polling interrupts status

[PATCH] usb: typec: tps6598x: Add support for polling interrupts status

Posted by Roger Quadros 2 years, 10 months ago

From: Aswath Govindraju <a-govindraju@ti.com>

Some development boards don't have the interrupt line connected.

In such cases we can resort to polling the interrupt status.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/usb/typec/tipd/core.c | 41 ++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 485b90c13078..d28ffa10a122 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -16,6 +16,7 @@
 #include <linux/usb/typec.h>
 #include <linux/usb/typec_altmode.h>
 #include <linux/usb/role.h>
+#include <linux/workqueue.h>
 
 #include "tps6598x.h"
 #include "trace.h"
@@ -97,6 +98,8 @@ struct tps6598x {
 
 	int wakeup;
 	u16 pwr_status;
+	struct delayed_work	wq_poll;
+	irq_handler_t irq_handler;
 };
 
 static enum power_supply_property tps6598x_psy_props[] = {
@@ -568,6 +571,18 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 	return IRQ_NONE;
 }
 
+/* Time interval for Polling */
+#define POLL_INTERVAL	500 /* msecs */
+static void tps6598x_poll_work(struct work_struct *work)
+{
+	struct tps6598x *tps = container_of(to_delayed_work(work),
+					    struct tps6598x, wq_poll);
+
+	tps->irq_handler(0, tps);
+	queue_delayed_work(system_power_efficient_wq,
+			   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
+}
+
 static int tps6598x_check_mode(struct tps6598x *tps)
 {
 	char mode[5] = { };
@@ -746,6 +761,7 @@ static int tps6598x_probe(struct i2c_client *client)
 			TPS_REG_INT_PLUG_EVENT;
 	}
 
+	tps->irq_handler = irq_handler;
 	/* Make sure the controller has application firmware running */
 	ret = tps6598x_check_mode(tps);
 	if (ret)
@@ -837,10 +853,18 @@ static int tps6598x_probe(struct i2c_client *client)
 			dev_err(&client->dev, "failed to register partner\n");
 	}
 
-	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
-					irq_handler,
-					IRQF_SHARED | IRQF_ONESHOT,
-					dev_name(&client->dev), tps);
+	if (client->irq) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+						irq_handler,
+						IRQF_SHARED | IRQF_ONESHOT,
+						dev_name(&client->dev), tps);
+	} else {
+		dev_warn(tps->dev, "Unable to find the interrupt, switching to polling\n");
+		INIT_DELAYED_WORK(&tps->wq_poll, tps6598x_poll_work);
+		queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
+				   msecs_to_jiffies(POLL_INTERVAL));
+	}
+
 	if (ret)
 		goto err_disconnect;
 
@@ -848,7 +872,7 @@ static int tps6598x_probe(struct i2c_client *client)
 	fwnode_handle_put(fwnode);
 
 	tps->wakeup = device_property_read_bool(tps->dev, "wakeup-source");
-	if (tps->wakeup) {
+	if (tps->wakeup && client->irq) {
 		device_init_wakeup(&client->dev, true);
 		enable_irq_wake(client->irq);
 	}
@@ -887,6 +911,9 @@ static int __maybe_unused tps6598x_suspend(struct device *dev)
 		enable_irq_wake(client->irq);
 	}
 
+	if (!client->irq)
+		cancel_delayed_work_sync(&tps->wq_poll);
+
 	return 0;
 }
 
@@ -900,6 +927,10 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
 		enable_irq(client->irq);
 	}
 
+	if (client->irq)
+		queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
+				   msecs_to_jiffies(POLL_INTERVAL));
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH] usb: typec: tps6598x: Add support for polling interrupts status

Posted by Heikki Krogerus 2 years, 10 months ago

On Fri, Mar 24, 2023 at 03:18:53PM +0200, Roger Quadros wrote:
> From: Aswath Govindraju <a-govindraju@ti.com>
> 
> Some development boards don't have the interrupt line connected.
> 
> In such cases we can resort to polling the interrupt status.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> Signed-off-by: Roger Quadros <rogerq@kernel.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 41 ++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 485b90c13078..d28ffa10a122 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -16,6 +16,7 @@
>  #include <linux/usb/typec.h>
>  #include <linux/usb/typec_altmode.h>
>  #include <linux/usb/role.h>
> +#include <linux/workqueue.h>
>  
>  #include "tps6598x.h"
>  #include "trace.h"
> @@ -97,6 +98,8 @@ struct tps6598x {
>  
>  	int wakeup;
>  	u16 pwr_status;
> +	struct delayed_work	wq_poll;
> +	irq_handler_t irq_handler;
>  };
>  
>  static enum power_supply_property tps6598x_psy_props[] = {
> @@ -568,6 +571,18 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  	return IRQ_NONE;
>  }
>  
> +/* Time interval for Polling */
> +#define POLL_INTERVAL	500 /* msecs */
> +static void tps6598x_poll_work(struct work_struct *work)
> +{
> +	struct tps6598x *tps = container_of(to_delayed_work(work),
> +					    struct tps6598x, wq_poll);
> +
> +	tps->irq_handler(0, tps);
> +	queue_delayed_work(system_power_efficient_wq,
> +			   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
> +}
> +
>  static int tps6598x_check_mode(struct tps6598x *tps)
>  {
>  	char mode[5] = { };
> @@ -746,6 +761,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  			TPS_REG_INT_PLUG_EVENT;
>  	}
>  
> +	tps->irq_handler = irq_handler;
>  	/* Make sure the controller has application firmware running */
>  	ret = tps6598x_check_mode(tps);
>  	if (ret)
> @@ -837,10 +853,18 @@ static int tps6598x_probe(struct i2c_client *client)
>  			dev_err(&client->dev, "failed to register partner\n");
>  	}
>  
> -	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> -					irq_handler,
> -					IRQF_SHARED | IRQF_ONESHOT,
> -					dev_name(&client->dev), tps);
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +						irq_handler,
> +						IRQF_SHARED | IRQF_ONESHOT,
> +						dev_name(&client->dev), tps);
> +	} else {
> +		dev_warn(tps->dev, "Unable to find the interrupt, switching to polling\n");
> +		INIT_DELAYED_WORK(&tps->wq_poll, tps6598x_poll_work);
> +		queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
> +				   msecs_to_jiffies(POLL_INTERVAL));
> +	}
> +
>  	if (ret)
>  		goto err_disconnect;
>  
> @@ -848,7 +872,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  	fwnode_handle_put(fwnode);
>  
>  	tps->wakeup = device_property_read_bool(tps->dev, "wakeup-source");
> -	if (tps->wakeup) {
> +	if (tps->wakeup && client->irq) {
>  		device_init_wakeup(&client->dev, true);
>  		enable_irq_wake(client->irq);
>  	}
> @@ -887,6 +911,9 @@ static int __maybe_unused tps6598x_suspend(struct device *dev)
>  		enable_irq_wake(client->irq);
>  	}
>  
> +	if (!client->irq)
> +		cancel_delayed_work_sync(&tps->wq_poll);
> +
>  	return 0;
>  }
>  
> @@ -900,6 +927,10 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
>  		enable_irq(client->irq);
>  	}
>  
> +	if (client->irq)
> +		queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
> +				   msecs_to_jiffies(POLL_INTERVAL));
> +
>  	return 0;
>  }
>  
> -- 
> 2.34.1

-- 
heikki

[PATCH] dt-bindings: usb: tps6598x: make interrupts optional

Posted by Roger Quadros 2 years, 10 months ago

The driver can poll for interrupt status so interrupts
can be optional. It is still recommended to use the
interrupt line. Polling should only be used for debug
and prototyping.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
index 348a715d61f4..8c2db282735a 100644
--- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
@@ -35,8 +35,6 @@ properties:
 required:
   - compatible
   - reg
-  - interrupts
-  - interrupt-names
 
 additionalProperties: true
 
-- 
2.34.1

Re: [PATCH] dt-bindings: usb: tps6598x: make interrupts optional

Posted by Roger Quadros 2 years, 10 months ago

Hi Heikki & Rob,

On 24/03/2023 15:37, Roger Quadros wrote:
> The driver can poll for interrupt status so interrupts
> can be optional. It is still recommended to use the
> interrupt line. Polling should only be used for debug
> and prototyping.
> 
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> ---
>  Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> index 348a715d61f4..8c2db282735a 100644
> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> @@ -35,8 +35,6 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - interrupts
> -  - interrupt-names
>  
>  additionalProperties: true
>  

We need this patch as well along with the driver changes [1]
Could you please Ack. Thanks!

[1] - https://lore.kernel.org/r/20230324131853.41102-1-rogerq@kernel.org

cheers,
-roger

Re: [PATCH] dt-bindings: usb: tps6598x: make interrupts optional

Posted by Rob Herring 2 years, 10 months ago

On Wed, Mar 29, 2023 at 10:05:33AM +0300, Roger Quadros wrote:
> Hi Heikki & Rob,
> 
> On 24/03/2023 15:37, Roger Quadros wrote:
> > The driver can poll for interrupt status so interrupts
> > can be optional. It is still recommended to use the
> > interrupt line. Polling should only be used for debug
> > and prototyping.
> > 
> > Signed-off-by: Roger Quadros <rogerq@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > index 348a715d61f4..8c2db282735a 100644
> > --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > @@ -35,8 +35,6 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > -  - interrupts
> > -  - interrupt-names
> >  
> >  additionalProperties: true
> >  
> 
> We need this patch as well along with the driver changes [1]
> Could you please Ack. Thanks!

If get_maintainers.pl had been properly used, then it probably would 
have been acked by Krzysztof already. Otherwise, it's in my queue if the 
DT list is copied.

Acked-by: Rob Herring <robh@kernel.org>

Rob

Re: [PATCH] dt-bindings: usb: tps6598x: make interrupts optional

Posted by Heikki Krogerus 2 years, 10 months ago

On Wed, Mar 29, 2023 at 10:05:33AM +0300, Roger Quadros wrote:
> Hi Heikki & Rob,
> 
> On 24/03/2023 15:37, Roger Quadros wrote:
> > The driver can poll for interrupt status so interrupts
> > can be optional. It is still recommended to use the
> > interrupt line. Polling should only be used for debug
> > and prototyping.
> > 
> > Signed-off-by: Roger Quadros <rogerq@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > index 348a715d61f4..8c2db282735a 100644
> > --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > @@ -35,8 +35,6 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > -  - interrupts
> > -  - interrupt-names
> >  
> >  additionalProperties: true
> >  
> 
> We need this patch as well along with the driver changes [1]
> Could you please Ack. Thanks!

I can give my ack FWIW, but we should still wait for Rob.

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

thanks,

-- 
heikki