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

[Adrian Schmutzler]

Hi,

> > > +			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?

Because with boardname-specific LED labels you cannot share the LED definitions in device tree, so they cannot be moved into a common DTSI.
Despite, if you had several similar d-link devices, you could also share definitions in 01_leds for those again.
But 01_leds is just a workaround for stuff that cannot be defined in DTS, so IMO the impact on DT is more important than the impact on 01_leds.

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

I haven't been able to extract that precise information from your previous comments (I don't want to blame you for that, maybe I just overlooked it ...).

If stock firmware works without those partitions, removing them is fine for me. Please explicitly state in your commit message (after telling about the removal) that you tested successfully on vendor firmware. :-)

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

Well, I do not think we are using DTS files like this so far. Normally, what's in board.d files is not added to DT "again".

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

To make it really useful, you would at least have to add the "location" in nvram to the comment, too. With your current solution, you'd still have to look into 02_network to find out about that.

And if you then really followed that strategy, you would also have to add comments for netdev/switch triggers, the caldata, etc.

Anyway, my opposition is not strong enough to reject this right now, maybe someone else can provide an additional opinion on the subject.

Best

Adrian

> 
> --
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openpgp-digital-signature.asc
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20191111/f7f72943/attachment.sig>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel