Re: [PATCH v3 2/2] usb: typec: ucsi: add ITE885x I2C transport driver

Thanks,

I will add the explicit includes and use guard(mutex) for event_lock.

I kept the explicit IRQ handling because ucsi_resume() queues work which
issues UCSI commands. If the callbacks are moved to the noirq phase,
that work can run before the parent I2C controller's resume_early or
resume callback, while the controller may still be unavailable.

The current ordering disables the device IRQ before quiescing UCSI, then
re-enables it during normal resumption after the I2C bus has resumed. Does
that address your concern, or would it be preferable the IRQ and UCSI
handling to be split across separate PM phases?

Thanks,
Edward

On Tue, 1 Sept 2026 at 11:57, Heikki Krogerus
<[email protected]> wrote:
>
> On Sat, Aug 29, 2026 at 03:55:16PM +0100, Edward Blair wrote:
> > Add a UCSI transport driver for ITE8853 and ITE8800 through ITE8805
> > USB Type-C controllers found on desktop motherboards.
> >
> > These controllers expose CCI, MESSAGE_IN and CONTROL at ITE-specific I2C
> > offsets and signal UCSI and vendor events through a shared interrupt
> > status register. Read and cache each complete UCSI event before
> > acknowledging it so command data remains coherent between the interrupt
> > handler and UCSI core.
> >
> > The interface does not expose a VERSION register and does not accept
> > PPM_RESET over I2C. Report UCSI 1.0, limit MESSAGE_IN to its 16-byte
> > window and handle PPM_RESET locally, matching the vendor driver's
> > behavior.
> >
> > Signed-off-by: Edward Blair <[email protected]>
> > ---
> >  drivers/usb/typec/ucsi/Kconfig    |  11 +
> >  drivers/usb/typec/ucsi/Makefile   |   1 +
> >  drivers/usb/typec/ucsi/ucsi_ite.c | 395 ++++++++++++++++++++++++++++++
> >  3 files changed, 407 insertions(+)
> >  create mode 100644 drivers/usb/typec/ucsi/ucsi_ite.c
> >
> > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> > index 87dd992a4..3819c4f73 100644
> > --- a/drivers/usb/typec/ucsi/Kconfig
> > +++ b/drivers/usb/typec/ucsi/Kconfig
> > @@ -104,4 +104,15 @@ config UCSI_HUAWEI_GAOKUN
> >         To compile the driver as a module, choose M here: the module will be
> >         called ucsi_huawei_gaokun.
> >
> > +config UCSI_ITE
> > +     tristate "UCSI Interface Driver for ITE885x"
> > +     depends on ACPI && I2C
> > +     help
> > +       This driver enables UCSI support on platforms that expose an ITE8853
> > +       or ITE8800-ITE8805 USB Type-C controller over I2C, commonly found
> > +       on ASUS Z690/Z790/X670E motherboards.
> > +
> > +       To compile the driver as a module, choose M here: the module will be
> > +       called ucsi_ite.
> > +
> >  endif
> > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> > index c7e38bf01..9bc1d6bbb 100644
> > --- a/drivers/usb/typec/ucsi/Makefile
> > +++ b/drivers/usb/typec/ucsi/Makefile
> > @@ -28,3 +28,4 @@ obj-$(CONFIG_UCSI_PMIC_GLINK)               += ucsi_glink.o
> >  obj-$(CONFIG_CROS_EC_UCSI)           += cros_ec_ucsi.o
> >  obj-$(CONFIG_UCSI_LENOVO_YOGA_C630)  += ucsi_yoga_c630.o
> >  obj-$(CONFIG_UCSI_HUAWEI_GAOKUN)     += ucsi_huawei_gaokun.o
> > +obj-$(CONFIG_UCSI_ITE)                       += ucsi_ite.o
> > diff --git a/drivers/usb/typec/ucsi/ucsi_ite.c b/drivers/usb/typec/ucsi/ucsi_ite.c
> > new file mode 100644
> > index 000000000..16b22c77f
> > --- /dev/null
> > +++ b/drivers/usb/typec/ucsi/ucsi_ite.c
> > @@ -0,0 +1,395 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * UCSI I2C transport driver for ITE885x USB-C controllers
> > + *
> > + * ITE8853/ITE8800-ITE8805 are UCSI-compliant USB-C controllers found on
> > + * desktop motherboards. They communicate over I2C using UCSI registers at
> > + * ITE-specific offsets and signal events through a vendor interrupt register.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
>
> We need to include everything instead of relying on nested headers
> nowadays.
>
>         linux/device.h
>         linux/device/devres.h
>         linux/dev_printk.h
>         linux/pm.h
>         linux/string.h
>
> > +static int ucsi_ite_process_event(struct ucsi_ite *ite, u32 *cci)
> > +{
> > +     u8 message_in[ITE_MESSAGE_IN_MAX_LEN] = {};
> > +     __le32 raw_cci;
> > +     u8 status;
> > +     u8 len = 0;
> > +     int event;
> > +     int err = 0;
> > +     int ret;
> > +
> > +     mutex_lock(&ite->event_lock);
>
> guard(mutex)(@ite->event_lock);
>
> > +     ret = ucsi_ite_read(ite, ITE_REG_INT_STATUS, &status, sizeof(status));
> > +     if (ret)
> > +             goto out_unlock;
> > +
> > +     status &= ITE_INT_MASK;
> > +     if (!status) {
> > +             mutex_lock(&ite->received_lock);
> > +             *cci = ite->cci;
> > +             mutex_unlock(&ite->received_lock);
> > +             ret = ITE_EVENT_NONE;
> > +             goto out_unlock;
> > +     }
> > +
> > +     if (status & ITE_INT_CCI) {
> > +             err = ucsi_ite_read(ite, ITE_REG_CCI, &raw_cci,
> > +                                 sizeof(raw_cci));
> > +             if (!err) {
> > +                     *cci = le32_to_cpu(raw_cci);
> > +                     len = UCSI_CCI_LENGTH(*cci);
> > +
> > +                     if (len > sizeof(message_in)) {
> > +                             len = sizeof(message_in);
> > +                             *cci &= ~GENMASK(15, 8);
> > +                             *cci |= UCSI_SET_CCI_LENGTH(len);
> > +                     }
> > +                     if (len) {
> > +                             err = ucsi_ite_read(ite, ITE_REG_MESSAGE_IN,
> > +                                                 message_in, len);
> > +                     }
> > +             }
> > +     }
> > +
> > +     /* Acknowledge each latched event with the value expected by the PPM. */
> > +     if (status & ITE_INT_VENDOR_ALERT) {
> > +             u8 ack = ITE_INT_VENDOR_ALERT;
> > +
> > +             ret = ucsi_ite_write(ite, ITE_REG_INT_ACK, &ack, sizeof(ack));
> > +             if (ret)
> > +                     goto out_unlock;
> > +     }
> > +
> > +     if ((status & ITE_INT_CCI) && !err) {
> > +             u8 ack = ITE_INT_CCI;
> > +
> > +             ret = ucsi_ite_write(ite, ITE_REG_INT_ACK, &ack, sizeof(ack));
> > +             if (ret)
> > +                     goto out_unlock;
> > +     }
> > +
> > +     if (err) {
> > +             ret = err;
> > +             goto out_unlock;
> > +     }
> > +
> > +     if (status & ITE_INT_CCI) {
> > +             mutex_lock(&ite->received_lock);
> > +             ite->cci = *cci;
> > +             memset(ite->message_in, 0, sizeof(ite->message_in));
> > +             memcpy(ite->message_in, message_in, len);
> > +             mutex_unlock(&ite->received_lock);
> > +             event = ITE_EVENT_CCI;
> > +     } else {
> > +             mutex_lock(&ite->received_lock);
> > +             *cci = ite->cci;
> > +             mutex_unlock(&ite->received_lock);
> > +             event = ITE_EVENT_VENDOR;
> > +     }
> > +
> > +     ret = event;
> > +
> > +out_unlock:
> > +     mutex_unlock(&ite->event_lock);
> > +     return ret;
> > +}
>
> > +static int ucsi_ite_suspend(struct device *dev)
> > +{
> > +     struct ucsi_ite *ite = dev_get_drvdata(dev);
> > +     int ret;
> > +
> > +     disable_irq(ite->client->irq);
> > +     ret = ucsi_suspend(ite->ucsi);
> > +     if (ret)
> > +             enable_irq(ite->client->irq);
> > +
> > +     return ret;
> > +}
> > +
> > +static int ucsi_ite_resume(struct device *dev)
> > +{
> > +     struct ucsi_ite *ite = dev_get_drvdata(dev);
> > +
> > +     enable_irq(ite->client->irq);
> > +     return ucsi_resume(ite->ucsi);
> > +}
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(ucsi_ite_pm, ucsi_ite_suspend,
> > +                             ucsi_ite_resume);
>
> Is the enable/disable_irq really necessary - couldn't you just use
> NOIRQ version of the PM operations (DEFINE_NOIRQ_DEV_PM_OPS)?
>
> Thanks,
>
> --
> heikki