Re: [PATCH net-next v2 5/5] net: ravb: Add runtime PM support

[claudiu beznea]

On 09.02.2024 23:00, Sergey Shtylyov wrote:
> On 2/9/24 8:04 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>
>> Add runtime PM support for the ravb driver. As the driver is used by
>> different IP variants, with different behaviors, to be able to have the
>> runtime PM support available for all devices, the preparatory commits
>> moved all the resources parsing and allocations in the driver's probe
>> function and kept the settings for ravb_open(). This is due to the fact
>> that on some IP variants-platforms tuples disabling/enabling the clocks
>> will switch the IP to the reset operation mode where registers' content is
> 
>    This pesky "registers' content" somehow evaded me -- should be "register
> contents" as well...
> 
>> lost and reconfiguration needs to be done. For this the rabv_open()
>> function enables the clocks, switches the IP to configuration mode, applies
>> all the registers settings and switches the IP to the operational mode. At
>> the end of ravb_open() IP is ready to send/receive data.
>>
>> In ravb_close() necessary reverts are done (compared with ravb_open()), the
>> IP is switched to reset mode and clocks are disabled.
>>
>> The ethtool APIs or IOCTLs that might execute while the interface is down
>> are either cached (and applied in ravb_open()) or rejected (as at that time
>> the IP is in reset mode). Keeping the IP in the reset mode also increases
>> the power saved (according to the hardware manual).
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>> Reviewed-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index f4be08f0198d..5bbfdfeef8a9 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1939,16 +1939,21 @@ static int ravb_open(struct net_device *ndev)
>>  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	const struct ravb_hw_info *info = priv->info;
>> +	struct device *dev = &priv->pdev->dev;
>>  	int error;
>>  
>>  	napi_enable(&priv->napi[RAVB_BE]);
>>  	if (info->nc_queues)
>>  		napi_enable(&priv->napi[RAVB_NC]);
>>  
>> +	error = pm_runtime_resume_and_get(dev);
>> +	if (error < 0)
>> +		goto out_napi_off;
> 
>    Well, s/error/ret/ -- it would fit better here...

Using error is the "trademark" of this driver, it is used all around the
driver. I haven't introduced it here, I don't like it. The variable error
in this particular function is here from the beginning of the driver.

So, I don't consider changing error to ret is the scope of this series.

> 
> [...]
>> @@ -3066,6 +3089,12 @@ static void ravb_remove(struct platform_device *pdev)
>>  	struct net_device *ndev = platform_get_drvdata(pdev);
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	const struct ravb_hw_info *info = priv->info;
>> +	struct device *dev = &priv->pdev->dev;
>> +	int error;
>> +
>> +	error = pm_runtime_resume_and_get(dev);
>> +	if (error < 0)
>> +		return;
> 
>    Again, s/erorr/ret/ in this case.

error was used here to comply with the rest of the driver. So, if you still
want me to change it here and in ravb_remove() please confirm.

Thank you,
Claudiu Beznea

> 
> [...]
> 
> MBR, Sergey