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