Re: [PATCH v8 3/3] usb: typec: tipd: Add sn201202x support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] usb: typec: tipd: missing no_mode_control in tipd_sn201202x_data
- [Medium] usb: typec: tipd: sn201202x_probe() ignores 0 from of_irq_get_byname()
- [Medium] usb: typec: tipd: race with tps->irq in sn201202x_suspend() and resume
- [Medium] usb: typec: tipd: missing sleep_completion wait after spmi_command_sleep()
- [Low] usb: typec: tipd: missing select REGMAP for TYPEC_SN201202X in Kconfig
--

commit 716779207041b8e609172ef9763d47b121f79a6d
Author: Sasha Finkelstein <k@chaosmail.tech>

usb: typec: tipd: Add sn201202x support

This commit adds support for the TI SN201202x (ACE3) USB Power Delivery
controller, which exposes a TPS6598x-compatible logical register map over
the SPMI bus on Apple M3 and later systems.

> diff --git a/drivers/usb/typec/tipd/Kconfig b/drivers/usb/typec/tipd/Kconfig
> index c87cbe6109ca6..8177f860f89de 100644
> --- a/drivers/usb/typec/tipd/Kconfig
> +++ b/drivers/usb/typec/tipd/Kconfig
> @@ -14,3 +14,15 @@ config TYPEC_TPS6598X
>  
>  	  If you choose to build this driver as a dynamically linked module, the
>  	  module will be called tps6598x.ko.
> +
> +config TYPEC_SN201202X
> +	tristate "TI SN201202x USB Power Delivery controller support"
> +	depends on SPMI
> +	select TYPEC_TPS6598X_CORE

[Severity: Low]
Can this fail to build when TYPEC_SN201202X is enabled without
TYPEC_TPS6598X or another option that enables REGMAP?

In drivers/usb/typec/tipd/Kconfig, TYPEC_TPS6598X selects REGMAP_I2C,
whereas TYPEC_TPS6598X_CORE only selects USB_ROLE_SWITCH and POWER_SUPPLY:

drivers/usb/typec/tipd/Kconfig:
config TYPEC_TPS6598X_CORE
	tristate
	select USB_ROLE_SWITCH
	select POWER_SUPPLY

Because neither SPMI, TYPEC_TPS6598X_CORE, nor TYPEC_SN201202X selects
REGMAP, tps6598x_regmap_config in drivers/usb/typec/tipd/core.c and
regmap_sn201202x and __devm_regmap_init_sn201202x() in
drivers/usb/typec/tipd/spmi.c reference struct regmap_config,
struct regmap_bus, REGMAP_ENDIAN_NATIVE, and __devm_regmap_init(), which
are only declared under CONFIG_REGMAP.

Should TYPEC_SN201202X or TYPEC_TPS6598X_CORE select REGMAP?

