Florian
On 10/03/2017 01:31 PM, Florian Fainelli wrote:
> On 10/03/2017 11:03 AM, Dan Murphy wrote:
>> Florian
>>
>> Thanks for the review
>>
>> On 10/03/2017 12:15 PM, Florian Fainelli wrote:
>>>> + } else {
>>>> + value &= ~DP83822_WOL_SECURE_ON;
>>>> + }
>>>> +
>>>> + value |= (DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION |
>>>> + DP83822_WOL_CLR_INDICATION);
>>>
>>> The extra parenthesis should not be required here.
>>
>> I did not code that in. I had to add it after Checkpatch cribbed about it.
>> Let me know if you want me to remove it.
>
> Let's keep those, that does not change much.
>
>>
>>>
>>>> + phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
>>>> + value);
>>>> + } else {
>>>> + value =
>>>> + phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
>>>> + value &= (~DP83822_WOL_EN);
>>>
>>> Same here, parenthesis should not be needed.
>>
>> There are three lines of code in the else. This code all needs to be
>> excuted in the else case.
>> I might reformat it to read better. Lindent messed that one up.
>
> sorry, I meant to write that you don't need the parenthesis around
> DP83822_WOL_EN since that is just a single bit here.
>
> [snip]
>
>>>> +
>>>> + mutex_unlock(&phydev->lock);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int dp83822_resume(struct phy_device *phydev)
>>>> +{
>>>> + int value;
>>>> +
>>>> + mutex_lock(&phydev->lock);
>>>> +
>>>> + value = phy_read(phydev, MII_BMCR);
>>>> + phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);
>>>
>>> And genphy_resume() here as well?
>>
>> genphy_resume does not have WoL.
>
> I should have been cleared, I meant using genphy_{suspend,resume} to
> avoid open coding the setting of the BMCR_PDOWN bit, conversely clearing
> of that bit. Because of the locking, maybe you could introduce unlocked
> versions of these two routines, or you acquire and release the lock
> outside of genphy_{suspend,resume}?
OK I have addressed all of the open comments and will be posting v2 shortly.
I do have a question on this request.
Are you indicating to exclusively call genphy_(suspend/resume) from within the
over ridden
routine and then take care of the WoL bits in the phy specific code?
Since the genphy code is duplicated here for the BMCR value that would make the
most sense
to me. The genphy code is exported so I can call it explicitly then do the WoL
Dan
[snip]--
------------------
Dan Murphy
