[PATCH 3.7.0-rc4] of/net/mdio-gpio: Fix pdev->id issue when using devicetrees.

[PATCH 3.7.0-rc4] of/net/mdio-gpio: Fix pdev->id issue when using devicetrees.

[Srinivas KANDAGATLA]

From: Srinivas Kandagatla <srinivas.kandagatla@st.com>

When the mdio-gpio driver is probed via device trees, the platform
device id is set as -1, However the id is re-used in the code while
creating an mdio bus.
So, setting up the id via aliases from device tree is a sensible
solution to fix this issue.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>
---
 .../devicetree/bindings/net/mdio-gpio.txt          |    9 ++++++++-
 drivers/net/phy/mdio-gpio.c                        |    1 +
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mdio-gpio.txt b/Documentation/devicetree/bindings/net/mdio-gpio.txt
index bc95495..c79bab0 100644
--- a/Documentation/devicetree/bindings/net/mdio-gpio.txt
+++ b/Documentation/devicetree/bindings/net/mdio-gpio.txt
@@ -8,9 +8,16 @@ gpios property as described in section VIII.1 in the following order:
 
 MDC, MDIO.
 
+Note: Each gpio-mdio bus should have an alias correctly numbered in "aliases"
+node.
+
 Example:
 
-mdio {
+aliases {
+	mdio-gpio0 = <&mdio0>;
+};
+
+mdio0: mdio {
 	compatible = "virtual,mdio-gpio";
 	#address-cells = <1>;
 	#size-cells = <0>;
diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 899274f..e3f3115 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -56,6 +56,7 @@ static void *mdio_gpio_of_get_data(struct platform_device *pdev)
 	if (ret < 0)
 		return NULL;
 	pdata->mdio = ret;
+	pdev->id = of_alias_get_id(np, "mdio-gpio");
 
 	return pdata;
 }
-- 
1.7.0.4

Re: [PATCH 3.7.0-rc4] of/net/mdio-gpio: Fix pdev->id issue when using devicetrees.

[David Miller]

From: Srinivas KANDAGATLA <srinivas.kandagatla@st.com>
Date: Tue, 13 Nov 2012 14:26:13 +0000

> From: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> 
> When the mdio-gpio driver is probed via device trees, the platform
> device id is set as -1, However the id is re-used in the code while
> creating an mdio bus.
> So, setting up the id via aliases from device tree is a sensible
> solution to fix this issue.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>

This seems rather pointless unless you also update every single device
tree out there.

Also you need to describe what are the ramifications of this problem
otherwise it is impossible to figure out how serious this change is.

Does it prevent probing?  Does it cause a crash?

Basically, what I'm saying is that this is a very poor submission and
you need to substantially improve it and communicate better.

If the problem is basically benign, then you should target this change
to net-next instead of the net tree, along with the necessary dt file
updates.

Thanks.

Re: [PATCH 3.7.0-rc4] of/net/mdio-gpio: Fix pdev->id issue when using devicetrees.

[Grant Likely]

On Tue, 13 Nov 2012 14:26:13 +0000, Srinivas KANDAGATLA <srinivas.kandagatla@st.com> wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> 
> When the mdio-gpio driver is probed via device trees, the platform
> device id is set as -1, However the id is re-used in the code while
> creating an mdio bus.
> So, setting up the id via aliases from device tree is a sensible
> solution to fix this issue.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> ---
>  .../devicetree/bindings/net/mdio-gpio.txt          |    9 ++++++++-
>  drivers/net/phy/mdio-gpio.c                        |    1 +
>  2 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/mdio-gpio.txt b/Documentation/devicetree/bindings/net/mdio-gpio.txt
> index bc95495..c79bab0 100644
> --- a/Documentation/devicetree/bindings/net/mdio-gpio.txt
> +++ b/Documentation/devicetree/bindings/net/mdio-gpio.txt
> @@ -8,9 +8,16 @@ gpios property as described in section VIII.1 in the following order:
>  
>  MDC, MDIO.
>  
> +Note: Each gpio-mdio bus should have an alias correctly numbered in "aliases"
> +node.
> +
>  Example:
>  
> -mdio {
> +aliases {
> +	mdio-gpio0 = <&mdio0>;
> +};
> +
> +mdio0: mdio {
>  	compatible = "virtual,mdio-gpio";
>  	#address-cells = <1>;
>  	#size-cells = <0>;
> diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
> index 899274f..e3f3115 100644
> --- a/drivers/net/phy/mdio-gpio.c
> +++ b/drivers/net/phy/mdio-gpio.c
> @@ -56,6 +56,7 @@ static void *mdio_gpio_of_get_data(struct platform_device *pdev)
>  	if (ret < 0)
>  		return NULL;
>  	pdata->mdio = ret;
> +	pdev->id = of_alias_get_id(np, "mdio-gpio");

This is actually illegal. Once a device is registered, pdev->id must not
be changed. Same goes for pdev->name and pdev->dev.platform_data.

g.

Re: [PATCH 3.7.0-rc4] of/net/mdio-gpio: Fix pdev->id issue when using devicetrees.

[Srinivas KANDAGATLA]

On 14/11/12 23:59, David Miller wrote:
> From: Srinivas KANDAGATLA <srinivas.kandagatla@st.com>
> Date: Tue, 13 Nov 2012 14:26:13 +0000
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@st.com>
>>
>> When the mdio-gpio driver is probed via device trees, the platform
>> device id is set as -1, However the id is re-used in the code while
>> creating an mdio bus.
>> So, setting up the id via aliases from device tree is a sensible
>> solution to fix this issue.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> This seems rather pointless unless you also update every single device
> tree out there.
>
> Also you need to describe what are the ramifications of this problem
> otherwise it is impossible to figure out how serious this change is.
>
> Does it prevent probing?  Does it cause a crash?
I apologies, I should have explained the full use-case.

use-case is if the mac driver want to connect via phy_connect() to
mdio-gpio phy it would use bus name to do so.

mdio-gpio phy bus name is formated as "gpio-<bus-number>:<phy-addr>.
In the existing code the bus number for mdio-gpio if probed from device
trees will be set to -1 which results in bus name set to
"gpio-ffffffff:<phy-addr>" which is the problem here.
fffffff is result of pdev->id set to -1 which should be set to a logical
number, and this is only possible via aliases.

Having fffffff as bus-id also means that we can't have two mdio-gpio
buses via device trees as it will result in same bus-id.
This patch attempts to fix this issue.
So getting the id from alias would be a right choice.

I also agree with Grant's comments about setting up pdev->id.

Will send v2 patch with considering Grant's comments.

>
> Basically, what I'm saying is that this is a very poor submission and
> you need to substantially improve it and communicate better.
>
> If the problem is basically benign, then you should target this change
> to net-next instead of the net tree, along with the necessary dt file
> updates.
I have looked in net-next and there are no dt files which use this driver.

Thanks,
srini
> Thanks.
>
>

[PATCH v2 3.7.0-rc4] of/net/mdio-gpio: Fix pdev->id issue when using devicetrees.

[Srinivas KANDAGATLA]

From: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>

When the mdio-gpio driver is probed via device trees, the platform
device id is set as -1, However the pdev->id is re-used as bus-id for
while creating mdio gpio bus.
So
For device tree case the mdio-gpio bus name appears as "gpio-ffffffff"
where as
for non-device tree case the bus name appears as "gpio-<bus-num>"

Which means the bus_id is fixed in device tree case, so we can't have
two mdio gpio buses via device trees. Assigning a logical bus number
via device tree solves the problem and the bus name is much consistent
with non-device tree bus name.

Without this patch
1. we can't support two mdio-gpio buses via device trees.
2. we should always pass gpio-ffffffff as bus name to phy_connect, very
different to non-device tree bus name.

So, setting up the bus_id via aliases from device tree is the right
solution and other drivers do similar thing.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
---
 .../devicetree/bindings/net/mdio-gpio.txt          |    9 ++++++++-
 drivers/net/phy/mdio-gpio.c                        |   11 +++++++----
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mdio-gpio.txt b/Documentation/devicetree/bindings/net/mdio-gpio.txt
index bc95495..c79bab0 100644
--- a/Documentation/devicetree/bindings/net/mdio-gpio.txt
+++ b/Documentation/devicetree/bindings/net/mdio-gpio.txt
@@ -8,9 +8,16 @@ gpios property as described in section VIII.1 in the following order:
 
 MDC, MDIO.
 
+Note: Each gpio-mdio bus should have an alias correctly numbered in "aliases"
+node.
+
 Example:
 
-mdio {
+aliases {
+	mdio-gpio0 = <&mdio0>;
+};
+
+mdio0: mdio {
 	compatible = "virtual,mdio-gpio";
 	#address-cells = <1>;
 	#size-cells = <0>;
diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 899274f..2ed1140 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -185,17 +185,20 @@ static int __devinit mdio_gpio_probe(struct platform_device *pdev)
 {
 	struct mdio_gpio_platform_data *pdata;
 	struct mii_bus *new_bus;
-	int ret;
+	int ret, bus_id;
 
-	if (pdev->dev.of_node)
+	if (pdev->dev.of_node) {
 		pdata = mdio_gpio_of_get_data(pdev);
-	else
+		bus_id = of_alias_get_id(pdev->dev.of_node, "mdio-gpio");
+	} else {
 		pdata = pdev->dev.platform_data;
+		bus_id = pdev->id;
+	}
 
 	if (!pdata)
 		return -ENODEV;
 
-	new_bus = mdio_gpio_bus_init(&pdev->dev, pdata, pdev->id);
+	new_bus = mdio_gpio_bus_init(&pdev->dev, pdata, bus_id);
 	if (!new_bus)
 		return -ENODEV;
 
-- 
1.7.0.4

Re: [PATCH v2 3.7.0-rc4] of/net/mdio-gpio: Fix pdev->id issue when using devicetrees.

[David Miller]

From: Srinivas KANDAGATLA <srinivas.kandagatla@st.com>
Date: Fri, 16 Nov 2012 10:33:59 +0000

> From: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> 
> When the mdio-gpio driver is probed via device trees, the platform
> device id is set as -1, However the pdev->id is re-used as bus-id for
> while creating mdio gpio bus.
> So
> For device tree case the mdio-gpio bus name appears as "gpio-ffffffff"
> where as
> for non-device tree case the bus name appears as "gpio-<bus-num>"
> 
> Which means the bus_id is fixed in device tree case, so we can't have
> two mdio gpio buses via device trees. Assigning a logical bus number
> via device tree solves the problem and the bus name is much consistent
> with non-device tree bus name.
> 
> Without this patch
> 1. we can't support two mdio-gpio buses via device trees.
> 2. we should always pass gpio-ffffffff as bus name to phy_connect, very
> different to non-device tree bus name.
> 
> So, setting up the bus_id via aliases from device tree is the right
> solution and other drivers do similar thing.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>

Applied, thank you.