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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 24 Mar 2014 21:23:39 +0800
From:	Zhangfei Gao <zhangfei.gao@...il.com>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	Zhangfei Gao <zhangfei.gao@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH 3/3] net: hisilicon: new hip04 ethernet driver

On Mon, Mar 24, 2014 at 6:02 PM, Arnd Bergmann <arnd@...db.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@...db.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@...0000 {
>> >>                         compatible = "hisilicon,hip04-ppe";
>> >>                         reg = <0x28c0000 0x10000>;
>> >>                         #address-cells = <1>;
>> >>                         #size-cells = <0>;
>> >>                 };
>> >>                 fe: ethernet@...0000 {
>> >>                         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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html