On 18/08/2015 12:21, Karl Palsson wrote:
> John Crispin <blo...@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@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
