Re: [PATCH v2] net: fec: fix pinctrl default state restore order on resume

[Tapio Reijonen <tapio.reijonen@vaisala.com>]

Hi,

On 5/27/26 10:47, Wei Fang wrote:
> Please wait at least 24 hours before sending a revised version. Give reviewers
> enough time to provide feedback.
> See https://elixir.bootlin.com/linux/v7.1-rc5/source/Documentation/process/maintainer-netdev.rst#L15
> 
>> @@ -5594,12 +5594,12 @@ static int fec_resume(struct device *dev)
>>                  if (fep->rpm_active)
>>                          pm_runtime_force_resume(dev);
>>
>> -               ret = fec_enet_clk_enable(ndev, true);
>> -               if (ret) {
>> -                       rtnl_unlock();
>> -                       goto failed_clk;
>> -               }
>>                  if (fep->wol_flag & FEC_WOL_FLAG_ENABLE) {
>> +                       ret = fec_enet_clk_enable(ndev, true);
>> +                       if (ret) {
>> +                               rtnl_unlock();
>> +                               goto failed_clk;
>> +                       }
>>                          fec_enet_stop_mode(fep, false);
>>                          if (fep->wake_irq) {
>>                                  disable_irq_wake(fep->wake_irq);
>> @@ -5612,6 +5612,11 @@ static int fec_resume(struct device *dev)
>>                          fep->wol_flag &= ~FEC_WOL_FLAG_SLEEP_ON;
>>                  } else {
>>
>> pinctrl_pm_select_default_state(&fep->pdev->dev);
>> +                       ret = fec_enet_clk_enable(ndev, true);
>> +                       if (ret) {
>> +                               rtnl_unlock();
>> +                               goto failed_clk;
>> +                       }
>>                  }
> 
> I think fec_enet_clk_enable() can be moved before fec_restart(), and there is
> no need to call fec_enet_clk_enable() separately in the if...else... branches.
> 
> Or move pinctrl_pm_select_default_state() before fec_enet_clk_enable(),
> as shown below:
> 
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5644,6 +5644,7 @@ static int fec_resume(struct device *dev)
>                  if (fep->rpm_active)
>                          pm_runtime_force_resume(dev);
> 
> +               pinctrl_pm_select_default_state(&fep->pdev->dev);
>                  ret = fec_enet_clk_enable(ndev, true);
>                  if (ret) {
>                          rtnl_unlock();
> 
>>                  fec_restart(ndev);
>>                  netif_tx_lock_bh(ndev);
> 
Agreed, the second form is cleaner -- it keeps the fix to a one-line 
move of pinctrl_pm_select_default_state() and drops the branch 
duplication I had in v2.


Calling it unconditionally is safe for the WoL path as well: 
fec_suspend() only selects the sleep pinctrl state on the non-WoL path 
and leaves the pins in the default state when WoL is enabled, so on a 
WoL resume the device is already in the default state and the call is a 
no-op.


I'll send v3 with this shape.


Many thanks for the review,
Tapio Reijonen