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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
