[OpenWrt-Devel] [PATCH v2 2/3] ramips: Get ethernet ports to be disabled from the device tree.

[Vittorio G (VittGam)]

Hi,

On 12/12/2015 07:20:43 CET, John Crispin wrote:
> On 12/12/2015 07:19, John Crispin wrote:
>> why a semicolon ? i believe a "," needs to be added instead please also
>> remove the superfluous "," while you are at it to make the comment
>> orthographically correct. it seems to be have been copied from SDK code.
>
> you already removed the extra "," sorry i missed that

Ok. Would "Enable all ports, Back Pressure and Flow Control" be okay?

> > -	/* Copy disabled port configuration from bootloader setup */
> > -	port_disable = esw_get_port_disable(esw);
> > +	/* Copy disabled port configuration from device tree setup */
> > +	if (esw->port_disable)
> > +		port_disable = esw->port_disable;
> this changes the behavior of the code and will most likely break several
> board.

Why? In the previous code we're going to enable all ports anyway by clearing
all of the port disabled bits, so esw_get_port_disable would have returned
zero; and now port_disable is 0 by default if it isn't set in the device tree.
The only case where esw_get_port_disable would have returned a value different
from zero is when port 5 is not implemented in the SoC, but the port power
down logic that follows is not going to handle port 5 anyway. Furthermore,
user space reporting of the disabled status (eg. swconfig dev switch0 show)
is read again from the chip at every report request, so port 5 will show as
disabled correctly for SoCs where it is not implemented like RT5350.

Anyway, if you think that an "else port_disable = esw_get_port_disable(esw);"
would be better anyway, I can add it. (But I think it'd be superfluous...
Unless there's some SoC that behaves differently than this.)

> port_map is named as such as it is a port_map and not a port_disable. we
> already concluded yesterday that this is a bad code style.

It's the mapping of the disabled ports... But okay, I'll change every variable
to be "tmp" instead this time.

Cheers,
Vittorio G
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel