[PATCH RFC] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected

[PATCH RFC] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected

[Aswath Govindraju]

In some cases the interrupt line from the pd controller may not be
connected. In these cases, poll the status of various events.

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

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 16b4560216ba..fa52d2067d6d 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -15,6 +15,8 @@
 #include <linux/interrupt.h>
 #include <linux/usb/typec.h>
 #include <linux/usb/role.h>
+#include <linux/workqueue.h>
+#include <linux/devm-helpers.h>
 
 #include "tps6598x.h"
 #include "trace.h"
@@ -93,6 +95,8 @@ struct tps6598x {
 	struct power_supply *psy;
 	struct power_supply_desc psy_desc;
 	enum power_supply_usb_type usb_type;
+
+	struct delayed_work wq_poll;
 };
 
 static enum power_supply_property tps6598x_psy_props[] = {
@@ -473,9 +477,8 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
 	}
 }
 
-static irqreturn_t cd321x_interrupt(int irq, void *data)
+static int cd321x_handle_interrupt_status(struct tps6598x *tps)
 {
-	struct tps6598x *tps = data;
 	u64 event;
 	u32 status;
 	int ret;
@@ -513,14 +516,45 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
 err_unlock:
 	mutex_unlock(&tps->lock);
 
+	if (ret)
+		return ret;
+
 	if (event)
-		return IRQ_HANDLED;
-	return IRQ_NONE;
+		return 0;
+	return 1;
 }
 
