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.
> 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
