Re: [PATCH v2 2/8] usb: phy: tegra: Support waking up from a low power mode

[Dmitry Osipenko]

17.12.2020 16:33, Thierry Reding пишет:
>> +	/* PHY won't resume if reset is asserted */
>> +	if (phy->wakeup_enabled)
>> +		goto chrg_cfg0;
>>  
>>  	val = readl_relaxed(base + USB_SUSP_CTRL);
>>  	val |= UTMIP_RESET;
>>  	writel_relaxed(val, base + USB_SUSP_CTRL);
>>  
>> +chrg_cfg0:
> I found this diffcult to read until I realized that it was basically
> just the equivalent of this:
> 
> 	if (!phy->wakeup_enabled) {
> 		val = readl_relaxed(base + USB_SUSP_CTRL);
> 		val |= UTMIP_RESET;
> 		writel_relaxed(val, base + USB_SUSP_CTRL);
> 	}
> 
>>  	val = readl_relaxed(base + UTMIP_BAT_CHRG_CFG0);
>>  	val |= UTMIP_PD_CHRG;
>>  	writel_relaxed(val, base + UTMIP_BAT_CHRG_CFG0);
>>  
>> +	if (phy->wakeup_enabled)
>> +		goto xcvr_cfg1;
>> +
>>  	val = readl_relaxed(base + UTMIP_XCVR_CFG0);
>>  	val |= UTMIP_FORCE_PD_POWERDOWN | UTMIP_FORCE_PD2_POWERDOWN |
>>  	       UTMIP_FORCE_PDZI_POWERDOWN;
>>  	writel_relaxed(val, base + UTMIP_XCVR_CFG0);
>>  
>> +xcvr_cfg1:
> Similarly, I think this is more readable as:
> 
> 	if (!phy->wakeup_enabled) {
> 		val = readl_relaxed(base + UTMIP_XCVR_CFG0);
> 		val |= UTMIP_FORCE_PD_POWERDOWN | UTMIP_FORCE_PD2_POWERDOWN |
> 		       UTMIP_FORCE_PDZI_POWERDOWN;
> 		writel_relaxed(val, base + UTMIP_XCVR_CFG0);
> 	}
> 
>>  	val = readl_relaxed(base + UTMIP_XCVR_CFG1);
>>  	val |= UTMIP_FORCE_PDDISC_POWERDOWN | UTMIP_FORCE_PDCHRP_POWERDOWN |
>>  	       UTMIP_FORCE_PDDR_POWERDOWN;
>>  	writel_relaxed(val, base + UTMIP_XCVR_CFG1);
>>  
>> +	if (phy->wakeup_enabled) {
> Which then also matches the style of this conditional here.

I'll change it in v3, thanks.