Re: [PATCH v2 2/2] ucsi_ccg: Do not hardcode interrupt polarity and type

[Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>]

Hi Sanket,

On Fri, May 20, 2022 at 04:57:04PM +0530, Sanket Goswami wrote:
> The current implementation supports only Level trigger with ACTIVE HIGH,
> which is overriding level and polarity set by the ACPI table, hence
> Implement the common utility function to manage irq requests.
> 
> Suggested-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx>
> ---
> Changes in v2:
> - Implemented the new routine ccg_request_irq to handle irq requests.
> 
>  drivers/usb/typec/ucsi/ucsi_ccg.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 7585599bacfd..950efb2363f7 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -1239,6 +1239,12 @@ static int ccg_fw_update(struct ucsi_ccg *uc, enum enum_flash_mode flash_mode)
>  	return err;
>  }
>  
> +static int ccg_request_irq(int irq, struct ucsi_ccg *uc)
> +{
> +	return request_threaded_irq(irq, NULL, ccg_irq_handler,
> +				    IRQF_ONESHOT, dev_name(uc->dev), uc);
> +}

Like that this function would be completely useless...

>  static int ccg_restart(struct ucsi_ccg *uc)
>  {
>  	struct device *dev = uc->dev;
> @@ -1250,9 +1256,7 @@ static int ccg_restart(struct ucsi_ccg *uc)
>  		return status;
>  	}
>  
> -	status = request_threaded_irq(uc->irq, NULL, ccg_irq_handler,
> -				      IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> -				      dev_name(dev), uc);
> +	status = ccg_request_irq(uc->irq, uc);
>  	if (status < 0) {
>  		dev_err(dev, "request_threaded_irq failed - %d\n", status);
>  		return status;
> @@ -1366,9 +1370,7 @@ static int ucsi_ccg_probe(struct i2c_client *client,
>  
>  	ucsi_set_drvdata(uc->ucsi, uc);
>  
> -	status = request_threaded_irq(client->irq, NULL, ccg_irq_handler,
> -				      IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> -				      dev_name(dev), uc);
> +	status = ccg_request_irq(client->irq, uc);
>  	if (status < 0) {
>  		dev_err(uc->dev, "request_threaded_irq failed - %d\n", status);
>  		goto out_ucsi_destroy;

This will break the boards that don't support ACPI. I told you that
you need to now consider those as the exception. Something like this
should cover them (and make ccg_request_irq() actually useful):

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 6db7c8ddd51cd..0707a71562991 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -627,6 +627,16 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
        return IRQ_HANDLED;
 }
 
+static int ccg_request_irq(struct ucsi_ccg *uc)
+{
+       unsigned long flags = IRQF_ONESHOT;
+
+       if (!has_acpi_companion(uc->dev))
+               flags |= IRQF_TRIGGER_HIGH;
+
+       return request_threaded_irq(uc->irq, NULL, ccg_irq_handler, flags, dev_name(uc->dev), uc);
+}
+
 static void ccg_pm_workaround_work(struct work_struct *pm_work)
 {
        ccg_irq_handler(0, container_of(pm_work, struct ucsi_ccg, pm_work));
@@ -1250,9 +1260,7 @@ static int ccg_restart(struct ucsi_ccg *uc)
                return status;
        }
 
-       status = request_threaded_irq(uc->irq, NULL, ccg_irq_handler,
-                                     IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
-                                     dev_name(dev), uc);
+       status = ccg_request_irq(uc);
        if (status < 0) {
                dev_err(dev, "request_threaded_irq failed - %d\n", status);
                return status;
@@ -1331,6 +1339,7 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 
        uc->dev = dev;
        uc->client = client;
+       uc->irq = client->irq;
        mutex_init(&uc->lock);
        init_completion(&uc->complete);
        INIT_WORK(&uc->work, ccg_update_firmware);
@@ -1366,16 +1375,12 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 
        ucsi_set_drvdata(uc->ucsi, uc);
 
-       status = request_threaded_irq(client->irq, NULL, ccg_irq_handler,
-                                     IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
-                                     dev_name(dev), uc);
+       status = ccg_request_irq(uc);
        if (status < 0) {
                dev_err(uc->dev, "request_threaded_irq failed - %d\n", status);
                goto out_ucsi_destroy;
        }
 
-       uc->irq = client->irq;
-
        status = ucsi_register(uc->ucsi);
        if (status)
                goto out_free_irq;

thanks,

-- 
heikki