Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB

[Sebastian Frias]

Hi Daniel,

Would you mind commenting on this thread?
Uwe and I are in a sort of deadlock because we each have a different
understanding of the intention of your commit 13a56b449325.

Basically the questions are:
- What is the intention of 13a56b449325?
- Did you mean for "reset" to be mandatory for faulty PHYs?
Mandatory meaning that you do not want the driver to work without.
- What do you think about the dependency on GPIOLIB?
You did not introduced such dependency with your change so it may point
to "reset" not being mandatory.

Best regards,

Sebastian

On 03/18/2016 04:56 PM, Sebastian Frias wrote:
> Hi Uwe, Daniel,
> 
> On 03/18/2016 01:54 PM, Uwe Kleine-König wrote:
>> [expand cc a bit more]
>>
>> Hello,
>>
>> On Wed, Mar 16, 2016 at 06:25:59PM +0100, Sebastian Frias wrote:
>>> Commit 687908c2b649 ("net: phy: at803x: simplify using
>>> devm_gpiod_get_optional and its 4th argument") introduced a dependency
>>> on GPIOLIB that was not there before.
>>>
>>> This commit removes such dependency by checking the return code and
>>> comparing it against ENOSYS which is returned when GPIOLIB is not
>>> selected.
>>>
>>> Fixes: 687908c2b649 ("net: phy: at803x: simplify using devm_gpiod_get_optional and its 4th argument")
>>>
>>> Signed-off-by: Sebastian Frias <sf84@xxxxxxxxxxx>
>>> ---
>>>  drivers/net/phy/at803x.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>>> index 2174ec9..88b7ff3 100644
>>> --- a/drivers/net/phy/at803x.c
>>> +++ b/drivers/net/phy/at803x.c
>>> @@ -252,7 +252,9 @@ static int at803x_probe(struct phy_device *phydev)
>>>  		return -ENOMEM;
>>>
>>>  	gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>>> -	if (IS_ERR(gpiod_reset))
>>> +	if (PTR_ERR(gpiod_reset) == -ENOSYS)
>>> +		gpiod_reset = NULL;
>>> +	else if (IS_ERR(gpiod_reset))
>>
>> this isn't better either (IMHO it's worse, but maybe this is debatable
>> and also depends on your POV).
> 
> Well, from the code, the reset hack is only required when the PHY is
> ATH8030_PHY_ID, right?
> However, such change was introduced by Daniel Mack on commit 13a56b449325.
> Hopefully he can chime in and give his opinion on this.
> 
> Daniel, while on the subject, I have a question for you:
> 
> Change 2b8f2a28eac1 introduced "link_status_notify" callback which is
> called systematically on the PHY state machine.
> Any reason to make the call systematic as opposed to let say:
> 
> 	if (phydev->state != old_state) {
> 		if (phydev->drv->link_change_notify)
> 			phydev->drv->link_change_notify(phydev);
> 	}
> 
> (it does means that the callback would be called after the state machine
> processing though)
> 
>>
>> With 687908c2b649 I made kernels without GPIOLIB fail to bind this
>> device. I admit this is not maximally nice.
> 
> I see, that was not clear from the commit message, sorry.
> 
>>
>> Your change makes the driver bind in this case again and then the reset
>> gpio isn't handled at all which might result in a later and harder to
>> debug error.
>>
>> The better approach to fix your problem is: make the driver depend (or
>> force) on GPIOLIB. 
> 
> It was one of the solutions I had in mind, but:
> - since the dependency on GPIOLIB was not included on the patch
> - and given that the previous code had provision to work without GPIO
> I thought it was an overlook.
> 
>> Or alternatively, drop reset-handling from the
>> driver.
>>
>> From a driver perspecitive, it would be nice if devm_gpiod_get_optional
>> returned NULL iff the respective gpio isn't specified even with
>> GPIOLIB=n, but this isn't sensible either because it would result in
>> quite some gpiolib code to not being conditionally compiled on
>> CONFIG_GPIOLIB any more.
> 
> Let's say that was the case, what would the PHY code do?
> 
> I mean, it did not get a GPIO, whether it was because GPIOLIB=n or
> because there's no 'reset' GPIO attached
> Shall it fail? Shall it continue in a sort of degraded mode? Shall it warn?
> Because that's the real question here.
> 
> What would you think of making at803x_link_change_notify() print a
> message every time it should do a reset but does not has a way to do it?
> I can make such change to my patch if everybody agrees on it.
> By the way, in that case, can somebody suggest a way to print such warning?
> Would printk() be ok or should I use dev_dbg() ?
> 
> Best regards,
> 
> Sebastian
> 
>>
>> Best regards
>> Uwe
>>