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

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

[Sebastian Frias]

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@laposte.net>
---
 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))
 		return PTR_ERR(gpiod_reset);

 	priv->gpiod_reset = gpiod_reset;
-- 
2.1.4

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

[Mason]

[ CCing a few devs who might be interested ]

On 16/03/2016 18:25, 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@laposte.net>
> ---
>  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))
>  		return PTR_ERR(gpiod_reset);
> 
>  	priv->gpiod_reset = gpiod_reset;
> 

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

[Uwe Kleine-König]

[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@laposte.net>
> ---
>  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).

With 687908c2b649 I made kernels without GPIOLIB fail to bind this
device. I admit this is not maximally nice.

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Uwe Kleine-König]

[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@laposte.net>
> ---
>  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).

With 687908c2b649 I made kernels without GPIOLIB fail to bind this
device. I admit this is not maximally nice.

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Sebastian Frias]

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@laposte.net>
>> ---
>>  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
> 

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

[Uwe Kleine-König]

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. For consistency I'd recommend to do the
same for reset even though there is a chance to get a working device.

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Mason]

On 18/03/2016 20:12, Uwe Kleine-König wrote:

> On Fri, Mar 18, 2016 at 04:56:21PM +0100, Sebastian Frias wrote:
> 
>> 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 don't understand this statement.

What does it mean for a question to be obsolete?

Regards.

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

[Uwe Kleine-König]

Hello,

On Fri, Mar 18, 2016 at 08:31:20PM +0100, Mason wrote:
> On 18/03/2016 20:12, Uwe Kleine-König wrote:
> 
> > On Fri, Mar 18, 2016 at 04:56:21PM +0100, Sebastian Frias wrote:
> > 
> >> 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 don't understand this statement.
> 
> What does it mean for a question to be obsolete?

If the driver doesn't probe because it cannot control the reset line,
you don't need to think about how it should behave in
at803x_link_change_notify without control of the reset line, because
this code isn't reached then.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Mason]

On 18/03/2016 21:11, Uwe Kleine-König wrote:

> Hello,
> 
> On Fri, Mar 18, 2016 at 08:31:20PM +0100, Mason wrote:
>
>> On 18/03/2016 20:12, Uwe Kleine-König wrote:
>>
>>> On Fri, Mar 18, 2016 at 04:56:21PM +0100, Sebastian Frias wrote:
>>>
>>>> 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 don't understand this statement.
>>
>> What does it mean for a question to be obsolete?
> 
> If the driver doesn't probe because it cannot control the reset line,
> you don't need to think about how it should behave in
> at803x_link_change_notify without control of the reset line, because
> this code isn't reached then.

If I understand correctly, it is possible to soft-reset the PHY
by writing to a specific register. The GPIO pin is useful only to
force a hardware-reset when the PHY is wedged by some random event.

(Or am I completely off the mark?)

Regards.

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

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 18/03/2016 21:11, Uwe Kleine-König wrote:
>
>> Hello,
>> 
>> On Fri, Mar 18, 2016 at 08:31:20PM +0100, Mason wrote:
>>
>>> On 18/03/2016 20:12, Uwe Kleine-König wrote:
>>>
>>>> On Fri, Mar 18, 2016 at 04:56:21PM +0100, Sebastian Frias wrote:
>>>>
>>>>> 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 don't understand this statement.
>>>
>>> What does it mean for a question to be obsolete?
>> 
>> If the driver doesn't probe because it cannot control the reset line,
>> you don't need to think about how it should behave in
>> at803x_link_change_notify without control of the reset line, because
>> this code isn't reached then.
>
> If I understand correctly, it is possible to soft-reset the PHY
> by writing to a specific register. The GPIO pin is useful only to
> force a hardware-reset when the PHY is wedged by some random event.

Yes, and some variants of this phy are broken and require a hard reset
in certain situations.  At least that's what the comment in the code
says.

-- 
Måns Rullgård

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

[Sebastian Frias]

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

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

[Sebastian Frias]

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

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

[Uwe Kleine-König]

Hello Sebastian,

On Mon, Mar 21, 2016 at 01:48:45PM +0100, Sebastian Frias wrote:
> On 03/18/2016 08:12 PM, Uwe Kleine-König wrote:
> > 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

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

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

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

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

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

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

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Sebastian Frias]

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

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

[Uwe Kleine-König]

Hello Sebastian,

On Mon, Mar 21, 2016 at 04:36:47PM +0100, Sebastian Frias wrote:
> On 03/21/2016 02:54 PM, Uwe Kleine-König wrote:
> >> - 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.

Assuming the source for the hardware description is dt (the same
argument applies if it's ACPI or something else).

The driver uses devm_gpiod_get_optional(..."reset"...). That means some
hardware has a reset line, some don't. If a reset gpio specification is
there, that means the hardware has such a signal and it seems that means
it must not be ignored. If there is no reset gpio specification that
means that driver has to assume there is no reset line. (In the real
world there might be other reasons the reset line isn't in the device
tree, but it's not in the scope of the driver to guess why it's missing.
If it's not there the only sensible thing to assume for the driver is:
There is no reset line.)

So the conclusions are: If there is a reset line in dt, it must be used.
If you don't know if there is a reset line (because GPIOLIB=n) the
driver should not bind. Everything else yields to more problems than
good.

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

GPIOLIB=n means "the driver doesn't know if a reset line is
present/necessary", not having reset means "there is no reset line".

And don't do error handling by printk assuming anyone will read it. That
doesn't work.

> The fact that GPIOLIB=y does not means that the "reset" key will be there.

Right, but with GPIOLIB=y you know if it's there, and if yes, you can
control the line.

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

The driver must assume that the device tree is right. If it's not fix
the device tree, don't implement clumsy work arounds in the driver.

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

Then fix the device tree.

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

If you can detect by other means that "reset is necessary", then yes,
the driver should ideally also fail in this case with no reset specified
in dt.

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

Sometimes no driver is better than an unreliable one.

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

s/even//. And returning NULL is not an error. It means: There is no
reset line specified in dt.

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

NULL is not invalid. With devm_gpiod_get_optional the driver asks: "Is
there a reset gpio, and if yes, which one?". The reset line is optional,
that means "some devices have one, some others don't.". It does NOT mean
"You can ignore if there is a reset line or not."!

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

My expectation is that if the dt includes a reset property, it should be
used. So, there is a fine option for each situation:

 - device that doesn't need the hack => everything is fine;
 - AT8030 with reset to a gpio => specify the reset in dt;
 - AT8030 without a controllable reset line => don't specify a reset
   property;

if you have a broken device with a reset line you can even drop the
reset property from dt and not make use of the reset gpio if you think
it's a good idea.

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

  if (GPIOLIB=y):
    if (device has a reset property):
      driver runs fine with being able to reset.
    else:
      if (device needs reset to function properly):
        ideally fail to probe
      else:
        driver runs fine without reset and without the need
	  to bother the user about the absense of the gpio
  else:
    driver fails to bind

In no branch of these cases there is a situation where you must issue a
reset but cannot. So there is no need to ask if you should issue a
message each time this happens. That's what I meant with "the question
is obsolete".

> Unless I'm missing something, the only way the driver won't bind
> correctly with current code is if GPIOLIB=n

Right, because with GPIOLIB=n you cannot control a gpio line, but you
don't know if you must.

Consider you are an electrician (driver) and you should repair a power
socket (control a device) but forgot your phase tester (GPIOLIB=n).
So there is no way you can determine if it's save to modify the socket.
You can choose the optimistic way and say: "If there is no voltage on the
socket, I can repair it successfully" and just try it. Or you take the
safe approach and say: "I don't know if it's safe to do, so I better
don't.".

The same applies to drivers: If it might be necessary to handle a gpio,
but you cannot know if that's the case or not: Don't try it.

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

dt compatibility is fine. So if you want to handle the situation
properly, change the compatible and there make sure that the presence or
absence of the reset property matches reality with the new binding. And
continue to handle the old compatible the way you did before. But don't
ignore errors returned by gpiod_get et al.

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

You want GPIOLIB to know that you don't need to handle the reset gpio.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Sebastian Frias]

Hi Uwe,

I think we are in a deadlock :-)
I'm going to reply inline below, but I will also send a different email
to Daniel with a small recap.
I think he should share the intent of the "reset" mechanism he
introduced, in particular if it is mandatory.


On 03/21/2016 09:12 PM, Uwe Kleine-König wrote:
>>
>> 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.
> 
> Assuming the source for the hardware description is dt (the same
> argument applies if it's ACPI or something else).
> 
> The driver uses devm_gpiod_get_optional(..."reset"...). That means some
> hardware has a reset line, some don't. If a reset gpio specification is
> there, that means the hardware has such a signal and it seems that means
> it must not be ignored. 

The problem is all those "it seems" :-)
"it seems it must not be ignored" is also an hypothesis considering the
driver is not refusing to work even if it detects that the faulty PHY
has not been provided with a reset line.
Also, further down the thread you acknowledge that's a possibility.

>If there is no reset gpio specification that
> means that driver has to assume there is no reset line. (In the real
> world there might be other reasons the reset line isn't in the device
> tree, but it's not in the scope of the driver to guess why it's missing.
> If it's not there the only sensible thing to assume for the driver is:
> There is no reset line.)

But that is not what the current code does, the current code does not
checks if the DT presents the "reset" property directly.
It assumes that GPIOLIB works and checks GPIOLIB return codes, to
indirectly (and incorrectly) guess if a given device has a "reset" property.
It has not been clearly explained anywhere that the "reset" property for
AT8030 is supposed to be mandatory and that the driver should fail if
not provided with one.
The code does not back up such hypothesis either, ie: GPIOLIB=y +
missing "reset" will still allow the driver to work.

> 
> So the conclusions are: If there is a reset line in dt, it must be used.
> If you don't know if there is a reset line (because GPIOLIB=n) the
> driver should not bind. Everything else yields to more problems than
> good.

a) "If there is a reset line in dt, it must be used": that is an
hypothesis (I would say "it can be used");
b) if it were true, then another way of knowing if the "reset" key is
present should be used;
indeed, there should be:
c) a way to unambiguously determine if the "reset" key is in the DT
(regardless of GPIOLIB status);
that in order to know if the DT is specifying that the "reset" mechanism
must be used, then:
d) iff the "reset" is absolutely necessary (ie: the faulty PHY is in use
AND the original intention of the hack was for it *mandatory* for faulty
PHYs), some decision can be taken;

However, right now those conditions are not met.

>>> 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.
> 
> GPIOLIB=n means "the driver doesn't know if a reset line is
> present/necessary", not having reset means "there is no reset line".

That's the problem, that the driver is relying on GPIOLIB to know if:
- "reset" key is present
- it should fail to bind or not

and it does that based solely on GPIOLIB return codes, thus indirectly.
As opposed to checking directly the presence of the "reset" key
(regardless of GPIOLIB status) and failing immediately.

> 
> And don't do error handling by printk assuming anyone will read it. That
> doesn't work.

I proposed a warning, not error handling.
Also if we assume people won't read the logs, why put any at all?
Some people may not read the logs, some others will, it is their choice
to bang their heads one way or another.
Currently there's no warning.
So I had to debug why the driver was not binding, and it is not obvious
that GPIOLIB must be selected, considering:
- I don't have a faulty PHY
- I don't have a "reset" key on DT
- Even if I wanted, I don't have a GPIO to connect to it

(also, even if this driver does not bind, some generic one would take over)

> 
>> The fact that GPIOLIB=y does not means that the "reset" key will be there.
> 
> Right, but with GPIOLIB=y you know if it's there, and if yes, you can
> control the line.

Exactly, and what I say is that knowing if "reset" is there or not
should not depend on having GPIOLIB=y

To me the problem comes from the aliasing between "GPIOLIB=y" and
"reset" present.
Indeed, the driver should query if the "reset" key is present
(regardless of GPIOLIB status), and then try to acquire the GPIO line.
If it cannot get the reset GPIO, then it can give up saying "you told me
I need a reset line but then you did not give me one".

> 
>> 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.
> 
> The driver must assume that the device tree is right. If it's not fix
> the device tree, don't implement clumsy work arounds in the driver.

I doubt it is like that.
I mean, it may sound easy to say "just update the DT and add the 'reset'
key", but the DT is describing a physical connections in this case.
Changing that may not be as easy.

IMHO it would be far better if the driver made the check on the PHY ID
(8030) at probe time and refused to bind if:
- the "reset" is absolutely necessary per design (ie: intended by Daniel)
and
- it did not get a reset line (for whatever reason).

That was not done.
Maybe because it is not possible because the ID may not be known at that
time (strange since it is being probed);
Maybe because Daniel meant the whole thing to be optional;
Or maybe for other reasons.

> 
>> 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.
> 
> Then fix the device tree.

Changing the device tree does not automagically creates a connection
between a given GPIO (which must be available) and the PHY reset line.
GPIOs and reset lines are physical things.

But maybe I'm missing something and board designers always route GPIOs
to each and every peripheral's reset line.

> 
>> 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.
> 
> If you can detect by other means that "reset is necessary", then yes,
> the driver should ideally also fail in this case with no reset specified
> in dt.

Well, the problem is that right now we don't know what was the intention
of the person that implemented the "reset" mechanism.
Both of us are arguing about our own hypothesis.
You have yours "reset is necessary and driver should not bind without".
And I have mine "nothing in the code implies it is necessary".

> 
>> 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?
> 
> Sometimes no driver is better than an unreliable one.

It appears that if this driver does not binds, then a generic one (which
arguably will also lack the reset behaviour) will take over.
How can that be any better?

>>>
>>> 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
> 
> s/even//. And returning NULL is not an error. It means: There is no
> reset line specified in dt.

Indeed, and it is totally possible that the PHY ID is a faulty one, yet
the driver will bind anyway.
You argue about a case "reset is absolutely necessary for some devices",
but the code does not enforce that.

>>
>> I think somehow you try to make a difference on the reason why the
>> reset=INVALID (NULL or ERR), but IMHO there's none.
> 
> NULL is not invalid. With devm_gpiod_get_optional the driver asks: "Is
> there a reset gpio, and if yes, which one?". The reset line is optional,
> that means "some devices have one, some others don't.". It does NOT mean
> "You can ignore if there is a reset line or not."!

But the current code does ignore it :-)
Even for faulty devices.

Again, if you take a faulty PHY and a DT without the "reset" key, the
driver will bind.
Don't you think that goes against your hypothesis of "reset must be
present and available for some devices"?

By the way, is the "reset" key documented somewhere? How would the DT
look like?
Maybe in such documentation there's some explanation about the key being
compulsory or not.

