Messages in this thread | | | Subject | Re: [PATCH RFT 1/2] phylib: add device reset GPIO support | From | Sergei Shtylyov <> | Date | Sat, 14 May 2016 00:49:04 +0300 |
| |
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
| |