Messages in this thread | | | Date | Sun, 26 Mar 2023 09:57:17 +0200 | From | Simon Horman <> | Subject | Re: [PATCH net-next v4 10/10] net: sunhme: Consolidate common probe tasks |
| |
On Sat, Mar 25, 2023 at 12:06:41PM -0400, Sean Anderson wrote: > 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.
Thanks, I see that now :)
And I have no further questions about this patch.
Reviewed-by: Simon Horman <simon.horman@corigine.com>
| |