netdev - Re: [PATCH] phy: add support for a reset-gpio specification

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160513062631.GA28415@pengutronix.de>
Date:	Fri, 13 May 2016 08:26:31 +0200
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
Cc:	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	Florian Fainelli <f.fainelli@...il.com>, kernel@...gutronix.de,
	"Andrew F . Davis" <afd@...com>, Nishanth Menon <nm@...com>
Subject: Re: [PATCH] phy: add support for a reset-gpio specification

Hello Sergei,

On Fri, May 13, 2016 at 12:41:47AM +0300, Sergei Shtylyov wrote:
> On 05/12/2016 01:00 PM, Uwe Kleine-König wrote:
> 
> >The framework only asserts (for now) that the reset gpio is not active.
> >
> >Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> >---
> > Documentation/devicetree/bindings/net/phy.txt | 3 +++
> > drivers/net/phy/phy_device.c                  | 8 ++++++++
> > 2 files changed, 11 insertions(+)
> >
> >diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> >index bc1c3c8bf8fa..c00a9a894547 100644
> >--- a/Documentation/devicetree/bindings/net/phy.txt
> >+++ b/Documentation/devicetree/bindings/net/phy.txt
> >@@ -35,6 +35,8 @@ Optional Properties:
> > - broken-turn-around: If set, indicates the PHY device does not correctly
> >   release the turn around line low at the end of a MDIO transaction.
> >
> >+- reset-gpios: Reference to a GPIO used to reset the phy.
> >+
> > Example:
> >
> > ethernet-phy@0 {
> >@@ -42,4 +44,5 @@ ethernet-phy@0 {
> > 	interrupt-parent = <40000>;
> > 	interrupts = <35 1>;
> > 	reg = <0>;
> >+	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
> >  };
> 
>    OK, the example has PHY ID specified in the "compatible" prop. It's a
> vital detail, you should mention it somewhere...

Something like that in the commit log:

	Note that reset is only deasserted when the PHY ID is already
	determined. If your phy doesn't support reading the PHY ID while
	reset is asserted it's recommended to specify it in the
	compatible string. Otherwise the device might fail to probe.

> >diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> >index e551f3a89cfd..7d666ab47271 100644
> >--- a/drivers/net/phy/phy_device.c
> >+++ b/drivers/net/phy/phy_device.c
> [...]
> >@@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
> > 	struct device_driver *drv = phydev->mdio.dev.driver;
> > 	struct phy_driver *phydrv = to_phy_driver(drv);
> > 	int err = 0;
> >+	struct gpio_descs *reset_gpios;
> >
> > 	phydev->drv = phydrv;
> >
> >+	/* take phy out of reset */
> >+	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
> >+						    GPIOD_OUT_LOW);
> >+	if (IS_ERR(reset_gpios))
> >+		return PTR_ERR(reset_gpios);
> >+
> 
>    It would have been logical to assert back the reset GPIO in phy_remove(),

Not necessarily. If you want to save some energy it might make sense. If
you only focus on making the device work, ensuring that reset is not
applied is enough.

> no? Also, you'd need to store the GPIO permanently somewhere...

I need to store it, iff I want to do something with the reset line
later. Currently that's not the case.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |