Re: [PATCH 1/4] usb: phy: tegra: Add support for device tree-based vbus regulator control

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 06/26/2013 03:59 AM, Mikko Perttunen wrote:
> After this patch, usb vbus regulators for tegra usb phy devices can be specified
> with the device tree attribute vbus-supply = <&x> where x is a regulator defined
> in the device tree.

> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c

> @@ -250,12 +251,24 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
>  		return PTR_ERR(phy->pad_clk);
>  	}
>  
> +	phy->vbus = devm_regulator_get(phy->dev, "vbus");
> +	/* On some boards, the VBUS regulator doesn't need to be controlled */
> +	if (IS_ERR(phy->vbus)) {
> +		if (PTR_ERR(phy->vbus) == -ENODEV) {
> +			dev_notice(phy->dev, "no vbus regulator");
> +			phy->vbus = NULL;
> +		} else {
> +			return PTR_ERR(phy->vbus);
> +		}
> +	}

I think this code should be added to some more core initialization
function; IIRC, there are separate utmip_pad_open() and some other
function for ULPI mode, and in the future there may be more for HSIC, etc.

For the error-handling, I think it'd be better to do:

* If property doesn't exist, set phy->vbus to some error value, e.g.
ERR_PTR(-ENODEV).
* If property does exist, call devm_regulator_get().
** If devm_regulator_get() returned any error, return it.

Or, does devm_regulator_get() return -ENODEV if-and-only-if the
vbus-supply DT property does not exist?

and ...

> @@ -280,6 +293,14 @@ static void utmip_pad_power_on(struct tegra_usb_phy *phy)
>  	spin_unlock_irqrestore(&utmip_pad_lock, flags);
>  
>  	clk_disable_unprepare(phy->pad_clk);
> +
> +	if (phy->vbus) {

Here, check if (IS_ERR(phy->vbus) instead. The reason is if
devm_regulator_get() returns either a valid value or an error-pointer,
then NULL could in theory be a valid value (it's up the the regulator
API to determine that), and hence this code shouldn't assume that it can
use NULL to represent "no regulator".