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

[John Crispin]

On 12/12/2015 08:07, Vittorio G (VittGam) wrote:
> 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?

i believe that would be correct punctuation

> 
>> > -    /* 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.)
> 

tl;dr

>> 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.
> 

if you want this merged you will need to add a port_disabled variable

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