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

["Goswami, Sanket" <Sanket.Goswami@amd.com>]

Hi Heikki,

On 11-May-22 13:36, Heikki Krogerus wrote:
> On Wed, May 11, 2022 at 10:59:42AM +0300, Heikki Krogerus wrote:
>> On Tue, May 10, 2022 at 10:54:37AM +0530, Sanket Goswami wrote:
>>> The current implementation supports only Level trigger with ACTIVE HIGH.
>>> Some of the AMD platforms have different PD firmware implementation which
>>> needs different polarity. This patch checks the polarity and type based
>>> on the device properties set and registers the interrupt handler
>>> accordingly.
>>>
>>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>>> ---
>>>  drivers/usb/typec/ucsi/ucsi_ccg.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
>>> index 7585599bacfd..0db935bd8473 100644
>>> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
>>> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
>>> @@ -20,6 +20,7 @@
>>>  
>>>  #include <asm/unaligned.h>
>>>  #include "ucsi.h"
>>> +#define INTR_POL_TYPE	BIT(0)
>>>  
>>>  enum enum_fw_mode {
>>>  	BOOT,   /* bootloader */
>>> @@ -1324,6 +1325,8 @@ static int ucsi_ccg_probe(struct i2c_client *client,
>>>  	struct device *dev = &client->dev;
>>>  	struct ucsi_ccg *uc;
>>>  	int status;
>>> +	/* Keep the IRQ type and polarity default as Level trigger Active High */
>>> +	int irqtype = IRQF_TRIGGER_HIGH;
>>>  
>>>  	uc = devm_kzalloc(dev, sizeof(*uc), GFP_KERNEL);
>>>  	if (!uc)
>>> @@ -1366,8 +1369,12 @@ static int ucsi_ccg_probe(struct i2c_client *client,
>>>  
>>>  	ucsi_set_drvdata(uc->ucsi, uc);
>>>  
>>> +	status = (uintptr_t)device_get_match_data(dev);
>>> +	if (status & INTR_POL_TYPE)
>>> +		irqtype = IRQF_TRIGGER_FALLING;
>>> +
>>>  	status = request_threaded_irq(client->irq, NULL, ccg_irq_handler,
>>> -				      IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
>>> +				      IRQF_ONESHOT | irqtype,
>>>  				      dev_name(dev), uc);
>>
>> Please note that you would need to update ccg_restart() as well, but
>> there is something else wrong here...
>>
>>>  	if (status < 0) {
>>>  		dev_err(uc->dev, "request_threaded_irq failed - %d\n", status);
>>> @@ -1419,7 +1426,7 @@ 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"},
>>> +	{"AMDI0042", INTR_POL_TYPE},
>>>  	{}
>>>  };
>>
>> This should not be necessary. That information comes from the ACPI
>> tables.
>>
>> I don't think that you need to set the polarity/level flags at all in
>> case of ACPI. I'll double check that, but if that is the case, then you
>> need to make the case where the device is not ACPI enumerated the
>> special case instead.
> 
> Actually, can you just test if this works for you:
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 6db7c8ddd51cd..f13c10e815d7d 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -1251,8 +1251,7 @@ static int ccg_restart(struct ucsi_ccg *uc)
>         }
>  
>         status = request_threaded_irq(uc->irq, NULL, ccg_irq_handler,
> -                                     IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> -                                     dev_name(dev), uc);
> +                                     IRQF_ONESHOT, dev_name(dev), uc);

For AMD platforms, this change is not require as we are not doing firmware
download using driver.

>         if (status < 0) {
>                 dev_err(dev, "request_threaded_irq failed - %d\n", status);
>                 return status;
> @@ -1367,8 +1366,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);
> +                                     IRQF_ONESHOT, dev_name(dev), uc);

Thanks for this suggestion, I have validated the same on AMD platforms and it is
functional. Will re-spin the new patch series with this change.

>         if (status < 0) {
>                 dev_err(uc->dev, "request_threaded_irq failed - %d\n", status);
>                 goto out_ucsi_destroy;
> 
> thanks,
> 

Thanks,
Sanket