On 09/14/2015 05:34 PM, Luca Ceresoli wrote:
Dear Larry, All,
Luca Ceresoli wrote:
Dear Larry,
thanks for your review.
Larry Finger wrote:
On 08/28/2015 03:45 PM, Luca Ceresoli wrote:
Some of the report_*() functions are internal and will be declared
static,
and their declaration removed from rtw_mlme_ext.h, in a later commit.
This would break compilation, since they are are referenced before their
definition.
Reorder these functions so that symbols are defined before they are
referenced, without the need for forward declarations.
Also move near the beginning of the file the collect_bss_info() and
process_80211d(), since they are called from the report_*() functions.
This commit only reorders code, there is no content change.
Signed-off-by: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
Cc: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxx>
Even though you are just moving code, I think you should fix the long
line warnings from scripts/checkpatch, otherwise note that they are not
fixed here, but they will be fixed in a later patch. As for the other
warnings, they should be noted and fixed later.
How much strict is this rule?
Below is the reason for my question.
I understand your point. However, the coding style of rtw_mlme_ext.c is
so far away from the Linux requirements that cleaning it up is a
daunting task.
There are 151 lines over 80 characters, and several (maybe one third)
cannot or should not be fixed by simply adding newlines. Restructuring
the code is needed in some cases, and the complexity of the change
(and of the code review, correspondingly) can well be larger than the
whole impact of my first patchset. In some cases it will require more
understanding of the code that it took to produce this patchset.
Nevertheless I started the cleanup job, but it will take time. When
completed I expect the number of patches in this patchset to at least
double...
And it turns out I was too optimistic. rtw_mlme_ext.c is huge, and it's
a goldmine of coding style errors. The cleanup work is in progress, it
currently counts 50 patches (not including these 16 patches) and there
are still several checkpatch errors to fix, mostly lines over 80
characters.
The problem is most of these lines cannot or should not be fixed by just
adding newlines, as I told earlier. In several cases I first need to
restructure the code, which in turn makes new checkpatch errors visible
in my patches. To fix them I need more patches, which highlight more
errors etc etc... I have no idea how many patches are needed to converge
so I can submit a patchset with a clean
'git diff staging/staging-testing HEAD'.
Thus my question above, clarified: would you accept a patchset that:
1. fixes checkpatch and other coding style errors, and has minor
improvements;
2. does not introduce any coding style error;
3. _does_ have errors in
'git diff staging/staging-testing HEAD | ./scripts/checkpatch.pl -';
4. contains a written promise that the errors in point 3 will be fixed
by one or more later patchsets?
After these patchsets I'd be able to resubmit this one.
Just run checkpatch.pl on each patch that you submit. It should be clean, or you
must explain the specific reason for ignoring a warning. Errors are *never*
acceptable, I think that covers points 1, 2 and 4. Number 3 is only valid under
the conditions of point 4, and only with warnings, not errors.
Yes, cleaning up this driver would be a major task, which is why I have not done
any of it. In addition, many of the checkpatch warnings were not present when
the driver was first submitted. The script's requirements are a moving target.
My plan is to replace the driver with one that uses mac80211, and is programmed
more cleanly. You, however, are free to hack on this as much as you want. Just
do not break it as it is still needed.
Larry
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
