On Monday 24 March 2014 16:17:42 Zhangfei Gao wrote:
> On Thu, Mar 20, 2014 at 10:31 PM, Arnd Bergmann <arnd@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
