netdev - Re: [PATCH 3/3] net: ethernet: ixp4xx: Use OF MDIO bus registration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdb8L=V+=5XVSV_viC5dLcLPWH5s9ztuESXjyRBWJOu9iA@mail.gmail.com>
Date:   Tue, 20 Apr 2021 10:44:16 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev <netdev@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Zoltan HERPAI <wigyori@...0.hu>,
        Raylynn Knight <rayknight@...com>
Subject: Re: [PATCH 3/3] net: ethernet: ixp4xx: Use OF MDIO bus registration

On Tue, Apr 20, 2021 at 3:47 AM Andrew Lunn <andrew@...n.ch> wrote:

> > @@ -1381,25 +1382,12 @@ static struct eth_plat_info *ixp4xx_of_get_platdata(struct device *dev)
> >       /* NPE ID 0x00, 0x10, 0x20... */
> >       plat->npe = (val << 4);
> >
> > -     phy_np = of_parse_phandle(np, "phy-handle", 0);
> > -     if (phy_np) {
> > -             ret = of_property_read_u32(phy_np, "reg", &val);
> > -             if (ret) {
> > -                     dev_err(dev, "cannot find phy reg\n");
> > -                     return NULL;
> > -             }
> > -             of_node_put(phy_np);
> > -     } else {
> > -             dev_err(dev, "cannot find phy instance\n");
> > -             val = 0;
> > -     }
> > -     plat->phy = val;
> > -
>
> Isn't this code you just added in the previous patch?

Yep. It's by the token of "one technical step per patch" but I
suggested that maybe you prefer to take two technical
steps in one big patch, then we can just squash
patches 2 & 3. I'll fix it as you want it just tell me how :)

> > -     snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT,
> > -              mdio_bus->id, plat->phy);
> > -     phydev = phy_connect(ndev, phy_id, &ixp4xx_adjust_link,
> > -                          PHY_INTERFACE_MODE_MII);
> > +     if (np) {
> > +             phydev = of_phy_get_and_connect(ndev, np, ixp4xx_adjust_link);
> > +     } else {
> > +             snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT,
> > +                      mdio_bus->id, plat->phy);
>
> mdiobus_get_phy() and phy_connect_direct() might be better.

Do you mean for the legacy code path (else clause), or the
new code path with of_phy_get_and_connect() or both?

I tried not to change the legacy code in order to not introduce
regressions, so if I change that I suppose it should be a
separate patch.

On the other hand this driver has not been much maintained
the recent years so we might need to be a bit rough when
bringing it into shape. (After migrating all of IXP4xx to
device tree a lot of the legacy code will eventually be deleted.)

Yours,
Linus Walleij