| Message ID | 20230127014812.1656340-3-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: renesas: rswitch: Modify initialization for SERDES and PHY | expand |
Hi Shimoda-san, On Fri, Jan 27, 2023 at 2:49 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > Simplify struct phy *serdes handling by keeping the valiable in > the struct rswitch_device. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Thanks for your patch! > --- a/drivers/net/ethernet/renesas/rswitch.c > +++ b/drivers/net/ethernet/renesas/rswitch.c > @@ -1222,49 +1222,40 @@ static void rswitch_phylink_deinit(struct rswitch_device *rdev) > phylink_destroy(rdev->phylink); > } > > -static int rswitch_serdes_set_params(struct rswitch_device *rdev) > +static int rswitch_serdes_phy_get(struct rswitch_device *rdev) > { > struct device_node *port = rswitch_get_port_node(rdev); > struct phy *serdes; > - int err; > > serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL); > of_node_put(port); > if (IS_ERR(serdes)) > return PTR_ERR(serdes); You may as well just return serdes... > + rdev->serdes = serdes; ... and move the above assignment into the caller. That would save one if (...) check. After that, not much is left in this function, so I'm wondering if it can just be inlined at the single callsite? BTW, there seem to be several calls to rswitch_get_port_node(), which calls into DT tree traversal, so you may want to call it once, and store a pointer to the port device node, too. Then rswitch_serdes_phy_get() becomes a candidate for manual inlining for sure. > + > + return 0; > +} > + > +static int rswitch_serdes_set_params(struct rswitch_device *rdev) > +{ > + int err; > > - err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET, > + err = phy_set_mode_ext(rdev->serdes, PHY_MODE_ETHERNET, > rdev->etha->phy_interface); > if (err < 0) > return err; > > - return phy_set_speed(serdes, rdev->etha->speed); > + return phy_set_speed(rdev->serdes, rdev->etha->speed); > } > > static int rswitch_serdes_init(struct rswitch_device *rdev) > { > - struct device_node *port = rswitch_get_port_node(rdev); > - struct phy *serdes; > - > - serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL); > - of_node_put(port); > - if (IS_ERR(serdes)) > - return PTR_ERR(serdes); > - > - return phy_init(serdes); > + return phy_init(rdev->serdes); > } As this is now a one-line function, just call phy_init() in all callers instead? > > static int rswitch_serdes_deinit(struct rswitch_device *rdev) > { > - struct device_node *port = rswitch_get_port_node(rdev); > - struct phy *serdes; > - > - serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL); > - of_node_put(port); > - if (IS_ERR(serdes)) > - return PTR_ERR(serdes); > - > - return phy_exit(serdes); > + return phy_exit(rdev->serdes); > } Just call phy_exit() in all callers instead? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Friday, January 27, 2023 5:35 PM > > Hi Shimoda-san, > > On Fri, Jan 27, 2023 at 2:49 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > Simplify struct phy *serdes handling by keeping the valiable in > > the struct rswitch_device. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Thanks for your patch! Thank you for your review! > > --- a/drivers/net/ethernet/renesas/rswitch.c > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > @@ -1222,49 +1222,40 @@ static void rswitch_phylink_deinit(struct rswitch_device *rdev) > > phylink_destroy(rdev->phylink); > > } > > > > -static int rswitch_serdes_set_params(struct rswitch_device *rdev) > > +static int rswitch_serdes_phy_get(struct rswitch_device *rdev) > > { > > struct device_node *port = rswitch_get_port_node(rdev); > > struct phy *serdes; > > - int err; > > > > serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL); > > of_node_put(port); > > if (IS_ERR(serdes)) > > return PTR_ERR(serdes); > > You may as well just return serdes... > > > + rdev->serdes = serdes; > > ... and move the above assignment into the caller. > That would save one if (...) check. > > After that, not much is left in this function, so I'm wondering if it > can just be inlined at the single callsite? I think so. Thank you for your suggestion! > BTW, there seem to be several calls to rswitch_get_port_node(), which > calls into DT tree traversal, so you may want to call it once, and store > a pointer to the port device node, too. Then rswitch_serdes_phy_get() > becomes a candidate for manual inlining for sure. I understood it. I'll modify it on v4 patch. > > + > > + return 0; > > +} > > + > > +static int rswitch_serdes_set_params(struct rswitch_device *rdev) > > +{ > > + int err; > > > > - err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET, > > + err = phy_set_mode_ext(rdev->serdes, PHY_MODE_ETHERNET, > > rdev->etha->phy_interface); > > if (err < 0) > > return err; > > > > - return phy_set_speed(serdes, rdev->etha->speed); > > + return phy_set_speed(rdev->serdes, rdev->etha->speed); > > } > > > > static int rswitch_serdes_init(struct rswitch_device *rdev) > > { > > - struct device_node *port = rswitch_get_port_node(rdev); > > - struct phy *serdes; > > - > > - serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL); > > - of_node_put(port); > > - if (IS_ERR(serdes)) > > - return PTR_ERR(serdes); > > - > > - return phy_init(serdes); > > + return phy_init(rdev->serdes); > > } > > As this is now a one-line function, just call phy_init() in all > callers instead? I think so. > > > > static int rswitch_serdes_deinit(struct rswitch_device *rdev) > > { > > - struct device_node *port = rswitch_get_port_node(rdev); > > - struct phy *serdes; > > - > > - serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL); > > - of_node_put(port); > > - if (IS_ERR(serdes)) > > - return PTR_ERR(serdes); > > - > > - return phy_exit(serdes); > > + return phy_exit(rdev->serdes); > > } > > Just call phy_exit() in all callers instead? I got it. I'll fix it. Best regards, Yoshihiro Shimoda
diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c index 14fc0af304ce..b0c1ea72772e 100644 --- a/drivers/net/ethernet/renesas/rswitch.c +++ b/drivers/net/ethernet/renesas/rswitch.c @@ -1222,49 +1222,40 @@ static void rswitch_phylink_deinit(struct rswitch_device *rdev) phylink_destroy(rdev->phylink); } -static int rswitch_serdes_set_params(struct rswitch_device *rdev) +static int rswitch_serdes_phy_get(struct rswitch_device *rdev) { struct device_node *port = rswitch_get_port_node(rdev); struct phy *serdes; - int err; serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL); of_node_put(port); if (IS_ERR(serdes)) return PTR_ERR(serdes); + rdev->serdes = serdes; + + return 0; +} + +static int rswitch_serdes_set_params(struct rswitch_device *rdev) +{ + int err; - err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET, + err = phy_set_mode_ext(rdev->serdes, PHY_MODE_ETHERNET, rdev->etha->phy_interface); if (err < 0) return err; - return phy_set_speed(serdes, rdev->etha->speed); + return phy_set_speed(rdev->serdes, rdev->etha->speed); } static int rswitch_serdes_init(struct rswitch_device *rdev) { - struct device_node *port = rswitch_get_port_node(rdev); - struct phy *serdes; - - serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL); - of_node_put(port); - if (IS_ERR(serdes)) - return PTR_ERR(serdes); - - return phy_init(serdes); + return phy_init(rdev->serdes); } static int rswitch_serdes_deinit(struct rswitch_device *rdev) { - struct device_node *port = rswitch_get_port_node(rdev); - struct phy *serdes; - - serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL); - of_node_put(port); - if (IS_ERR(serdes)) - return PTR_ERR(serdes); - - return phy_exit(serdes); + return phy_exit(rdev->serdes); } static int rswitch_ether_port_init_one(struct rswitch_device *rdev) @@ -1286,6 +1277,10 @@ static int rswitch_ether_port_init_one(struct rswitch_device *rdev) if (err < 0) goto err_phylink_init; + err = rswitch_serdes_phy_get(rdev); + if (err < 0) + goto err_serdes_phy_get; + err = rswitch_serdes_set_params(rdev); if (err < 0) goto err_serdes_set_params; @@ -1293,6 +1288,7 @@ static int rswitch_ether_port_init_one(struct rswitch_device *rdev) return 0; err_serdes_set_params: +err_serdes_phy_get: rswitch_phylink_deinit(rdev); err_phylink_init: diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h index 49efb0f31c77..c79b1bdd8072 100644 --- a/drivers/net/ethernet/renesas/rswitch.h +++ b/drivers/net/ethernet/renesas/rswitch.h @@ -953,6 +953,7 @@ struct rswitch_device { int port; struct rswitch_etha *etha; + struct phy *serdes; }; struct rswitch_mfwd_mac_table_entry {
Simplify struct phy *serdes handling by keeping the valiable in the struct rswitch_device. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/net/ethernet/renesas/rswitch.c | 40 ++++++++++++-------------- drivers/net/ethernet/renesas/rswitch.h | 1 + 2 files changed, 19 insertions(+), 22 deletions(-)