Re: Forest Bond <forest@alittletooquiet.net>,Greg Kroah-Hartman <gregkh@linuxfoundation.org>,devel@driverdev.osuosl.org,linux-kernel@vger.kernel.org

[Joe Perches <joe@perches.com>]

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.