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

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

Hi Heikki,

On 13-May-22 15:30, Heikki Krogerus wrote:
> On Thu, May 12, 2022 at 10:53:25PM +0530, Goswami, Sanket wrote:
>> 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.
> 
> It really does not matter if your platform does not require this -
> other platforms still need it.
> 
> Add a function where you handle the irq request, for example
> ccg_request_irq(), and then just call that function in both places.

Apologize for delay, I will re-spin the v2 based on this changes shortly.

Thanks,
Sanket