* [PATCH 0/2] Updates to ucsi_ccg driver @ 2022-05-10 5:24 Sanket Goswami 2022-05-10 5:24 ` [PATCH 1/2] ucsi_ccg: ACPI based I2c client enumeration for AMD ASICs Sanket Goswami 2022-05-10 5:24 ` [PATCH 2/2] ucsi_ccg: Do not hardcode interrupt polarity and type Sanket Goswami 0 siblings, 2 replies; 8+ messages in thread From: Sanket Goswami @ 2022-05-10 5:24 UTC (permalink / raw) To: heikki.krogerus, gregkh, ajayg, linux-usb; +Cc: Sanket Goswami This patch series includes: - Add ACPI ID to enumerate ucsi_ccg client - Add interrupt type and polarity check for some AMD ASICs 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 | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] ucsi_ccg: ACPI based I2c client enumeration for AMD ASICs 2022-05-10 5:24 [PATCH 0/2] Updates to ucsi_ccg driver Sanket Goswami @ 2022-05-10 5:24 ` Sanket Goswami 2022-05-10 5:24 ` [PATCH 2/2] ucsi_ccg: Do not hardcode interrupt polarity and type Sanket Goswami 1 sibling, 0 replies; 8+ messages in thread From: Sanket Goswami @ 2022-05-10 5:24 UTC (permalink / raw) To: heikki.krogerus, gregkh, ajayg, linux-usb; +Cc: 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] ucsi_ccg: Do not hardcode interrupt polarity and type 2022-05-10 5:24 [PATCH 0/2] Updates to ucsi_ccg driver Sanket Goswami 2022-05-10 5:24 ` [PATCH 1/2] ucsi_ccg: ACPI based I2c client enumeration for AMD ASICs Sanket Goswami @ 2022-05-10 5:24 ` Sanket Goswami 2022-05-11 7:59 ` Heikki Krogerus 1 sibling, 1 reply; 8+ messages in thread From: Sanket Goswami @ 2022-05-10 5:24 UTC (permalink / raw) To: heikki.krogerus, gregkh, ajayg, linux-usb; +Cc: Sanket Goswami 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); 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}, {} }; MODULE_DEVICE_TABLE(acpi, amd_i2c_ucsi_match); -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] ucsi_ccg: Do not hardcode interrupt polarity and type 2022-05-10 5:24 ` [PATCH 2/2] ucsi_ccg: Do not hardcode interrupt polarity and type Sanket Goswami @ 2022-05-11 7:59 ` Heikki Krogerus 2022-05-11 8:06 ` Heikki Krogerus 0 siblings, 1 reply; 8+ messages in thread From: Heikki Krogerus @ 2022-05-11 7:59 UTC (permalink / raw) To: Sanket Goswami; +Cc: gregkh, ajayg, linux-usb 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. thanks, -- heikki ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] ucsi_ccg: Do not hardcode interrupt polarity and type 2022-05-11 7:59 ` Heikki Krogerus @ 2022-05-11 8:06 ` Heikki Krogerus 2022-05-12 17:23 ` Goswami, Sanket 0 siblings, 1 reply; 8+ messages in thread From: Heikki Krogerus @ 2022-05-11 8:06 UTC (permalink / raw) To: Sanket Goswami; +Cc: gregkh, ajayg, linux-usb 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); 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); if (status < 0) { dev_err(uc->dev, "request_threaded_irq failed - %d\n", status); goto out_ucsi_destroy; thanks, -- heikki ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] ucsi_ccg: Do not hardcode interrupt polarity and type 2022-05-11 8:06 ` Heikki Krogerus @ 2022-05-12 17:23 ` Goswami, Sanket 2022-05-13 10:00 ` Heikki Krogerus 0 siblings, 1 reply; 8+ messages in thread From: Goswami, Sanket @ 2022-05-12 17:23 UTC (permalink / raw) To: Heikki Krogerus; +Cc: gregkh, ajayg, linux-usb 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] ucsi_ccg: Do not hardcode interrupt polarity and type 2022-05-12 17:23 ` Goswami, Sanket @ 2022-05-13 10:00 ` Heikki Krogerus 2022-05-20 11:25 ` Goswami, Sanket 0 siblings, 1 reply; 8+ messages in thread From: Heikki Krogerus @ 2022-05-13 10:00 UTC (permalink / raw) To: Goswami, Sanket; +Cc: gregkh, ajayg, linux-usb 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. > > 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. thanks, -- heikki ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] ucsi_ccg: Do not hardcode interrupt polarity and type 2022-05-13 10:00 ` Heikki Krogerus @ 2022-05-20 11:25 ` Goswami, Sanket 0 siblings, 0 replies; 8+ messages in thread From: Goswami, Sanket @ 2022-05-20 11:25 UTC (permalink / raw) To: Heikki Krogerus; +Cc: gregkh, ajayg, linux-usb 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-05-20 11:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-05-10 5:24 [PATCH 0/2] Updates to ucsi_ccg driver Sanket Goswami 2022-05-10 5:24 ` [PATCH 1/2] ucsi_ccg: ACPI based I2c client enumeration for AMD ASICs Sanket Goswami 2022-05-10 5:24 ` [PATCH 2/2] ucsi_ccg: Do not hardcode interrupt polarity and type Sanket Goswami 2022-05-11 7:59 ` Heikki Krogerus 2022-05-11 8:06 ` Heikki Krogerus 2022-05-12 17:23 ` Goswami, Sanket 2022-05-13 10:00 ` Heikki Krogerus 2022-05-20 11:25 ` Goswami, Sanket
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.