Re: [PATCH 9/9] usb: phy: tegra: Use DT helpers for dr_mode

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

On 06/28/2013 03:37 PM, Tuomas Tynkkynen wrote:
> Use the new of_usb_get_dr_mode helper function for parsing dr_mode
> from the device tree. Also replace the usage of the custom
> tegra_usb_phy_mode enum with the standard enum.

> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
> +	tegra_phy->mode = of_usb_get_dr_mode(np);
> +	if (tegra_phy->mode == USB_DR_MODE_UNKNOWN) {
> +		dev_err(&pdev->dev, "dr_mode is invalid\n");
> +		return -EINVAL;
> +	}

Unfortunately, of_usb_get_dr_mode() returns USB_DR_MODE_UNKNOWN if the
property is missing, rather than defaulting to host mode as the original
code here did. I would suggest solving this by:

> diff --git a/drivers/usb/usb-common.c b/drivers/usb/usb-common.c
> index 675384d..6391de5 100644
> --- a/drivers/usb/usb-common.c
> +++ b/drivers/usb/usb-common.c
> @@ -103,7 +103,7 @@ enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
>  
>         err = of_property_read_string(np, "dr_mode", &dr_mode);
>         if (err < 0)
> -               return USB_DR_MODE_UNKNOWN;
> +               return USB_DR_MODE_HOST;
>  
>         for (i = 0; i < ARRAY_SIZE(usb_dr_modes); i++)
>                 if (!strcmp(dr_mode, usb_dr_modes[i]))

This can't be done by the caller, since of_usb_get_dr_mode() returns
UNKNOWN in two cases, which the caller can't distinguish without
manually checking whether the property exists first:

a) Property is not present (which should default to HOST mode at least
for the Tegra binding, as in the patch I show above).

b) Property is present, but contains an invalid value, which probably
should cause the driver to error-out.

This is only a problem for dr_mode in the Tegra binding: dr_mode is an
optional property, whereas the phy_type property as parsed by patch 8/9
is mandatory, so this issue doesn't come up.