Re: [PATCH net-next 3/3] net: phy: bcm7xxx: request and manage GPHY clock

[Florian Fainelli <f.fainelli@gmail.com>]

On 9/3/2020 11:15 PM, Marco Felsch wrote:
> Hi Florian,
> 
> On 20-09-02 21:39, Florian Fainelli wrote:
>> The internal Gigabit PHY on Broadcom STB chips has a digital clock which
>> drives its MDIO interface among other things, the driver now requests
>> and manage that clock during .probe() and .remove() accordingly.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>   drivers/net/phy/bcm7xxx.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
>> index 692048d86ab1..f0ffcdcaef03 100644
>> --- a/drivers/net/phy/bcm7xxx.c
>> +++ b/drivers/net/phy/bcm7xxx.c
>> @@ -11,6 +11,7 @@
>>   #include "bcm-phy-lib.h"
>>   #include <linux/bitops.h>
>>   #include <linux/brcmphy.h>
>> +#include <linux/clk.h>
>>   #include <linux/mdio.h>
>>   
>>   /* Broadcom BCM7xxx internal PHY registers */
>> @@ -39,6 +40,7 @@
>>   
>>   struct bcm7xxx_phy_priv {
>>   	u64	*stats;
>> +	struct clk *clk;
>>   };
>>   
>>   static int bcm7xxx_28nm_d0_afe_config_init(struct phy_device *phydev)
>> @@ -534,7 +536,19 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
>>   	if (!priv->stats)
>>   		return -ENOMEM;
>>   
>> -	return 0;
>> +	priv->clk = devm_clk_get_optional(&phydev->mdio.dev, NULL);
> 
> Since the clock is binded to the mdio-dev here..
> 
>> +	if (IS_ERR(priv->clk))
>> +		return PTR_ERR(priv->clk);
>> +
>> +	return clk_prepare_enable(priv->clk);
> 
> clould we use devm_add_action_or_reset() here so we don't have to
> register the .remove() hook?

Maybe, more on that below.

> 
>> +}
>> +
>> +static void bcm7xxx_28nm_remove(struct phy_device *phydev)
>> +{
>> +	struct bcm7xxx_phy_priv *priv = phydev->priv;
>> +
>> +	clk_disable_unprepare(priv->clk);
>> +	devm_clk_put(&phydev->mdio.dev, priv->clk);
> 
> Is this really necessary? The devm_clk_get_optional() function already
> registers the devm_clk_release() hook.

Yes, because you can unbind the PHY driver from sysfs, and if you want 
to bind that driver again, which will call .probe() again, you must undo 
strictly everything that .probe() did. The embedded mdio_device does not 
go away, so there will be no automatic freeing of resources. Using 
devm_* may be confusing, so using just the plain clk_get() and clk_put() 
may be clearer here.
-- 
Florian