[OpenWrt-Devel] [PATCH v2] ath79: add D-Link DIR-615 E4

[Paul Fertser]

Hello Adrian,

Thank you very much for the review.

On Mon, Nov 11, 2019 at 12:12:47PM +0100, Adrian Schmutzler wrote:
> > +		power_green: power_green {
> 
> As stated above, change the _label_ to include a "led_" prefix, so
> this becomes "led_power_green: power_green {". Same for power_amber
> below.

Noted.

> > +			label = "dir-615-e4:green:power";
> 
> Sorry for causing confusion here. I have had a look into ar71xx mach
> files and they consistent use "d-link" as vendor for the led
> labels. Thus, I think it makes more sense to revert that to the
> previous version "d-link:green:power".

Why do you think so? If board name is a good idea and allows sharing
led arrangements (and sharing is desired) then why should we stick to
the old way of doing it?

> > +		wlan {
> > +			label = "dir-615-e4:green:wlan";
> > +			gpios = <&ath9k 1 GPIO_ACTIVE_LOW>;
> > +			linux,default-trigger = "phy0tpt";
> > +		};
> 
> At some point, we started to put ath9k leds into a node of their own:

Noted.

> > +&spi {
> > +	status = "okay";
> > +	num-cs = <1>;
> 
> Please add empty line after status.

Yep.

> > +			nvram: partition at 30000 {
> > +				reg = <0x30000 0x10000>;
> > +				label = "nvram";
> > +				read-only;
> > +			};
> 
> Remove the node labels for the two partitions above, as they are not used anyway (but not the label properties).

Noted.

> > +			/*
> > +			These two partitions are defined by CameoAP99 layout
> > +			but not needed for vendor firmware: MAC address is
> > taken
> > +			from "nvram", language pack can be flashed separately.
> > +
> > +			mac: partition at 3b0000 {
> > +				reg = <0x3b0000 0x10000>;
> > +				label = "mac";
> > +				read-only;
> > +			};
> > +
> > +			lp: partition at 3c0000 {
> > +				reg = <0x3c0000 0x30000>;
> > +				label = "lp";
> > +				read-only;
> > +			};
> > +			*/
> 
> Well, we still do not know whether they are present in vendor
> firmware. I'm still skeptical about removing them.  Nevertheless, I
> won't prevent you from doing that, but please remove this comment
> from the DTS then and put an extensive description into the commit
> message instead.

I've made specific effort to flash vendor firmware and confirmed by
testing on hardware that the vendor firmware doesn't need those
partitions. Isn't that enough? What important aspects did I not check?

Regarding removing comments in DTS, this I am not yet sure is the
right way to go, please consider this rationale: DTS files are not
only OpenWrt-specific, they're supposed to specify hardware
arrangements for the upstream Linux, and for all the other low-level
software that can be booted on hardware too (such as barebox and
u-boot bootloaders). This said, whatever can't be expressed as a set
of properties should still be mentioned in the comments so that
whoever is dealing with this hardware has extensive information.

> > +&eth0 {
> > +	status = "okay";
> > +
> > +	/* ethernet MAC is stored in nvram */
> 
> Remove those comments. You are setting up stuff in 02_network, which
> are relatively standard, so I think extra comments are not
> necessary.

E.g. when (if at all ever) the kernel gains support for parsing nvram
partition, this comment will be changed to a proper DT property. But
it doesn't mean that a person looking at this file before that happens
should need to consult OpenWrt sources to understand how to deal with
this board properly.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav at gmail.com

_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel