lkml.org 
[lkml]   [2016]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH RFT 1/2] phylib: add device reset GPIO support
From
Date
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

\
 
 \ /
  Last update: 2016-05-14 00:01    [W:0.126 / U:0.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site