[PATCHv2 0/3] Add devm_of_phy_get_by_index and update platform drivers

[PATCHv2 0/3] Add devm_of_phy_get_by_index and update platform drivers

[Arun Ramamurthy]

This patch set adds a new API to get phy by index when multiple 
phys are present. This patch is based on discussion with Arnd Bergmann
about dt bindings for multiple phys.

History:
v1:
    - Removed null pointers on Dmitry's suggestion
    - Improved documentation in commit messages
    - Exported new phy api
v2:
    - EHCI and OHCI platform Kconfigs select Generic Phy
      to fix build errors in certain configs. 

Arun Ramamurthy (3):
  phy: core: Add devm_of_phy_get_by_index to phy-core
  usb: ehci-platform: Use devm_of_phy_get_by_index
  usb: ohci-platform: Use devm_of_phy_get_by_index

 drivers/phy/phy-core.c           | 32 ++++++++++++++++++
 drivers/usb/host/Kconfig         |  2 ++
 drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
 drivers/usb/host/ohci-platform.c | 70 ++++++++++++++--------------------------
 include/linux/phy/phy.h          |  2 ++
 5 files changed, 86 insertions(+), 90 deletions(-)

-- 
2.3.4

[PATCHv2 1/3] phy: core: Add devm_of_phy_get_by_index to phy-core

[Arun Ramamurthy]

