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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
>>>>>> Signed-off-by: Aswath Govindraju <a-govindraju@xxxxxx>
>>>>>> ---
>>>>>>  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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux