net: phy: at803x: don't depend on GPIOLIB

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 <[email protected]>

---
 drivers/net/phy/at803x.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/phy/at803x.c +3 -1

index 2174ec9..88b7ff3 100644
@@ -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;

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 <[email protected]>

---
 drivers/net/phy/at803x.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/phy/at803x.c +0 -0

index 2174ec9..88b7ff3 100644

[expand cc a bit more]

Hello,

Quoted 22 lines · 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 <[email protected]>

---
 drivers/net/phy/at803x.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/phy/at803x.c +0 -0

index 2174ec9..88b7ff3 100644
@@ -252,7 +252,9 @@ static int at803x_probe(struct phy_device *phydev)
 		return -ENOMEM;
 

drivers/net/phy/at803x.c static int at803x_probe(struct phy_device *phydev) · line 257

Quoted 5 lines

drivers/net/phy/at803x.c +3 -1

 	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

Quoted 5 lines · Uwe Kleine-König

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?

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?

Quoted 2 lines · Sebastian Frias

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.

Quoted 2 lines · Uwe Kleine-König

On Fri, Mar 18, 2016 at 04:56:21PM +0100, Sebastian Frias wrote:

Quoted 4 lines

Quoted 2 lines

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?

Hello,

Quoted 9 lines · Mason

Quoted 6 lines · Uwe Kleine-König

Quoted 2 lines · Sebastian Frias

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.

Quoted 3 lines

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.

Quoted 17 lines · Uwe Kleine-König

Hello,

Quoted 12 lines · Mason

Quoted 6 lines · Uwe Kleine-König

Quoted 2 lines · Sebastian Frias

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?

Quoted 4 lines

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.

Hello Sebastian,

Quoted 8 lines · Sebastian Frias

Quoted 5 lines · Uwe Kleine-König

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.

Quoted 3 lines

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.

Quoted 18 lines · Uwe Kleine-König

Quoted 8 lines · Sebastian Frias

Quoted 5 lines · Uwe Kleine-König

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");

Quoted 3 lines

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)

Quoted 2 lines

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.

Quoted 1 line

Quoted 4 lines

Quoted 2 lines

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)

Quoted 4 lines

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

Quoted 2 lines

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

Quoted 3 lines

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!

Quoted 10 lines

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 5 lines

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

Quoted 24 lines

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.

Quoted 4 lines

Quoted 3 lines

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?

Quoted 4 lines

Quoted 1 line

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.

Quoted 3 lines

As such, my proposal was to:
- keep my proposed patch

I don't agree.

Quoted 2 lines

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

Quoted 4 lines · Uwe Kleine-König

Quoted 1 line

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

Quoted 5 lines

Quoted 2 lines

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?

Quoted 12 lines

Quoted 11 lines

Quoted 2 lines

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.

Quoted 2 lines

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

Quoted 5 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 5 lines

Quoted 2 lines

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

Quoted 8 lines

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 10 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 5 lines

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.

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 4 lines

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.

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.

Hello Sebastian,

Quoted 5 lines · Sebastian Frias

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

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 <[email protected]>

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 <[email protected]>

Hello.

Quoted 13 lines · 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.

Quoted 3 lines

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

Signed-off-by Sebastian Frias <[email protected]>

[...]

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

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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

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.

Quoted 17 lines

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.

Quoted 8 lines

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:

drivers/net/phy/at803x.c +0 -0

index 2d020a3ec0b5..576e7873e049 100644

drivers/net/phy/at803x.c +1 -0

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

Regards.

Quoted 2 lines

gpiod_reset = devm_gpiod_get_optional(dev, "reset",
GPIOD_OUT_HIGH);

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

Hi Uwe, Daniel,

Quoted 27 lines · Uwe Kleine-König

[expand cc a bit more]

Hello,

Quoted 22 lines · 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 <[email protected]>

---
 drivers/net/phy/at803x.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/phy/at803x.c +0 -0

index 2174ec9..88b7ff3 100644
@@ -252,7 +252,9 @@ static int at803x_probe(struct phy_device *phydev)
 		return -ENOMEM;
 

Quoted 8 lines

drivers/net/phy/at803x.c static int at803x_probe(struct phy_device *phydev) · line 257

Quoted 5 lines

drivers/net/phy/at803x.c +3 -1

 	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)

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

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

Quoted 4 lines

Best regards
Uwe

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 <[email protected]>

---
  drivers/net/phy/at803x.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/phy/at803x.c +0 -0

index 2174ec9..88b7ff3 100644

Quoted 4 lines · 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.

Quoted 8 lines

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 <[email protected]>

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

Quoted 9 lines

---
 drivers/net/phy/at803x.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/phy/at803x.c +0 -0

index 2174ec9..88b7ff3 100644

drivers/net/phy/at803x.c static int at803x_probe(struct phy_device *phydev) · line 257

Quoted 7 lines

drivers/net/phy/at803x.c +3 -1

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

Quoted 36 lines

Quoted 4 lines

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.

Quoted 8 lines

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 <[email protected]>

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

Quoted 9 lines

---
  drivers/net/phy/at803x.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/phy/at803x.c +0 -0

index 2174ec9..88b7ff3 100644

drivers/net/phy/at803x.c static int at803x_probe(struct phy_device *phydev) · line 257

Quoted 7 lines

drivers/net/phy/at803x.c +3 -1

@@ -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);

Quoted 5 lines

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

Quoted 5 lines

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

Do you have the PHY that requires the GPIO reset workaround?