> 
>> 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.
> 
> My expectation is that if the dt includes a reset property, it should be
> used. So, there is a fine option for each situation:
> 
>  - device that doesn't need the hack => everything is fine;
>  - AT8030 with reset to a gpio => specify the reset in dt;
>  - AT8030 without a controllable reset line => don't specify a reset
>    property;

You do realise that in this last case the device will work in degraded
mode, right?

> 
> if you have a broken device with a reset line you can even drop the
> reset property from dt and not make use of the reset gpio if you think
> it's a good idea.
> 

So you are saying that "reset" is optional, even for faulty PHYs.

>>
>> Ok, then I did not understand what you meant with "the question is
>> obsolete because the device doesn't probe".
> 
>   if (GPIOLIB=y):
>     if (device has a reset property):
>       driver runs fine with being able to reset.
>     else:
>       if (device needs reset to function properly):
>         ideally fail to probe
>       else:
>         driver runs fine without reset and without the need
> 	  to bother the user about the absense of the gpio
>   else:
>     driver fails to bind

To me it should be:

   if (device needs reset):
      if (DT has reset):
         if (GPIOLIB=y):
            gpiod=callback, driver gets a reset line and can operate
properly (ie: perform the hack)
         else
            gpiod=-ENOSYS, driver does not get reset line: should it
fail to bind? should it continue?
      else
         gpiod=NULL, driver does not get a reset line: should it fail to
bind? should it continue?
   else
      gpiod is not even requested, driver does not need a reset line and
can operate properly.

> 
> In no branch of these cases there is a situation where you must issue a
> reset but cannot. 

Well, the thing is that what you described does not match the code,
AFAIK current code does:

   if (GPIOLIB=y):
      if (DT has reset):
         gpiod=callback
         bind driver
         if (faulty PHY):
            use gpiod
      else
         gpiod=NULL
         bind driver
         if (faulty PHY)
            cannot use gpiod, but continue anyway without warning
   else
      gpiod=-ENOSYS, fail to bind, regardless of PHY being faulty, and
regardless of "reset" presence (*)


(*) the way the code is written, "reset" presence is not checked
directly, but as a side-effect of GPIOLIB

>So there is no need to ask if you should issue a
> message each time this happens. That's what I meant with "the question
> is obsolete".
> 
>> Unless I'm missing something, the only way the driver won't bind
>> correctly with current code is if GPIOLIB=n
> 
> Right, because with GPIOLIB=n you cannot control a gpio line, but you
> don't know if you must.

Well right now nobody knows if the "reset" is actually a must, even for
faulty PHYs.
The driver binds happily without, even on faulty devices.

> 
> Consider you are an electrician (driver) and you should repair a power
> socket (control a device) but forgot your phase tester (GPIOLIB=n).
> So there is no way you can determine if it's save to modify the socket.
> You can choose the optimistic way and say: "If there is no voltage on the
> socket, I can repair it successfully" and just try it. Or you take the
> safe approach and say: "I don't know if it's safe to do, so I better
> don't.".

:-) thanks for the analogy.

> 
> The same applies to drivers: If it might be necessary to handle a gpio,
> but you cannot know if that's the case or not: Don't try it.
> 

But that's not how the code was written.
It is not mentioned anywhere that that was the initial intention either.

>>
>> 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?
> 
> You want GPIOLIB to know that you don't need to handle the reset gpio.

But that's like saying "you want all possible drivers linked-in, if they
are not in DT they won't load/probe, so it's fine"

We don't want GPIOLIB because it is a bunch of useless code in our case.
We know in advance that the DT will not have the "reset" key.
We know in advance that the PHY is not faulty.
Why should we link-in a bunch of useless code then?

Alternatively, if GPIOLIB is mandatory due to the way drivers are
written or the way DT works, then why make it optional?

Best regards,

Sebastian

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

[Uwe Kleine-König]

Hello Sebastian,

On Tue, Mar 22, 2016 at 03:34:23PM +0100, Sebastian Frias wrote:
> I think we are in a deadlock :-)
> I'm going to reply inline below, but I will also send a different email
> to Daniel with a small recap.
> I think he should share the intent of the "reset" mechanism he
> introduced, in particular if it is mandatory.

The things I said in my mail are valid in general, not only for the
at803x phy.

Let me repeat them once more:

Preconditions:
 - Some of the devices a given driver handles have a reset line and
   others don't.
 - A non-empty subset (maybe all) of the devices that have a reset line
   require that this reset line is used.

Then the way to handle this in the driver should be done as follows:

  unless reset_handling_not_necessary():
    gpio = gpiod_get_optional("reset")
    if IS_ERR(gpio):
      return PTR_ERR(gpio)

Checking for -ENOSYS or GPIOLIB=n is not allowed because the device
you're currently handling might need the GPIO, so you must not continue
without the ability to control the line.

So the options you have (as you have a phy that doesn't need the reset
handling):

 - enable GPIOLIB (either in your .config or introduce a Kconfig
   dependency)
 - improve reset_handling_not_necessary() to return true for your case

