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
