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

[Zhangfei Gao <zhangfei.gao@gmail.com>]

On Mon, Mar 24, 2014 at 6:02 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 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).
>

Understand now.
Will update with syscon, as it is simpler.
Thanks for your patience.