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@xxxxxxx>
>>>>> ---
>>>>> 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
