Re: [PATCH] net:ethernet:intel:Remove outdated fix me comment in the function, gb_acquire_swfw_sync_i210

[Nick Krause <xerofoify@gmail.com>]

On Mon, Feb 9, 2015 at 12:03 PM, Hisashi T Fujinaka <htodd@twofifty.com> wrote:
> On Mon, 9 Feb 2015, Bj?rn Mork wrote:
>
>> Jeff Kirsher <jeffrey.t.kirsher@intel.com> writes:
>>
>>> If you want to see Nick's patch, feel free to view his patch on
>>> my queue tree:
>>> https://git.kernel.org/cgit/linux/kernel/git/jkirsher/queue.git/
>>
>> which said:
>>  - s32 i = 0, timeout = 200; /* FIXME: find real value to use here */
>>  + s32 i = 0, timeout = 200;
>>
>>
>> Comments like the one deleted by that patch do have some value in my
>> opinion: They document that the constant is chosen arbitrarily and is
>> not taken from some datasheet.
>>
>> Not a big deal, but leaving such comments, even if the value never ever
>> changes, could save someone from trying to figure out where the heck you
>> got that constant.  And there's noone actually misinterpreting this
>> comment as "something needs to be fixed here", is there? So the cost of
>> leaving the comment is exactly zero.
>>
>> Just my .02 ?.  I'm not going to tell you how to maintain your driver,
>> of course.  At least I try not to :-)
>
> I think, for now, we should NAK this. It's a short comment and you don't
> want me to replace it with a philosophical statement on how you could
> tune the timeout. That's what the data sheet is for.
>
> (Replying from my home account and not my work account -
> todd.fujinaka@intel.com)
>
> ------------------------------------------------------------------------------
> Dive into the World of Parallel Programming. The Go Parallel Website,
> sponsored by Intel and developed in partnership with Slashdot Media, is your
> hub for all things parallel software development, from weekly thought
> leadership blogs to news, videos, case studies, tutorials and more. Take a
> look and join the conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> E1000-devel mailing list
> E1000-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/e1000-devel
> To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired
That's fine, however if that is the case why not just explain the
reasons for these values in
another patch with a added comment rather then just removing the FIXME.
 Nick

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired