[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170104132235.GG10768@lunn.ch>
Date: Wed, 4 Jan 2017 14:22:35 +0100
From: Andrew Lunn <andrew@...n.ch>
To: John Crispin <john@...ozen.org>
Cc: "David S. Miller" <davem@...emloft.net>,
Florian Fainelli <f.fainelli@...il.com>,
Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
netdev@...r.kernel.org
Subject: Re: [RFC 3/4] net-next: dsa: Add support for multiple cpu ports.
On Wed, Jan 04, 2017 at 08:38:03AM +0100, John Crispin wrote:
> static int dsa_user_port_apply(struct device_node *port, u32 index,
> @@ -475,6 +475,28 @@ static int dsa_cpu_parse(struct device_node *port, u32 index,
>
> dst->rcv = dst->tag_ops->rcv;
>
> + dev_hold(ethernet_dev);
> + ds->cd->port_ethernet[index] = ethernet_dev;
> +
> + return 0;
> +}
> +
> +static int dsa_user_parse(struct device_node *port, u32 index,
> + struct dsa_switch *ds)
> +{
Please put this function next to dsa_cpu_parse(). All the
apply/unapply functions are together, and all the _parse functions
should be together.
> + struct device_node *cpu_port;
> + const unsigned int *cpu_port_reg;
> + int cpu_port_index;
> +
> + cpu_port = of_parse_phandle(port, "cpu", 0);
> + if (cpu_port) {
> + cpu_port_reg = of_get_property(cpu_port, "reg", NULL);
> + if (!cpu_port_reg)
> + return -EINVAL;
> + cpu_port_index = be32_to_cpup(cpu_port_reg);
> + ds->cd->port_cpu[index] = cpu_port_index;
> + }
> +
> return 0;
> }
>
> @@ -482,18 +504,20 @@ static int dsa_ds_parse(struct dsa_switch_tree *dst, struct dsa_switch *ds)
> {
> struct device_node *port;
> u32 index;
> - int err;
> + int err = 0;
>
> for (index = 0; index < DSA_MAX_PORTS; index++) {
> port = ds->ports[index].dn;
> if (!port)
> continue;
>
> - if (dsa_port_is_cpu(port)) {
> + if (dsa_port_is_cpu(port))
> err = dsa_cpu_parse(port, index, dst, ds);
> - if (err)
> - return err;
> - }
> + else if (!dsa_port_is_dsa(port))
> + err = dsa_user_parse(port, index, ds);
> +
> + if (err)
> + return err;
Having this if (err) here is correct, but it goes against the general
pattern we have in the code. Please indent it so it is under the
err =, and remove the initialisation of err.
Also, if one branch of an if/else has {}, the coding style says the
other branch should also use {}.
Just to make this code look nicer, i would be tempted to add a helper,
dsa_port_is_user().
> }
>
> pr_info("DSA: switch %d %d parsed\n", dst->tree, ds->index);
Thanks
Andrew
Powered by blists - more mailing lists