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

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

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