[OpenWrt-Devel] [PATCH v2] AR71xx: Add Minibox V1.0 support

[Karl Palsson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Crispin <blogic at openwrt.org> wrote:
> 
> 
> On 17/08/2015 23:25, Karl Palsson wrote:
> > Personally I'd like to see Gainstrong mentioned _somewhere_ in the patch
> > itself.
> > 
> >> +--- a/arch/mips/ath79/machtypes.h
> >> ++++ b/arch/mips/ath79/machtypes.h
> >> +@@ -74,6 +74,7 @@ enum ath79_mach_type {
> >> +	ATH79_MACH_JA76PF2,		/* jjPlus JA76PF2 */
> >> +	ATH79_MACH_JWAP003,		/* jjPlus JWAP003 */
> >> +	ATH79_MACH_HORNET_UB,		/* ALFA Networks Hornet-UB */
> >> ++	ATH79_MACH_MINIBOX_V1,	/* MINIBOX V1.0 */
> > 
> 
> Its a comment, but feel free to send a patch to modify the comment.

And my review comment is that retyping the define is useless, most of
the defines use the comment to include a full name, normally including
the vendor, in this case Gainstrong.  This entire patch has no mention
of the word Gainstrong in it at all, just "minibox" in various
capitalizations.   Do you have a particular reason to _encourage_
omitting useful, common information?  Is there a reason that this
version as submitted would be _preferable_ to a version that followed
the existing examples and included useful vendor information?  Is there
any reason that I would want to send a _patch_ to fix someone else's
patch, that hasn't even been merged?

> 
> > Maybe here?  Otherwise that comment is pretty irrelevant...
> > 
> >> +	ATH79_MACH_MR12,		/* Cisco Meraki MR12 */
> >> +	ATH79_MACH_MR16,		/* Cisco Meraki MR16 */
> >> +	ATH79_MACH_MR600V2,		/* OpenMesh MR600v2 */
> > 
> > The ALLCAPS_PREFIX_ in the board file is... special? Have you seen that
> > anywhere else?
> 
> -EPARSE
> 
> 

- -EAGAIN?

>> +static const char *MINIBOX_V1_part_probes[] = {
>> +static struct flash_platform_data MINIBOX_V1_flash_data = {
>> +static struct gpio_led MINIBOX_V1_leds_gpio[] __initdata = {
>> +static struct gpio_keys_button MINIBOX_V1_gpio_keys[] __initdata = {
>> +static void __init MINIBOX_V1_setup(void)

I've never seen all caps used in the prefix of methods in board files. 
Truly, I'm not widely read, but that would seem to be very out of style.
 Is this a style you wish to encourage?  If so, please let the community know so we can patch other files to bring them inline.  

Sincerely,
Karl Palsson

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJV0wcWAAoJEBmotQ/U1cr2FUAQALqSPtknCawz8zqL5mfmjC4N
ynsNs6y3YQ8ZOc6RVTMdCLTvtNZmE1NZcrk7nAKVzrZBpXDCB6j6iPIal7fkM55m
+mrilQUwj2ZdFJ0Egfj4QFsdqc6LIth5WalpnkNqrnBsDEgUpUX6Vs+Xb8alJFhC
Iv+ILHvEif2ejlhM7Z/k39EzGsnDxLHUBY/tyJoVtJbOisO4oL73PXigjpx8cdrY
LcUoB3I84Pi2eLCo+iA2YOBioc3dR+ZY/lpSp+h0HF3XP8CYePsjbAa7H0LOJzC/
M44uXJoeGYQFSv23sVt6SZKFjFF6+rthMrf1WDLTH9NHU4venJsX/7Jn1XcVhcte
6QEZ60IYBTbXzw2GrN+p6dWYtGZia+blECXjhDCRqH6/72GdoD/XnVly4fH9M9HE
ekSjBMCCd7P6AWCwzf1z1Kg9adUQWOaajwSFyW75psb9f3p44tU8bpt0d82/aFam
+oCtcgjGoU6O7ndgH5QIYERIk9O65CZu90VQh5G2B1cYn1OE2ODoGMGr/yXNOWbs
t01vrbDtKKmkkpuyXwtDIulq8sOQhrAw0O37c7bIgGsH1CYUipEFyTDw2ngSalsO
7V3xsIRHK4/JHI4wUi17kK9r1h+5FSvFQIkHZuEyDR6/mdZ+GsO4zCf+YTghRjaK
bl+7Nsr+645uGFMuFfpb
=8o2o
-----END PGP SIGNATURE-----
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel