Re: [PATCH 2/2] usb: typec: ucsi: Add ITE IT885x Type-C PD controller driver

[Heikki Krogerus]

Hi,

On Mon, Jun 15, 2026 at 09:47:40PM +0800, Amber Kao wrote:
> Add core, UCSI, and Alternate Mode support for the ITE IT885x
> Type-C Power Delivery controller over I2C. The driver uses the
> auxiliary bus to spawn UCSI and Alternate Mode child devices from
> the main I2C core driver.
> 
> Cc: Yaode Fang <Yaode.Fang@xxxxxxxxxx>
> Cc: Jeson Yang <jeson.yang@xxxxxxxxxx>
> Cc: Bling Chiang <Bling.Chiang@xxxxxxxxxx>
> Cc: Eric Su <Eric.Su@xxxxxxxxxx>
> Cc: Doreen Lin <doreen.lin@xxxxxxxxxx>
> 
> Signed-off-by: Amber Kao <amber.kao@xxxxxxxxxx>
> ---
>  MAINTAINERS                            |   4 +
>  drivers/usb/typec/ucsi/Kconfig         |  15 +
>  drivers/usb/typec/ucsi/Makefile        |   1 +
>  drivers/usb/typec/ucsi/itepd.c         | 481 ++++++++++++++++++++++++++++
>  drivers/usb/typec/ucsi/itepd.h         |  64 ++++
>  drivers/usb/typec/ucsi/itepd_altmode.c | 438 ++++++++++++++++++++++++++
>  drivers/usb/typec/ucsi/ucsi_itepd.c    | 558 +++++++++++++++++++++++++++++++++
>  7 files changed, 1561 insertions(+)

I could not figure out what is the purpose of the separated altmode
handling. Why would you need separate handling for the retimers and
muxes? If the typec core is not doing something properly, then we need
to fix that. I also did not quite understand why are you creating
child device for the ucsi and altmode.

You need to split this into smaller patches and provide a bit of
explanation for each feature. Start with the bare minimum. So UCSI
without support for altmodes, just register the ports and partners.
Then add things one by one, each feature in its own patch.

<snip>

> +static void ucsi_itepd_command_hook(struct ucsi_itepd *ucsi_itepd, u64 *cmd)
> +{
> +	/* Translate UCSI 1.2 commands/fields to ITE PD controller (v2.1) */
> +	switch (UCSI_COMMAND(*cmd)) {
> +	case UCSI_SET_NOTIFICATION_ENABLE:
> +		if (*cmd & UCSI_ENABLE_NTFY_CMD_COMPLETE)
> +			/* Enable Attention Notification for alt. mode */
> +			*cmd |= FIELD_PREP(GENMASK_ULL(32, 16), BIT(3));
> +		break;
> +	case UCSI_GET_PDOS:
> +		*cmd &= ~GENMASK_ULL(38, 37);
> +		break;
> +	case UCSI_GET_ERROR_STATUS:
> +		*cmd &= ~GENMASK_ULL(22, 16);
> +		*cmd |= UCSI_CONNECTOR_NUMBER(ucsi_itepd->cmd_port + 1);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	/* Track the connector number associated with this command */
> +	switch (UCSI_COMMAND(*cmd)) {
> +	case UCSI_PPM_RESET:
> +	case UCSI_CANCEL:
> +	case UCSI_SET_NOTIFICATION_ENABLE:
> +	case UCSI_GET_CAPABILITY:
> +		ucsi_itepd->cmd_port = 0;
> +		break;
> +	case UCSI_CONNECTOR_RESET:
> +	case UCSI_GET_CONNECTOR_CAPABILITY:
> +	case UCSI_SET_CCOM:		/* 0x08 - SET_UOM in older specs */
> +	case UCSI_SET_UOR:
> +	case UCSI_SET_PDR:
> +	case UCSI_GET_CAM_SUPPORTED:
> +	case UCSI_GET_CURRENT_CAM:
> +	case UCSI_SET_NEW_CAM:
> +	case UCSI_GET_PDOS:
> +	case UCSI_GET_CABLE_PROPERTY:
> +	case UCSI_GET_CONNECTOR_STATUS:
> +	case UCSI_SET_POWER_LEVEL:	/* 0x14 */
> +	case UCSI_GET_PD_MESSAGE:	/* 0x15 */
> +	case UCSI_GET_ATTENTION_VDO:	/* 0x16 */
> +	case UCSI_GET_CAM_CS:		/* 0x18 */
> +	case 0x19:
> +	case 0x1A:
> +	case 0x1B:

Add definitions for these.

> +	case UCSI_SET_SINK_PATH:	/* 0x1C */
> +	case 0x1D:
> +	case UCSI_READ_POWER_LEVEL:	/* 0x1E */
> +	case 0x1F:
> +		ucsi_itepd->cmd_port =
> +			FIELD_GET(GENMASK(22, 16), *cmd) - 1;

Use the existing definitions with these.

		ucsi_itepd->cmd_port = UCSI_DEFAULT_GET_CONNECTOR_NUMBER(cmd) - 1;

> +		break;
> +	case UCSI_GET_ALTERNATE_MODES:
> +		ucsi_itepd->cmd_port =
> +			FIELD_GET(GENMASK(30, 24), *cmd) - 1;
> +		break;
> +	}
> +
> +	ucsi_itepd->cmd = *cmd;
> +}

I won't do complete review yet, but I'm just pointing out that since
you are going over all the commands here, it should not be a problem
to first disable some of them, and then add them in separate patches.

Thanks,

-- 
heikki