There is nothing else.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Sebastian Frias]

Hi Uwe,

On 03/22/2016 08:42 PM, Uwe Kleine-König wrote:
> Hello Sebastian,
> 
> On Tue, Mar 22, 2016 at 03:34:23PM +0100, Sebastian Frias wrote:
>> I think we are in a deadlock :-)
>> I'm going to reply inline below, but I will also send a different email
>> to Daniel with a small recap.
>> I think he should share the intent of the "reset" mechanism he
>> introduced, in particular if it is mandatory.
> 
> The things I said in my mail are valid in general, not only for the
> at803x phy.
> 
> Let me repeat them once more:
> 
> Preconditions:
>  - Some of the devices a given driver handles have a reset line and
>    others don't.
>  - A non-empty subset (maybe all) of the devices that have a reset line
>    require that this reset line is used.
> 
> Then the way to handle this in the driver should be done as follows:
> 
>   unless reset_handling_not_necessary():
>     gpio = gpiod_get_optional("reset")
>     if IS_ERR(gpio):
>       return PTR_ERR(gpio)
> 
> Checking for -ENOSYS or GPIOLIB=n is not allowed because the device
> you're currently handling might need the GPIO, so you must not continue
> without the ability to control the line.
> 
> So the options you have (as you have a phy that doesn't need the reset
> handling):
> 
>  - enable GPIOLIB (either in your .config or introduce a Kconfig
>    dependency)
>  - improve reset_handling_not_necessary() to return true for your case
> 

I will see if I can "improve reset_handling_not_necessary() to return true".

Best regards,

Sebastian

[PATCH] net: phy: at803x: Request 'reset' GPIO only for AT8030 PHY

[Sebastian Frias]

This removes the dependency on GPIOLIB for non faulty PHYs.

Indeed, without this patch, if GPIOLIB is not selected
devm_gpiod_get_optional() will return -ENOSYS and the driver probe
call will fail, regardless of the actual PHY hardware.

Out of the 3 PHYs supported by this driver (AT8030, AT8031, AT8035),
only AT8030 presents the issues that commit 13a56b449325 ("net: phy:
at803x: Add support for hardware reset") attempts to work-around by
using a 'reset' GPIO line.

Hence, only AT8030 should depend on GPIOLIB operating properly.

Fixes: 13a56b449325 ("net: phy: at803x: Add support for hardware reset")

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
 drivers/net/phy/at803x.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 2174ec9..dcecf25 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -251,12 +251,16 @@ static int at803x_probe(struct phy_device *phydev)
 	if (!priv)
 		return -ENOMEM;

+	if (phydev->drv->phy_id != ATH8030_PHY_ID)
+		goto does_not_require_reset_workaround;
+
 	gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(gpiod_reset))
 		return PTR_ERR(gpiod_reset);

 	priv->gpiod_reset = gpiod_reset;

+does_not_require_reset_workaround:
 	phydev->priv = priv;

 	return 0;
-- 
2.1.4

Re: [PATCH] net: phy: at803x: Request 'reset' GPIO only for AT8030 PHY

[David Miller]

From: Sebastian Frias <sf84@laposte.net>
Date: Wed, 23 Mar 2016 11:49:09 +0100

> This removes the dependency on GPIOLIB for non faulty PHYs.
> 
> Indeed, without this patch, if GPIOLIB is not selected
> devm_gpiod_get_optional() will return -ENOSYS and the driver probe
> call will fail, regardless of the actual PHY hardware.
> 
> Out of the 3 PHYs supported by this driver (AT8030, AT8031, AT8035),
> only AT8030 presents the issues that commit 13a56b449325 ("net: phy:
> at803x: Add support for hardware reset") attempts to work-around by
> using a 'reset' GPIO line.
> 
> Hence, only AT8030 should depend on GPIOLIB operating properly.
> 
> Fixes: 13a56b449325 ("net: phy: at803x: Add support for hardware reset")
> 
> Signed-off-by: Sebastian Frias <sf84@laposte.net>

Applied.

Re: [PATCH] net: phy: at803x: Request 'reset' GPIO only for AT8030 PHY

[Sergei Shtylyov]

Hello.

On 03/23/2016 01:49 PM, Sebastian Frias wrote:

> This removes the dependency on GPIOLIB for non faulty PHYs.
>
> Indeed, without this patch, if GPIOLIB is not selected
> devm_gpiod_get_optional() will return -ENOSYS and the driver probe
> call will fail, regardless of the actual PHY hardware.
>
> Out of the 3 PHYs supported by this driver (AT8030, AT8031, AT8035),
> only AT8030 presents the issues that commit 13a56b449325 ("net: phy:
> at803x: Add support for hardware reset") attempts to work-around by
> using a 'reset' GPIO line.
>
> Hence, only AT8030 should depend on GPIOLIB operating properly.
>
> Fixes: 13a56b449325 ("net: phy: at803x: Add support for hardware reset")
>
> Signed-off-by: Sebastian Frias <sf84@laposte.net>
[...]

    What I don't understand is why the link_change_notify() method ptr is 
populated for all 3 supported chips while only being needed on 8030...

MBR, Sergei

Re: [PATCH] net: phy: at803x: Request 'reset' GPIO only for AT8030 PHY

[Sebastian Frias]

Hi Sergei,

On 03/23/2016 08:42 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 03/23/2016 01:49 PM, Sebastian Frias wrote:
> 
>> This removes the dependency on GPIOLIB for non faulty PHYs.
>>
>> Indeed, without this patch, if GPIOLIB is not selected
>> devm_gpiod_get_optional() will return -ENOSYS and the driver probe
>> call will fail, regardless of the actual PHY hardware.
>>
>> Out of the 3 PHYs supported by this driver (AT8030, AT8031, AT8035),
>> only AT8030 presents the issues that commit 13a56b449325 ("net: phy:
>> at803x: Add support for hardware reset") attempts to work-around by
>> using a 'reset' GPIO line.
>>
>> Hence, only AT8030 should depend on GPIOLIB operating properly.
>>
>> Fixes: 13a56b449325 ("net: phy: at803x: Add support for hardware reset")
>>
>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> [...]
> 
>    What I don't understand is why the link_change_notify() method ptr is
> populated for all 3 supported chips while only being needed on 8030...

You are right.
I had commented on that here
https://marc.info/?l=linux-netdev&m=145856582932498&w=2

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

I was waiting for comments from the original implementor.

However, I guess we can remove them as well.
I will make another patch.

Best regards,

Sebastian

Re: [PATCH] net: phy: at803x: Request 'reset' GPIO only for AT8030 PHY

[Sebastian Frias]

Hi Sergei,

>>    What I don't understand is why the link_change_notify() method ptr is
>> populated for all 3 supported chips while only being needed on 8030...
> 
> You are right.

I made the patch but I'm unsure about it because it could conflict with
yours.
I mean, I think you submitted a patch to change the GPIO handling on the
link_change_notify() function, right?

Well, if we only register the callback for the AT8030, then there is no
more need for the callback to check the PHY ID.
However, if I change that, the whole block moves as I remove one
indentation level (the one required by the PHY ID check).

Any suggestions on how to create a patch that won't conflict? I probably
need to use a tree that already has your patch applied.

Best regards,

Sebastian

Re: [PATCH] net: phy: at803x: Request 'reset' GPIO only for AT8030 PHY

[Sergei Shtylyov]

On 03/24/2016 01:10 PM, Sebastian Frias wrote:

>>>     What I don't understand is why the link_change_notify() method ptr is
>>> populated for all 3 supported chips while only being needed on 8030...
>>
>> You are right.
>
> I made the patch but I'm unsure about it because it could conflict with
> yours.
> I mean, I think you submitted a patch to change the GPIO handling on the
> link_change_notify() function, right?

> Well, if we only register the callback for the AT8030, then there is no
> more need for the callback to check the PHY ID.
> However, if I change that, the whole block moves as I remove one
> indentation level (the one required by the PHY ID check).
>
> Any suggestions on how to create a patch that won't conflict? I probably
> need to use a tree that already has your patch applied.

    My patch is already in Linus' tree, so should be merged back from net.git 
into net-next.git
soon. I suggest that you wait until net-next is open again.

> Best regards,
>
> Sebastian

MBR, Sergei

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

[Mason]

On 22/03/2016 20:42, Uwe Kleine-König wrote:

> Preconditions:
>  - Some of the devices a given driver handles have a reset line and
>    others don't.
>  - A non-empty subset (maybe all) of the devices that have a reset line
>    require that this reset line is used.
> 
> Then the way to handle this in the driver should be done as follows:
> 
>   unless reset_handling_not_necessary():
>     gpio = gpiod_get_optional("reset")
>     if IS_ERR(gpio):
>       return PTR_ERR(gpio)
> 
> Checking for -ENOSYS or GPIOLIB=n is not allowed because the device
> you're currently handling might need the GPIO, so you must not continue
> without the ability to control the line.
> 
> So the options you have (as you have a phy that doesn't need the reset
> handling):
> 
>  - enable GPIOLIB (either in your .config or introduce a Kconfig
>    dependency)
>  - improve reset_handling_not_necessary() to return true for your case
> 
> There is nothing else.

Here are some numbers for GPIOLIB, on an ARM build:

   text	   data	    bss	    dec	    hex	filename
   1830	      0	      0	   1830	    726	devres.o
    627	      0	      0	    627	    273	gpiolib-legacy.o
  11018	     40	      4	  11062	   2b36	gpiolib.o
   1598	      0	      0	   1598	    63e	gpiolib-of.o
--------------------------------------------------------
  15073	     40	      4	  15117	   3b0d	built-in.o

So ~15 kilobytes.


By the way, since the "reset-by-GPIO" solution is used only for
the Atheros 8030, would it be possible to make the
devm_gpiod_get_optional conditional on ATH8030_PHY_ID?

I'm thinking of something along these lines, for illustration:

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 2d020a3ec0b5..576e7873e049 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -198,12 +198,16 @@ static int at803x_probe(struct phy_device *phydev)
        if (!priv)
                return -ENOMEM;
 
+       if (phydev->drv->phy_id != ATH8030_PHY_ID)
+               goto no_gpio;
+
        gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
        if (IS_ERR(gpiod_reset))
                return PTR_ERR(gpiod_reset);
 
        priv->gpiod_reset = gpiod_reset;
 
+no_gpio:
        phydev->priv = priv;
 
        return 0;


Regards.

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

[Sergei Shtylyov]

Hello.

On 3/23/2016 1:17 PM, Mason wrote:

>> Preconditions:
>>   - Some of the devices a given driver handles have a reset line and
>>     others don't.
>>   - A non-empty subset (maybe all) of the devices that have a reset line
>>     require that this reset line is used.
>>
>> Then the way to handle this in the driver should be done as follows:
>>
>>    unless reset_handling_not_necessary():
>>      gpio = gpiod_get_optional("reset")
>>      if IS_ERR(gpio):
>>        return PTR_ERR(gpio)
>>
>> Checking for -ENOSYS or GPIOLIB=n is not allowed because the device
>> you're currently handling might need the GPIO, so you must not continue
>> without the ability to control the line.
>>
>> So the options you have (as you have a phy that doesn't need the reset
>> handling):
>>
>>   - enable GPIOLIB (either in your .config or introduce a Kconfig
>>     dependency)
>>   - improve reset_handling_not_necessary() to return true for your case
>>
>> There is nothing else.
>
> Here are some numbers for GPIOLIB, on an ARM build:
>
>     text	   data	    bss	    dec	    hex	filename
>     1830	      0	      0	   1830	    726	devres.o
>      627	      0	      0	    627	    273	gpiolib-legacy.o
>    11018	     40	      4	  11062	   2b36	gpiolib.o
>     1598	      0	      0	   1598	    63e	gpiolib-of.o
> --------------------------------------------------------
>    15073	     40	      4	  15117	   3b0d	built-in.o
>
> So ~15 kilobytes.
>
>
> By the way, since the "reset-by-GPIO" solution is used only for
> the Atheros 8030, would it be possible to make the
> devm_gpiod_get_optional conditional on ATH8030_PHY_ID?
>
> I'm thinking of something along these lines, for illustration:
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 2d020a3ec0b5..576e7873e049 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -198,12 +198,16 @@ static int at803x_probe(struct phy_device *phydev)
>          if (!priv)
>                  return -ENOMEM;
>
> +       if (phydev->drv->phy_id != ATH8030_PHY_ID)
> +               goto no_gpio;
> +
>          gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);

    We shouldn't call _optional() then, should we?

>          if (IS_ERR(gpiod_reset))
>                  return PTR_ERR(gpiod_reset);
>
>          priv->gpiod_reset = gpiod_reset;
>
> +no_gpio:
>          phydev->priv = priv;
>
>          return 0;
>
>
> Regards.

MBR, Sergei

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

[Sebastian Frias]

Hi Sergei,

On 03/23/2016 11:39 AM, Sergei Shtylyov wrote:
>>          gpiod_reset = devm_gpiod_get_optional(dev, "reset",
>> GPIOD_OUT_HIGH);
> 
>    We shouldn't call _optional() then, should we?

I could imagine the original intention was to be backward compatible.

Indeed, if this call is not optional, systems using AT8030 and lacking a
reset line on DT will have their behaviour affected.
NOTE: they would still work because even if this driver fails to bind a
generic one will take over.

Best regards,

Sebastian

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@laposte.net>
>>> ---
>>>  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
>>

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

[Sergei Shtylyov]

Hello.

On 03/16/2016 08:25 PM, 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@laposte.net>

    Do you have the PHY that requires the GPIO reset workaround?
Askinjg because I have the patch adding the "reset-gpios" prop handling to 
phylib and your patch made me aware that I'll have to modify this driver in 
order to do that...

> ---
>   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))
    		return PTR_ERR(gpiod_reset);

    My patch basically gets rid of all this code. The thing that worries me is 
