| Message ID | 56F274A5.6030502@laposte.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Sebastian Frias <sf84@laposte.net> Date: Wed, 23 Mar 2016 11:49:09 +0100 > This removes the dependency on GPIOLIB for non faulty PHYs. > > Indeed, without this patch, if GPIOLIB is not selected > devm_gpiod_get_optional() will return -ENOSYS and the driver probe > call will fail, regardless of the actual PHY hardware. > > Out of the 3 PHYs supported by this driver (AT8030, AT8031, AT8035), > only AT8030 presents the issues that commit 13a56b449325 ("net: phy: > at803x: Add support for hardware reset") attempts to work-around by > using a 'reset' GPIO line. > > Hence, only AT8030 should depend on GPIOLIB operating properly. > > Fixes: 13a56b449325 ("net: phy: at803x: Add support for hardware reset") > > Signed-off-by: Sebastian Frias <sf84@laposte.net> Applied.
Hello. On 03/23/2016 01:49 PM, Sebastian Frias wrote: > This removes the dependency on GPIOLIB for non faulty PHYs. > > Indeed, without this patch, if GPIOLIB is not selected > devm_gpiod_get_optional() will return -ENOSYS and the driver probe > call will fail, regardless of the actual PHY hardware. > > Out of the 3 PHYs supported by this driver (AT8030, AT8031, AT8035), > only AT8030 presents the issues that commit 13a56b449325 ("net: phy: > at803x: Add support for hardware reset") attempts to work-around by > using a 'reset' GPIO line. > > Hence, only AT8030 should depend on GPIOLIB operating properly. > > Fixes: 13a56b449325 ("net: phy: at803x: Add support for hardware reset") > > Signed-off-by: Sebastian Frias <sf84@laposte.net> [...] What I don't understand is why the link_change_notify() method ptr is populated for all 3 supported chips while only being needed on 8030... MBR, Sergei
Hi Sergei, On 03/23/2016 08:42 PM, Sergei Shtylyov wrote: > Hello. > > On 03/23/2016 01:49 PM, Sebastian Frias wrote: > >> This removes the dependency on GPIOLIB for non faulty PHYs. >> >> Indeed, without this patch, if GPIOLIB is not selected >> devm_gpiod_get_optional() will return -ENOSYS and the driver probe >> call will fail, regardless of the actual PHY hardware. >> >> Out of the 3 PHYs supported by this driver (AT8030, AT8031, AT8035), >> only AT8030 presents the issues that commit 13a56b449325 ("net: phy: >> at803x: Add support for hardware reset") attempts to work-around by >> using a 'reset' GPIO line. >> >> Hence, only AT8030 should depend on GPIOLIB operating properly. >> >> Fixes: 13a56b449325 ("net: phy: at803x: Add support for hardware reset") >> >> Signed-off-by: Sebastian Frias <sf84@laposte.net> > [...] > > What I don't understand is why the link_change_notify() method ptr is > populated for all 3 supported chips while only being needed on 8030... You are right. I had commented on that here https://marc.info/?l=linux-netdev&m=145856582932498&w=2 <quote> Furthermore, I think we should not even register the "link_change_notify" callback when dealing with AT803x PHYs that do not require the hack. Another solution (considering the callback is statically registered in the "phy_driver" structs for all AT803x PHYs) would be for the callback to disable itself. </quote> I was waiting for comments from the original implementor. However, I guess we can remove them as well. I will make another patch. Best regards, Sebastian
Hi Sergei, >> What I don't understand is why the link_change_notify() method ptr is >> populated for all 3 supported chips while only being needed on 8030... > > You are right. I made the patch but I'm unsure about it because it could conflict with yours. I mean, I think you submitted a patch to change the GPIO handling on the link_change_notify() function, right? Well, if we only register the callback for the AT8030, then there is no more need for the callback to check the PHY ID. However, if I change that, the whole block moves as I remove one indentation level (the one required by the PHY ID check). Any suggestions on how to create a patch that won't conflict? I probably need to use a tree that already has your patch applied. Best regards, Sebastian
On 03/24/2016 01:10 PM, Sebastian Frias wrote: >>> What I don't understand is why the link_change_notify() method ptr is >>> populated for all 3 supported chips while only being needed on 8030... >> >> You are right. > > I made the patch but I'm unsure about it because it could conflict with > yours. > I mean, I think you submitted a patch to change the GPIO handling on the > link_change_notify() function, right? > Well, if we only register the callback for the AT8030, then there is no > more need for the callback to check the PHY ID. > However, if I change that, the whole block moves as I remove one > indentation level (the one required by the PHY ID check). > > Any suggestions on how to create a patch that won't conflict? I probably > need to use a tree that already has your patch applied. My patch is already in Linus' tree, so should be merged back from net.git into net-next.git soon. I suggest that you wait until net-next is open again. > Best regards, > > Sebastian MBR, Sergei
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index 2174ec9..dcecf25 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -251,12 +251,16 @@ static int at803x_probe(struct phy_device *phydev) if (!priv) return -ENOMEM; + if (phydev->drv->phy_id != ATH8030_PHY_ID) + goto does_not_require_reset_workaround; + gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(gpiod_reset)) return PTR_ERR(gpiod_reset); priv->gpiod_reset = gpiod_reset; +does_not_require_reset_workaround: phydev->priv = priv; return 0;
This removes the dependency on GPIOLIB for non faulty PHYs. Indeed, without this patch, if GPIOLIB is not selected devm_gpiod_get_optional() will return -ENOSYS and the driver probe call will fail, regardless of the actual PHY hardware. Out of the 3 PHYs supported by this driver (AT8030, AT8031, AT8035), only AT8030 presents the issues that commit 13a56b449325 ("net: phy: at803x: Add support for hardware reset") attempts to work-around by using a 'reset' GPIO line. Hence, only AT8030 should depend on GPIOLIB operating properly. Fixes: 13a56b449325 ("net: phy: at803x: Add support for hardware reset") Signed-off-by: Sebastian Frias <sf84@laposte.net> --- drivers/net/phy/at803x.c | 4 ++++ 1 file changed, 4 insertions(+)