Message ID | 56E99727.9040702@laposte.net |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
[ 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; >
[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
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 >
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
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.
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
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.
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.
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
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
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
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
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
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
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
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
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 >>
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
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 >
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
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
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;
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(-)