[PATCH v2 0/2] Updates to ucsi_ccg driver

[PATCH v2 0/2] Updates to ucsi_ccg driver

[Sanket Goswami]

This patch series includes:
- Add ACPI ID to enumerate ucsi_ccg client
- Add new routine ccg_request_irq() to handle irq requests

Sanket Goswami (2):
  ucsi_ccg: ACPI based I2c client enumeration for AMD ASICs
  ucsi_ccg: Do not hardcode interrupt polarity and type

 drivers/usb/typec/ucsi/ucsi_ccg.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] ucsi_ccg: ACPI based I2c client enumeration for AMD ASICs

[Sanket Goswami]

Some of the AMD platforms have Cypress CCGX PD controller connected
to system I2C i.e designware I2C controller. Added support to enumerate
the CCGX client by adding ACPI ID to the firmware.

Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
---
 drivers/usb/typec/ucsi/ucsi_ccg.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 6db7c8ddd51c..7585599bacfd 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -1418,6 +1418,12 @@ static const struct i2c_device_id ucsi_ccg_device_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
 
+static const struct acpi_device_id amd_i2c_ucsi_match[] = {
+	{"AMDI0042"},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, amd_i2c_ucsi_match);
+
 static int ucsi_ccg_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -1459,6 +1465,7 @@ static struct i2c_driver ucsi_ccg_driver = {
 		.name = "ucsi_ccg",
 		.pm = &ucsi_ccg_pm,
 		.dev_groups = ucsi_ccg_groups,
+		.acpi_match_table = amd_i2c_ucsi_match,
 	},
 	.probe = ucsi_ccg_probe,
 	.remove = ucsi_ccg_remove,
-- 
2.25.1

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

[Sanket Goswami]

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@linux.intel.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
---
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);
+}
+
 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;
-- 
2.25.1

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

[Heikki Krogerus]

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@linux.intel.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> ---
> 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

Re: [PATCH v2 1/2] ucsi_ccg: ACPI based I2c client enumeration for AMD ASICs

[Heikki Krogerus]

On Fri, May 20, 2022 at 04:57:03PM +0530, Sanket Goswami wrote:
> Some of the AMD platforms have Cypress CCGX PD controller connected
> to system I2C i.e designware I2C controller. Added support to enumerate
> the CCGX client by adding ACPI ID to the firmware.

Oh, this patch needs to come only after the patch 2/2. Otherwise
you'll break bisectability. Please swap the places of these patches in
the next version.

thanks,

> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> ---
>  drivers/usb/typec/ucsi/ucsi_ccg.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 6db7c8ddd51c..7585599bacfd 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -1418,6 +1418,12 @@ static const struct i2c_device_id ucsi_ccg_device_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
>  
> +static const struct acpi_device_id amd_i2c_ucsi_match[] = {
> +	{"AMDI0042"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, amd_i2c_ucsi_match);
> +
>  static int ucsi_ccg_resume(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> @@ -1459,6 +1465,7 @@ static struct i2c_driver ucsi_ccg_driver = {
>  		.name = "ucsi_ccg",
>  		.pm = &ucsi_ccg_pm,
>  		.dev_groups = ucsi_ccg_groups,
> +		.acpi_match_table = amd_i2c_ucsi_match,
>  	},
>  	.probe = ucsi_ccg_probe,
>  	.remove = ucsi_ccg_remove,
> -- 
> 2.25.1

-- 
heikki