Messages in this thread | | | Date | Sat, 25 Mar 2023 10:10:30 +0100 | From | Simon Horman <> | Subject | Re: [PATCH net-next v4 10/10] net: sunhme: Consolidate common probe tasks |
| |
On Fri, Mar 24, 2023 at 01:51:36PM -0400, Sean Anderson wrote: > Most of the second half of the PCI/SBUS probe functions are the same. > Consolidate them into a common function. > > Signed-off-by: Sean Anderson <seanga2@gmail.com>
Hi Sean,
overall this looks good. But I (still?) have some concerns about handling hm_revision.
...
> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > index bd1925f575c4..ec85aef35bf9 100644 > --- a/drivers/net/ethernet/sun/sunhme.c > +++ b/drivers/net/ethernet/sun/sunhme.c > @@ -2430,6 +2430,58 @@ static void happy_meal_addr_init(struct happy_meal *hp, > } > } > > +static int happy_meal_common_probe(struct happy_meal *hp, > + struct device_node *dp) > +{ > + struct net_device *dev = hp->dev; > + int err; > + > +#ifdef CONFIG_SPARC > + hp->hm_revision = of_getintprop_default(dp, "hm-rev", hp->hm_revision);
Previously the logic, for SPARC for PCI went something like this:
/* in happy_meal_pci_probe() */ hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff); if (hp->hm_revision == 0xff) hp->hm_revision = 0xc0 | (pdev->revision & 0x0f);
Now it goes something like this:
/* in happy_meal_pci_probe() */ hp->hm_revision = 0xc0 | (pdev->revision & 0x0f); /* in happy_meal_common_probe() */ hp->hm_revision = of_getintprop_default(dp, "hm-rev", hp->hm_revision);
Is this intentional?
Likewise, for sbus (which implies SPARC) the logic was something like:
/* in happy_meal_sbus_probe_one() */ hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff); if (hp->hm_revision == 0xff) hp->hm_revision = 0xa0;
And now goes something like this:
/* in happy_meal_pci_probe() */ hp->hm_revision = 0xa0; /* in happy_meal_common_probe() */ hp->hm_revision = of_getintprop_default(dp, "hm-rev", hp->hm_revision);
> +#endif > + > + /* Now enable the feature flags we can. */ > + if (hp->hm_revision == 0x20 || hp->hm_revision == 0x21) > + hp->happy_flags |= HFLAG_20_21; > + else if (hp->hm_revision != 0xa0) > + hp->happy_flags |= HFLAG_NOT_A0; > + > + hp->happy_block = dmam_alloc_coherent(hp->dma_dev, PAGE_SIZE, > + &hp->hblock_dvma, GFP_KERNEL); > + if (!hp->happy_block) > + return -ENOMEM; > + > + /* Force check of the link first time we are brought up. */ > + hp->linkcheck = 0; > + > + /* Force timer state to 'asleep' with count of zero. */ > + hp->timer_state = asleep; > + hp->timer_ticks = 0; > + > + timer_setup(&hp->happy_timer, happy_meal_timer, 0); > + > + dev->netdev_ops = &hme_netdev_ops; > + dev->watchdog_timeo = 5 * HZ; > + dev->ethtool_ops = &hme_ethtool_ops; > + > + /* Happy Meal can do it all... */ > + dev->hw_features = NETIF_F_SG | NETIF_F_HW_CSUM; > + dev->features |= dev->hw_features | NETIF_F_RXCSUM; > + > +
nit: one blank line is enough.
> + /* Grrr, Happy Meal comes up by default not advertising > + * full duplex 100baseT capabilities, fix this. > + */ > + spin_lock_irq(&hp->happy_lock); > + happy_meal_set_initial_advertisement(hp); > + spin_unlock_irq(&hp->happy_lock); > + > + err = devm_register_netdev(hp->dma_dev, dev); > + if (err) > + dev_err(hp->dma_dev, "Cannot register net device, aborting.\n"); > + return err; > +} > +
...
| |