Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1374498 > unrolled thread

[PATCH RFT 2/2] macb: kill PHY reset code

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH RFT 2/2] macb: kill PHY reset code Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-04-09 00:30 +0200
    Re: [PATCH RFT 2/2] macb: kill PHY reset code Andrew Lunn <andrew@lunn.ch> - 2016-04-11 04:30 +0200
      Re: [PATCH RFT 2/2] macb: kill PHY reset code Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-04-11 19:50 +0200
        Re: [PATCH RFT 2/2] macb: kill PHY reset code Andrew Lunn <andrew@lunn.ch> - 2016-04-11 20:20 +0200
          Re: [PATCH RFT 2/2] macb: kill PHY reset code Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-04-11 20:40 +0200
            Re: [PATCH RFT 2/2] macb: kill PHY reset code Andrew Lunn <andrew@lunn.ch> - 2016-04-11 21:00 +0200
              Re: [PATCH RFT 2/2] macb: kill PHY reset code Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-04-11 21:10 +0200
              Re: [PATCH RFT 2/2] macb: kill PHY reset code Nicolas Ferre <nicolas.ferre@atmel.com> - 2016-04-12 11:30 +0200
      Re: [PATCH RFT 2/2] macb: kill PHY reset code Nicolas Ferre <nicolas.ferre@atmel.com> - 2016-04-12 11:30 +0200
        Re: [PATCH RFT 2/2] macb: kill PHY reset code Andrew Lunn <andrew@lunn.ch> - 2016-04-12 15:50 +0200
          Re: [PATCH RFT 2/2] macb: kill PHY reset code Nicolas Ferre <nicolas.ferre@atmel.com> - 2016-04-12 16:50 +0200
        Re: [PATCH RFT 2/2] macb: kill PHY reset code Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-04-12 16:00 +0200
          Re: [PATCH RFT 2/2] macb: kill PHY reset code Nicolas Ferre <nicolas.ferre@atmel.com> - 2016-04-12 17:00 +0200

#1374498 — [PATCH RFT 2/2] macb: kill PHY reset code

With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
there should be no need to frob the PHY reset in this  driver anymore...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/ethernet/cadence/macb.c |   17 -----------------
 drivers/net/ethernet/cadence/macb.h |    1 -
 2 files changed, 18 deletions(-)

Index: net-next/drivers/net/ethernet/cadence/macb.c
===================================================================
--- net-next.orig/drivers/net/ethernet/cadence/macb.c
+++ net-next/drivers/net/ethernet/cadence/macb.c
@@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
 					      = macb_clk_init;
 	int (*init)(struct platform_device *) = macb_init;
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *phy_node;
 	const struct macb_config *macb_config = NULL;
 	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
 	unsigned int queue_mask, num_queues;
@@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
 	else
 		macb_get_hwaddr(bp);
 
