Re: [PATCH RFT 1/2] phylib: add device reset GPIO support

[Sergei Shtylyov]

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