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

[Roger Quadros <rogerq@kernel.org>]

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