-static irqreturn_t tps6598x_interrupt(int irq, void *data)
+static irqreturn_t cd321x_interrupt(int irq, void *data)
 {
 	struct tps6598x *tps = data;
+	int ret;
+
+	ret = cd321x_handle_interrupt_status(tps);
+	if (ret)
+		return IRQ_NONE;
+	return IRQ_HANDLED;
+}
+
+/* Time interval for Polling */
+#define POLL_INTERVAL   500 /* msecs */
+static void cd321x_poll_work(struct work_struct *work)
+{
+	struct tps6598x *tps = container_of(to_delayed_work(work),
+					    struct tps6598x, wq_poll);
+	int ret;
+
+	ret = cd321x_handle_interrupt_status(tps);
+	/*
+	 * If there is an error while reading the interrupt registers
+	 * then stop polling else, schedule another poll work item
+	 */
+	if (!(ret < 0))
+		queue_delayed_work(system_power_efficient_wq,
+				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
+}
+
+static int tps6598x_handle_interrupt_status(struct tps6598x *tps)
+{
 	u64 event1;
 	u64 event2;
 	u32 status;
@@ -561,9 +595,39 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 err_unlock:
 	mutex_unlock(&tps->lock);
 
+	if (ret)
+		return ret;
+
 	if (event1 | event2)
-		return IRQ_HANDLED;
-	return IRQ_NONE;
+		return 0;
+	return 1;
+}
+
+static irqreturn_t tps6598x_interrupt(int irq, void *data)
+{
+	struct tps6598x *tps = data;
+	int ret;
+
+	ret = tps6598x_handle_interrupt_status(tps);
+	if (ret)
+		return IRQ_NONE;
+	return IRQ_HANDLED;
+}
+
+static void tps6598x_poll_work(struct work_struct *work)
+{
+	struct tps6598x *tps = container_of(to_delayed_work(work),
+					    struct tps6598x, wq_poll);
+	int ret;
+
+	ret = tps6598x_handle_interrupt_status(tps);
+	/*
+	 * If there is an error while reading the interrupt registers
+	 * then stop polling else, schedule another poll work item
+	 */
+	if (!(ret < 0))
+		queue_delayed_work(system_power_efficient_wq,
+				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
 }
 
 static int tps6598x_check_mode(struct tps6598x *tps)
@@ -704,6 +768,7 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
 static int tps6598x_probe(struct i2c_client *client)
 {
 	irq_handler_t irq_handler = tps6598x_interrupt;
+	work_func_t work_poll_handler = tps6598x_poll_work;
 	struct device_node *np = client->dev.of_node;
 	struct typec_capability typec_cap = { };
 	struct tps6598x *tps;
@@ -748,6 +813,7 @@ static int tps6598x_probe(struct i2c_client *client)
 			APPLE_CD_REG_INT_PLUG_EVENT;
 
 		irq_handler = cd321x_interrupt;
+		work_poll_handler = cd321x_poll_work;
 	} else {
 		/* Enable power status, data status and plug event interrupts */
 		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
@@ -846,6 +912,16 @@ static int tps6598x_probe(struct i2c_client *client)
 					irq_handler,
 					IRQF_SHARED | IRQF_ONESHOT,
 					dev_name(&client->dev), tps);
+	if (ret == -EINVAL) {
+		dev_warn(&client->dev, "Unable to find the interrupt, switching to polling\n");
+		ret = devm_delayed_work_autocancel(tps->dev, &tps->wq_poll, work_poll_handler);
+		if (ret)
+			dev_err(&client->dev, "error while initializing workqueue\n");
+		else
+			queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
+					   msecs_to_jiffies(POLL_INTERVAL));
+	}
+
 	if (ret) {
 		tps6598x_disconnect(tps, 0);
 		typec_unregister_port(tps->port);
-- 
2.17.1

Re: [PATCH RFC] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected

[Heikki Krogerus]

Hi Aswath,

On Tue, Apr 12, 2022 at 08:20:58PM +0530, Aswath Govindraju wrote:
> In some cases the interrupt line from the pd controller may not be
> connected. In these cases, poll the status of various events.

Well, if the alert/interrupt line is not connected anywhere, then
polling is the only way to go. I'm fine with that, but the driver
really should be told that there is no interrupt. Using polling
whenever request_threaded_irq() returns -EINVAL is wrong. We really
should not even attempt to request the interrupt if there is no
interrupt for the device.

Isn't there any way you can get that information from DT? Or how is
the device enumerated in your case?

> Suggested-by: Roger Quadros <rogerq@kernel.org>
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  drivers/usb/typec/tipd/core.c | 90 ++++++++++++++++++++++++++++++++---
>  1 file changed, 83 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 16b4560216ba..fa52d2067d6d 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -15,6 +15,8 @@
>  #include <linux/interrupt.h>
>  #include <linux/usb/typec.h>
>  #include <linux/usb/role.h>
> +#include <linux/workqueue.h>
> +#include <linux/devm-helpers.h>
>  
>  #include "tps6598x.h"
>  #include "trace.h"
> @@ -93,6 +95,8 @@ struct tps6598x {
>  	struct power_supply *psy;
>  	struct power_supply_desc psy_desc;
>  	enum power_supply_usb_type usb_type;
> +
> +	struct delayed_work wq_poll;
>  };
>  
>  static enum power_supply_property tps6598x_psy_props[] = {
> @@ -473,9 +477,8 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
>  	}
>  }
>  
> -static irqreturn_t cd321x_interrupt(int irq, void *data)
> +static int cd321x_handle_interrupt_status(struct tps6598x *tps)
>  {
> -	struct tps6598x *tps = data;
>  	u64 event;
>  	u32 status;
>  	int ret;
> @@ -513,14 +516,45 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>  err_unlock:
>  	mutex_unlock(&tps->lock);
>  
> +	if (ret)
> +		return ret;
> +
>  	if (event)
> -		return IRQ_HANDLED;
> -	return IRQ_NONE;
> +		return 0;
> +	return 1;
>  }
>  
> -static irqreturn_t tps6598x_interrupt(int irq, void *data)
> +static irqreturn_t cd321x_interrupt(int irq, void *data)
>  {
>  	struct tps6598x *tps = data;
> +	int ret;
> +
> +	ret = cd321x_handle_interrupt_status(tps);
> +	if (ret)
> +		return IRQ_NONE;
> +	return IRQ_HANDLED;
> +}
> +
> +/* Time interval for Polling */
> +#define POLL_INTERVAL   500 /* msecs */
> +static void cd321x_poll_work(struct work_struct *work)
> +{
> +	struct tps6598x *tps = container_of(to_delayed_work(work),
> +					    struct tps6598x, wq_poll);
> +	int ret;
> +
> +	ret = cd321x_handle_interrupt_status(tps);
> +	/*
> +	 * If there is an error while reading the interrupt registers
> +	 * then stop polling else, schedule another poll work item
> +	 */
> +	if (!(ret < 0))
> +		queue_delayed_work(system_power_efficient_wq,
> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
> +}
> +
> +static int tps6598x_handle_interrupt_status(struct tps6598x *tps)
> +{
>  	u64 event1;
>  	u64 event2;
>  	u32 status;
> @@ -561,9 +595,39 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  err_unlock:
>  	mutex_unlock(&tps->lock);
>  
> +	if (ret)
> +		return ret;
> +
>  	if (event1 | event2)
> -		return IRQ_HANDLED;
> -	return IRQ_NONE;
> +		return 0;
> +	return 1;
> +}
> +
> +static irqreturn_t tps6598x_interrupt(int irq, void *data)
> +{
> +	struct tps6598x *tps = data;
> +	int ret;
> +
> +	ret = tps6598x_handle_interrupt_status(tps);
> +	if (ret)
> +		return IRQ_NONE;
> +	return IRQ_HANDLED;
> +}
> +
> +static void tps6598x_poll_work(struct work_struct *work)
> +{
> +	struct tps6598x *tps = container_of(to_delayed_work(work),
> +					    struct tps6598x, wq_poll);
> +	int ret;
> +
> +	ret = tps6598x_handle_interrupt_status(tps);
> +	/*
> +	 * If there is an error while reading the interrupt registers
> +	 * then stop polling else, schedule another poll work item
> +	 */
> +	if (!(ret < 0))
> +		queue_delayed_work(system_power_efficient_wq,
> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>  }
>  
>  static int tps6598x_check_mode(struct tps6598x *tps)
> @@ -704,6 +768,7 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
>  static int tps6598x_probe(struct i2c_client *client)
>  {
>  	irq_handler_t irq_handler = tps6598x_interrupt;
> +	work_func_t work_poll_handler = tps6598x_poll_work;
>  	struct device_node *np = client->dev.of_node;
>  	struct typec_capability typec_cap = { };
>  	struct tps6598x *tps;
> @@ -748,6 +813,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  			APPLE_CD_REG_INT_PLUG_EVENT;
>  
>  		irq_handler = cd321x_interrupt;
> +		work_poll_handler = cd321x_poll_work;
>  	} else {
>  		/* Enable power status, data status and plug event interrupts */
>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
> @@ -846,6 +912,16 @@ static int tps6598x_probe(struct i2c_client *client)
>  					irq_handler,
>  					IRQF_SHARED | IRQF_ONESHOT,
>  					dev_name(&client->dev), tps);
> +	if (ret == -EINVAL) {
> +		dev_warn(&client->dev, "Unable to find the interrupt, switching to polling\n");
> +		ret = devm_delayed_work_autocancel(tps->dev, &tps->wq_poll, work_poll_handler);
> +		if (ret)
> +			dev_err(&client->dev, "error while initializing workqueue\n");
> +		else
> +			queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
> +					   msecs_to_jiffies(POLL_INTERVAL));
> +	}
> +
>  	if (ret) {
>  		tps6598x_disconnect(tps, 0);
>  		typec_unregister_port(tps->port);

thanks,

-- 
heikki

Re: [PATCH RFC] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected

[Aswath Govindraju]

Hi Heikki,

On 13/04/22 15:04, Heikki Krogerus wrote:
> Hi Aswath,
> 
> On Tue, Apr 12, 2022 at 08:20:58PM +0530, Aswath Govindraju wrote:
>> In some cases the interrupt line from the pd controller may not be
>> connected. In these cases, poll the status of various events.
> 
> Well, if the alert/interrupt line is not connected anywhere, then
> polling is the only way to go. I'm fine with that, but the driver
> really should be told that there is no interrupt. Using polling
> whenever request_threaded_irq() returns -EINVAL is wrong. We really
> should not even attempt to request the interrupt if there is no
> interrupt for the device.
> 
> Isn't there any way you can get that information from DT? Or how is
> the device enumerated in your case?
> 

Would checking if (client->irq) field is populated, to decide between
polling and interrupts be a good approach?

I am sorry but I did not understand what you meant by device getting
enumerated. The device is on an I2C bus and gets enumerated based on the
I2C address provided. The device does not have I2C_IRQ line connected,
in my case.

Thanks,
Aswath

>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>  drivers/usb/typec/tipd/core.c | 90 ++++++++++++++++++++++++++++++++---
>>  1 file changed, 83 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index 16b4560216ba..fa52d2067d6d 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -15,6 +15,8 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/usb/typec.h>
>>  #include <linux/usb/role.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/devm-helpers.h>
>>  
>>  #include "tps6598x.h"
>>  #include "trace.h"
>> @@ -93,6 +95,8 @@ struct tps6598x {
>>  	struct power_supply *psy;
>>  	struct power_supply_desc psy_desc;
>>  	enum power_supply_usb_type usb_type;
>> +
>> +	struct delayed_work wq_poll;
>>  };
>>  
>>  static enum power_supply_property tps6598x_psy_props[] = {
>> @@ -473,9 +477,8 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
>>  	}
>>  }
>>  
>> -static irqreturn_t cd321x_interrupt(int irq, void *data)
>> +static int cd321x_handle_interrupt_status(struct tps6598x *tps)
>>  {
>> -	struct tps6598x *tps = data;
>>  	u64 event;
>>  	u32 status;
>>  	int ret;
>> @@ -513,14 +516,45 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>>  err_unlock:
>>  	mutex_unlock(&tps->lock);
>>  
>> +	if (ret)
>> +		return ret;
>> +
>>  	if (event)
>> -		return IRQ_HANDLED;
>> -	return IRQ_NONE;
>> +		return 0;
>> +	return 1;
>>  }
>>  
>> -static irqreturn_t tps6598x_interrupt(int irq, void *data)
>> +static irqreturn_t cd321x_interrupt(int irq, void *data)
>>  {
>>  	struct tps6598x *tps = data;
>> +	int ret;
>> +
>> +	ret = cd321x_handle_interrupt_status(tps);
>> +	if (ret)
>> +		return IRQ_NONE;
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +/* Time interval for Polling */
>> +#define POLL_INTERVAL   500 /* msecs */
>> +static void cd321x_poll_work(struct work_struct *work)
>> +{
>> +	struct tps6598x *tps = container_of(to_delayed_work(work),
>> +					    struct tps6598x, wq_poll);
>> +	int ret;
>> +
>> +	ret = cd321x_handle_interrupt_status(tps);
>> +	/*
>> +	 * If there is an error while reading the interrupt registers
>> +	 * then stop polling else, schedule another poll work item
>> +	 */
>> +	if (!(ret < 0))
>> +		queue_delayed_work(system_power_efficient_wq,
>> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>> +}
>> +
>> +static int tps6598x_handle_interrupt_status(struct tps6598x *tps)
>> +{
>>  	u64 event1;
>>  	u64 event2;
>>  	u32 status;
>> @@ -561,9 +595,39 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>  err_unlock:
>>  	mutex_unlock(&tps->lock);
>>  
>> +	if (ret)
>> +		return ret;
>> +
>>  	if (event1 | event2)
>> -		return IRQ_HANDLED;
>> -	return IRQ_NONE;
>> +		return 0;
>> +	return 1;
>> +}
>> +
>> +static irqreturn_t tps6598x_interrupt(int irq, void *data)
>> +{
>> +	struct tps6598x *tps = data;
>> +	int ret;
>> +
>> +	ret = tps6598x_handle_interrupt_status(tps);
>> +	if (ret)
>> +		return IRQ_NONE;
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void tps6598x_poll_work(struct work_struct *work)
>> +{
>> +	struct tps6598x *tps = container_of(to_delayed_work(work),
>> +					    struct tps6598x, wq_poll);
>> +	int ret;
>> +
>> +	ret = tps6598x_handle_interrupt_status(tps);
>> +	/*
>> +	 * If there is an error while reading the interrupt registers
>> +	 * then stop polling else, schedule another poll work item
>> +	 */
>> +	if (!(ret < 0))
>> +		queue_delayed_work(system_power_efficient_wq,
>> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>>  }
>>  
>>  static int tps6598x_check_mode(struct tps6598x *tps)
>> @@ -704,6 +768,7 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
>>  static int tps6598x_probe(struct i2c_client *client)
>>  {
>>  	irq_handler_t irq_handler = tps6598x_interrupt;
>> +	work_func_t work_poll_handler = tps6598x_poll_work;
>>  	struct device_node *np = client->dev.of_node;
>>  	struct typec_capability typec_cap = { };
>>  	struct tps6598x *tps;
>> @@ -748,6 +813,7 @@ static int tps6598x_probe(struct i2c_client *client)
>>  			APPLE_CD_REG_INT_PLUG_EVENT;
>>  
>>  		irq_handler = cd321x_interrupt;
>> +		work_poll_handler = cd321x_poll_work;
>>  	} else {
>>  		/* Enable power status, data status and plug event interrupts */
>>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
>> @@ -846,6 +912,16 @@ static int tps6598x_probe(struct i2c_client *client)
>>  					irq_handler,
>>  					IRQF_SHARED | IRQF_ONESHOT,
>>  					dev_name(&client->dev), tps);
>> +	if (ret == -EINVAL) {
>> +		dev_warn(&client->dev, "Unable to find the interrupt, switching to polling\n");
>> +		ret = devm_delayed_work_autocancel(tps->dev, &tps->wq_poll, work_poll_handler);
>> +		if (ret)
>> +			dev_err(&client->dev, "error while initializing workqueue\n");
>> +		else
>> +			queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
>> +					   msecs_to_jiffies(POLL_INTERVAL));
>> +	}
>> +
>>  	if (ret) {
>>  		tps6598x_disconnect(tps, 0);
>>  		typec_unregister_port(tps->port);
> 
> thanks,
> 


-- 
Thanks,
Aswath

Re: [PATCH RFC] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected

[Heikki Krogerus]

On Wed, Apr 13, 2022 at 03:32:50PM +0530, Aswath Govindraju wrote:
> Hi Heikki,
> 
> On 13/04/22 15:04, Heikki Krogerus wrote:
> > Hi Aswath,
> > 
> > On Tue, Apr 12, 2022 at 08:20:58PM +0530, Aswath Govindraju wrote:
> >> In some cases the interrupt line from the pd controller may not be
> >> connected. In these cases, poll the status of various events.
> > 
> > Well, if the alert/interrupt line is not connected anywhere, then
> > polling is the only way to go. I'm fine with that, but the driver
> > really should be told that there is no interrupt. Using polling
> > whenever request_threaded_irq() returns -EINVAL is wrong. We really
> > should not even attempt to request the interrupt if there is no
> > interrupt for the device.
> > 
> > Isn't there any way you can get that information from DT? Or how is
> > the device enumerated in your case?
> > 
> 
> Would checking if (client->irq) field is populated, to decide between
> polling and interrupts be a good approach?
> 
> I am sorry but I did not understand what you meant by device getting
> enumerated. The device is on an I2C bus and gets enumerated based on the
> I2C address provided. The device does not have I2C_IRQ line connected,
> in my case.

"I2C devices are not enumerated at hardware level":
https://www.kernel.org/doc/html/latest/i2c/instantiating-devices.html

So your PD controller I2C slave device has to be either described in
Devicetree or ACPI tables, or there is a board file or platform driver
that actually populates the device for it.

Can you tell a little bit about the platform you are running? Is it
ARM, x86, or what, and is it ACPI or DT platform?

thanks,

-- 
heikki

Re: [PATCH RFC] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected

[Roger Quadros]

Hi Aswath,

On 13/04/2022 13:02, Aswath Govindraju wrote:
> Hi Heikki,
> 
> On 13/04/22 15:04, Heikki Krogerus wrote:
>> Hi Aswath,
>>
>> On Tue, Apr 12, 2022 at 08:20:58PM +0530, Aswath Govindraju wrote:
>>> In some cases the interrupt line from the pd controller may not be
>>> connected. In these cases, poll the status of various events.
>>
>> Well, if the alert/interrupt line is not connected anywhere, then
>> polling is the only way to go. I'm fine with that, but the driver
>> really should be told that there is no interrupt. Using polling
>> whenever request_threaded_irq() returns -EINVAL is wrong. We really
>> should not even attempt to request the interrupt if there is no
>> interrupt for the device.
>>
>> Isn't there any way you can get that information from DT? Or how is
>> the device enumerated in your case?
>>
> 
> Would checking if (client->irq) field is populated, to decide between
> polling and interrupts be a good approach?

'interrupt' and 'interrupt-names' are required properties in DT binding doc
Documentation/devicetree/bindings/usb/ti,tps6598x.yaml

You will need to add a new property to indicate that polling mode must be used
and then interrupt properties become optional.

> 
> I am sorry but I did not understand what you meant by device getting
> enumerated. The device is on an I2C bus and gets enumerated based on the
> I2C address provided. The device does not have I2C_IRQ line connected,
> in my case.

I think he meant whether you are using device tree or something else.

> 
> Thanks,
> Aswath
> 
>>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>> ---
>>>  drivers/usb/typec/tipd/core.c | 90 ++++++++++++++++++++++++++++++++---
>>>  1 file changed, 83 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>>> index 16b4560216ba..fa52d2067d6d 100644
>>> --- a/drivers/usb/typec/tipd/core.c
>>> +++ b/drivers/usb/typec/tipd/core.c
>>> @@ -15,6 +15,8 @@
>>>  #include <linux/interrupt.h>
>>>  #include <linux/usb/typec.h>
>>>  #include <linux/usb/role.h>
>>> +#include <linux/workqueue.h>
>>> +#include <linux/devm-helpers.h>
>>>  
>>>  #include "tps6598x.h"
>>>  #include "trace.h"
>>> @@ -93,6 +95,8 @@ struct tps6598x {
>>>  	struct power_supply *psy;
>>>  	struct power_supply_desc psy_desc;
>>>  	enum power_supply_usb_type usb_type;
>>> +
>>> +	struct delayed_work wq_poll;
>>>  };
>>>  
>>>  static enum power_supply_property tps6598x_psy_props[] = {
>>> @@ -473,9 +477,8 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
>>>  	}
>>>  }
>>>  
>>> -static irqreturn_t cd321x_interrupt(int irq, void *data)
>>> +static int cd321x_handle_interrupt_status(struct tps6598x *tps)
>>>  {
>>> -	struct tps6598x *tps = data;
>>>  	u64 event;
>>>  	u32 status;
>>>  	int ret;
>>> @@ -513,14 +516,45 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>>>  err_unlock:
>>>  	mutex_unlock(&tps->lock);
>>>  
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>  	if (event)
>>> -		return IRQ_HANDLED;
>>> -	return IRQ_NONE;
>>> +		return 0;
>>> +	return 1;
>>>  }
>>>  
>>> -static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>> +static irqreturn_t cd321x_interrupt(int irq, void *data)
>>>  {
>>>  	struct tps6598x *tps = data;
>>> +	int ret;
>>> +
>>> +	ret = cd321x_handle_interrupt_status(tps);
>>> +	if (ret)
>>> +		return IRQ_NONE;
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +/* Time interval for Polling */
>>> +#define POLL_INTERVAL   500 /* msecs */
>>> +static void cd321x_poll_work(struct work_struct *work)
>>> +{
>>> +	struct tps6598x *tps = container_of(to_delayed_work(work),
>>> +					    struct tps6598x, wq_poll);
>>> +	int ret;
>>> +
>>> +	ret = cd321x_handle_interrupt_status(tps);
>>> +	/*
>>> +	 * If there is an error while reading the interrupt registers
>>> +	 * then stop polling else, schedule another poll work item
>>> +	 */
>>> +	if (!(ret < 0))
>>> +		queue_delayed_work(system_power_efficient_wq,
>>> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>>> +}
>>> +
>>> +static int tps6598x_handle_interrupt_status(struct tps6598x *tps)
>>> +{
>>>  	u64 event1;
>>>  	u64 event2;
>>>  	u32 status;
>>> @@ -561,9 +595,39 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>>  err_unlock:
>>>  	mutex_unlock(&tps->lock);
>>>  
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>  	if (event1 | event2)
>>> -		return IRQ_HANDLED;
>>> -	return IRQ_NONE;
>>> +		return 0;
>>> +	return 1;
>>> +}
>>> +
>>> +static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>> +{
>>> +	struct tps6598x *tps = data;
>>> +	int ret;
>>> +
>>> +	ret = tps6598x_handle_interrupt_status(tps);
>>> +	if (ret)
>>> +		return IRQ_NONE;
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static void tps6598x_poll_work(struct work_struct *work)
>>> +{
>>> +	struct tps6598x *tps = container_of(to_delayed_work(work),
>>> +					    struct tps6598x, wq_poll);
>>> +	int ret;
>>> +
>>> +	ret = tps6598x_handle_interrupt_status(tps);
>>> +	/*
>>> +	 * If there is an error while reading the interrupt registers
>>> +	 * then stop polling else, schedule another poll work item
>>> +	 */
>>> +	if (!(ret < 0))
>>> +		queue_delayed_work(system_power_efficient_wq,
>>> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>>>  }
>>>  
>>>  static int tps6598x_check_mode(struct tps6598x *tps)
>>> @@ -704,6 +768,7 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
>>>  static int tps6598x_probe(struct i2c_client *client)
>>>  {
>>>  	irq_handler_t irq_handler = tps6598x_interrupt;
>>> +	work_func_t work_poll_handler = tps6598x_poll_work;
>>>  	struct device_node *np = client->dev.of_node;
>>>  	struct typec_capability typec_cap = { };
>>>  	struct tps6598x *tps;
>>> @@ -748,6 +813,7 @@ static int tps6598x_probe(struct i2c_client *client)
>>>  			APPLE_CD_REG_INT_PLUG_EVENT;
>>>  
>>>  		irq_handler = cd321x_interrupt;
>>> +		work_poll_handler = cd321x_poll_work;
>>>  	} else {
>>>  		/* Enable power status, data status and plug event interrupts */
>>>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
>>> @@ -846,6 +912,16 @@ static int tps6598x_probe(struct i2c_client *client)
>>>  					irq_handler,
>>>  					IRQF_SHARED | IRQF_ONESHOT,
>>>  					dev_name(&client->dev), tps);
>>> +	if (ret == -EINVAL) {
>>> +		dev_warn(&client->dev, "Unable to find the interrupt, switching to polling\n");
>>> +		ret = devm_delayed_work_autocancel(tps->dev, &tps->wq_poll, work_poll_handler);
>>> +		if (ret)
>>> +			dev_err(&client->dev, "error while initializing workqueue\n");
>>> +		else
>>> +			queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
>>> +					   msecs_to_jiffies(POLL_INTERVAL));
>>> +	}
>>> +
>>>  	if (ret) {
>>>  		tps6598x_disconnect(tps, 0);
>>>  		typec_unregister_port(tps->port);
>>
>> thanks,
>>
> 
> 

--
cheers,
-roger

Re: [PATCH RFC] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected

[Aswath Govindraju]

Hi Heikki,

On 13/04/22 16:07, Heikki Krogerus wrote:
> On Wed, Apr 13, 2022 at 03:32:50PM +0530, Aswath Govindraju wrote:
>> Hi Heikki,
>>
>> On 13/04/22 15:04, Heikki Krogerus wrote:
>>> Hi Aswath,
>>>
>>> On Tue, Apr 12, 2022 at 08:20:58PM +0530, Aswath Govindraju wrote:
>>>> In some cases the interrupt line from the pd controller may not be
>>>> connected. In these cases, poll the status of various events.
>>>
>>> Well, if the alert/interrupt line is not connected anywhere, then
>>> polling is the only way to go. I'm fine with that, but the driver
>>> really should be told that there is no interrupt. Using polling
>>> whenever request_threaded_irq() returns -EINVAL is wrong. We really
>>> should not even attempt to request the interrupt if there is no
>>> interrupt for the device.
>>>
>>> Isn't there any way you can get that information from DT? Or how is
>>> the device enumerated in your case?
>>>
>>
>> Would checking if (client->irq) field is populated, to decide between
>> polling and interrupts be a good approach?
>>
>> I am sorry but I did not understand what you meant by device getting
>> enumerated. The device is on an I2C bus and gets enumerated based on the
>> I2C address provided. The device does not have I2C_IRQ line connected,
>> in my case.
> 
> "I2C devices are not enumerated at hardware level":
> https://www.kernel.org/doc/html/latest/i2c/instantiating-devices.html
> 
> So your PD controller I2C slave device has to be either described in
> Devicetree or ACPI tables, or there is a board file or platform driver
> that actually populates the device for it.
> 
> Can you tell a little bit about the platform you are running? Is it
> ARM, x86, or what, and is it ACPI or DT platform?
> 

Got it. Currently I am testing on a ARM platform and the I2C device tree
nodes are populated in the device tree. This is how the PD controller
gets enumerated.


> thanks,
> 


-- 
Thanks,
Aswath

Re: [PATCH RFC] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected

[Aswath Govindraju]

Hi Roger,

On 13/04/22 16:08, Roger Quadros wrote:
> Hi Aswath,
> 
> On 13/04/2022 13:02, Aswath Govindraju wrote:
>> Hi Heikki,
>>
>> On 13/04/22 15:04, Heikki Krogerus wrote:
>>> Hi Aswath,
>>>
>>> On Tue, Apr 12, 2022 at 08:20:58PM +0530, Aswath Govindraju wrote:
>>>> In some cases the interrupt line from the pd controller may not be
>>>> connected. In these cases, poll the status of various events.
>>>
>>> Well, if the alert/interrupt line is not connected anywhere, then
>>> polling is the only way to go. I'm fine with that, but the driver
>>> really should be told that there is no interrupt. Using polling
>>> whenever request_threaded_irq() returns -EINVAL is wrong. We really
>>> should not even attempt to request the interrupt if there is no
>>> interrupt for the device.
>>>
>>> Isn't there any way you can get that information from DT? Or how is
>>> the device enumerated in your case?
>>>
>>
>> Would checking if (client->irq) field is populated, to decide between
>> polling and interrupts be a good approach?
> 
> 'interrupt' and 'interrupt-names' are required properties in DT binding doc
> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> 
> You will need to add a new property to indicate that polling mode must be used
> and then interrupt properties become optional.
> 

Doesn't adding polling support mean that we are making interrupts
optional? So, can't we directly make the properties optional in the
bindings?


>>
>> I am sorry but I did not understand what you meant by device getting
>> enumerated. The device is on an I2C bus and gets enumerated based on the
>> I2C address provided. The device does not have I2C_IRQ line connected,
>> in my case.
> 
> I think he meant whether you are using device tree or something else.
> 

ohh okay, Thank you.

Regards,
Aswath

>>
>> Thanks,
>> Aswath
>>
>>>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>>> ---
>>>>  drivers/usb/typec/tipd/core.c | 90 ++++++++++++++++++++++++++++++++---
>>>>  1 file changed, 83 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>>>> index 16b4560216ba..fa52d2067d6d 100644
>>>> --- a/drivers/usb/typec/tipd/core.c
>>>> +++ b/drivers/usb/typec/tipd/core.c
>>>> @@ -15,6 +15,8 @@
>>>>  #include <linux/interrupt.h>
>>>>  #include <linux/usb/typec.h>
>>>>  #include <linux/usb/role.h>
>>>> +#include <linux/workqueue.h>
>>>> +#include <linux/devm-helpers.h>
>>>>  
>>>>  #include "tps6598x.h"
>>>>  #include "trace.h"
>>>> @@ -93,6 +95,8 @@ struct tps6598x {
>>>>  	struct power_supply *psy;
>>>>  	struct power_supply_desc psy_desc;
>>>>  	enum power_supply_usb_type usb_type;
>>>> +
>>>> +	struct delayed_work wq_poll;
>>>>  };
>>>>  
>>>>  static enum power_supply_property tps6598x_psy_props[] = {
>>>> @@ -473,9 +477,8 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
>>>>  	}
>>>>  }
>>>>  
>>>> -static irqreturn_t cd321x_interrupt(int irq, void *data)
>>>> +static int cd321x_handle_interrupt_status(struct tps6598x *tps)
>>>>  {
>>>> -	struct tps6598x *tps = data;
>>>>  	u64 event;
>>>>  	u32 status;
>>>>  	int ret;
>>>> @@ -513,14 +516,45 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>>>>  err_unlock:
>>>>  	mutex_unlock(&tps->lock);
>>>>  
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>>  	if (event)
>>>> -		return IRQ_HANDLED;
>>>> -	return IRQ_NONE;
>>>> +		return 0;
>>>> +	return 1;
>>>>  }
>>>>  
>>>> -static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>>> +static irqreturn_t cd321x_interrupt(int irq, void *data)
>>>>  {
>>>>  	struct tps6598x *tps = data;
>>>> +	int ret;
>>>> +
>>>> +	ret = cd321x_handle_interrupt_status(tps);
>>>> +	if (ret)
>>>> +		return IRQ_NONE;
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +/* Time interval for Polling */
>>>> +#define POLL_INTERVAL   500 /* msecs */
>>>> +static void cd321x_poll_work(struct work_struct *work)
>>>> +{
>>>> +	struct tps6598x *tps = container_of(to_delayed_work(work),
>>>> +					    struct tps6598x, wq_poll);
>>>> +	int ret;
>>>> +
>>>> +	ret = cd321x_handle_interrupt_status(tps);
>>>> +	/*
>>>> +	 * If there is an error while reading the interrupt registers
>>>> +	 * then stop polling else, schedule another poll work item
>>>> +	 */
>>>> +	if (!(ret < 0))
>>>> +		queue_delayed_work(system_power_efficient_wq,
>>>> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>>>> +}
>>>> +
>>>> +static int tps6598x_handle_interrupt_status(struct tps6598x *tps)
>>>> +{
>>>>  	u64 event1;
>>>>  	u64 event2;
>>>>  	u32 status;
>>>> @@ -561,9 +595,39 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>>>  err_unlock:
>>>>  	mutex_unlock(&tps->lock);
>>>>  
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>>  	if (event1 | event2)
>>>> -		return IRQ_HANDLED;
>>>> -	return IRQ_NONE;
>>>> +		return 0;
>>>> +	return 1;
>>>> +}
>>>> +
>>>> +static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>>> +{
>>>> +	struct tps6598x *tps = data;
>>>> +	int ret;
>>>> +
>>>> +	ret = tps6598x_handle_interrupt_status(tps);
>>>> +	if (ret)
>>>> +		return IRQ_NONE;
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static void tps6598x_poll_work(struct work_struct *work)
>>>> +{
>>>> +	struct tps6598x *tps = container_of(to_delayed_work(work),
>>>> +					    struct tps6598x, wq_poll);
>>>> +	int ret;
>>>> +
>>>> +	ret = tps6598x_handle_interrupt_status(tps);
>>>> +	/*
>>>> +	 * If there is an error while reading the interrupt registers
>>>> +	 * then stop polling else, schedule another poll work item
>>>> +	 */
>>>> +	if (!(ret < 0))
>>>> +		queue_delayed_work(system_power_efficient_wq,
>>>> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>>>>  }
>>>>  
>>>>  static int tps6598x_check_mode(struct tps6598x *tps)
>>>> @@ -704,6 +768,7 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
>>>>  static int tps6598x_probe(struct i2c_client *client)
>>>>  {
>>>>  	irq_handler_t irq_handler = tps6598x_interrupt;
>>>> +	work_func_t work_poll_handler = tps6598x_poll_work;
>>>>  	struct device_node *np = client->dev.of_node;
>>>>  	struct typec_capability typec_cap = { };
>>>>  	struct tps6598x *tps;
>>>> @@ -748,6 +813,7 @@ static int tps6598x_probe(struct i2c_client *client)
>>>>  			APPLE_CD_REG_INT_PLUG_EVENT;
>>>>  
>>>>  		irq_handler = cd321x_interrupt;
>>>> +		work_poll_handler = cd321x_poll_work;
>>>>  	} else {
>>>>  		/* Enable power status, data status and plug event interrupts */
>>>>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
>>>> @@ -846,6 +912,16 @@ static int tps6598x_probe(struct i2c_client *client)
>>>>  					irq_handler,
>>>>  					IRQF_SHARED | IRQF_ONESHOT,
>>>>  					dev_name(&client->dev), tps);
>>>> +	if (ret == -EINVAL) {
>>>> +		dev_warn(&client->dev, "Unable to find the interrupt, switching to polling\n");
>>>> +		ret = devm_delayed_work_autocancel(tps->dev, &tps->wq_poll, work_poll_handler);
>>>> +		if (ret)
>>>> +			dev_err(&client->dev, "error while initializing workqueue\n");
>>>> +		else
>>>> +			queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
>>>> +					   msecs_to_jiffies(POLL_INTERVAL));
>>>> +	}
>>>> +
>>>>  	if (ret) {
>>>>  		tps6598x_disconnect(tps, 0);
>>>>  		typec_unregister_port(tps->port);
>>>
>>> thanks,
>>>
>>
>>
> 
> --
> cheers,
> -roger


-- 
Thanks,
Aswath

Re: [PATCH RFC] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected

[Roger Quadros]

On 13/04/2022 13:51, Aswath Govindraju wrote:
> Hi Roger,
> 
> On 13/04/22 16:08, Roger Quadros wrote:
>> Hi Aswath,
>>
>> On 13/04/2022 13:02, Aswath Govindraju wrote:
>>> Hi Heikki,
>>>
>>> On 13/04/22 15:04, Heikki Krogerus wrote:
>>>> Hi Aswath,
>>>>
>>>> On Tue, Apr 12, 2022 at 08:20:58PM +0530, Aswath Govindraju wrote:
>>>>> In some cases the interrupt line from the pd controller may not be
>>>>> connected. In these cases, poll the status of various events.
>>>>
>>>> Well, if the alert/interrupt line is not connected anywhere, then
>>>> polling is the only way to go. I'm fine with that, but the driver
>>>> really should be told that there is no interrupt. Using polling
>>>> whenever request_threaded_irq() returns -EINVAL is wrong. We really
>>>> should not even attempt to request the interrupt if there is no
>>>> interrupt for the device.
>>>>
>>>> Isn't there any way you can get that information from DT? Or how is
>>>> the device enumerated in your case?
>>>>
>>>
>>> Would checking if (client->irq) field is populated, to decide between
>>> polling and interrupts be a good approach?
>>
>> 'interrupt' and 'interrupt-names' are required properties in DT binding doc
>> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
>>
>> You will need to add a new property to indicate that polling mode must be used
>> and then interrupt properties become optional.
>>
> 
> Doesn't adding polling support mean that we are making interrupts
> optional? So, can't we directly make the properties optional in the
> bindings?

I don't see why not. It must be clear from the binding documentation that
interrupt property is required unless polling mode is specified.

> 
> 
>>>
>>> I am sorry but I did not understand what you meant by device getting
>>> enumerated. The device is on an I2C bus and gets enumerated based on the
>>> I2C address provided. The device does not have I2C_IRQ line connected,
>>> in my case.
>>
>> I think he meant whether you are using device tree or something else.
>>
> 
> ohh okay, Thank you.
> 
> Regards,
> Aswath
> 
>>>
>>> Thanks,
>>> Aswath
>>>
>>>>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>>>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>>>> ---
>>>>>  drivers/usb/typec/tipd/core.c | 90 ++++++++++++++++++++++++++++++++---
>>>>>  1 file changed, 83 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>>>>> index 16b4560216ba..fa52d2067d6d 100644
>>>>> --- a/drivers/usb/typec/tipd/core.c
>>>>> +++ b/drivers/usb/typec/tipd/core.c
>>>>> @@ -15,6 +15,8 @@
>>>>>  #include <linux/interrupt.h>
>>>>>  #include <linux/usb/typec.h>
>>>>>  #include <linux/usb/role.h>
>>>>> +#include <linux/workqueue.h>
>>>>> +#include <linux/devm-helpers.h>
>>>>>  
>>>>>  #include "tps6598x.h"
>>>>>  #include "trace.h"
>>>>> @@ -93,6 +95,8 @@ struct tps6598x {
>>>>>  	struct power_supply *psy;
>>>>>  	struct power_supply_desc psy_desc;
>>>>>  	enum power_supply_usb_type usb_type;
>>>>> +
>>>>> +	struct delayed_work wq_poll;
>>>>>  };
>>>>>  
>>>>>  static enum power_supply_property tps6598x_psy_props[] = {
>>>>> @@ -473,9 +477,8 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
>>>>>  	}
>>>>>  }
>>>>>  
>>>>> -static irqreturn_t cd321x_interrupt(int irq, void *data)
>>>>> +static int cd321x_handle_interrupt_status(struct tps6598x *tps)
>>>>>  {
>>>>> -	struct tps6598x *tps = data;
>>>>>  	u64 event;
>>>>>  	u32 status;
>>>>>  	int ret;
>>>>> @@ -513,14 +516,45 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>>>>>  err_unlock:
>>>>>  	mutex_unlock(&tps->lock);
>>>>>  
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>>  	if (event)
>>>>> -		return IRQ_HANDLED;
>>>>> -	return IRQ_NONE;
>>>>> +		return 0;
>>>>> +	return 1;
>>>>>  }
>>>>>  
>>>>> -static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>>>> +static irqreturn_t cd321x_interrupt(int irq, void *data)
>>>>>  {
>>>>>  	struct tps6598x *tps = data;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = cd321x_handle_interrupt_status(tps);
>>>>> +	if (ret)
>>>>> +		return IRQ_NONE;
>>>>> +	return IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +/* Time interval for Polling */
>>>>> +#define POLL_INTERVAL   500 /* msecs */
>>>>> +static void cd321x_poll_work(struct work_struct *work)
>>>>> +{
>>>>> +	struct tps6598x *tps = container_of(to_delayed_work(work),
>>>>> +					    struct tps6598x, wq_poll);
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = cd321x_handle_interrupt_status(tps);
>>>>> +	/*
>>>>> +	 * If there is an error while reading the interrupt registers
>>>>> +	 * then stop polling else, schedule another poll work item
>>>>> +	 */
>>>>> +	if (!(ret < 0))
>>>>> +		queue_delayed_work(system_power_efficient_wq,
>>>>> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>>>>> +}
>>>>> +
>>>>> +static int tps6598x_handle_interrupt_status(struct tps6598x *tps)
>>>>> +{
>>>>>  	u64 event1;
>>>>>  	u64 event2;
>>>>>  	u32 status;
>>>>> @@ -561,9 +595,39 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>>>>  err_unlock:
>>>>>  	mutex_unlock(&tps->lock);
>>>>>  
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>>  	if (event1 | event2)
>>>>> -		return IRQ_HANDLED;
>>>>> -	return IRQ_NONE;
>>>>> +		return 0;
>>>>> +	return 1;
>>>>> +}
>>>>> +
>>>>> +static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>>>> +{
>>>>> +	struct tps6598x *tps = data;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = tps6598x_handle_interrupt_status(tps);
>>>>> +	if (ret)
>>>>> +		return IRQ_NONE;
>>>>> +	return IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +static void tps6598x_poll_work(struct work_struct *work)
>>>>> +{
>>>>> +	struct tps6598x *tps = container_of(to_delayed_work(work),
>>>>> +					    struct tps6598x, wq_poll);
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = tps6598x_handle_interrupt_status(tps);
>>>>> +	/*
>>>>> +	 * If there is an error while reading the interrupt registers
>>>>> +	 * then stop polling else, schedule another poll work item
>>>>> +	 */
>>>>> +	if (!(ret < 0))
>>>>> +		queue_delayed_work(system_power_efficient_wq,
>>>>> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>>>>>  }
>>>>>  
>>>>>  static int tps6598x_check_mode(struct tps6598x *tps)
>>>>> @@ -704,6 +768,7 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
>>>>>  static int tps6598x_probe(struct i2c_client *client)
>>>>>  {
>>>>>  	irq_handler_t irq_handler = tps6598x_interrupt;
>>>>> +	work_func_t work_poll_handler = tps6598x_poll_work;
>>>>>  	struct device_node *np = client->dev.of_node;
>>>>>  	struct typec_capability typec_cap = { };
>>>>>  	struct tps6598x *tps;
>>>>> @@ -748,6 +813,7 @@ static int tps6598x_probe(struct i2c_client *client)
>>>>>  			APPLE_CD_REG_INT_PLUG_EVENT;
>>>>>  
>>>>>  		irq_handler = cd321x_interrupt;
>>>>> +		work_poll_handler = cd321x_poll_work;
>>>>>  	} else {
>>>>>  		/* Enable power status, data status and plug event interrupts */
>>>>>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
>>>>> @@ -846,6 +912,16 @@ static int tps6598x_probe(struct i2c_client *client)
>>>>>  					irq_handler,
>>>>>  					IRQF_SHARED | IRQF_ONESHOT,
>>>>>  					dev_name(&client->dev), tps);
>>>>> +	if (ret == -EINVAL) {
>>>>> +		dev_warn(&client->dev, "Unable to find the interrupt, switching to polling\n");
>>>>> +		ret = devm_delayed_work_autocancel(tps->dev, &tps->wq_poll, work_poll_handler);
>>>>> +		if (ret)
>>>>> +			dev_err(&client->dev, "error while initializing workqueue\n");
>>>>> +		else
>>>>> +			queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
>>>>> +					   msecs_to_jiffies(POLL_INTERVAL));
>>>>> +	}
>>>>> +
>>>>>  	if (ret) {
>>>>>  		tps6598x_disconnect(tps, 0);
>>>>>  		typec_unregister_port(tps->port);
>>>>
>>>> thanks,
>>>>
>>>
>>>

cheers,
-roger

Re: [PATCH RFC] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected

[Aswath Govindraju]

Hi Roger,

On 13/04/22 17:12, Roger Quadros wrote:
> 
> 
> On 13/04/2022 13:51, Aswath Govindraju wrote:
>> Hi Roger,
>>
>> On 13/04/22 16:08, Roger Quadros wrote:
>>> Hi Aswath,
>>>
>>> On 13/04/2022 13:02, Aswath Govindraju wrote:
>>>> Hi Heikki,
>>>>
>>>> On 13/04/22 15:04, Heikki Krogerus wrote:
>>>>> Hi Aswath,
>>>>>
>>>>> On Tue, Apr 12, 2022 at 08:20:58PM +0530, Aswath Govindraju wrote:
>>>>>> In some cases the interrupt line from the pd controller may not be
>>>>>> connected. In these cases, poll the status of various events.
>>>>>
>>>>> Well, if the alert/interrupt line is not connected anywhere, then
>>>>> polling is the only way to go. I'm fine with that, but the driver
>>>>> really should be told that there is no interrupt. Using polling
>>>>> whenever request_threaded_irq() returns -EINVAL is wrong. We really
>>>>> should not even attempt to request the interrupt if there is no
>>>>> interrupt for the device.
>>>>>
>>>>> Isn't there any way you can get that information from DT? Or how is
>>>>> the device enumerated in your case?
>>>>>
>>>>
>>>> Would checking if (client->irq) field is populated, to decide between
>>>> polling and interrupts be a good approach?
>>>
>>> 'interrupt' and 'interrupt-names' are required properties in DT binding doc
>>> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
>>>
>>> You will need to add a new property to indicate that polling mode must be used
>>> and then interrupt properties become optional.
>>>
>>
>> Doesn't adding polling support mean that we are making interrupts
>> optional? So, can't we directly make the properties optional in the
>> bindings?
> 
> I don't see why not. It must be clear from the binding documentation that
> interrupt property is required unless polling mode is specified.
> 

To a get a confirmation on whether I understood correctly, would the
following update in the bindings and using (client->irq) to
differentiate between polling and interrupts be the correct approach?

diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
index a4c53b1f1af3..1c4b8c6233e5 100644
--- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
@@ -25,6 +25,8 @@ properties:

   interrupts:
     maxItems: 1
+    description:
+      If interrupts are not populated then by default polling will be used.

   interrupt-names:
     items:
@@ -33,8 +35,6 @@ properties:
 required:
   - compatible
   - reg
-  - interrupts
-  - interrupt-names

 additionalProperties: true

Thanks,
Aswath

>>
>>
>>>>
>>>> I am sorry but I did not understand what you meant by device getting
>>>> enumerated. The device is on an I2C bus and gets enumerated based on the
>>>> I2C address provided. The device does not have I2C_IRQ line connected,
>>>> in my case.
>>>
>>> I think he meant whether you are using device tree or something else.
>>>
>>
>> ohh okay, Thank you.
>>
>> Regards,
>> Aswath
>>
>>>>
>>>> Thanks,
>>>> Aswath
>>>>
>>>>>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>>>>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>>>>> ---
>>>>>>  drivers/usb/typec/tipd/core.c | 90 ++++++++++++++++++++++++++++++++---
>>>>>>  1 file changed, 83 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>>>>>> index 16b4560216ba..fa52d2067d6d 100644
>>>>>> --- a/drivers/usb/typec/tipd/core.c
>>>>>> +++ b/drivers/usb/typec/tipd/core.c
>>>>>> @@ -15,6 +15,8 @@
>>>>>>  #include <linux/interrupt.h>
>>>>>>  #include <linux/usb/typec.h>
>>>>>>  #include <linux/usb/role.h>
>>>>>> +#include <linux/workqueue.h>
>>>>>> +#include <linux/devm-helpers.h>
>>>>>>  
>>>>>>  #include "tps6598x.h"
>>>>>>  #include "trace.h"
>>>>>> @@ -93,6 +95,8 @@ struct tps6598x {
>>>>>>  	struct power_supply *psy;
>>>>>>  	struct power_supply_desc psy_desc;
>>>>>>  	enum power_supply_usb_type usb_type;
>>>>>> +
>>>>>> +	struct delayed_work wq_poll;
>>>>>>  };
>>>>>>  
>>>>>>  static enum power_supply_property tps6598x_psy_props[] = {
>>>>>> @@ -473,9 +477,8 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
>>>>>>  	}
>>>>>>  }
>>>>>>  
>>>>>> -static irqreturn_t cd321x_interrupt(int irq, void *data)
>>>>>> +static int cd321x_handle_interrupt_status(struct tps6598x *tps)
>>>>>>  {
>>>>>> -	struct tps6598x *tps = data;
>>>>>>  	u64 event;
>>>>>>  	u32 status;
>>>>>>  	int ret;
>>>>>> @@ -513,14 +516,45 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>>>>>>  err_unlock:
>>>>>>  	mutex_unlock(&tps->lock);
>>>>>>  
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>>  	if (event)
>>>>>> -		return IRQ_HANDLED;
>>>>>> -	return IRQ_NONE;
>>>>>> +		return 0;
>>>>>> +	return 1;
>>>>>>  }
>>>>>>  
>>>>>> -static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>>>>> +static irqreturn_t cd321x_interrupt(int irq, void *data)
>>>>>>  {
>>>>>>  	struct tps6598x *tps = data;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = cd321x_handle_interrupt_status(tps);
>>>>>> +	if (ret)
>>>>>> +		return IRQ_NONE;
>>>>>> +	return IRQ_HANDLED;
>>>>>> +}
>>>>>> +
>>>>>> +/* Time interval for Polling */
>>>>>> +#define POLL_INTERVAL   500 /* msecs */
>>>>>> +static void cd321x_poll_work(struct work_struct *work)
>>>>>> +{
>>>>>> +	struct tps6598x *tps = container_of(to_delayed_work(work),
>>>>>> +					    struct tps6598x, wq_poll);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = cd321x_handle_interrupt_status(tps);
>>>>>> +	/*
>>>>>> +	 * If there is an error while reading the interrupt registers
>>>>>> +	 * then stop polling else, schedule another poll work item
>>>>>> +	 */
>>>>>> +	if (!(ret < 0))
>>>>>> +		queue_delayed_work(system_power_efficient_wq,
>>>>>> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>>>>>> +}
>>>>>> +
>>>>>> +static int tps6598x_handle_interrupt_status(struct tps6598x *tps)
>>>>>> +{
>>>>>>  	u64 event1;
>>>>>>  	u64 event2;
>>>>>>  	u32 status;
>>>>>> @@ -561,9 +595,39 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>>>>>  err_unlock:
>>>>>>  	mutex_unlock(&tps->lock);
>>>>>>  
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>>  	if (event1 | event2)
>>>>>> -		return IRQ_HANDLED;
>>>>>> -	return IRQ_NONE;
>>>>>> +		return 0;
>>>>>> +	return 1;
>>>>>> +}
>>>>>> +
>>>>>> +static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>>>>> +{
>>>>>> +	struct tps6598x *tps = data;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = tps6598x_handle_interrupt_status(tps);
>>>>>> +	if (ret)
>>>>>> +		return IRQ_NONE;
>>>>>> +	return IRQ_HANDLED;
>>>>>> +}
>>>>>> +
>>>>>> +static void tps6598x_poll_work(struct work_struct *work)
>>>>>> +{
>>>>>> +	struct tps6598x *tps = container_of(to_delayed_work(work),
>>>>>> +					    struct tps6598x, wq_poll);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = tps6598x_handle_interrupt_status(tps);
>>>>>> +	/*
>>>>>> +	 * If there is an error while reading the interrupt registers
>>>>>> +	 * then stop polling else, schedule another poll work item
>>>>>> +	 */
>>>>>> +	if (!(ret < 0))
>>>>>> +		queue_delayed_work(system_power_efficient_wq,
>>>>>> +				   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>>>>>>  }
>>>>>>  
>>>>>>  static int tps6598x_check_mode(struct tps6598x *tps)
>>>>>> @@ -704,6 +768,7 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
>>>>>>  static int tps6598x_probe(struct i2c_client *client)
>>>>>>  {
>>>>>>  	irq_handler_t irq_handler = tps6598x_interrupt;
>>>>>> +	work_func_t work_poll_handler = tps6598x_poll_work;
>>>>>>  	struct device_node *np = client->dev.of_node;
>>>>>>  	struct typec_capability typec_cap = { };
>>>>>>  	struct tps6598x *tps;
>>>>>> @@ -748,6 +813,7 @@ static int tps6598x_probe(struct i2c_client *client)
>>>>>>  			APPLE_CD_REG_INT_PLUG_EVENT;
>>>>>>  
>>>>>>  		irq_handler = cd321x_interrupt;
>>>>>> +		work_poll_handler = cd321x_poll_work;
>>>>>>  	} else {
>>>>>>  		/* Enable power status, data status and plug event interrupts */
>>>>>>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
>>>>>> @@ -846,6 +912,16 @@ static int tps6598x_probe(struct i2c_client *client)
>>>>>>  					irq_handler,
>>>>>>  					IRQF_SHARED | IRQF_ONESHOT,
>>>>>>  					dev_name(&client->dev), tps);
>>>>>> +	if (ret == -EINVAL) {
>>>>>> +		dev_warn(&client->dev, "Unable to find the interrupt, switching to polling\n");
>>>>>> +		ret = devm_delayed_work_autocancel(tps->dev, &tps->wq_poll, work_poll_handler);
>>>>>> +		if (ret)
>>>>>> +			dev_err(&client->dev, "error while initializing workqueue\n");
>>>>>> +		else
>>>>>> +			queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
>>>>>> +					   msecs_to_jiffies(POLL_INTERVAL));
>>>>>> +	}
>>>>>> +
>>>>>>  	if (ret) {
>>>>>>  		tps6598x_disconnect(tps, 0);
>>>>>>  		typec_unregister_port(tps->port);
>>>>>
>>>>> thanks,
>>>>>
>>>>
>>>>
> 
> cheers,
> -roger


-- 
Thanks,
Aswath