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

[John Crispin]

On 18/08/2015 12:21, Karl Palsson wrote:
> 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)
> 

these certainly need to be fixed ...

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

aha, its a rethoric question ... no, that that is not encouraged and
please don't send patches for that kind of stuff as it would not make
much sense

> 
> Sincerely,
> Karl Palsson
> 
> 
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel