[PATCH net-next 0/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt

[PATCH net-next 0/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt

[Heiner Kallweit]

Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
using interrupts isn't possible if a driver defines neither
config_intr nor ack_interrupts callback. So we can replace checking
flag PHY_HAS_INTERRUPT with checking for these callbacks.

This allows to remove this flag from a lot of driver configs, let's
start with the Realtek driver.

Heiner Kallweit (2):
  net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt
  net: phy: realtek: remove flag PHY_HAS_INTERRUPT from driver configs

 drivers/net/phy/phy_device.c | 5 +++--
 drivers/net/phy/realtek.c    | 7 -------
 2 files changed, 3 insertions(+), 9 deletions(-)

-- 
2.19.1

[PATCH net-next 1/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt

[Heiner Kallweit]

Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
using interrupts isn't possible if a driver defines neither
config_intr nor ack_interrupts callback. So we can replace checking
flag PHY_HAS_INTERRUPT with checking for these callbacks.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d165a2c82..33e51b955 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2104,8 +2104,9 @@ static int phy_probe(struct device *dev)
 	/* Disable the interrupt if the PHY doesn't support it
 	 * but the interrupt is still a valid one
 	 */
-	if (!(phydrv->flags & PHY_HAS_INTERRUPT) &&
-	    phy_interrupt_is_valid(phydev))
+	 if (!phydrv->config_intr &&
+	     !phydrv->ack_interrupt &&
+	     phy_interrupt_is_valid(phydev))
 		phydev->irq = PHY_POLL;
 
 	if (phydrv->flags & PHY_IS_INTERNAL)
-- 
2.19.1

Re: [PATCH net-next 1/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt

[Florian Fainelli]

On 11/8/18 1:55 PM, Heiner Kallweit wrote:
> Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
> using interrupts isn't possible if a driver defines neither
> config_intr nor ack_interrupts callback. So we can replace checking
> flag PHY_HAS_INTERRUPT with checking for these callbacks.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy_device.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index d165a2c82..33e51b955 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2104,8 +2104,9 @@ static int phy_probe(struct device *dev)
>  	/* Disable the interrupt if the PHY doesn't support it
>  	 * but the interrupt is still a valid one
>  	 */
> -	if (!(phydrv->flags & PHY_HAS_INTERRUPT) &&
> -	    phy_interrupt_is_valid(phydev))
> +	 if (!phydrv->config_intr &&
> +	     !phydrv->ack_interrupt &&
> +	     phy_interrupt_is_valid(phydev))
>  		phydev->irq = PHY_POLL;

I would introduce an inline helper function which checks for
drv->config_intr and config_ack_interrupt, that way if we ever have to
introduce an additional function in the future, we just update the
helper to check for that.

Other than that, LGTM
-- 
Florian

Re: [PATCH net-next 1/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt

[Heiner Kallweit]

On 08.11.2018 23:33, Florian Fainelli wrote:
> On 11/8/18 1:55 PM, Heiner Kallweit wrote:
>> Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
>> using interrupts isn't possible if a driver defines neither
>> config_intr nor ack_interrupts callback. So we can replace checking
>> flag PHY_HAS_INTERRUPT with checking for these callbacks.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy_device.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index d165a2c82..33e51b955 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -2104,8 +2104,9 @@ static int phy_probe(struct device *dev)
>>  	/* Disable the interrupt if the PHY doesn't support it
>>  	 * but the interrupt is still a valid one
>>  	 */
>> -	if (!(phydrv->flags & PHY_HAS_INTERRUPT) &&
>> -	    phy_interrupt_is_valid(phydev))
>> +	 if (!phydrv->config_intr &&
>> +	     !phydrv->ack_interrupt &&
>> +	     phy_interrupt_is_valid(phydev))
>>  		phydev->irq = PHY_POLL;
> 
> I would introduce an inline helper function which checks for
> drv->config_intr and config_ack_interrupt, that way if we ever have to
> introduce an additional function in the future, we just update the
> helper to check for that.
> 
> Other than that, LGTM
> 
OK, will add a helper and remove PHY_HAS_INTERRUPT from all drivers.

[PATCH net-next 2/2] net: phy: realtek: remove flag PHY_HAS_INTERRUPT from driver configs

[Heiner Kallweit]

Now that flag PHY_HAS_INTERRUPT has been replaced with a check for
callbacks config_intr and ack_interrupt, we can remove setting this
flag from driver configs.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/realtek.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index abff4cdc9..3985b4a4d 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -216,12 +216,10 @@ static struct phy_driver realtek_drvs[] = {
 		.phy_id         = 0x00008201,
 		.name           = "RTL8201CP Ethernet",
 		.features       = PHY_BASIC_FEATURES,
-		.flags          = PHY_HAS_INTERRUPT,
 	}, {
 		.phy_id		= 0x001cc816,
 		.name		= "RTL8201F Fast Ethernet",
 		.features	= PHY_BASIC_FEATURES,
-		.flags		= PHY_HAS_INTERRUPT,
 		.ack_interrupt	= &rtl8201_ack_interrupt,
 		.config_intr	= &rtl8201_config_intr,
 		.suspend	= genphy_suspend,
@@ -239,7 +237,6 @@ static struct phy_driver realtek_drvs[] = {
 		.phy_id		= 0x001cc912,
 		.name		= "RTL8211B Gigabit Ethernet",
 		.features	= PHY_GBIT_FEATURES,
-		.flags		= PHY_HAS_INTERRUPT,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211b_config_intr,
 		.read_mmd	= &genphy_read_mmd_unsupported,
@@ -257,7 +254,6 @@ static struct phy_driver realtek_drvs[] = {
 		.phy_id		= 0x001cc914,
 		.name		= "RTL8211DN Gigabit Ethernet",
 		.features	= PHY_GBIT_FEATURES,
-		.flags		= PHY_HAS_INTERRUPT,
 		.ack_interrupt	= rtl821x_ack_interrupt,
 		.config_intr	= rtl8211e_config_intr,
 		.suspend	= genphy_suspend,
@@ -266,7 +262,6 @@ static struct phy_driver realtek_drvs[] = {
 		.phy_id		= 0x001cc915,
 		.name		= "RTL8211E Gigabit Ethernet",
 		.features	= PHY_GBIT_FEATURES,
-		.flags		= PHY_HAS_INTERRUPT,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211e_config_intr,
 		.suspend	= genphy_suspend,
@@ -275,7 +270,6 @@ static struct phy_driver realtek_drvs[] = {
 		.phy_id		= 0x001cc916,
 		.name		= "RTL8211F Gigabit Ethernet",
 		.features	= PHY_GBIT_FEATURES,
-		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= &rtl8211f_config_init,
 		.ack_interrupt	= &rtl8211f_ack_interrupt,
 		.config_intr	= &rtl8211f_config_intr,
@@ -287,7 +281,6 @@ static struct phy_driver realtek_drvs[] = {
 		.phy_id		= 0x001cc961,
 		.name		= "RTL8366RB Gigabit Ethernet",
 		.features	= PHY_GBIT_FEATURES,
-		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= &rtl8366rb_config_init,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
-- 
2.19.1

Re: [PATCH net-next 0/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt

[Andrew Lunn]

On Thu, Nov 08, 2018 at 10:54:50PM +0100, Heiner Kallweit wrote:
> Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
> using interrupts isn't possible if a driver defines neither
> config_intr nor ack_interrupts callback. So we can replace checking
> flag PHY_HAS_INTERRUPT with checking for these callbacks.
> 
> This allows to remove this flag from a lot of driver configs, let's
> start with the Realtek driver.

Hi Heiner

Ideally, we should do this to all the drivers, not just one. If we
leave PHY_HAS_INTERRUPT, people are going to use it. That then makes
the realtek driver then different to all the other drivers, and at
some point somebody will do something which breaks it because they
don't know the realtek driver is special.

	Andrew

Re: [PATCH net-next 0/2] net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt

[Florian Fainelli]

On 11/8/18 2:30 PM, Andrew Lunn wrote:
> On Thu, Nov 08, 2018 at 10:54:50PM +0100, Heiner Kallweit wrote:
>> Flag PHY_HAS_INTERRUPT is used only here for this small check. I think
>> using interrupts isn't possible if a driver defines neither
>> config_intr nor ack_interrupts callback. So we can replace checking
>> flag PHY_HAS_INTERRUPT with checking for these callbacks.
>>
>> This allows to remove this flag from a lot of driver configs, let's
>> start with the Realtek driver.
> 
> Hi Heiner
> 
> Ideally, we should do this to all the drivers, not just one. If we
> leave PHY_HAS_INTERRUPT, people are going to use it. That then makes
> the realtek driver then different to all the other drivers, and at
> some point somebody will do something which breaks it because they
> don't know the realtek driver is special.

Agreed.
-- 
Florian