Hi Andrew,
On Sat, Mar 10, 2018 at 05:17:18PM +0100, Andrew Lunn wrote:
> On Fri, Mar 09, 2018 at 04:12:31PM -0600, Brad Mouring wrote:
> > The macb mii setup (mii_probe() and mii_init()) previously was
> > somewhat interspersed, likely a result of organic growth and hacking.
>
> Hi Brad
>
> For netdev it is normal to include a cover note for patch series,
> which explains the big picture of the series. The cover note is then
> used as the merge commit message.
>
> >
> > This change moves mii bus registration into mii_init and probing the
> > bus for devices into mii_probe.
> >
> > Signed-off-by: Brad Mouring <brad.mour...@ni.com>
> > Suggested-by: Andrew Lunn <and...@lunn.ch>
> > ---
> > drivers/net/ethernet/cadence/macb_main.c | 89
> > ++++++++++++++++----------------
> > 1 file changed, 45 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c
> > b/drivers/net/ethernet/cadence/macb_main.c
> > index e84afcf1ecb5..7b69a291ab7a 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -470,10 +470,48 @@ static void macb_handle_link_change(struct net_device
> > *dev)
> > static int macb_mii_probe(struct net_device *dev)
> > {
> > struct macb *bp = netdev_priv(dev);
> > - struct macb_platform_data *pdata;
> > + struct macb_platform_data *pdata = dev_get_platdata(&bp->pdev->dev);
> > struct phy_device *phydev;
> > + struct device_node *np = bp->pdev->dev.of_node;
> > int phy_irq;
> > - int ret;
> > + int ret, i;
>
> These should be in 'Reverse Christmas Tree' order. i.e. longest
> first. That makes pdata tricky, so do the assignment in the body of
> the function.
>
> > +
> > + if (np) {
> > + if (of_phy_is_fixed_link(np)) {
> > + if (of_phy_register_fixed_link(np) < 0) {
> > + dev_err(&bp->pdev->dev,
> > + "broken fixed-link specification\n");
> > + return -ENODEV;
> > + }
> > + bp->phy_node = of_node_get(np);
> > + } else {
> > + /* fallback to standard phy registration if no phy were
> > + * found during dt phy registration
> > + */
> > + if (!phy_find_first(bp->mii_bus)) {
> > + for (i = 0; i < PHY_MAX_ADDR; i++) {
> > + struct phy_device *phydev;
> > +
> > + phydev = mdiobus_scan(bp->mii_bus, i);
> > + if (IS_ERR(phydev) &&
> > + PTR_ERR(phydev) != -ENODEV) {
> > + ret = PTR_ERR(phydev);
> > + break;
> > + }
> > + }
> > +
> > + if (ret)
> > + return -ENODEV;
> > + }
> > + }
> > + } else {
> > + for (i = 0; i < PHY_MAX_ADDR; i++)
> > + bp->mii_bus->irq[i] = PHY_POLL;
> > +
> > + if (pdata)
> > + bp->mii_bus->phy_mask = pdata->phy_mask;
>
> I would say these two are to do with the mdio bus setup. Setting irq
> to POLL is not required, the core will do that. So you could add one
> patch removing that.
>
> Andrew
Thank you for the excellent feedback and patience. I've prepped a set
of patches addressing your points, but I'd like to test the changes
on hardware first (both with a DT that calls out the phys and one
that depends on default discovery) prior to a v3 (with appropriate
cover letter).
Brad
