Messages in this thread | | | Subject | Re: Forest Bond <>,Greg Kroah-Hartman <>,, | From | Joe Perches <> | Date | Mon, 08 Jun 2020 16:33:06 -0700 |
| |
On Mon, 2020-06-08 at 22:58 +0000, Rodolfo C Villordo wrote: > On Mon, Jun 08, 2020 at 01:41:11AM -0700, Joe Perches wrote: > > On Mon, 2020-06-08 at 07:59 +0200, Julia Lawall wrote: > > > On Mon, 8 Jun 2020, Al Viro wrote: > > > > > > > On Sun, Jun 07, 2020 at 10:41:56PM +0000, Rodolfo C. Villordo wrote: > > > > > Multiple line over 80 characters fixes by splitting in multiple lines. > > > > > Warning found by checkpatch.pl > > > > > > > > I doubt that checkpatch.pl can catch the real problems there: > > > > > > > > * Hungarian Notation Sucks. Really. > > > > * so does CamelCase, especially for wonders like s_uGetRTSCTSRsvTime > > Yes, I agree with that. > > > > Rodolfo, > > > > > > If you work hard with Coccinelle and python scripting, it can help with > > > the first two problems. > > > > These VIA vt6655/vt6656 drivers have been in staging for more than > > a decade. There are relatively few checkpatch coding style > > cleanups to do but there are many overall style issues to resolve. > > > > Yes, vt6655/rxtx.c needs lots of work. I was avoiding submit bigger changes > because this is my second patch submission. > > Thank you all for the comments. I'm really sorry for the odd subject. > > How should I move forward with this? > > 1 - Update this patch with the changes pointed by Dan Carpenter?
Keep your changes small until you really know how this style of linux kernel staging changes is done.
> 2 - Do a more elaborated and bigger change, like suggested by Al Viro > and Joe Perches?
A patch series is much preferred to a single large change.
If you decide to refactor various functions, please do that in separate, discrete patches.
Adding a #define and doing a sed like:
$ sed -i 's/(BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW/AL2230_RLEN_CTL/' drivers/staging/vt6655/*.[ch]
should be a single patch.
And if you do that, another should be done for AL7230
$ sed -i 's/(BY_AL7230_REG_LEN << 3) + IFREGCTL_REGW/AL7230_RLEN_CTL/' drivers/staging/vt6655/*.[ch]
etc...
Maybe the #define BY_AL2230_REG_LEN should be 0x17 so that the << 3 is more obviously constrained to the low byte
Maybe the + uses in the macros should be bitwise |.
Go wild after you figure out the process, just keep your patches to obvious, small and verifiable changes.
| |