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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `tipd_sn201202x_data` structure fails to initialize the `.read_power_status` callback, causing a NULL pointer dereference when invoked.
- [Medium] Enabling `CONFIG_TYPEC_SN201202X` does not guarantee compilation because the Makefile skips the `tipd/` directory if `CONFIG_TYPEC_TPS6598X` is disabled.
- [Medium] Missing `spmi_command_sleep()` on wakeup completion timeout leaves the controller awake on error paths.
- [Low] Pointer to integer cast of different size in `sn201202x_probe` triggers compiler warnings on 64-bit architectures.
- [Low] Implicit positional structure initialization in `sn201202x_of_match` bypasses designated initializer syntax.
--

--- Patch [3]: [PATCH v5 3/3] usb: typec: tipd: Add sn201202x support ---
commit cf60ef5ec1afc63ccae95643a77a2844f4dd0245
Author: Sasha Finkelstein <[email protected]>

usb: typec: tipd: Add sn201202x support
    
Add support for sn201202x (aka ACE3), a tipd variant that uses a very
similar register map, that is exposed over a "logical register"
interface on the SPMI bus.

> 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: Medium]
Does enabling CONFIG_TYPEC_SN201202X guarantee that this driver will be
built?

It appears that the parent drivers/usb/typec/Makefile only descends into
the tipd/ directory if CONFIG_TYPEC_TPS6598X is enabled. If someone selects
CONFIG_TYPEC_SN201202X without CONFIG_TYPEC_TPS6598X, could this module
be silently excluded from the build entirely?

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 36fccd45858bf..296bc8adcc91e 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
[ ... ]
> @@ -1950,11 +1950,31 @@ 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,
> +	.unregister_port = cd321x_unregister_port,
> +	.trace_data_status = trace_cd321x_data_status,
> +	.trace_power_status = trace_tps6598x_power_status,
> +	.trace_status = trace_tps6598x_status,
> +	.init = cd321x_init,
> +	.read_data_status = cd321x_read_data_status,
> +	.reset = cd321x_reset,
> +	.switch_power_state = cd321x_switch_power_state,
> +	.connect = cd321x_connect,
> +};

[Severity: High]
Is the .read_power_status callback intentionally omitted here?

Without it, the uninitialized callback defaults to NULL. 

Would this result in a NULL pointer dereference during device probe? In 
tipd_init(), if a plug is present (status & TPS_STATUS_PLUG_PRESENT), 
the code invokes the callback unconditionally:

    if (!tps->data->read_power_status(tps))
        goto err_unregister_port;

This same NULL dereference could also happen in cd321x_interrupt() when a
power status update is received (event & APPLE_CD_REG_INT_POWER_STATUS_UPDATE).

> diff --git a/drivers/usb/typec/tipd/spmi.c b/drivers/usb/typec/tipd/spmi.c
> new file mode 100644
> index 0000000000000..15976d15d9bce
> --- /dev/null
> +++ b/drivers/usb/typec/tipd/spmi.c
[ ... ]
> +static const struct of_device_id sn201202x_of_match[] = {
> +	{ .compatible = "apple,sn201202x", (void *)TIPD_VARIANT_SN201202X},

[Severity: Low]
Is it intentional to use positional initialization for the data field here?

Mixing designated initializers (.compatible) with positional ones can
bypass designated initializer syntax and be error-prone if the structure
changes. Should this explicitly use .data = (void *)TIPD_VARIANT_SN201202X?

> +	{}
> +};
> +
> +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)match->data];

[Severity: Low]
Could this direct cast from a pointer (match->data) to an enum cause
compiler warnings?

On 64-bit architectures where pointers are 64-bit and enums are 32-bit
(like ARM64), this might trigger a -Wpointer-to-int-cast warning. Should
this cast through an unsigned long or uintptr_t first?

[ ... ]
> +	ret = spmi_command_wakeup(device);
> +	if (ret)
> +		return ret;
> +	if (!wait_for_completion_timeout(&sn->wake_completion, msecs_to_jiffies(100)))
> +		return -ETIMEDOUT;

[Severity: Medium]
If this wait_for_completion_timeout() times out, should the hardware be
put back to sleep?

By returning -ETIMEDOUT without calling spmi_command_sleep(), could the
device be left in an active state indefinitely and drain power?

[ ... ]
> +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;

[Severity: Medium]
Similarly here in the resume path, does this error return need to send a
sleep command to revert the hardware state if the wakeup times out?

> +	return tipd_resume(tps);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3