that the driver assumes that the reset singal is active low, despite what the 
GPIO specifier in the device tree has for the GPIO polarity. In fact, it will 
only work correctly if the specified has GPIO_ACTIVE_HIGH -- which is wrong 
because the reset signal is active low!
    Can you point me to an actual device tree using this erratic PHY?

WBR, Sergei

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

[Uwe Kleine-König]

Hello Sergei,

On Mon, Mar 21, 2016 at 11:15:13PM +0300, Sergei Shtylyov wrote:
> On 03/16/2016 08:25 PM, 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@laposte.net>
> 
>    Do you have the PHY that requires the GPIO reset workaround?
> Askinjg because I have the patch adding the "reset-gpios" prop handling to
> phylib and your patch made me aware that I'll have to modify this driver in
> order to do that...
> 
> >---
> >  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))
>    		return PTR_ERR(gpiod_reset);
> 
>    My patch basically gets rid of all this code. The thing that worries me
> is that the driver assumes that the reset singal is active low, despite what
> the GPIO specifier in the device tree has for the GPIO polarity. In fact, it
> will only work correctly if the specified has GPIO_ACTIVE_HIGH -- which is
> wrong because the reset signal is active low!

Note that gpio descriptors handle the polarity just fine (i.e. the pin
is set to 0 after doing gpiod_set_value(1) if the gpio is active low).

But having said that, the driver gets it wrong.

The right sequence to reset a device using a gpio is:

	gpiod_set_value(priv->gpiod_reset, 1);
	msleep(some_time);
	gpiod_set_value(priv->gpiod_reset, 0);

and if the gpio is active low, this should be specified in the device
tree. This was done wrong in 13a56b449325 (net: phy: at803x: Add support
for hardware reset).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Sergei Shtylyov]

On 03/21/2016 11:41 PM, Uwe Kleine-König 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@laposte.net>
>>
>>     Do you have the PHY that requires the GPIO reset workaround?
>> Askinjg because I have the patch adding the "reset-gpios" prop handling to
>> phylib and your patch made me aware that I'll have to modify this driver in
>> order to do that...
>>
>>> ---
>>>   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))
>>     		return PTR_ERR(gpiod_reset);
>>
>>     My patch basically gets rid of all this code. The thing that worries me
>> is that the driver assumes that the reset singal is active low, despite what
>> the GPIO specifier in the device tree has for the GPIO polarity. In fact, it
>> will only work correctly if the specified has GPIO_ACTIVE_HIGH -- which is
>> wrong because the reset signal is active low!
>
> Note that gpio descriptors handle the polarity just fine (i.e. the pin
> is set to 0 after doing gpiod_set_value(1) if the gpio is active low).

    I know. :-)

> But having said that, the driver gets it wrong.
>
> The right sequence to reset a device using a gpio is:
>
> 	gpiod_set_value(priv->gpiod_reset, 1);
> 	msleep(some_time);
> 	gpiod_set_value(priv->gpiod_reset, 0);
>
> and if the gpio is active low, this should be specified in the device
> tree. This was done wrong in 13a56b449325 (net: phy: at803x: Add support
> for hardware reset).

    I wonder if that was done before GPIO_ACTIVE_* thing was introduced... 
there are precedents in other MAC drivers that want a separate 
"phy-reset-active-low" or even -"high" prop...

> Best regards
> Uwe

MBR, Sergei

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

[Sebastian Frias]

Hi,

On 03/21/2016 09:41 PM, Uwe Kleine-König wrote:
>>    My patch basically gets rid of all this code. The thing that worries me
>> is that the driver assumes that the reset singal is active low, despite what
>> the GPIO specifier in the device tree has for the GPIO polarity. In fact, it
>> will only work correctly if the specified has GPIO_ACTIVE_HIGH -- which is
>> wrong because the reset signal is active low!
> 
> Note that gpio descriptors handle the polarity just fine (i.e. the pin
> is set to 0 after doing gpiod_set_value(1) if the gpio is active low).
> 

Isn't that source of bugs?
What about using some #define (or probably better, an enum)?, something
like:

    gpiod_set_value(gpiod, GPIO_SET_VALUE_ACTIVE)
    gpiod_set_value(gpiod, GPIO_SET_VALUE_INACTIVE)
    gpiod_set_value(gpiod, GPIO_SET_VALUE_TRISTATE)

then, somebody reading the code would have to stop and think what do
these mean.
IIUC, currently the "0" or "1" can easily be confused with the actual
logical value of the GPIO.

gpiod_set_value() could also return an int with the actual value it
applied to the GPIO.
For example: if gpiod is active low, gpiod_set_value(gpiod,
GPIO_SET_VALUE_ACTIVE) would return 0;
Conversely, if gpiod is active high, gpiod_set_value(gpiod,
GPIO_SET_VALUE_ACTIVE) would return 1;

Best regards,

Sebastian

> But having said that, the driver gets it wrong.
> 
> The right sequence to reset a device using a gpio is:
> 
> 	gpiod_set_value(priv->gpiod_reset, 1);
> 	msleep(some_time);
> 	gpiod_set_value(priv->gpiod_reset, 0);
> 
> and if the gpio is active low, this should be specified in the device
> tree. This was done wrong in 13a56b449325 (net: phy: at803x: Add support
> for hardware reset).
> 
> Best regards
> Uwe
> 

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

[Sebastian Frias]

Hi Sergei,

On 03/21/2016 09:15 PM, Sergei Shtylyov wrote:
> 
>    Do you have the PHY that requires the GPIO reset workaround?

Unfortunately (or luckily :-) ) I don't have the faulty PHY, sorry.

Best regards,

Sebastian