[ ... ]

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 6c953e4a66d5f..a21f013449249 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1963,11 +1963,32 @@ static const struct tipd_data tipd_tps25750_data = {
>  	.connect = tps6598x_connect,
>  };
>  
> +static const struct tipd_data tipd_sn201202x_data = {
> +	.irq_handler = cd321x_interrupt,
> +	.irq_mask1 = APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
> +		     APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
> +		     APPLE_CD_REG_INT_PLUG_EVENT,
> +	.tps_struct_size = sizeof(struct sn201202x),
> +	.remove = cd321x_remove,
> +	.register_port = cd321x_register_port,

[Severity: Medium]
Should tipd_sn201202x_data also set .no_mode_control = true?

tipd_sn201202x_data shares cd321x_register_port(), cd321x_connect(), and
cd321x_interrupt() with tipd_cd321x_data, where the controller firmware
manages alternate mode entry autonomously:

drivers/usb/typec/tipd/core.c:
static const struct tipd_data tipd_cd321x_data = {
	...
	.tps_struct_size = sizeof(struct cd321x),
	.no_mode_control = true,
	.remove = cd321x_remove,
	.register_port = cd321x_register_port,

Without .no_mode_control = true in tipd_sn201202x_data,
sn201202x_probe() -> tipd_init() -> cd321x_register_port() ->
tps6598x_register_port() leaves typec_cap.no_mode_control false and
exposes host alternate mode control attributes on the registered port.

[ ... ]

> diff --git a/drivers/usb/typec/tipd/spmi.c b/drivers/usb/typec/tipd/spmi.c
> new file mode 100644
> index 0000000000000..a165ec39a8ed8
> --- /dev/null
> +++ b/drivers/usb/typec/tipd/spmi.c

[ ... ]

> +static int sn201202x_probe(struct spmi_device *device)
> +{
> +	const struct of_device_id *match;
> +	const struct tipd_data *data;
> +	struct sn201202x *sn;
> +	struct tps6598x *tps;
> +	int irq_select, irq_sleep, irq_wake;
> +	int ret;
> +
> +	match = of_match_device(sn201202x_of_match, &device->dev);
> +	if (!match)
> +		return -EINVAL;
> +	data = tipd_hw_variants[(enum tipd_hw_variant)(uintptr_t)match->data];
> +
> +	sn = devm_kzalloc(&device->dev, data->tps_struct_size, GFP_KERNEL);
> +	if (!sn)
> +		return -ENOMEM;
> +	sn->sdev = device;
> +	tps = &sn->cd.tps;
> +
> +	mutex_init(&tps->lock);
> +	tps->dev = &device->dev;
> +	tps->data = data;
> +
> +	tps->irq = of_irq_get_byname(device->dev.of_node, "irq");
> +	if (tps->irq < 0)
> +		return tps->irq;
> +	irq_select = of_irq_get_byname(device->dev.of_node, "select");
> +	if (irq_select < 0)
> +		return irq_select;
> +	irq_sleep = of_irq_get_byname(device->dev.of_node, "sleep");
> +	if (irq_sleep < 0)
> +		return irq_sleep;
> +	irq_wake = of_irq_get_byname(device->dev.of_node, "wake");
> +	if (irq_wake < 0)
> +		return irq_wake;

[Severity: Medium]
As reported by Sashiko on v7 [1], what happens in sn201202x_probe() if
of_irq_get_byname() returns 0 on an IRQ mapping failure?

of_irq_get_byname() returns a negative errno on lookup error or probe
deferral, but returns 0 if IRQ mapping fails. Because sn201202x_probe()
only checks for values < 0, a 0 return for select, sleep, or wake will
pass IRQ 0 to devm_request_irq(), while a 0 return for irq leaves
tps->irq == 0 and causes tipd_init() to fall back to polling mode instead
of failing the probe.

[ ... ]

> +	ret = tipd_init(tps);
> +	if (ret)
> +		spmi_command_sleep(device);
> +	return ret;
> +}
> +
> +static void sn201202x_remove(struct spmi_device *device)
> +{
> +	struct tps6598x *tps = spmi_device_get_drvdata(device);
> +	struct sn201202x *sn = tps_to_sn(tps);
> +
> +	tipd_remove(tps);
> +	spmi_command_sleep(sn->sdev);
> +}

[Severity: Medium]
As reported by Sashiko on v7 [2], does calling spmi_command_sleep() in
sn201202x_probe() and sn201202x_remove() without waiting on
sn->sleep_completion race with IRQ teardown or subsequent re-probe?

In sn201202x_suspend(), spmi_command_sleep() is paired with
reinit_completion(&sn->sleep_completion) and
wait_for_completion_timeout(&sn->sleep_completion, ...). In the
tipd_init() error path of sn201202x_probe() (for example on -EPROBE_DEFER
when acquiring the USB role switch or Type-C mux) and in
sn201202x_remove(), spmi_command_sleep() is issued right before devres
frees irq_sleep without waiting for the sleep transition to complete.

> +
> +static int __maybe_unused sn201202x_resume(struct device *dev)
> +{
> +	struct tps6598x *tps = dev_get_drvdata(dev);
> +	struct sn201202x *sn = tps_to_sn(tps);
> +	int err;
> +
> +	reinit_completion(&sn->wake_completion);
> +	err = spmi_command_wakeup(sn->sdev);
> +	if (err)
> +		return err;
> +	if (!wait_for_completion_timeout(&sn->wake_completion, msecs_to_jiffies(100)))
> +		return -ETIMEDOUT;
> +	return tipd_resume(tps);
> +}
> +
> +static int __maybe_unused sn201202x_suspend(struct device *dev)
> +{
> +	struct tps6598x *tps = dev_get_drvdata(dev);
> +	struct sn201202x *sn = tps_to_sn(tps);
> +	int err;
> +
> +	err = tipd_suspend(tps);
> +	if (err)
> +		return err;
> +	reinit_completion(&sn->sleep_completion);
> +	err = spmi_command_sleep(sn->sdev);

[Severity: Medium]
Can cd321x_interrupt() or cd321x_update_work() run while the SPMI device
is asleep during sn201202x_suspend() and sn201202x_resume()?

In drivers/usb/typec/tipd/core.c, tipd_suspend() only disables tps->irq
when tps->wakeup is set, and does not cancel cd321x->update_work:

drivers/usb/typec/tipd/core.c:tipd_suspend() {
	if (tps->wakeup) {
		disable_irq(tps->irq);
		enable_irq_wake(tps->irq);
	} else if (tps->reset) {
		gpiod_set_value_cansleep(tps->reset, 1);
	}
	...
}

When !tps->wakeup, tps->irq remains enabled when sn201202x_suspend()
sends spmi_command_sleep(sn->sdev), and on resume dpm_resume_noirq()
re-enables device IRQs before sn201202x_resume() issues
spmi_command_wakeup(sn->sdev). If tps->irq fires or cd321x->update_work
runs during that window, cd321x_interrupt() will attempt SPMI register
accesses via regmap_sn201202x_select_reg() while the controller is in the
SPMI sleep state.

[1] https://lore.kernel.org/r/20260910-tipd-ace3-v7-3-6c6ebe2193f2@chaosmail.tech
[2] https://lore.kernel.org/r/20260910-tipd-ace3-v7-3-6c6ebe2193f2@chaosmail.tech

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-tipd-ace3-v8-0-770f42421753@chaosmail.tech?part=3