Re: [PATCH net] net: phy: fix phy_start to consider PHY_IGNORE_INTERRUPT

[Heiner Kallweit]

Am 07.02.2018 um 20:34 schrieb Florian Fainelli:
> 
> 
> On 02/07/2018 11:26 AM, Heiner Kallweit wrote:
>> Am 07.02.2018 um 20:06 schrieb Florian Fainelli:
>>>
>>>
>>> On 02/07/2018 10:44 AM, Heiner Kallweit wrote:
 This condition wasn't adjusted when PHY_IGNORE_INTERRUPT (-2) was added
 long ago. In case of PHY_IGNORE_INTERRUPT the MAC interrupt indicates
 also PHY state changes and we should do what the symbol says.
>>>
>>> Do you use phy_enable_interrupts() to configure how the PHY interrupts
>>> will be flowing through the Ethernet MAC?
>>>
>> No. And I'm not sure I understand your question correctly.
> 
> No wonder, my question does not make sense, I read the test wrong.
> 
>> The change applies the same behavior as e.g. in phy_connect_direct()
>> where phy_start_interrupts() is called only if phy_dev->irq > 0.
> 
> Not enough coffee, your change is fine, could you consider using
> phy_interrupt_is_valid() instead for this test?
> 
Sure. I was considering this already however wasn't sure because
currently both ways to check for a valid interrupt are used in
phylib.

>>

 Signed-off-by: Heiner Kallweit 
 ---
  drivers/net/phy/phy.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
 index f3313a129..50ed35a45 100644
 --- a/drivers/net/phy/phy.c
 +++ b/drivers/net/phy/phy.c
 @@ -822,7 +822,7 @@ void phy_start(struct phy_device *phydev)
phy_resume(phydev);
  
/* make sure interrupts are re-enabled for the PHY */
 -  if (phydev->irq != PHY_POLL) {
 +  if (phydev->irq > 0) {
err = phy_enable_interrupts(phydev);
if (err < 0)
break;

>>>
>>
> 

Re: [PATCH net] net: phy: fix phy_start to consider PHY_IGNORE_INTERRUPT

[Florian Fainelli]

On 02/07/2018 11:26 AM, Heiner Kallweit wrote:
> Am 07.02.2018 um 20:06 schrieb Florian Fainelli:
>>
>>
>> On 02/07/2018 10:44 AM, Heiner Kallweit wrote:
>>> This condition wasn't adjusted when PHY_IGNORE_INTERRUPT (-2) was added
>>> long ago. In case of PHY_IGNORE_INTERRUPT the MAC interrupt indicates
>>> also PHY state changes and we should do what the symbol says.
>>
>> Do you use phy_enable_interrupts() to configure how the PHY interrupts
>> will be flowing through the Ethernet MAC?
>>
> No. And I'm not sure I understand your question correctly.

No wonder, my question does not make sense, I read the test wrong.

> The change applies the same behavior as e.g. in phy_connect_direct()
> where phy_start_interrupts() is called only if phy_dev->irq > 0.

Not enough coffee, your change is fine, could you consider using
phy_interrupt_is_valid() instead for this test?

> 
>>>
>>> Signed-off-by: Heiner Kallweit 
>>> ---
>>>  drivers/net/phy/phy.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>> index f3313a129..50ed35a45 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -822,7 +822,7 @@ void phy_start(struct phy_device *phydev)
>>> phy_resume(phydev);
>>>  
>>> /* make sure interrupts are re-enabled for the PHY */
>>> -   if (phydev->irq != PHY_POLL) {
>>> +   if (phydev->irq > 0) {
>>> err = phy_enable_interrupts(phydev);
>>> if (err < 0)
>>> break;
>>>
>>
> 

-- 
Florian

Re: [PATCH net] net: phy: fix phy_start to consider PHY_IGNORE_INTERRUPT

[Heiner Kallweit]

Am 07.02.2018 um 20:06 schrieb Florian Fainelli:
> 
> 
> On 02/07/2018 10:44 AM, Heiner Kallweit wrote:
>> This condition wasn't adjusted when PHY_IGNORE_INTERRUPT (-2) was added
>> long ago. In case of PHY_IGNORE_INTERRUPT the MAC interrupt indicates
>> also PHY state changes and we should do what the symbol says.
> 
> Do you use phy_enable_interrupts() to configure how the PHY interrupts
> will be flowing through the Ethernet MAC?
> 
No. And I'm not sure I understand your question correctly.
The change applies the same behavior as e.g. in phy_connect_direct()
where phy_start_interrupts() is called only if phy_dev->irq > 0.

>>
>> Signed-off-by: Heiner Kallweit 
>> ---
>>  drivers/net/phy/phy.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index f3313a129..50ed35a45 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -822,7 +822,7 @@ void phy_start(struct phy_device *phydev)
>>  phy_resume(phydev);
>>  
>>  /* make sure interrupts are re-enabled for the PHY */
>> -if (phydev->irq != PHY_POLL) {
>> +if (phydev->irq > 0) {
>>  err = phy_enable_interrupts(phydev);
>>  if (err < 0)
>>  break;
>>
> 

Re: [PATCH net] net: phy: fix phy_start to consider PHY_IGNORE_INTERRUPT

[Florian Fainelli]

On 02/07/2018 10:44 AM, Heiner Kallweit wrote:
> This condition wasn't adjusted when PHY_IGNORE_INTERRUPT (-2) was added
> long ago. In case of PHY_IGNORE_INTERRUPT the MAC interrupt indicates
> also PHY state changes and we should do what the symbol says.

Do you use phy_enable_interrupts() to configure how the PHY interrupts
will be flowing through the Ethernet MAC?

> 
> Signed-off-by: Heiner Kallweit 
> ---
>  drivers/net/phy/phy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index f3313a129..50ed35a45 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -822,7 +822,7 @@ void phy_start(struct phy_device *phydev)
>   phy_resume(phydev);
>  
>   /* make sure interrupts are re-enabled for the PHY */
> - if (phydev->irq != PHY_POLL) {
> + if (phydev->irq > 0) {
>   err = phy_enable_interrupts(phydev);
>   if (err < 0)
>   break;
> 

-- 
Florian

[PATCH net] net: phy: fix phy_start to consider PHY_IGNORE_INTERRUPT

[Heiner Kallweit]

This condition wasn't adjusted when PHY_IGNORE_INTERRUPT (-2) was added
long ago. In case of PHY_IGNORE_INTERRUPT the MAC interrupt indicates
also PHY state changes and we should do what the symbol says.

Signed-off-by: Heiner Kallweit 
---
 drivers/net/phy/phy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f3313a129..50ed35a45 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -822,7 +822,7 @@ void phy_start(struct phy_device *phydev)
phy_resume(phydev);
 
/* make sure interrupts are re-enabled for the PHY */
-   if (phydev->irq != PHY_POLL) {
+   if (phydev->irq > 0) {
err = phy_enable_interrupts(phydev);
if (err < 0)
break;
-- 
2.16.1