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

[Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>]

On 9.09.26 г. 19:07 ч., Manivannan Sadhasivam wrote:
> 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.
> 
will spliting in two:

patch1: enable_uart + safe pinctrl
patch2: DCP detection + init on probe

be ok or you want me to split even more? To me it makes sense as 
enable_uart will be few lines only if sent as a separate patch and I 
don't think splitting DCP detection + init on probe makes sense.

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

an artefact from nth local rebase/merge before submission :) .

> 
>> ---
>>   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.
> 

Me neither, that's why I came up with a module parameter. Yes, I 
understand disabling it by default may cause regression for some 
(presumably knowledgeable) users, however, I think stripping ~25% from 
idle power usage for the others worths it.

>>   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().
> 

Vendor kernel does not do it and we are using the patch with 
CPCAP_BIT_USBSUSPEND not cleared for few months with no issues 
whatsoever, so I am not convinced this is needed. However, tests on the 
device didn't show any difference if I clear the bit so OK, will do.

>>   				   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.
> 

Ok.

Will send new series, just LMK if you want the patch split in 2 or more 
patches.

Thanks,
Ivo

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy