Re: [PATCH 3/3] net: hisilicon: new hip04 ethernet driver

[Arnd Bergmann <arnd@arndb.de>]

On Monday 24 March 2014 16:17:42 Zhangfei Gao wrote:
> On Thu, Mar 20, 2014 at 10:31 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> >
> >> >> +     if (!ppebase) {
> >> >> +             struct device_node *n;
> >> >> +
> >> >> +             n = of_find_compatible_node(NULL, NULL, "hisilicon,hip04-ppebase");
> >> >> +             if (!n) {
> >> >> +                     ret = -EINVAL;
> >> >> +                     netdev_err(ndev, "not find hisilicon,ppebase\n");
> >> >> +                     goto init_fail;
> >> >> +             }
> >> >> +             ppebase = of_iomap(n, 0);
> >> >> +     }
> >> >
> >> > How about using syscon_regmap_lookup_by_phandle() here? That way, you can have
> >> > a more generic abstraction of the ppe, and stick the port and id in there as
> >> > well, e.g.
> >> >
> >> >         ppe-syscon = <&hip04ppe 1 4>; // ppe, port, id
> 
> Even if using syscon_regmap_lookup_by_phandle, there still have static
> struct regmap, since three controllers
> share one regmap.

The regmap is then owned by the syscon driver, while each controller takes
a reference to the regmap that it can store in its own private data
structure. However, as we discussed using a ppe driver sounds nicer than
regmap.

> > It's probably a little simpler to avoid the sub-nodes and instead do
> >
> >>               ppe: ppe@28c0000 {
> >>                         compatible = "hisilicon,hip04-ppe";
> >>                         reg = <0x28c0000 0x10000>;
> >>                         #address-cells = <1>;
> >>                         #size-cells = <0>;
> >>                 };
> >>                 fe: ethernet@28b0000 {
> >>                         compatible = "hisilicon,hip04-mac";
> >>                         reg = <0x28b0000 0x10000>;
> >>                         interrupts = <0 413 4>;
> >>                         phy-mode = "mii";
> >>                         port-handle = <&ppe 31>;
> >>                 };
> >
> > In the code, I would create a simple ppe driver that exports
> > a few functions. you need. In the ethernet driver probe() function,
> > you should get a handle to the ppe using
> >
> >         /* look up "port-handle" property of the current device, find ppe and port */
> >         struct hip04_ppe *ppe = hip04_ppe_get(dev->of_node);
> >         if (IS_ERR(ppe))
> >                 return PTR_ERR(ptr); /* this handles -EPROBE_DEFER */
> >
> > and then in other code you can just do
> >
> >         hip04_ppe_set_foo(priv->ppe, foo_config);
> >
> > This is a somewhat more high-level abstraction that syscon, which
> > just gives you a 'struct regmap' structure for doing register-level
> > configuration like you have today.
> >
> 
> Do you mean create one additional file like ppe.c with some exported
> function to remove the static ppebase?

It doesn't have to be a separate file, as long as you register a
separate platform_driver for the ppe.

> Since the ppe is specifically bounded with ethernet, and does not used
> anywhere else,
> the exported function may not be used anywhere else.
> Is it make it more complicated since there are probe, remove etc.
> 
> So I may still prefer using "static void __iomem *ppebase", as it is simpler.

The trouble is that the driver should not rely on being only there
for a single instance, that's not how we write drivers.

I'm fine with either a syscon instance (which would be simpler) or a
separate ppe driver as part of the hip04-mac driver (which would be
a nicer abstraction).

	Arnd