Re: [PATCH net-next v3 2/4] net: ethernet: renesas: rswitch: Simplify struct phy * handling

[Geert Uytterhoeven <geert@linux-m68k.org>]

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