Message-ID: <56EFEDAD.9030903@laposte.net>
Date: Mon, 21 Mar 2016 13:48:45 +0100
From: Sebastian Frias <sf84@laposte.net>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0
MIME-Version: 1.0
To: =?windows-1252?Q?Uwe_Kleine-K=F6nig?= 
	<u.kleine-koenig@pengutronix.de>,
        Daniel Mack <daniel@zonque.org>
CC: "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        lkml <linux-kernel@vger.kernel.org>, mason <slash.tmp@free.fr>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Mans Rullgard <mans@mansr.com>, Fabio Estevam <festevam@gmail.com>,
        Martin Blumenstingl <martin.blumenstingl@gmail.com>,
        Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB
References: <56E99727.9040702@laposte.net> <20160318125455.GN4292@pengutronix.de> <56EC2525.8000706@laposte.net> <20160318191242.GQ4292@pengutronix.de>
In-Reply-To: <20160318191242.GQ4292@pengutronix.de>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3376
Lines: 89

Hi Uwe,

On 03/18/2016 08:12 PM, Uwe Kleine-K?nig wrote:
> Hello Sebastian,
> 
> On Fri, Mar 18, 2016 at 04:56:21PM +0100, Sebastian Frias wrote:
>> On 03/18/2016 01:54 PM, Uwe Kleine-K?nig wrote:
>>> 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?
> 
> With reset gpios it might not be that critical, but consider an optional
> enable gpio. (Optional in the sense, that some device have it and others
> don't, e.g. because the pin is pulled into active level by hardware.)
> 
> Now you do:
> 
> 	gpiod = gpiod_get_optional("enable");
> 
> and if gpiod now is an error pointer, you must assume that you cannot
> operate the device. And even with GPIOLIB=n (and gpiod = ERR_PTR(-ENOSYS))
> you cannot ignore the error. 

Two things:
- I suppose that in such hypothetical case the dependency on GPIOLIB
would be required and thus be there
- We'd also need to check that 'gpiod' is not NULL

In the scenario at hand only some devices require a hack and
gpiod_reset=NULL is valid (ie: device will not fail probing)

>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.
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.

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.

As such, my proposal was to:
- keep my proposed patch
- make another patch to print a warning when gpiod_reset is NULL (which
can happen, even without my patch)

What do you think?

Best regards,

Sebastian