Messages in this thread | | | Date | Sat, 25 Mar 2023 12:06:41 -0400 | Subject | Re: [PATCH net-next v4 10/10] net: sunhme: Consolidate common probe tasks | From | Sean Anderson <> |
| |
On 3/25/23 05:10, Simon Horman wrote: > 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);
Yes, this is intentional. Logically, they are the same; we just set up the default before calling of_getintprop_default instead of after.
>> +#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.
Ah, oops.
--Sean
>> + /* 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; >> +} >> + > > ...
| |