Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 13 Apr 2022 12:34:26 +0300
From:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
To:     Aswath Govindraju <a-govindraju@ti.com>
Cc:     Vignesh Raghavendra <vigneshr@ti.com>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Roger Quadros <rogerq@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Sven Peter <sven@svenpeter.dev>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Hector Martin <marcan@marcan.st>,
        Saranya Gopal <saranya.gopal@intel.com>,
        Jens Axboe <axboe@kernel.dk>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC] usb: typec: tipd: Add support for polling interrupts
 status when interrupt line is not connected
Message-ID: <YlaZIual4Fa/a81I@kuha.fi.intel.com>
References: <20220412145059.4717-1-a-govindraju@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220412145059.4717-1-a-govindraju@ti.com>
Precedence: bulk

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