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

[Manivannan Sadhasivam]

On Sat, Jul 11, 2026 at 11:42:07PM +0300, Ivaylo Dimitrov wrote:
> 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. When disabled (default), the PHY remains in its USB/charger
> detection configuration while idle.
> 
> Also initialize the PHY into the baseline configuration required for
> reliable charger detection during probe.
> 
> Use the optional "safe" pinctrl state before switching between modes to
> avoid glitches on USB or UART lines.
> 

Looks like this change is doing multiple things at once. Please split the
changes logically to separate patches.

> Note: Enabling UART idle mode increases idle power consumption (by 25mW
> on droid4).
> 
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@xxxxxxxxx>
> 
> # Conflicts:
> #	drivers/phy/motorola/phy-cpcap-usb.c

What is this conflict?

> ---
>  drivers/phy/motorola/phy-cpcap-usb.c | 301 +++++++++++++++++++++------
>  1 file changed, 238 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> index 741145c89e5b..2d770ff19e93 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c
> @@ -110,6 +110,15 @@ enum cpcap_gpio_mode {
>  	CPCAP_OTG_DM_DP,
>  };
>  
> +enum cpcap_mode {
> +	CPCAP_UNKNOWN,
> +	CPCAP_IDLE,
> +	CPCAP_CHARGER,
> +	CPCAP_USB,
> +	CPCAP_USB_HOST,
> +	CPCAP_DOCK,
> +};
> +
>  struct cpcap_phy_ddata {
>  	struct regmap *reg;
>  	struct device *dev;
> @@ -119,15 +128,19 @@ struct cpcap_phy_ddata {
>  	struct pinctrl_state *pins_ulpi;
>  	struct pinctrl_state *pins_utmi;
>  	struct pinctrl_state *pins_uart;
> +	struct pinctrl_state *pins_safe;
>  	struct gpio_desc *gpio[2];
>  	struct iio_channel *vbus;
>  	struct iio_channel *id;
>  	struct regulator *vusb;
>  	atomic_t active;
> -	unsigned int vbus_provider:1;
> -	unsigned int docked:1;
> +	enum cpcap_mode mode;
>  };
>  
> +static bool cpcap_enable_uart;
> +module_param_named(enable_uart, cpcap_enable_uart, bool, 0644);
> +MODULE_PARM_DESC(enable_uart,
> +		 "Enable UART on the USB connector while idle (increases power consumption)");

Use of module params is discouraged these days. Also, you are disabling it by
default, which could cause surprises to users who have boards wired up for debug
console. But considering that it consumes a lot of power, I think it is OK to
disable it this way. I can't think of another way to add this knob.

>  static bool cpcap_usb_vbus_valid(struct cpcap_phy_ddata *ddata)
>  {
>  	int error, value = 0;
> @@ -196,8 +209,9 @@ static int cpcap_phy_get_ints_state(struct cpcap_phy_ddata *ddata,
>  	return 0;
>  }
>  

[...]

> @@ -473,21 +559,10 @@ static int cpcap_usb_set_usb_mode(struct cpcap_phy_ddata *ddata)
>  {
>  	int error;
>  
> -	/* Disable lines to prevent glitches from waking up mdm6600 */
> -	error = cpcap_usb_gpio_set_mode(ddata, CPCAP_UNKNOWN_DISABLED);
> +	error = cpcap_usb_set_safe_mode(ddata);
>  	if (error)
>  		return error;
>  
> -	if (ddata->pins_utmi) {
> -		error = pinctrl_select_state(ddata->pins, ddata->pins_utmi);
> -		if (error) {
> -			dev_err(ddata->dev, "could not set usb mode: %i\n",
> -				error);
> -
> -			return error;
> -		}
> -	}
> -
>  	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC1,
>  				   CPCAP_BIT_VBUSPD, 0);
>  	if (error)
> @@ -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,

As Sashiko noted, you are not clearing CPCAP_BIT_USBSUSPEND bit set in
cpcap_usb_set_idle_mode().

>  				   CPCAP_BIT_USBXCVREN);
>  	if (error)
>  		goto out_err;
>  
> +	if (ddata->pins_utmi) {
> +		error = pinctrl_select_state(ddata->pins, ddata->pins_utmi);
> +		if (error) {
> +			dev_err(ddata->dev, "could not set usb mode: %i\n",
> +				error);
> +
> +			return error;
> +		}
> +	}
> +
>  	/* Enable USB mode */
>  	error = cpcap_usb_gpio_set_mode(ddata, CPCAP_OTG_DM_DP);
>  	if (error)
> @@ -521,6 +608,38 @@ static int cpcap_usb_set_usb_mode(struct cpcap_phy_ddata *ddata)
>  	return error;
>  }
>  
> +static int cpcap_usb_set_dcp_mode(struct cpcap_phy_ddata *ddata)
> +{
> +	int error;
> +
> +	error = cpcap_usb_set_safe_mode(ddata);
> +	if (error)
> +		return error;
> +
> +	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC2,
> +				   CPCAP_BIT_USBXCVREN |
> +				   CPCAP_BIT_UARTMUX0 |
> +				   CPCAP_BIT_EMUMODE0, 0);
> +	if (error)
> +		goto out_err;
> +
> +	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3,
> +				   CPCAP_BIT_SUSPEND_SPI, 0);
> +	if (error)
> +		goto out_err;
> +
> +	error = cpcap_usb_gpio_set_mode(ddata, CPCAP_DM_DP);
> +	if (error)
> +		goto out_err;
> +
> +	return 0;
> +
> +out_err:
> +	dev_err(ddata->dev, "%s failed with %i\n", __func__, error);

Don't print function names in the error log.

- Mani

-- 
மணிவண்ணன் சதாசிவம்