Date:	Mon, 21 Mar 2016 16:36:47 +0100
From:	Sebastian Frias <sf84@...oste.net>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
CC:	Daniel Mack <daniel@...que.org>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	lkml <linux-kernel@...r.kernel.org>, mason <slash.tmp@...e.fr>,
	Florian Fainelli <f.fainelli@...il.com>,
	Mans Rullgard <mans@...sr.com>,
	Fabio Estevam <festevam@...il.com>,
	Martin Blumenstingl <martin.blumenstingl@...il.com>,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB

Hi Uwe,

On 03/21/2016 02:54 PM, Uwe Kleine-König wrote:
>>
>> Two things:
>> - I suppose that in such hypothetical case the dependency on GPIOLIB
>>   would be required and thus be there
> 
> I don't agree. There are bugs out there, and maybe the reason is as
> simple as "the implementor of the reset-gpio handling had GPIOLIB on and
> didn't test without GPIOLIB".

:-) fair enough, let's wait for his comments then.

> 
>> - We'd also need to check that 'gpiod' is not NULL
> 
> That is the optional part. If gpiod is NULL, you have one of the devices
> that don't need to handle the enable line.

gpiod=NULL (because the key is not there) or gpiod=ERR (because
GPIOLIB=n + my patch) will result in the same behaviour, ie: driver
binds, but fails to reset.

> 
>> In the scenario at hand only some devices require a hack and
>> gpiod_reset=NULL is valid (ie: device will not fail probing)
> 
> With your patch and GPIOLIB=n you bind the driver even for the devices
> that need the hack. This is broken!

It is a degraded mode I agree and had proposed to print a warning.
However, I fail to see how is GPIOLIB=n different from just not having
"reset" present.

The fact that GPIOLIB=y does not means that the "reset" key will be there.
I mean, you assume that "reset" will be present for devices that need
the hack, yet there is no such guarantee and the code clearly assumes
that it can be missing.
For all we know, the key is actually missing on systems that do use the
faulty PHY (all systems designed prior to the hack being implemented)
And that, regardless of GPIOLIB status.

Since the reset line can be missing, it can be missing for whatever reason.
Whether it is because the "reset" key is not present or because
GPIOLIB=n, it does not matter, it will result on the same outcome.

Furthermore, if "reset" is really required for certain devices that need
the hack, then both cases:
- GPIOLIB=n
- "reset" not present
should be handled the same way (ie: not bind the driver) for such devices.

By the way, I think not binding the driver is too strong too.
Having a GPIO free and being able to route it to the PHY for reset may
not even be possible on some systems.
Are they supposed to stop working?

> 
>>> For consistency I'd recommend to do the
>>> same for reset even though there is a chance to get a working device.
>>
>> I'm not so sure, because then information would be lost, handling both
>> ("enable" and "reset") the same way aliases different intents and
>> requirements.
>> Indeed, only "enable" would be really mandatory, "reset" is essentially
>> optional (only 1 of 3 devices of the family require the hack).
>>
>> Besides, if "reset" was really mandatory, we would also fail the probe
>> if the pointer for the reset GPIO is NULL.
> 
> No, if reset was mandatory you'd use gpiod_get instead of
> gpiod_get_optional and fail iff that call fails, too.

Ok, but the current code for "reset" is using devm_gpiod_get_optional()
so "reset" is clearly optional.
And that call will return NULL if "reset" is not present, even with
GPIOLIB=y

> 
>> Indeed, even with GPIOLIB=y the pointer can be NULL if the "reset" (or
>> "enable" in your scenario) is not present.
>> From a functionality perspective, a NULL pointer for "reset" will result
>> in the same behaviour as GPIOLIB=n, ie: not being able to reset the PHY.
> 
> Right, but you only get reset=NULL iff the device tree (or whatever is
> the source of information for gpiod_get) doesn't specify a reset gpio
> which then means "This device doesn't need a reset gpio.". This is
> different from the GPIOLIB=n case, where the return code signals "It's
> unknown if the device needs a reset gpio.".

I think somehow you try to make a difference on the reason why the
reset=INVALID (NULL or ERR), but IMHO there's none.

If somebody using AT8030 PHY (the one that requires the hack) either
does not configure a "reset" key on the DT, either does not enable
GPIOLIB, the result will be the same.
There is no warning in either case and it will run on a degraded mode.

> 
>> So, the current code (your commit) and previous code (Daniel's commit)
>> clearly points to "reset" being optional.
>>
>> Furthermore, I think we should not even register the
>> "link_change_notify" callback when dealing with AT803x PHYs that do not
>> require the hack.
>> Another solution (considering the callback is statically registered in
>> the "phy_driver" structs for all AT803x PHYs) would be for the callback
>> to disable itself.
>> Once it detects that the PHY does not require the hack, it could just
>> perform:
>>
>>     phydev->drv->link_change_notify = NULL;
>>
>> or call a new generic function to do the same if encapsulation is required.
>>
>> If everybody agrees I can make such change as well, but I really think
>> Daniel should give his opinion first.
>>
>>>
>>>> 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?
>>>
>>> Then this question is obsolete because the device doesn't probe.
>>
>> I think you assume that "reset" is mandatory for all AT803x devices, but
>> that's not what the code says.
> 
> No, you're wrong here. I'm aware that the code supports devices without
> reset.

Ok, then I did not understand what you meant with "the question is
obsolete because the device doesn't probe".
Unless I'm missing something, the only way the driver won't bind
correctly with current code is if GPIOLIB=n

Systems using the faulty PHY and having GPIOLIB=y but with an outdated
DT that does not has a "reset" key would have the PHY driver bind yet
have it fail due to missing "reset".

> 
>> As such, my proposal was to:
>> - keep my proposed patch
> 
> I don't agree.
> 
>> - make another patch to print a warning when gpiod_reset is NULL (which
>>   can happen, even without my patch)
> 
> This only happens if no reset gpio is needed and in this case the driver
> does the right thing. So if you ask me, there is no need to modify the
> driver. Better add a dependency to GPIOLIB. This is necessary even for
> devices which don't need a reset gpio to answer the question "does this
> driver need a reset gpio?" correctly.

I don't see how the dependency on GPIOLIB=y improves the situation in
any useful way.

To put an example: in our case we don't use the faulty PHY.
So, the DT does not has the "reset" key.
Why should then the PHY driver be dependent on GPIOLIB?

Best regards,

Sebastian

Powered by blists - more mailing lists