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

On Monday 24 March 2014 16:17:42 Zhangfei Gao wrote:

On Thu, Mar 20, 2014 at 10:31 PM, Arnd Bergmann [off-list ref] wrote:

quoted

quoted

quoted

quoted

+     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.

quoted

It's probably a little simpler to avoid the sub-nodes and instead do

quoted

              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