Florian
On 10/04/2017 11:18 AM, Florian Fainelli wrote:
> On 10/04/2017 08:41 AM, Dan Murphy wrote:
>> 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
>
> I would expect you to wrap your own calls around genphy_suspend and
> genphy_resume, such your functions become:
>
> static int dp83822_suspend(struct phy_device *phydev)
> {
> int value;
>
> mutex_lock(&phydev->lock);
> value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
> mutex_unlock(&phydev->lock);
>
> if (~value & DP83822_WOL_EN)
> genphy_suspend(phydev);
> }
>
> static int dp83822_resume(struct phy_device *phydev)
> {
> int value;
>
> genphy_resume(phydev);
>
> mutex_lock(&phydev->lock);
> value = phy_read_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG);
>
> phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, value |
> DP83822_WOL_CLR_INDICATION);
>
> mutex_unlock(&phydev->lock);
>
> return 0;
> }
>
Great thats exactly what I did.
Dan
>>
>> Dan
>>
>> [snip]--
>> ------------------
>> Dan Murphy
>>
>
>
--
------------------
Dan Murphy
