On Mon, Mar 24, 2014 at 6:02 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> 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).
>
Understand now.
Will update with syscon, as it is simpler.
Thanks for your patience.
--
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
