Re: [PATCH v5 4/7] phy: cpcap-usb: add DCP detection and make UART idle mode optional

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The new charger detection logic introduces an infinite connect/disconnect flapping loop for active USB peripheral connections and an infinite polling loop for proprietary chargers.
- [High] Transitioning from idle mode to USB mode leaves the PHY transceiver improperly suspended because CPCAP_BIT_USBSUSPEND is not cleared.
--

--- Patch [4]: [PATCH v5 4/7] phy: cpcap-usb: add DCP detection and make UART idle mode optional ---
commit a10ef479eac91f75ee1175abda606fb7eaa3fb70
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

phy: cpcap-usb: add DCP detection and make UART idle mode optional

Handle DCP separately from USB host connections using CPCAP charger
detection status. Make the existing idle UART mode optional via the
"enable_uart" module parameter. Initialize the PHY into the baseline
configuration required for reliable charger detection during probe.

> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> index 741145c89e5bb..2d770ff19e934 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c

[ ... ]

> @@ -292,38 +301,74 @@ static void cpcap_usb_detect(struct work_struct *work)

[ ... ]

> -	/* Otherwise assume we're connected to a USB host */
> -	if (vbus) {
> -		dev_dbg(ddata->dev, "connected to USB host\n");
> -		error = cpcap_usb_set_usb_mode(ddata);
> +	/* Check if we're connected to a USB host or charger */
> +	if (vbus && ddata->mode != CPCAP_USB_HOST) {
> +		enum cpcap_mode new_mode;
> +
> +		if (s.se1)
> +			new_mode = CPCAP_CHARGER;
> +		else if (!s.dm && !s.dp)
> +			new_mode = CPCAP_USB;
> +		else {
> +			error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC2,
> +						   CPCAP_BIT_USBXCVREN |
> +						   CPCAP_BIT_UARTMUX0 |
> +						   CPCAP_BIT_EMUMODE0, 0);
> +			if (error)
> +				goto out_err;
> +
> +			schedule_delayed_work(&ddata->detect_work,
> +					      msecs_to_jiffies(100));
> +			return;
> +		}

[Severity: High]
Does this else block create a regression with an infinite connect/disconnect
flapping loop for active USB peripheral connections?

When the device successfully enters CPCAP_USB mode, normal USB pull-ups or
traffic will assert D+ high (s.dp == 1). Any subsequent interrupt triggers
detect_work(), and if s.se1 is false, it falls into this else block. It then
disables the transceiver (USBXCVREN = 0) and reschedules itself in 100ms,
repeatedly dropping standard PC connections or proprietary chargers.

[ ... ]

> @@ -424,43 +469,84 @@ static int cpcap_usb_gpio_set_mode(struct cpcap_phy_ddata *ddata,

[ ... ]

> +static int cpcap_usb_set_idle_mode(struct cpcap_phy_ddata *ddata)
> +{

[ ... ]

> +	val = CPCAP_BIT_USBSUSPEND;
> +
> +	if (enable_uart)
> +		val |= (CPCAP_BIT_UARTMUX0 | CPCAP_BIT_EMUMODE0);
> +
> +	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC2, 0xffff, val);
> +	if (error)
> +		goto out_err;

[ ... ]

> @@ -503,11 +578,23 @@ static int cpcap_usb_set_usb_mode(struct cpcap_phy_ddata *ddata)
>  		goto out_err;
>  
>  	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC2,
> -				   CPCAP_BIT_USBXCVREN,
> +				   CPCAP_BIT_USBXCVREN |
> +				   CPCAP_BIT_UARTMUX0 |
> +				   CPCAP_BIT_EMUMODE0,
>  				   CPCAP_BIT_USBXCVREN);

[Severity: High]
When transitioning back to USB mode, does this cause a regression where the
PHY transceiver is left improperly suspended?

In cpcap_usb_set_idle_mode(), CPCAP_REG_USBC2 is updated with a 0xffff mask to
explicitly set CPCAP_BIT_USBSUSPEND. When returning to USB mode here, the mask
is limited to CPCAP_BIT_USBXCVREN | CPCAP_BIT_UARTMUX0 | CPCAP_BIT_EMUMODE0.
Since this mask does not include CPCAP_BIT_USBSUSPEND, it looks like it fails
to clear the suspension state, which could cause communication failures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711204210.197144-1-ivo.g.dimitrov.75@gmail.com?part=4