-	/* Power up the PHY if there is a GPIO reset */
-	phy_node =  of_get_next_available_child(np, NULL);
-	if (phy_node) {
-		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
-
-		if (gpio_is_valid(gpio)) {
-			bp->reset_gpio = gpio_to_desc(gpio);
-			gpiod_direction_output(bp->reset_gpio, 1);
-		}
-	}
-	of_node_put(phy_node);
-
 	err = of_get_phy_mode(np);
 	if (err < 0) {
 		pdata = dev_get_platdata(&pdev->dev);
@@ -3054,10 +3041,6 @@ static int macb_remove(struct platform_d
 		mdiobus_unregister(bp->mii_bus);
 		mdiobus_free(bp->mii_bus);
 
-		/* Shutdown the PHY if there is a GPIO reset */
-		if (bp->reset_gpio)
-			gpiod_set_value(bp->reset_gpio, 0);
-
 		unregister_netdev(dev);
 		clk_disable_unprepare(bp->tx_clk);
 		clk_disable_unprepare(bp->hclk);
Index: net-next/drivers/net/ethernet/cadence/macb.h
===================================================================
--- net-next.orig/drivers/net/ethernet/cadence/macb.h
+++ net-next/drivers/net/ethernet/cadence/macb.h
@@ -832,7 +832,6 @@ struct macb {
 	unsigned int		dma_burst_length;
 
 	phy_interface_t		phy_interface;
-	struct gpio_desc	*reset_gpio;
 
 	/* AT91RM9200 transmit */
 	struct sk_buff *skb;			/* holds skb until xmit interrupt completes */

[toc] | [next] | [standalone]

#1375424

On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote:
> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
> there should be no need to frob the PHY reset in this  driver anymore...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
>  drivers/net/ethernet/cadence/macb.c |   17 -----------------
>  drivers/net/ethernet/cadence/macb.h |    1 -
>  2 files changed, 18 deletions(-)
> 
> Index: net-next/drivers/net/ethernet/cadence/macb.c
> ===================================================================
> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
> +++ net-next/drivers/net/ethernet/cadence/macb.c
> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
>  					      = macb_clk_init;
>  	int (*init)(struct platform_device *) = macb_init;
>  	struct device_node *np = pdev->dev.of_node;
> -	struct device_node *phy_node;
>  	const struct macb_config *macb_config = NULL;
>  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
>  	unsigned int queue_mask, num_queues;
> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>  	else
>  		macb_get_hwaddr(bp);
>  
> -	/* Power up the PHY if there is a GPIO reset */
> -	phy_node =  of_get_next_available_child(np, NULL);
> -	if (phy_node) {
> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
> -
> -		if (gpio_is_valid(gpio)) {
> -			bp->reset_gpio = gpio_to_desc(gpio);
> -			gpiod_direction_output(bp->reset_gpio, 1);

Hi Sergei

The code you are deleting would of ignored the flags in the gpio
property, i.e. active low. The new code in the previous patch does
however take the flags into account. Did you check if there are any
device trees which have flags, which were never used, but are now
going to be used and thus break...

      Andrew

[toc] | [prev] | [next] | [standalone]

#1376179

Hello.

On 04/11/2016 05:28 AM, Andrew Lunn wrote:

>> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
>> there should be no need to frob the PHY reset in this  driver anymore...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>>   drivers/net/ethernet/cadence/macb.c |   17 -----------------
>>   drivers/net/ethernet/cadence/macb.h |    1 -
>>   2 files changed, 18 deletions(-)
>>
>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>> ===================================================================
>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>> +++ net-next/drivers/net/ethernet/cadence/macb.c
[...]
>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>   	else
>>   		macb_get_hwaddr(bp);
>>
>> -	/* Power up the PHY if there is a GPIO reset */
>> -	phy_node =  of_get_next_available_child(np, NULL);
>> -	if (phy_node) {
>> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>> -
>> -		if (gpio_is_valid(gpio)) {
>> -			bp->reset_gpio = gpio_to_desc(gpio);
>> -			gpiod_direction_output(bp->reset_gpio, 1);
>
> Hi Sergei
>
> The code you are deleting would of ignored the flags in the gpio
> property, i.e. active low.

    Hm, you're right -- I forgot about that... :-/

> The new code in the previous patch does
> however take the flags into account. Did you check if there are any
> device trees which have flags, which were never used, but are now
> going to be used and thus break...

    Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts. Looks 
like it needs to be fixed indeed...

>        Andrew

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1376194

> >The code you are deleting would of ignored the flags in the gpio
> >property, i.e. active low.
> 
>    Hm, you're right -- I forgot about that... :-/
> 
> >The new code in the previous patch does
> >however take the flags into account. Did you check if there are any
> >device trees which have flags, which were never used, but are now
> >going to be used and thus break...
> 
>    Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts.
> Looks like it needs to be fixed indeed...

And this is where it gets tricky. You are breaking backwards
compatibility by now respecting the flag. An old DT blob is not going
to work.

You potentially need to add a new property and deprecate the old one.

    Andrew

[toc] | [prev] | [next] | [standalone]

#1376205

Hello.

On 04/11/2016 09:19 PM, Andrew Lunn wrote:

>>> The code you are deleting would of ignored the flags in the gpio
>>> property, i.e. active low.
>>
>>     Hm, you're right -- I forgot about that... :-/
>>
>>> The new code in the previous patch does
>>> however take the flags into account. Did you check if there are any
>>> device trees which have flags, which were never used, but are now
>>> going to be used and thus break...
>>
>>     Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts.
>> Looks like it needs to be fixed indeed...
>>
> And this is where it gets tricky. You are breaking backwards
> compatibility by now respecting the flag. An old DT blob is not going
> to work.

    Do we care that much about the DT blobs that are just *wrong*?

> You potentially need to add a new property and deprecate the old one.

    I would like to avoid that...

>      Andrew

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1376215

On Mon, Apr 11, 2016 at 09:39:02PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 04/11/2016 09:19 PM, Andrew Lunn wrote:
> 
> >>>The code you are deleting would of ignored the flags in the gpio
> >>>property, i.e. active low.
> >>
> >>    Hm, you're right -- I forgot about that... :-/
> >>
> >>>The new code in the previous patch does
> >>>however take the flags into account. Did you check if there are any
> >>>device trees which have flags, which were never used, but are now
> >>>going to be used and thus break...
> >>
> >>    Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts.
> >>Looks like it needs to be fixed indeed...
> >>
> >And this is where it gets tricky. You are breaking backwards
> >compatibility by now respecting the flag. An old DT blob is not going
> >to work.
> 
>    Do we care that much about the DT blobs that are just *wrong*?

Wrong, but currently works.

> >You potentially need to add a new property and deprecate the old one.
> 
>    I would like to avoid that...

You will need the agreement from the at91-vinco maintainer.

    Andrew

[toc] | [prev] | [next] | [standalone]

#1376238

On 04/11/2016 09:51 PM, Andrew Lunn wrote:

>>>>> The code you are deleting would of ignored the flags in the gpio
>>>>> property, i.e. active low.
>>>>
>>>>     Hm, you're right -- I forgot about that... :-/
>>>>
>>>>> The new code in the previous patch does
>>>>> however take the flags into account. Did you check if there are any
>>>>> device trees which have flags, which were never used, but are now
>>>>> going to be used and thus break...
>>>>
>>>>     Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts.
>>>> Looks like it needs to be fixed indeed...
>>>>
>>> And this is where it gets tricky. You are breaking backwards
>>> compatibility by now respecting the flag. An old DT blob is not going
>>> to work.
>>
>>     Do we care that much about the DT blobs that are just *wrong*?
>
> Wrong, but currently works.

    Note that it's not only using GPIO_ACTIVE_HIGH but does that against what 
the MACB binding documents.

>>> You potentially need to add a new property and deprecate the old one.
>>
>>     I would like to avoid that...
>
> You will need the agreement from the at91-vinco maintainer.

   I'll try submitting a formal DT patch...

>      Andrew

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1376587

Le 11/04/2016 20:51, Andrew Lunn a écrit :
> On Mon, Apr 11, 2016 at 09:39:02PM +0300, Sergei Shtylyov wrote:
>> Hello.
>>
>> On 04/11/2016 09:19 PM, Andrew Lunn wrote:
>>
>>>>> The code you are deleting would of ignored the flags in the gpio
>>>>> property, i.e. active low.
>>>>
>>>>    Hm, you're right -- I forgot about that... :-/
>>>>
>>>>> The new code in the previous patch does
>>>>> however take the flags into account. Did you check if there are any
>>>>> device trees which have flags, which were never used, but are now
>>>>> going to be used and thus break...
>>>>
>>>>    Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts.
>>>> Looks like it needs to be fixed indeed...
>>>>
>>> And this is where it gets tricky. You are breaking backwards
>>> compatibility by now respecting the flag. An old DT blob is not going
>>> to work.
>>
>>    Do we care that much about the DT blobs that are just *wrong*?
> 
> Wrong, but currently works.
> 
>>> You potentially need to add a new property and deprecate the old one.
>>
>>    I would like to avoid that...
> 
> You will need the agreement from the at91-vinco maintainer.

If the at91-vinco has to be modified, you have my agreement that it can
be modified.

Bye,
-- 
Nicolas Ferre

[toc] | [prev] | [next] | [standalone]

#1376588

Le 11/04/2016 04:28, Andrew Lunn a écrit :
> On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote:
>> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
>> there should be no need to frob the PHY reset in this  driver anymore...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>>  drivers/net/ethernet/cadence/macb.c |   17 -----------------
>>  drivers/net/ethernet/cadence/macb.h |    1 -
>>  2 files changed, 18 deletions(-)
>>
>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>> ===================================================================
>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>> +++ net-next/drivers/net/ethernet/cadence/macb.c
>> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
>>  					      = macb_clk_init;
>>  	int (*init)(struct platform_device *) = macb_init;
>>  	struct device_node *np = pdev->dev.of_node;
>> -	struct device_node *phy_node;
>>  	const struct macb_config *macb_config = NULL;
>>  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
>>  	unsigned int queue_mask, num_queues;
>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>  	else
>>  		macb_get_hwaddr(bp);
>>  
>> -	/* Power up the PHY if there is a GPIO reset */
>> -	phy_node =  of_get_next_available_child(np, NULL);
>> -	if (phy_node) {
>> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>> -
>> -		if (gpio_is_valid(gpio)) {
>> -			bp->reset_gpio = gpio_to_desc(gpio);
>> -			gpiod_direction_output(bp->reset_gpio, 1);
> 
> Hi Sergei
> 
> The code you are deleting would of ignored the flags in the gpio

I don't parse this.

The code deleted does take the flag into account. And the DT property
associated to it seems correct to me (I mean, with proper flag
specification).

> property, i.e. active low. The new code in the previous patch does
> however take the flags into account. Did you check if there are any
> device trees which have flags, which were never used, but are now
> going to be used and thus break...

Flag was used and you are saying that it's taken into account in new
code... So, what's the issue?

I see a difference in the way the "value" of gpiod_* functions is used.
There may be a misunderstanding here...

Bye,
-- 
Nicolas Ferre

[toc] | [prev] | [next] | [standalone]

#1376812

On Tue, Apr 12, 2016 at 11:22:10AM +0200, Nicolas Ferre wrote:
> Le 11/04/2016 04:28, Andrew Lunn a écrit :
> > On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote:
> >> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
> >> there should be no need to frob the PHY reset in this  driver anymore...
> >>
> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>
> >> ---
> >>  drivers/net/ethernet/cadence/macb.c |   17 -----------------
> >>  drivers/net/ethernet/cadence/macb.h |    1 -
> >>  2 files changed, 18 deletions(-)
> >>
> >> Index: net-next/drivers/net/ethernet/cadence/macb.c
> >> ===================================================================
> >> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
> >> +++ net-next/drivers/net/ethernet/cadence/macb.c
> >> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
> >>  					      = macb_clk_init;
> >>  	int (*init)(struct platform_device *) = macb_init;
> >>  	struct device_node *np = pdev->dev.of_node;
> >> -	struct device_node *phy_node;
> >>  	const struct macb_config *macb_config = NULL;
> >>  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
> >>  	unsigned int queue_mask, num_queues;
> >> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
> >>  	else
> >>  		macb_get_hwaddr(bp);
> >>  
> >> -	/* Power up the PHY if there is a GPIO reset */
> >> -	phy_node =  of_get_next_available_child(np, NULL);
> >> -	if (phy_node) {
> >> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
> >> -
> >> -		if (gpio_is_valid(gpio)) {
> >> -			bp->reset_gpio = gpio_to_desc(gpio);
> >> -			gpiod_direction_output(bp->reset_gpio, 1);
> > 
> > Hi Sergei
> > 
> > The code you are deleting would of ignored the flags in the gpio
> I don't parse this.
> 
> The code deleted does take the flag into account. And the DT property
> associated to it seems correct to me (I mean, with proper flag
> specification).

Hi Nicolas
 
of_get_named_gpio() does not do anything with the flags. So for
example,

			gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;

the GPIO_ACTIVE_LOW would be ignored. If you want the flags to be
respected, you need to use the gpiod API for all calls, in particular,
you need to use something which calls gpiod_get_index(), since that is
the only function to call gpiod_parse_flags() to translate
GPIO_ACTIVE_LOW into a flag within the gpio descriptor.

	Andrew

[toc] | [prev] | [next] | [standalone]

#1376870

Le 12/04/2016 15:40, Andrew Lunn a écrit :
> On Tue, Apr 12, 2016 at 11:22:10AM +0200, Nicolas Ferre wrote:
>> Le 11/04/2016 04:28, Andrew Lunn a écrit :
>>> On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote:
>>>> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
>>>> there should be no need to frob the PHY reset in this  driver anymore...
>>>>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>>
>>>> ---
>>>>  drivers/net/ethernet/cadence/macb.c |   17 -----------------
>>>>  drivers/net/ethernet/cadence/macb.h |    1 -
>>>>  2 files changed, 18 deletions(-)
>>>>
>>>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>>>> ===================================================================
>>>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>>>> +++ net-next/drivers/net/ethernet/cadence/macb.c
>>>> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
>>>>  					      = macb_clk_init;
>>>>  	int (*init)(struct platform_device *) = macb_init;
>>>>  	struct device_node *np = pdev->dev.of_node;
>>>> -	struct device_node *phy_node;
>>>>  	const struct macb_config *macb_config = NULL;
>>>>  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
>>>>  	unsigned int queue_mask, num_queues;
>>>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>>>  	else
>>>>  		macb_get_hwaddr(bp);
>>>>  
>>>> -	/* Power up the PHY if there is a GPIO reset */
>>>> -	phy_node =  of_get_next_available_child(np, NULL);
>>>> -	if (phy_node) {
>>>> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>>>> -
>>>> -		if (gpio_is_valid(gpio)) {
>>>> -			bp->reset_gpio = gpio_to_desc(gpio);
>>>> -			gpiod_direction_output(bp->reset_gpio, 1);
>>>
>>> Hi Sergei
>>>
>>> The code you are deleting would of ignored the flags in the gpio
>> I don't parse this.
>>
>> The code deleted does take the flag into account. And the DT property
>> associated to it seems correct to me (I mean, with proper flag
>> specification).
> 
> Hi Nicolas
>  
> of_get_named_gpio() does not do anything with the flags. So for
> example,
> 
> 			gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
> 
> the GPIO_ACTIVE_LOW would be ignored. If you want the flags to be
> respected, you need to use the gpiod API for all calls, in particular,
> you need to use something which calls gpiod_get_index(), since that is
> the only function to call gpiod_parse_flags() to translate
> GPIO_ACTIVE_LOW into a flag within the gpio descriptor.

Ok, I remember what confused me now: this code, used to be something around:
devm_gpiod_get_optional(&bp->pdev->dev, "phy-reset", GPIOD_OUT_HIGH);
before it has been changed to the chunk above... So, yes, the DT flag
was not handled anyway...

Sorry for the noise and thanks for the clarification.

Bye,
-- 
Nicolas Ferre

[toc] | [prev] | [next] | [standalone]

#1376818

Hello.

On 4/12/2016 12:22 PM, Nicolas Ferre wrote:

>>> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
>>> there should be no need to frob the PHY reset in this  driver anymore...
>>>
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>
>>> ---
>>>   drivers/net/ethernet/cadence/macb.c |   17 -----------------
>>>   drivers/net/ethernet/cadence/macb.h |    1 -
>>>   2 files changed, 18 deletions(-)
>>>
>>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>>> ===================================================================
>>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>>> +++ net-next/drivers/net/ethernet/cadence/macb.c
[...]
>>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>>   	else
>>>   		macb_get_hwaddr(bp);
>>>
>>> -	/* Power up the PHY if there is a GPIO reset */
>>> -	phy_node =  of_get_next_available_child(np, NULL);
>>> -	if (phy_node) {
>>> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>>> -
>>> -		if (gpio_is_valid(gpio)) {
>>> -			bp->reset_gpio = gpio_to_desc(gpio);
>>> -			gpiod_direction_output(bp->reset_gpio, 1);
>>
>> Hi Sergei
>>
>> The code you are deleting would of ignored the flags in the gpio
>
> I don't parse this.

> The code deleted does take the flag into account.

    Not really -- you need to call of_get_named_gpio_flags() (with a valid 
last argument) for that.

> And the DT property
> associated to it seems correct to me (I mean, with proper flag
> specification).

    It apparently is not as it have GPIO_ACTIVE_HIGH and the driver assumes 
active-low reset signal.

[...]
> Bye,

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1376878

Le 12/04/2016 15:54, Sergei Shtylyov a écrit :
> Hello.
> 
> On 4/12/2016 12:22 PM, Nicolas Ferre wrote:
> 
>>>> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
>>>> there should be no need to frob the PHY reset in this  driver anymore...
>>>>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>>
>>>> ---
>>>>   drivers/net/ethernet/cadence/macb.c |   17 -----------------
>>>>   drivers/net/ethernet/cadence/macb.h |    1 -
>>>>   2 files changed, 18 deletions(-)
>>>>
>>>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>>>> ===================================================================
>>>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>>>> +++ net-next/drivers/net/ethernet/cadence/macb.c
> [...]
>>>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>>>   	else
>>>>   		macb_get_hwaddr(bp);
>>>>
>>>> -	/* Power up the PHY if there is a GPIO reset */
>>>> -	phy_node =  of_get_next_available_child(np, NULL);
>>>> -	if (phy_node) {
>>>> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>>>> -
>>>> -		if (gpio_is_valid(gpio)) {
>>>> -			bp->reset_gpio = gpio_to_desc(gpio);
>>>> -			gpiod_direction_output(bp->reset_gpio, 1);
>>>
>>> Hi Sergei
>>>
>>> The code you are deleting would of ignored the flags in the gpio
>>
>> I don't parse this.
> 
>> The code deleted does take the flag into account.
> 
>     Not really -- you need to call of_get_named_gpio_flags() (with a valid 
> last argument) for that.

Yep,

>> And the DT property
>> associated to it seems correct to me (I mean, with proper flag
>> specification).
> 
>     It apparently is not as it have GPIO_ACTIVE_HIGH and the driver assumes 
> active-low reset signal.

Yes, logic was inverted and... anyway, the flag never used for real...
Thanks Sergei.

No problem for me accepting a patch for the at91-vinco.dts.

Bye,
-- 
Nicolas Ferre

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web