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

From: Heikki Krogerus

Date: Mon Sep 07 2026 - 08:11:36 EST

On Tue, Sep 01, 2026 at 01:03:50PM +0100, Edward Blair wrote:
> 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.

Okay, got it.

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

I'm sure it's fine as it is now.

Thanks,

> Thanks,
> Edward
>
> On Tue, 1 Sept 2026 at 11:57, Heikki Krogerus
> <heikki.krogerus@xxxxxxxxxxxxxxx> 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 <edward.blair@xxxxxxxxx>
> > > ---
> > > 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

--
heikki