From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753695AbcCUV4z (ORCPT ); Mon, 21 Mar 2016 17:56:55 -0400 Received: from mail-lb0-f181.google.com ([209.85.217.181]:36270 "EHLO mail-lb0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751331AbcCUV4y (ORCPT ); Mon, 21 Mar 2016 17:56:54 -0400 Subject: Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= References: <56E99727.9040702@laposte.net> <56F05651.3030706@cogentembedded.com> <20160321204111.GB6191@pengutronix.de> Cc: Sebastian Frias , "David S. Miller" , netdev@vger.kernel.org, lkml , mason , Daniel Mack From: Sergei Shtylyov Organization: Cogent Embedded Message-ID: <56F06E21.7010507@cogentembedded.com> Date: Tue, 22 Mar 2016 00:56:49 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160321204111.GB6191@pengutronix.de> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/21/2016 11:41 PM, Uwe Kleine-König wrote: >>> Commit 687908c2b649 ("net: phy: at803x: simplify using >>> devm_gpiod_get_optional and its 4th argument") introduced a dependency >>> on GPIOLIB that was not there before. >>> >>> This commit removes such dependency by checking the return code and >>> comparing it against ENOSYS which is returned when GPIOLIB is not >>> selected. >>> >>> Fixes: 687908c2b649 ("net: phy: at803x: simplify using >>> devm_gpiod_get_optional and its 4th argument") >>> >>> Signed-off-by: Sebastian Frias >> >> Do you have the PHY that requires the GPIO reset workaround? >> Askinjg because I have the patch adding the "reset-gpios" prop handling to >> phylib and your patch made me aware that I'll have to modify this driver in >> order to do that... >> >>> --- >>> drivers/net/phy/at803x.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c >>> index 2174ec9..88b7ff3 100644 >>> --- a/drivers/net/phy/at803x.c >>> +++ b/drivers/net/phy/at803x.c >>> @@ -252,7 +252,9 @@ static int at803x_probe(struct phy_device *phydev) >>> return -ENOMEM; >>> >>> gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); >>> - if (IS_ERR(gpiod_reset)) >>> + if (PTR_ERR(gpiod_reset) == -ENOSYS) >>> + gpiod_reset = NULL; >>> + else if (IS_ERR(gpiod_reset)) >> return PTR_ERR(gpiod_reset); >> >> My patch basically gets rid of all this code. The thing that worries me >> is that the driver assumes that the reset singal is active low, despite what >> the GPIO specifier in the device tree has for the GPIO polarity. In fact, it >> will only work correctly if the specified has GPIO_ACTIVE_HIGH -- which is >> wrong because the reset signal is active low! > > Note that gpio descriptors handle the polarity just fine (i.e. the pin > is set to 0 after doing gpiod_set_value(1) if the gpio is active low). I know. :-) > But having said that, the driver gets it wrong. > > The right sequence to reset a device using a gpio is: > > gpiod_set_value(priv->gpiod_reset, 1); > msleep(some_time); > gpiod_set_value(priv->gpiod_reset, 0); > > and if the gpio is active low, this should be specified in the device > tree. This was done wrong in 13a56b449325 (net: phy: at803x: Add support > for hardware reset). I wonder if that was done before GPIO_ACTIVE_* thing was introduced... there are precedents in other MAC drivers that want a separate "phy-reset-active-low" or even -"high" prop... > Best regards > Uwe MBR, Sergei