Some generic drivers, such as ehci, may use multiple phys and for such
drivers referencing phy(s) by name(s) does not make sense. Instead of
inventing new naming schemes and using custom code to iterate through them,
such drivers are better of using nameless phy bindings and using this newly
introduced API to iterate through them.

Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/phy/phy-core.c  | 32 ++++++++++++++++++++++++++++++++
 include/linux/phy/phy.h |  2 ++
 2 files changed, 34 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 3791838..964a84d 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -623,6 +623,38 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
 EXPORT_SYMBOL_GPL(devm_of_phy_get);
 
 /**
+ * devm_of_phy_get_by_index() - lookup and obtain a reference to a phy by index.
+ * @dev: device that requests this phy
+ * @np: node containing the phy
+ * @index: index of the phy
+ *
+ * Gets the phy using _of_phy_get(), and associates a device with it using
+ * devres. On driver detach, release function is invoked on the devres data,
+ * then, devres data is freed.
+ *
+ */
+struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
+				     int index)
+{
+	struct phy **ptr, *phy;
+
+	ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	phy = _of_phy_get(np, index);
+	if (!IS_ERR(phy)) {
+		*ptr = phy;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return phy;
+}
+EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
+
+/**
  * phy_create() - create a new phy
  * @dev: device that is creating the new phy
  * @node: device node of the phy
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index a0197fa..ae2ffaf 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const char *string);
 struct phy *devm_phy_optional_get(struct device *dev, const char *string);
 struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
 			    const char *con_id);
+struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
+				     int index);
 void phy_put(struct phy *phy);
 void devm_phy_put(struct device *dev, struct phy *phy);
 struct phy *of_phy_get(struct device_node *np, const char *con_id);
-- 
2.3.4

[PATCHv2 1/3] phy: core: Add devm_of_phy_get_by_index to phy-core

[Kishon Vijay Abraham I]

Hi,

On Tuesday 14 April 2015 03:40 AM, Arun Ramamurthy wrote:
> Some generic drivers, such as ehci, may use multiple phys and for such
> drivers referencing phy(s) by name(s) does not make sense. Instead of
> inventing new naming schemes and using custom code to iterate through them,
> such drivers are better of using nameless phy bindings and using this newly
> introduced API to iterate through them.
>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>   drivers/phy/phy-core.c  | 32 ++++++++++++++++++++++++++++++++
>   include/linux/phy/phy.h |  2 ++
>   2 files changed, 34 insertions(+)
>
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 3791838..964a84d 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -623,6 +623,38 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>   EXPORT_SYMBOL_GPL(devm_of_phy_get);
>
>   /**
> + * devm_of_phy_get_by_index() - lookup and obtain a reference to a phy by index.
> + * @dev: device that requests this phy
> + * @np: node containing the phy
> + * @index: index of the phy
> + *
> + * Gets the phy using _of_phy_get(), and associates a device with it using
> + * devres. On driver detach, release function is invoked on the devres data,
> + * then, devres data is freed.
> + *
> + */
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> +				     int index)
> +{
> +	struct phy **ptr, *phy;
> +
> +	ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	phy = _of_phy_get(np, index);
> +	if (!IS_ERR(phy)) {
> +		*ptr = phy;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return phy;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
> +
> +/**
>    * phy_create() - create a new phy
>    * @dev: device that is creating the new phy
>    * @node: device node of the phy
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index a0197fa..ae2ffaf 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const char *string);
>   struct phy *devm_phy_optional_get(struct device *dev, const char *string);
>   struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>   			    const char *con_id);
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> +				     int index);

Add stubs for this function too. Also update the Documentation/phy.txt.

Thanks
Kishon

[PATCHv2 1/3] phy: core: Add devm_of_phy_get_by_index to phy-core

[Arun Ramamurthy]

On 15-04-15 02:59 AM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 14 April 2015 03:40 AM, Arun Ramamurthy wrote:
>> Some generic drivers, such as ehci, may use multiple phys and for such
>> drivers referencing phy(s) by name(s) does not make sense. Instead of
>> inventing new naming schemes and using custom code to iterate through
>> them,
>> such drivers are better of using nameless phy bindings and using this
>> newly
>> introduced API to iterate through them.
>>
>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> ---
>>   drivers/phy/phy-core.c  | 32 ++++++++++++++++++++++++++++++++
>>   include/linux/phy/phy.h |  2 ++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>> index 3791838..964a84d 100644
>> --- a/drivers/phy/phy-core.c
>> +++ b/drivers/phy/phy-core.c
>> @@ -623,6 +623,38 @@ struct phy *devm_of_phy_get(struct device *dev,
>> struct device_node *np,
>>   EXPORT_SYMBOL_GPL(devm_of_phy_get);
>>
>>   /**
>> + * devm_of_phy_get_by_index() - lookup and obtain a reference to a
>> phy by index.
>> + * @dev: device that requests this phy
>> + * @np: node containing the phy
>> + * @index: index of the phy
>> + *
>> + * Gets the phy using _of_phy_get(), and associates a device with it
>> using
>> + * devres. On driver detach, release function is invoked on the
>> devres data,
>> + * then, devres data is freed.
>> + *
>> + */
>> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct
>> device_node *np,
>> +                     int index)
>> +{
>> +    struct phy **ptr, *phy;
>> +
>> +    ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
>> +    if (!ptr)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    phy = _of_phy_get(np, index);
>> +    if (!IS_ERR(phy)) {
>> +        *ptr = phy;
>> +        devres_add(dev, ptr);
>> +    } else {
>> +        devres_free(ptr);
>> +    }
>> +
>> +    return phy;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
>> +
>> +/**
>>    * phy_create() - create a new phy
>>    * @dev: device that is creating the new phy
>>    * @node: device node of the phy
>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>> index a0197fa..ae2ffaf 100644
>> --- a/include/linux/phy/phy.h
>> +++ b/include/linux/phy/phy.h
>> @@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const
>> char *string);
>>   struct phy *devm_phy_optional_get(struct device *dev, const char
>> *string);
>>   struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>>                   const char *con_id);
>> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct
>> device_node *np,
>> +                     int index);
>
> Add stubs for this function too. Also update the Documentation/phy.txt.
>
Kishon, I have added stubs for this function in my next patch set. 
However I am still unclear on whether I need to make GENERIC_PHY an 
invisible option or change my "select" to "depend" ? It seems like there 
was no consensus on this? Do you have any final thoughts before i send 
out the next patch set? Thanks

> Thanks
> Kishon

[PATCHv2 1/3] phy: core: Add devm_of_phy_get_by_index to phy-core

[Kishon Vijay Abraham I]

Hi,

On Tuesday 21 April 2015 01:49 AM, Arun Ramamurthy wrote:
>
>
> On 15-04-15 02:59 AM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Tuesday 14 April 2015 03:40 AM, Arun Ramamurthy wrote:
>>> Some generic drivers, such as ehci, may use multiple phys and for such
>>> drivers referencing phy(s) by name(s) does not make sense. Instead of
>>> inventing new naming schemes and using custom code to iterate through
>>> them,
>>> such drivers are better of using nameless phy bindings and using this
>>> newly
>>> introduced API to iterate through them.
>>>
>>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
>>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>>> ---
>>>    drivers/phy/phy-core.c  | 32 ++++++++++++++++++++++++++++++++
>>>    include/linux/phy/phy.h |  2 ++
>>>    2 files changed, 34 insertions(+)
>>>
>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>> index 3791838..964a84d 100644
>>> --- a/drivers/phy/phy-core.c
>>> +++ b/drivers/phy/phy-core.c
>>> @@ -623,6 +623,38 @@ struct phy *devm_of_phy_get(struct device *dev,
>>> struct device_node *np,
>>>    EXPORT_SYMBOL_GPL(devm_of_phy_get);
>>>
>>>    /**
>>> + * devm_of_phy_get_by_index() - lookup and obtain a reference to a
>>> phy by index.
>>> + * @dev: device that requests this phy
>>> + * @np: node containing the phy
>>> + * @index: index of the phy
>>> + *
>>> + * Gets the phy using _of_phy_get(), and associates a device with it
>>> using
>>> + * devres. On driver detach, release function is invoked on the
>>> devres data,
>>> + * then, devres data is freed.
>>> + *
>>> + */
>>> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct
>>> device_node *np,
>>> +                     int index)
>>> +{
>>> +    struct phy **ptr, *phy;
>>> +
>>> +    ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
>>> +    if (!ptr)
>>> +        return ERR_PTR(-ENOMEM);
>>> +
>>> +    phy = _of_phy_get(np, index);
>>> +    if (!IS_ERR(phy)) {
>>> +        *ptr = phy;
>>> +        devres_add(dev, ptr);
>>> +    } else {
>>> +        devres_free(ptr);
>>> +    }
>>> +
>>> +    return phy;
>>> +}
>>> +EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
>>> +
>>> +/**
>>>     * phy_create() - create a new phy
>>>     * @dev: device that is creating the new phy
>>>     * @node: device node of the phy
>>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>>> index a0197fa..ae2ffaf 100644
>>> --- a/include/linux/phy/phy.h
>>> +++ b/include/linux/phy/phy.h
>>> @@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const
>>> char *string);
>>>    struct phy *devm_phy_optional_get(struct device *dev, const char
>>> *string);
>>>    struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>>>                    const char *con_id);
>>> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct
>>> device_node *np,
>>> +                     int index);
>>
>> Add stubs for this function too. Also update the Documentation/phy.txt.
>>
> Kishon, I have added stubs for this function in my next patch set.
> However I am still unclear on whether I need to make GENERIC_PHY an
> invisible option or change my "select" to "depend" ? It seems like there
> was no consensus on this? Do you have any final thoughts before i send
> out the next patch set? Thanks

You can follow Arnd's suggestion. You can have a separate patch to change the 
GENERIC_PHY to invisible option and change existing PHY drivers to select 
GENERIC_PHY.

Non-PHY drivers can either use depends on or have no explicit dependency if the 
PHY is optional for that controller.

Thanks
Kishon

[PATCHv2 1/3] phy: core: Add devm_of_phy_get_by_index to phy-core

[Peter Chen]

On Mon, Apr 13, 2015 at 03:10:45PM -0700, Arun Ramamurthy wrote:
> Some generic drivers, such as ehci, may use multiple phys and for such
> drivers referencing phy(s) by name(s) does not make sense. Instead of
> inventing new naming schemes and using custom code to iterate through them,
> such drivers are better of using nameless phy bindings and using this newly
> introduced API to iterate through them.
> 
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>  drivers/phy/phy-core.c  | 32 ++++++++++++++++++++++++++++++++
>  include/linux/phy/phy.h |  2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 3791838..964a84d 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -623,6 +623,38 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>  EXPORT_SYMBOL_GPL(devm_of_phy_get);
>  
>  /**
> + * devm_of_phy_get_by_index() - lookup and obtain a reference to a phy by index.
> + * @dev: device that requests this phy
> + * @np: node containing the phy
> + * @index: index of the phy
> + *
> + * Gets the phy using _of_phy_get(), and associates a device with it using
> + * devres. On driver detach, release function is invoked on the devres data,
> + * then, devres data is freed.
> + *
> + */
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> +				     int index)
> +{
> +	struct phy **ptr, *phy;
> +
> +	ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	phy = _of_phy_get(np, index);
> +	if (!IS_ERR(phy)) {
> +		*ptr = phy;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return phy;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
> +
> +/**
>   * phy_create() - create a new phy
>   * @dev: device that is creating the new phy
>   * @node: device node of the phy
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index a0197fa..ae2ffaf 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const char *string);
>  struct phy *devm_phy_optional_get(struct device *dev, const char *string);
>  struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>  			    const char *con_id);
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> +				     int index);

You may need to add an implementation if CONFIG_GENERIC_PHY is not enabled.

>  void phy_put(struct phy *phy);
>  void devm_phy_put(struct device *dev, struct phy *phy);
>  struct phy *of_phy_get(struct device_node *np, const char *con_id);


> -- 
> 2.3.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter Chen

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Arun Ramamurthy]

Getting phys by index instead of phy names so that we do
not have to create a naming scheme when multiple phys
are present

Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/usb/host/Kconfig         |  1 +
 drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
 2 files changed, 26 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 5ad60e4..563f22d 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -284,6 +284,7 @@ config USB_EHCI_ATH79
 
 config USB_EHCI_HCD_PLATFORM
 	tristate "Generic EHCI driver for a platform device"
+	select GENERIC_PHY
 	default n
 	---help---
 	  Adds an EHCI host driver for a generic platform device, which
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index d8a75a5..a7563b9 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -88,15 +88,13 @@ static int ehci_platform_power_on(struct platform_device *dev)
 	}
 
 	for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-		if (priv->phys[phy_num]) {
-			ret = phy_init(priv->phys[phy_num]);
-			if (ret)
-				goto err_exit_phy;
-			ret = phy_power_on(priv->phys[phy_num]);
-			if (ret) {
-				phy_exit(priv->phys[phy_num]);
-				goto err_exit_phy;
-			}
+		ret = phy_init(priv->phys[phy_num]);
+		if (ret)
+			goto err_exit_phy;
+		ret = phy_power_on(priv->phys[phy_num]);
+		if (ret) {
+			phy_exit(priv->phys[phy_num]);
+			goto err_exit_phy;
 		}
 	}
 
@@ -104,10 +102,8 @@ static int ehci_platform_power_on(struct platform_device *dev)
 
 err_exit_phy:
 	while (--phy_num >= 0) {
-		if (priv->phys[phy_num]) {
-			phy_power_off(priv->phys[phy_num]);
-			phy_exit(priv->phys[phy_num]);
-		}
+		phy_power_off(priv->phys[phy_num]);
+		phy_exit(priv->phys[phy_num]);
 	}
 err_disable_clks:
 	while (--clk >= 0)
@@ -123,10 +119,8 @@ static void ehci_platform_power_off(struct platform_device *dev)
 	int clk, phy_num;
 
 	for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-		if (priv->phys[phy_num]) {
-			phy_power_off(priv->phys[phy_num]);
-			phy_exit(priv->phys[phy_num]);
-		}
+		phy_power_off(priv->phys[phy_num]);
+		phy_exit(priv->phys[phy_num]);
 	}
 
 	for (clk = EHCI_MAX_CLKS - 1; clk >= 0; clk--)
@@ -154,7 +148,6 @@ static int ehci_platform_probe(struct platform_device *dev)
 	struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
 	struct ehci_platform_priv *priv;
 	struct ehci_hcd *ehci;
-	const char *phy_name;
 	int err, irq, phy_num, clk = 0;
 
 	if (usb_disabled())
@@ -204,36 +197,23 @@ static int ehci_platform_probe(struct platform_device *dev)
 
 		priv->num_phys = of_count_phandle_with_args(dev->dev.of_node,
 				"phys", "#phy-cells");
-		priv->num_phys = priv->num_phys > 0 ? priv->num_phys : 1;
 
-		priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
-				sizeof(struct phy *), GFP_KERNEL);
-		if (!priv->phys)
-			return -ENOMEM;
+		if (priv->num_phys > 0) {
+			priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
+					    sizeof(struct phy *), GFP_KERNEL);
+			if (!priv->phys)
+				return -ENOMEM;
+		} else
+			priv->num_phys = 0;
 
 		for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-				err = of_property_read_string_index(
-						dev->dev.of_node,
-						"phy-names", phy_num,
-						&phy_name);
-
-				if (err < 0) {
-					if (priv->num_phys > 1) {
-						dev_err(&dev->dev, "phy-names not provided");
-						goto err_put_hcd;
-					} else
-						phy_name = "usb";
-				}
-
-				priv->phys[phy_num] = devm_phy_get(&dev->dev,
-						phy_name);
-				if (IS_ERR(priv->phys[phy_num])) {
-					err = PTR_ERR(priv->phys[phy_num]);
-					if ((priv->num_phys > 1) ||
-					    (err == -EPROBE_DEFER))
-						goto err_put_hcd;
-					priv->phys[phy_num] = NULL;
-				}
+			priv->phys[phy_num] = devm_of_phy_get_by_index(&dev->dev
+						, dev->dev.of_node
+						, phy_num);
+			if (IS_ERR(priv->phys[phy_num])) {
+				err = PTR_ERR(priv->phys[phy_num]);
+					goto err_put_hcd;
+			}
 		}
 
 		for (clk = 0; clk < EHCI_MAX_CLKS; clk++) {
-- 
2.3.4

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Greg Kroah-Hartman]

On Mon, Apr 13, 2015 at 03:10:46PM -0700, Arun Ramamurthy wrote:
> Getting phys by index instead of phy names so that we do
> not have to create a naming scheme when multiple phys
> are present
> 
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>  drivers/usb/host/Kconfig         |  1 +
>  drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
>  2 files changed, 26 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 5ad60e4..563f22d 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -284,6 +284,7 @@ config USB_EHCI_ATH79
>  
>  config USB_EHCI_HCD_PLATFORM
>  	tristate "Generic EHCI driver for a platform device"
> +	select GENERIC_PHY

Configs should never select if at all possible.

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Arnd Bergmann]

On Tuesday 14 April 2015 13:19:34 Greg Kroah-Hartman wrote:
> On Mon, Apr 13, 2015 at 03:10:46PM -0700, Arun Ramamurthy wrote:
> > Getting phys by index instead of phy names so that we do
> > not have to create a naming scheme when multiple phys
> > are present
> > 
> > Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> > Reviewed-by: Ray Jui <rjui@broadcom.com>
> > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> > ---
> >  drivers/usb/host/Kconfig         |  1 +
> >  drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
> >  2 files changed, 26 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> > index 5ad60e4..563f22d 100644
> > --- a/drivers/usb/host/Kconfig
> > +++ b/drivers/usb/host/Kconfig
> > @@ -284,6 +284,7 @@ config USB_EHCI_ATH79
> >  
> >  config USB_EHCI_HCD_PLATFORM
> >       tristate "Generic EHCI driver for a platform device"
> > +     select GENERIC_PHY
> 
> Configs should never select if at all possible.
> 

This is true, but all other drivers do the same for GENERIC_PHY at the
moment. If this one gets changed, we should probably apply the same
solution to all current users and fix them consistently.

We can do one of these two:

a) make sure that the framework has 'static inline' stubs that let you
   build all drivers using it when the framework itself is disabled.
b) change the drivers using it to 'depends on', and make GENERIC_PHY
   itself a hidden option without a Kconfig prompt.

Either solution is probably simple enough that it can be done as
part of this series.

	Arnd

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Greg Kroah-Hartman]

On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
> On Tuesday 14 April 2015 13:19:34 Greg Kroah-Hartman wrote:
> > On Mon, Apr 13, 2015 at 03:10:46PM -0700, Arun Ramamurthy wrote:
> > > Getting phys by index instead of phy names so that we do
> > > not have to create a naming scheme when multiple phys
> > > are present
> > > 
> > > Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> > > Reviewed-by: Ray Jui <rjui@broadcom.com>
> > > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> > > ---
> > >  drivers/usb/host/Kconfig         |  1 +
> > >  drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
> > >  2 files changed, 26 insertions(+), 45 deletions(-)
> > > 
> > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> > > index 5ad60e4..563f22d 100644
> > > --- a/drivers/usb/host/Kconfig
> > > +++ b/drivers/usb/host/Kconfig
> > > @@ -284,6 +284,7 @@ config USB_EHCI_ATH79
> > >  
> > >  config USB_EHCI_HCD_PLATFORM
> > >       tristate "Generic EHCI driver for a platform device"
> > > +     select GENERIC_PHY
> > 
> > Configs should never select if at all possible.
> > 
> 
> This is true, but all other drivers do the same for GENERIC_PHY at the
> moment. If this one gets changed, we should probably apply the same
> solution to all current users and fix them consistently.
> 
> We can do one of these two:
> 
> a) make sure that the framework has 'static inline' stubs that let you
>    build all drivers using it when the framework itself is disabled.

Yes, please do that.

> b) change the drivers using it to 'depends on', and make GENERIC_PHY
>    itself a hidden option without a Kconfig prompt.

Then how could GENERIC_PHY ever get set?

> Either solution is probably simple enough that it can be done as
> part of this series.

That's fine with me, but please no more selects.

thanks,

greg k-h

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Arnd Bergmann]

On Tuesday 14 April 2015 14:37:37 Greg Kroah-Hartman wrote:
> On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
> > This is true, but all other drivers do the same for GENERIC_PHY at the
> > moment. If this one gets changed, we should probably apply the same
> > solution to all current users and fix them consistently.
> > 
> > We can do one of these two:
> > 
> > a) make sure that the framework has 'static inline' stubs that let you
> >    build all drivers using it when the framework itself is disabled.
> 
> Yes, please do that.
> 
> > b) change the drivers using it to 'depends on', and make GENERIC_PHY
> >    itself a hidden option without a Kconfig prompt.
> 
> Then how could GENERIC_PHY ever get set?

Right now, every driver that provides a phy uses 'select GENERIC_PHY',
and they would have to keep doing that. This is not unlike what we
do for other silent symbols like MFD_CORE, REGMAP_I2C, or PINCTRL,
and it's not as problematic as 'select' on a user-visible option,
or (worst) mixing 'select' and 'depends on'.

	Arnd

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Greg Kroah-Hartman]

On Tue, Apr 14, 2015 at 03:17:30PM +0200, Arnd Bergmann wrote:
> On Tuesday 14 April 2015 14:37:37 Greg Kroah-Hartman wrote:
> > On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
> > > This is true, but all other drivers do the same for GENERIC_PHY at the
> > > moment. If this one gets changed, we should probably apply the same
> > > solution to all current users and fix them consistently.
> > > 
> > > We can do one of these two:
> > > 
> > > a) make sure that the framework has 'static inline' stubs that let you
> > >    build all drivers using it when the framework itself is disabled.
> > 
> > Yes, please do that.
> > 
> > > b) change the drivers using it to 'depends on', and make GENERIC_PHY
> > >    itself a hidden option without a Kconfig prompt.
> > 
> > Then how could GENERIC_PHY ever get set?
> 
> Right now, every driver that provides a phy uses 'select GENERIC_PHY',
> and they would have to keep doing that. This is not unlike what we
> do for other silent symbols like MFD_CORE, REGMAP_I2C, or PINCTRL,
> and it's not as problematic as 'select' on a user-visible option,
> or (worst) mixing 'select' and 'depends on'.

Ok, that would make more sense, but it would be good for the PHY
maintainer to agree to it as well :)

thanks,

greg k-h

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Kishon Vijay Abraham I]

Hi,

On Tuesday 14 April 2015 06:57 PM, Greg Kroah-Hartman wrote:
> On Tue, Apr 14, 2015 at 03:17:30PM +0200, Arnd Bergmann wrote:
>> On Tuesday 14 April 2015 14:37:37 Greg Kroah-Hartman wrote:
>>> On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
>>>> This is true, but all other drivers do the same for GENERIC_PHY at the
>>>> moment. If this one gets changed, we should probably apply the same
>>>> solution to all current users and fix them consistently.
>>>>
>>>> We can do one of these two:
>>>>
>>>> a) make sure that the framework has 'static inline' stubs that let you
>>>>     build all drivers using it when the framework itself is disabled.
>>>
>>> Yes, please do that.
>>>
>>>> b) change the drivers using it to 'depends on', and make GENERIC_PHY
>>>>     itself a hidden option without a Kconfig prompt.
>>>
>>> Then how could GENERIC_PHY ever get set?
>>
>> Right now, every driver that provides a phy uses 'select GENERIC_PHY',
>> and they would have to keep doing that. This is not unlike what we
>> do for other silent symbols like MFD_CORE, REGMAP_I2C, or PINCTRL,
>> and it's not as problematic as 'select' on a user-visible option,
>> or (worst) mixing 'select' and 'depends on'.
>
> Ok, that would make more sense, but it would be good for the PHY
> maintainer to agree to it as well :)

looking at [1], we should use select only for non-visible symbols and for 
symbols with no dependencies. As such GENERIC_PHY is not dependent on other 
symbols but for now it is "visible".

phy-core has all the stubs already implemented in include/linux/phy/phy.h. So 
removing select GENERIC_PHY shouldn't be a problem. But then it might break a 
few platforms where GENERIC_PHY is indirectly enabled by selecting the config 
of the driver (using default defconfigs in arch/arm/configs).

The simplest thing would be to make GENERIC_PHY an invisible option?

[1] -> 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kbuild/kconfig-language.txt#n111

Thanks
Kishon

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Arun Ramamurthy]

On 15-04-14 07:21 AM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 14 April 2015 06:57 PM, Greg Kroah-Hartman wrote:
>> On Tue, Apr 14, 2015 at 03:17:30PM +0200, Arnd Bergmann wrote:
>>> On Tuesday 14 April 2015 14:37:37 Greg Kroah-Hartman wrote:
>>>> On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
>>>>> This is true, but all other drivers do the same for GENERIC_PHY at the
>>>>> moment. If this one gets changed, we should probably apply the same
>>>>> solution to all current users and fix them consistently.
>>>>>
>>>>> We can do one of these two:
>>>>>
>>>>> a) make sure that the framework has 'static inline' stubs that let you
>>>>>     build all drivers using it when the framework itself is disabled.
>>>>
>>>> Yes, please do that.
>>>>
>>>>> b) change the drivers using it to 'depends on', and make GENERIC_PHY
>>>>>     itself a hidden option without a Kconfig prompt.
>>>>
>>>> Then how could GENERIC_PHY ever get set?
>>>
>>> Right now, every driver that provides a phy uses 'select GENERIC_PHY',
>>> and they would have to keep doing that. This is not unlike what we
>>> do for other silent symbols like MFD_CORE, REGMAP_I2C, or PINCTRL,
>>> and it's not as problematic as 'select' on a user-visible option,
>>> or (worst) mixing 'select' and 'depends on'.
>>
>> Ok, that would make more sense, but it would be good for the PHY
>> maintainer to agree to it as well :)
>
> looking at [1], we should use select only for non-visible symbols and
> for symbols with no dependencies. As such GENERIC_PHY is not dependent
> on other symbols but for now it is "visible".
>
> phy-core has all the stubs already implemented in
> include/linux/phy/phy.h. So removing select GENERIC_PHY shouldn't be a
> problem. But then it might break a few platforms where GENERIC_PHY is
> indirectly enabled by selecting the config of the driver (using default
> defconfigs in arch/arm/configs).
>
> The simplest thing would be to make GENERIC_PHY an invisible option?
>
> [1] ->
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kbuild/kconfig-language.txt#n111
>
Kishon,removing select GENERIC_PHY also breaks the builds for certain 
architectures (i386 and x84_64). Is the consensus to leave the select 
but make GENERIC_PHY a invisible option? Thanks
>
> Thanks
> Kishon

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Arnd Bergmann]

On Tuesday 14 April 2015 11:05:35 Arun Ramamurthy wrote:
> >
> > [1] ->
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kbuild/kconfig-language.txt#n111
> >
> Kishon,removing select GENERIC_PHY also breaks the builds for certain 
> architectures (i386 and x84_64). Is the consensus to leave the select 
> but make GENERIC_PHY a invisible option? Thanks

I think the best solution is

- make GENERIC_PHY a silent option
- change PHY_RCAR_GEN2 to use 'select' instead of 'depends on', so
  it will still work when all other phy drivers are disabled
- change the non-phy drivers that select GENERIC_PHY to either
  use 'depends on' or no explicit dependency in case they are
  still functional without the API. Note that
  drivers/pinctrl/pinctrl-tegra-xusb.c is a phy provider as well,
  not a consumer, despite being outside of drivers/phy.

	Arnd

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Kishon Vijay Abraham I]

Arnd,

On Wednesday 15 April 2015 03:17 AM, Arnd Bergmann wrote:
> On Tuesday 14 April 2015 11:05:35 Arun Ramamurthy wrote:
>>>
>>> [1] ->
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kbuild/kconfig-language.txt#n111
>>>
>> Kishon,removing select GENERIC_PHY also breaks the builds for certain
>> architectures (i386 and x84_64). Is the consensus to leave the select
>> but make GENERIC_PHY a invisible option? Thanks
>
> I think the best solution is
>
> - make GENERIC_PHY a silent option
> - change PHY_RCAR_GEN2 to use 'select' instead of 'depends on', so
>    it will still work when all other phy drivers are disabled
> - change the non-phy drivers that select GENERIC_PHY to either
>    use 'depends on' or no explicit dependency in case they are
>    still functional without the API. Note that
>    drivers/pinctrl/pinctrl-tegra-xusb.c is a phy provider as well,
>    not a consumer, despite being outside of drivers/phy.

makes sense to me.

Thanks
Kishon

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Kishon Vijay Abraham I]

Hi Arnd,

On Tuesday 14 April 2015 06:47 PM, Arnd Bergmann wrote:
> On Tuesday 14 April 2015 14:37:37 Greg Kroah-Hartman wrote:
>> On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
>>> This is true, but all other drivers do the same for GENERIC_PHY at the
>>> moment. If this one gets changed, we should probably apply the same
>>> solution to all current users and fix them consistently.
>>>
>>> We can do one of these two:
>>>
>>> a) make sure that the framework has 'static inline' stubs that let you
>>>     build all drivers using it when the framework itself is disabled.
>>
>> Yes, please do that.
>>
>>> b) change the drivers using it to 'depends on', and make GENERIC_PHY
>>>     itself a hidden option without a Kconfig prompt.
>>
>> Then how could GENERIC_PHY ever get set?
>
> Right now, every driver that provides a phy uses 'select GENERIC_PHY',
> and they would have to keep doing that. This is not unlike what we
> do for other silent symbols like MFD_CORE, REGMAP_I2C, or PINCTRL,
> and it's not as problematic as 'select' on a user-visible option,
> or (worst) mixing 'select' and 'depends on'.

Sorry, didn't get how GENERIC_PHY could be set with option 'b'.

Thanks
Kishon

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Alan Stern]

On Mon, 13 Apr 2015, Arun Ramamurthy wrote:

> Getting phys by index instead of phy names so that we do
> not have to create a naming scheme when multiple phys
> are present
> 
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>

You have not responded to the comments I posted earlier:

	http://marc.info/?l=linux-usb&m=142798455925594&w=2

Alan Stern

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Arun Ramamurthy]

My apologies Alan, I missed that comment I was indeed trying to avoid 
the 80 column rule. It looks like i will have to resend this patch, so i 
will reformat the code then. Thanks

On 15-04-14 07:41 AM, Alan Stern wrote:
> On Mon, 13 Apr 2015, Arun Ramamurthy wrote:
>
>> Getting phys by index instead of phy names so that we do
>> not have to create a naming scheme when multiple phys
>> are present
>>
>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>
> You have not responded to the comments I posted earlier:
>
> 	http://marc.info/?l=linux-usb&m=142798455925594&w=2
>
> Alan Stern
>

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[rajeev kumar]

On Tue, Apr 14, 2015 at 3:40 AM, Arun Ramamurthy
<arun.ramamurthy@broadcom.com> wrote:
> Getting phys by index instead of phy names so that we do
> not have to create a naming scheme when multiple phys
> are present
>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>  drivers/usb/host/Kconfig         |  1 +
>  drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
>  2 files changed, 26 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 5ad60e4..563f22d 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -284,6 +284,7 @@ config USB_EHCI_ATH79
>
>  config USB_EHCI_HCD_PLATFORM
>         tristate "Generic EHCI driver for a platform device"
> +       select GENERIC_PHY
>         default n
>         ---help---
>           Adds an EHCI host driver for a generic platform device, which
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index d8a75a5..a7563b9 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -88,15 +88,13 @@ static int ehci_platform_power_on(struct platform_device *dev)
>         }
>
>         for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
> -               if (priv->phys[phy_num]) {
> -                       ret = phy_init(priv->phys[phy_num]);
> -                       if (ret)
> -                               goto err_exit_phy;
> -                       ret = phy_power_on(priv->phys[phy_num]);
> -                       if (ret) {
> -                               phy_exit(priv->phys[phy_num]);
> -                               goto err_exit_phy;
> -                       }
> +               ret = phy_init(priv->phys[phy_num]);
> +               if (ret)
> +                       goto err_exit_phy;

Jumping to err_exit_phy will perform phy_power_off also which is not
required as you are are powering on after phy_init. Wrong level
jumping

~Rajeev

> +               ret = phy_power_on(priv->phys[phy_num]);
> +               if (ret) {
> +                       phy_exit(priv->phys[phy_num]);
> +                       goto err_exit_phy;
>                 }
>         }
>
> @@ -104,10 +102,8 @@ static int ehci_platform_power_on(struct platform_device *dev)
>
>  err_exit_phy:
>         while (--phy_num >= 0) {
> -               if (priv->phys[phy_num]) {
> -                       phy_power_off(priv->phys[phy_num]);
> -                       phy_exit(priv->phys[phy_num]);
> -               }
> +               phy_power_off(priv->phys[phy_num]);
> +               phy_exit(priv->phys[phy_num]);
>         }
>  err_disable_clks:
>         while (--clk >= 0)
> @@ -123,10 +119,8 @@ static void ehci_platform_power_off(struct platform_device *dev)
>         int clk, phy_num;
>
>         for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
> -               if (priv->phys[phy_num]) {
> -                       phy_power_off(priv->phys[phy_num]);
> -                       phy_exit(priv->phys[phy_num]);
> -               }
> +               phy_power_off(priv->phys[phy_num]);
> +               phy_exit(priv->phys[phy_num]);
>         }
>
>         for (clk = EHCI_MAX_CLKS - 1; clk >= 0; clk--)
> @@ -154,7 +148,6 @@ static int ehci_platform_probe(struct platform_device *dev)
>         struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
>         struct ehci_platform_priv *priv;
>         struct ehci_hcd *ehci;
> -       const char *phy_name;
>         int err, irq, phy_num, clk = 0;
>
>         if (usb_disabled())
> @@ -204,36 +197,23 @@ static int ehci_platform_probe(struct platform_device *dev)
>
>                 priv->num_phys = of_count_phandle_with_args(dev->dev.of_node,
>                                 "phys", "#phy-cells");
> -               priv->num_phys = priv->num_phys > 0 ? priv->num_phys : 1;
>
> -               priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
> -                               sizeof(struct phy *), GFP_KERNEL);
> -               if (!priv->phys)
> -                       return -ENOMEM;
> +               if (priv->num_phys > 0) {
> +                       priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
> +                                           sizeof(struct phy *), GFP_KERNEL);
> +                       if (!priv->phys)
> +                               return -ENOMEM;
> +               } else
> +                       priv->num_phys = 0;
>
>                 for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
> -                               err = of_property_read_string_index(
> -                                               dev->dev.of_node,
> -                                               "phy-names", phy_num,
> -                                               &phy_name);
> -
> -                               if (err < 0) {
> -                                       if (priv->num_phys > 1) {
> -                                               dev_err(&dev->dev, "phy-names not provided");
> -                                               goto err_put_hcd;
> -                                       } else
> -                                               phy_name = "usb";
> -                               }
> -
> -                               priv->phys[phy_num] = devm_phy_get(&dev->dev,
> -                                               phy_name);
> -                               if (IS_ERR(priv->phys[phy_num])) {
> -                                       err = PTR_ERR(priv->phys[phy_num]);
> -                                       if ((priv->num_phys > 1) ||
> -                                           (err == -EPROBE_DEFER))
> -                                               goto err_put_hcd;
> -                                       priv->phys[phy_num] = NULL;
> -                               }
> +                       priv->phys[phy_num] = devm_of_phy_get_by_index(&dev->dev
> +                                               , dev->dev.of_node
> +                                               , phy_num);
> +                       if (IS_ERR(priv->phys[phy_num])) {
> +                               err = PTR_ERR(priv->phys[phy_num]);
> +                                       goto err_put_hcd;
> +                       }
>                 }
>
>                 for (clk = 0; clk < EHCI_MAX_CLKS; clk++) {
> --
> 2.3.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Alan Stern]

On Wed, 15 Apr 2015, rajeev kumar wrote:

> > @@ -88,15 +88,13 @@ static int ehci_platform_power_on(struct platform_device *dev)
> >         }
> >
> >         for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
> > -               if (priv->phys[phy_num]) {
> > -                       ret = phy_init(priv->phys[phy_num]);
> > -                       if (ret)
> > -                               goto err_exit_phy;
> > -                       ret = phy_power_on(priv->phys[phy_num]);
> > -                       if (ret) {
> > -                               phy_exit(priv->phys[phy_num]);
> > -                               goto err_exit_phy;
> > -                       }
> > +               ret = phy_init(priv->phys[phy_num]);
> > +               if (ret)
> > +                       goto err_exit_phy;
> 
> Jumping to err_exit_phy will perform phy_power_off also which is not
> required as you are are powering on after phy_init. Wrong level
> jumping

Look again, and this time pay more attention to the value of phy_num.

Alan Stern

> ~Rajeev
> 
> > +               ret = phy_power_on(priv->phys[phy_num]);
> > +               if (ret) {
> > +                       phy_exit(priv->phys[phy_num]);
> > +                       goto err_exit_phy;
> >                 }
> >         }
> >
> > @@ -104,10 +102,8 @@ static int ehci_platform_power_on(struct platform_device *dev)
> >
> >  err_exit_phy:
> >         while (--phy_num >= 0) {
> > -               if (priv->phys[phy_num]) {
> > -                       phy_power_off(priv->phys[phy_num]);
> > -                       phy_exit(priv->phys[phy_num]);
> > -               }
> > +               phy_power_off(priv->phys[phy_num]);
> > +               phy_exit(priv->phys[phy_num]);
> >         }

[PATCHv2 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[rajeev kumar]

On Wed, Apr 15, 2015 at 8:06 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 15 Apr 2015, rajeev kumar wrote:
>
>> > @@ -88,15 +88,13 @@ static int ehci_platform_power_on(struct platform_device *dev)
>> >         }
>> >
>> >         for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
>> > -               if (priv->phys[phy_num]) {
>> > -                       ret = phy_init(priv->phys[phy_num]);
>> > -                       if (ret)
>> > -                               goto err_exit_phy;
>> > -                       ret = phy_power_on(priv->phys[phy_num]);
>> > -                       if (ret) {
>> > -                               phy_exit(priv->phys[phy_num]);
>> > -                               goto err_exit_phy;
>> > -                       }
>> > +               ret = phy_init(priv->phys[phy_num]);
>> > +               if (ret)
>> > +                       goto err_exit_phy;
>>
>> Jumping to err_exit_phy will perform phy_power_off also which is not
>> required as you are are powering on after phy_init. Wrong level
>> jumping
>
> Look again, and this time pay more attention to the value of phy_num.
>
> Alan Stern

MIssed it ,  Thanks for the pointer.

~Rajeev

>
>> ~Rajeev
>>
>> > +               ret = phy_power_on(priv->phys[phy_num]);
>> > +               if (ret) {
>> > +                       phy_exit(priv->phys[phy_num]);
>> > +                       goto err_exit_phy;
>> >                 }
>> >         }
>> >
>> > @@ -104,10 +102,8 @@ static int ehci_platform_power_on(struct platform_device *dev)
>> >
>> >  err_exit_phy:
>> >         while (--phy_num >= 0) {
>> > -               if (priv->phys[phy_num]) {
>> > -                       phy_power_off(priv->phys[phy_num]);
>> > -                       phy_exit(priv->phys[phy_num]);
>> > -               }
>> > +               phy_power_off(priv->phys[phy_num]);
>> > +               phy_exit(priv->phys[phy_num]);
>> >         }
>
>

[PATCHv2 3/3] usb: ohci-platform: Use devm_of_phy_get_by_index

[Arun Ramamurthy]

Getting phys by index instead of phy names so that we do
not have to create a naming scheme when multiple phys are present

Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/usb/host/Kconfig         |  1 +
 drivers/usb/host/ohci-platform.c | 70 ++++++++++++++--------------------------
 2 files changed, 26 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 563f22d..1a8869c 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -558,6 +558,7 @@ config USB_CNS3XXX_OHCI
 
 config USB_OHCI_HCD_PLATFORM
 	tristate "Generic OHCI driver for a platform device"
+	select GENERIC_PHY
 	default n
 	---help---
 	  Adds an OHCI host driver for a generic platform device, which
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 185ceee..0f8edf6 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -57,15 +57,13 @@ static int ohci_platform_power_on(struct platform_device *dev)
 	}
 
 	for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-		if (priv->phys[phy_num]) {
-			ret = phy_init(priv->phys[phy_num]);
-			if (ret)
-				goto err_exit_phy;
-			ret = phy_power_on(priv->phys[phy_num]);
-			if (ret) {
-				phy_exit(priv->phys[phy_num]);
-				goto err_exit_phy;
-			}
+		ret = phy_init(priv->phys[phy_num]);
+		if (ret)
+			goto err_exit_phy;
+		ret = phy_power_on(priv->phys[phy_num]);
+		if (ret) {
+			phy_exit(priv->phys[phy_num]);
+			goto err_exit_phy;
 		}
 	}
 
@@ -73,10 +71,8 @@ static int ohci_platform_power_on(struct platform_device *dev)
 
 err_exit_phy:
 	while (--phy_num >= 0) {
-		if (priv->phys[phy_num]) {
-			phy_power_off(priv->phys[phy_num]);
-			phy_exit(priv->phys[phy_num]);
-		}
+		phy_power_off(priv->phys[phy_num]);
+		phy_exit(priv->phys[phy_num]);
 	}
 err_disable_clks:
 	while (--clk >= 0)
@@ -92,10 +88,8 @@ static void ohci_platform_power_off(struct platform_device *dev)
 	int clk, phy_num;
 
 	for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-		if (priv->phys[phy_num]) {
-			phy_power_off(priv->phys[phy_num]);
-			phy_exit(priv->phys[phy_num]);
-		}
+		phy_power_off(priv->phys[phy_num]);
+		phy_exit(priv->phys[phy_num]);
 	}
 
 	for (clk = OHCI_MAX_CLKS - 1; clk >= 0; clk--)
@@ -123,7 +117,6 @@ static int ohci_platform_probe(struct platform_device *dev)
 	struct usb_ohci_pdata *pdata = dev_get_platdata(&dev->dev);
 	struct ohci_platform_priv *priv;
 	struct ohci_hcd *ohci;
-	const char *phy_name;
 	int err, irq, phy_num, clk = 0;
 
 	if (usb_disabled())
@@ -174,36 +167,23 @@ static int ohci_platform_probe(struct platform_device *dev)
 
 		priv->num_phys = of_count_phandle_with_args(dev->dev.of_node,
 				"phys", "#phy-cells");
-		priv->num_phys = priv->num_phys > 0 ? priv->num_phys : 1;
 
-		priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
-				sizeof(struct phy *), GFP_KERNEL);
-		if (!priv->phys)
-			return -ENOMEM;
+		if (priv->num_phys > 0) {
+			priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
+					    sizeof(struct phy *), GFP_KERNEL);
+			if (!priv->phys)
+				return -ENOMEM;
+		} else
+			priv->num_phys = 0;
 
 		for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-				err = of_property_read_string_index(
-						dev->dev.of_node,
-						"phy-names", phy_num,
-						&phy_name);
-
-				if (err < 0) {
-					if (priv->num_phys > 1) {
-						dev_err(&dev->dev, "phy-names not provided");
-						goto err_put_hcd;
-					} else
-						phy_name = "usb";
-				}
-
-				priv->phys[phy_num] = devm_phy_get(&dev->dev,
-						phy_name);
-				if (IS_ERR(priv->phys[phy_num])) {
-					err = PTR_ERR(priv->phys[phy_num]);
-					if ((priv->num_phys > 1) ||
-					    (err == -EPROBE_DEFER))
-						goto err_put_hcd;
-					priv->phys[phy_num] = NULL;
-				}
+			priv->phys[phy_num] = devm_of_phy_get_by_index(&dev->dev
+						, dev->dev.of_node
+						, phy_num);
+			if (IS_ERR(priv->phys[phy_num])) {
+				err = PTR_ERR(priv->phys[phy_num]);
+				goto err_put_hcd;
+			}
 		}
 
 		for (clk = 0; clk < OHCI_MAX_CLKS; clk++) {
-- 
2.3.4