Hi Geert-san, > From: Geert Uytterhoeven, Sent: Friday, October 13, 2023 4:29 PM > > Hi Shimoda-san, > > On Fri, Oct 13, 2023 at 4:10 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote: > > > From: Geert Uytterhoeven, Sent: Thursday, October 12, 2023 9:35 PM > > > On Thu, Oct 12, 2023 at 2:16 PM Yoshihiro Shimoda > > > <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote: > > > > Add PM ops for Suspend to Idle. When the system suspended, > > > > the Ethernet Serdes's clock will be stopped. So, this driver needs > > > > to re-initialize the Ethernet Serdes by phy_init() in > > > > renesas_eth_sw_resume(). Otherwise, timeout happened in phy_power_on(). > > > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx> > > > > > > Thanks for your patch! > > > > Thank you for your review! > > > > > > --- a/drivers/net/ethernet/renesas/rswitch.c > > > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > > > @@ -17,6 +17,7 @@ > > > > #include <linux/of_net.h> > > > > #include <linux/phy/phy.h> > > > > #include <linux/platform_device.h> > > > > +#include <linux/pm.h> > > > > #include <linux/pm_runtime.h> > > > > #include <linux/rtnetlink.h> > > > > #include <linux/slab.h> > > > > @@ -1315,6 +1316,7 @@ static int rswitch_phy_device_init(struct rswitch_device *rdev) > > > > if (!phydev) > > > > goto out; > > > > __set_bit(rdev->etha->phy_interface, phydev->host_interfaces); > > > > + phydev->mac_managed_pm = true; > > > > > > > > phydev = of_phy_connect(rdev->ndev, phy, rswitch_adjust_link, 0, > > > > rdev->etha->phy_interface); > > > > @@ -1991,11 +1993,52 @@ static void renesas_eth_sw_remove(struct platform_device *pdev) > > > > platform_set_drvdata(pdev, NULL); > > > > } > > > > > > > > +static int __maybe_unused renesas_eth_sw_suspend(struct device *dev) > > > > +{ > > > > + struct rswitch_private *priv = dev_get_drvdata(dev); > > > > + struct net_device *ndev; > > > > + int i; > > > > > > unsigned int (also below) > > > > I don't know why unsigned int is needed. Other functions use > > There's this old mantra "prefer unsigned over signed in C", > and a valid port array index here is always unsigned. I understood it. > > rswitch_for_each_enabled_port{_continue_reverse}() with int. > > Especially, rswitch_for_each_enabled_port_continue_reverse() > > has the following code, unsigned int will not work correctly: > > Oh, there is also a reverse variant, which indeed needs a signed > iterator, currently... > > > --- > > #define rswitch_for_each_enabled_port_continue_reverse(priv, i) \ > > for (i--; i >= 0; i--) \ > > I think this can be made to work with an unsigned iterator using > > for (; i-- > 0; ) I think that this loop cannot access index 0 :) > > if (priv->rdev[i]->disabled) \ > > continue; \ > > else > > --- > > > > So, I would like to keep this for consistency with other functions' > > implementation. But, what do you think? > > Consistency is good... > Surprising readers (why is this signed?) is bad... > It's hard to keep a good balance... > > BTW, perhaps it would make sense to use the reverse order in suspend? > Although it probably doesn't matter, as rswitch_deinit() uses the > non-reverse order, too. Thank you for your suggestion. I also think that we can use the non-reverse order. So, 1) Change "_reverse()" macro to others somehow. 2) Change the type of i for "for_each" to unsigned int. 3) Add PM ops. Perhaps, can/should we merge the 1) and 2) to one patch? Best regards, Yoshihiro Shimoda > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds