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@xxxxxxxxxxx> wrote:
> > Simplify struct phy *serdes handling by keeping the valiable in
> > the struct rswitch_device.
> >
> > 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
> > @@ -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
