From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932578AbcEMVtL (ORCPT ); Fri, 13 May 2016 17:49:11 -0400 Received: from mail-lb0-f171.google.com ([209.85.217.171]:36460 "EHLO mail-lb0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932415AbcEMVtI (ORCPT ); Fri, 13 May 2016 17:49:08 -0400 Subject: Re: [PATCH RFT 1/2] phylib: add device reset GPIO support To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= References: <81129033.NXiOLTg1so@wasted.cogentembedded.com> <3641492.klKRrvS8tr@wasted.cogentembedded.com> <20160512184233.GJ30822@pengutronix.de> <20160513070620.GB28415@pengutronix.de> Cc: grant.likely@linaro.org, robh+dt@kernel.org, devicetree@vger.kernel.org, f.fainelli@gmail.com, netdev@vger.kernel.org, frowand.list@gmail.com, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, linux-kernel@vger.kernel.org, Linus Walleij From: Sergei Shtylyov Organization: Cogent Embedded Message-ID: Date: Sat, 14 May 2016 00:49:04 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <20160513070620.GB28415@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 Hello. On 05/13/2016 10:06 AM, Uwe Kleine-König wrote: [...] >>>> Index: net-next/drivers/of/of_mdio.c >>>> =================================================================== >>>> --- net-next.orig/drivers/of/of_mdio.c >>>> +++ net-next/drivers/of/of_mdio.c >>>> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n >>>> static void of_mdiobus_register_phy(struct mii_bus *mdio, >>>> struct device_node *child, u32 addr) >>>> { >>>> + struct gpio_desc *gpiod; >>>> struct phy_device *phy; >>>> bool is_c45; >>>> int rc; >>>> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru >>>> is_c45 = of_device_is_compatible(child, >>>> "ethernet-phy-ieee802.3-c45"); >>>> >>>> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); >>>> + /* Deassert the reset signal */ >>>> + if (!IS_ERR(gpiod)) >>>> + gpiod_direction_output(gpiod, 0); >>> >>> This is wrong I think. You must only ignore -ENODEV, all other error >> >> At least -ENOSYS should also be ignored (it's returned when gpiolib is >> not configured), right? > > No, that's a common misconception. If GPIOLIB is off you cannot > determine if dt specified a reset gpio. So you have the choice between: > > a) ignore -ENOSYS and so don't handle the reset line in the cases where > it's necessary probably yielding an "Error: phy not found" message. > b) fail to probe even if a reset handling isn't necessary, yielding > "Error: failed to get hold of reset gpio". > > I say b) is the better one. It's easier to debug because the error > message is better, GPIOLIB is enabled in most cases anyhow (still maybe > select it?) and it's ensured that we're operating in the limits of the > hardware specs (maybe a phy returns a random value when a register is > read while reset is applied?). It returns all ones in my case. >> When does -ENODEV gets returned (it's not easy to follow)? > > I don't know for sure for fwnode_get_named_gpiod, but the gpiod_get*() > family returns -ENODEV if the node doesn't have a reset-gpio property. Are you sure it's not -ENOENT? >>> codes should be passed to the caller. >> >> The caller doesn't care anyway... >> >>> (I see that's not trivial because >>> of_mdiobus_register_phy returns void.) >> >> I've made this function *void* in net-next. > > I'd say this is a step in the wrong direction. For example this makes it > impossible to handle the case where the phy should be probed, the gpio > driver isn't available yet, though. Normally you'd want to return > -EPROBE_DEFER in this case and retry probing later. Well, of_mdiobus_register() is not an easy function to add the checks whiuch were never there (and undo the done stuff on failure). I'll see what I can do but no promises... >>> In my patch I used devm_gpiod_get_array which has the nice property that >>> I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime >>> of the gpio to the device which is nice and IMHO the right direction for >>> the phylib (i.e. better embracing of the device model). >>> >>> This cannot be used here easily however because there is no struct >>> device yet and this is only created after the phy id is determined. >> >> Your last patch [1] didn't make use of the managed device API (devm) >> either, I didn't quite get to the bottom of that... > > Right, devm didn't work. My patch was a prototype for a way that allowed > to bind the lifetime of the gpio to the device. This might be longer > than the call to mdiobus_unregister. Ah, that was the reason... Well, then you hardly achieved anything with rehashing the code... > See the problems that i2c handles > because it doesn't handle lifetimes correctly in drivers/i2c/i2c-core.c > at the end of i2c_del_adapter. > >>> The phy id is either read from the device tree or must be read from >>> the phy which might fail if reset is not deasserted. >> >>> Principally there is no reason however that the phy_id must be known >>> before the struct device is created however. >> >> It's just that the code is cleaner that way... > > I don't agree, I don't see that > > determine_phyid() > allocate_device() > > is better than > > allocate_device() > determine_phyid() This is an oversimplified view. Actually, it is: error = determine_phyid() if (error) return allocate_device() versus allocate_device() error = determine_phyid() if (error) free_device() > . The former is maybe easier because that's the status quo and it > doesn't need patching. But IMHO the result is on a similar level of > cleanliness. I disagree. And I don't see why it's necessary at all. Just to use another gpiolib API? > Best regards > Uwe MBR, Sergei