Re: [PATCH net-next v3 2/3] net: phy: mscc: Consolidate probe functions into a common helper

[Lad, Prabhakar]

Hi Andrew,

On Tue, Nov 11, 2025 at 1:25 PM Andrew Lunn <andrew@xxxxxxx> wrote:
>
> On Tue, Nov 11, 2025 at 09:56:12AM +0000, Lad, Prabhakar wrote:
> > Hi Parthiban,
> >
> > Thank you for the review.
> >
> > On Tue, Nov 11, 2025 at 9:50 AM <Parthiban.Veerasooran@xxxxxxxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > > On 11/11/25 2:40 pm, Prabhakar wrote:
> > > > +static int vsc85xx_probe_common(struct phy_device *phydev,
> > > > +                               const struct vsc85xx_probe_config *cfg,
> > > > +                               const u32 *default_led_mode)
> > > > +{
> > > > +       struct vsc8531_private *vsc8531;
> > > > +       int ret;
> > > > +
> > > > +       vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
> > > > +       if (!vsc8531)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       phydev->priv = vsc8531;
> > > > +
> > > > +       /* Check rate magic if needed (only for non-package PHYs) */
> > > > +       if (cfg->check_rate_magic) {
> > > > +               ret = vsc85xx_edge_rate_magic_get(phydev);
> > > > +               if (ret < 0)
> > > > +                       return ret;
> > > > +
> > > > +               vsc8531->rate_magic = ret;
> > > > +       }
> > > > +
> > > > +       /* Set up package if needed */
> > > > +       if (cfg->use_package) {
> > > > +               vsc8584_get_base_addr(phydev);
> > > > +               devm_phy_package_join(&phydev->mdio.dev, phydev,
> > > > +                                     vsc8531->base_addr, cfg->shared_size);
> > > Don't you need to check the return value here?
> > >
> > Good point. The orignal code didn't check the return value. Would you
> > prefer a separate patch on top of this series or fix it in this
> > consolidation patch itself?
>
> When refactoring, it is best to not make changed, keep the code
> logically the same. Then add additions afterwards. If something
> breaks, a git bisect then tells you if it was the refactor or the
> additions that broke it.
>
Agreed, I will add a patch on top of this patch (as you have already
reviewed it)

Cheers,
Prabhakar