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 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
> 
> 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.


It's true the identifier transforms could be done with Coccinelle,
but the problem is larger than identifier types and line lengths.

Hungarian renaming can't really be automated, it's basically a
sed problem where the new identifiers have to be chosen by someone
with specific device knowledge.

Look at vt6655/rf.c:

Lots of things should be reduced/replaced/simplified.
For instance, these repeated patterns exist:

	"(BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW"
	"(BY_AL7230_REG_LEN << 3) + IFREGCTL_REGW"

There are 300+ uses just in this one file.

It's a lot of visual noise that should be minimized by using macros.
It would also reduce the number of checkpatch's long line warnings.

All the of unsigned char could be u8, unsigned short -> u16, etc.
Many arrays could be static const.

Nearly every function in the file could be improved.

For instance, this code could be written altogether:

bool RFbAL7230SelectChannelPostProcess(struct vnt_private *priv,
				       u16 byOldChannel,
				       u16 byNewChannel)
{
	bool ret;

	ret = true;

	/* if change between 11 b/g and 11a need to update the following
	 * register
	 * Channel Index 1~14
	 */
	if ((byOldChannel <= CB_MAX_CHANNEL_24G) && (byNewChannel > CB_MAX_CHANNEL_24G)) {
		/* Change from 2.4G to 5G [Reg] */
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[2]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[3]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[5]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[7]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[10]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[12]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[15]);
	} else if ((byOldChannel > CB_MAX_CHANNEL_24G) && (byNewChannel <= CB_MAX_CHANNEL_24G)) {
		/* Change from 5G to 2.4G [Reg] */
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTable[2]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTable[3]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTable[5]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTable[7]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTable[10]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTable[12]);
		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTable[15]);
	}

	return ret;
}

This could be something like: (written in email client, untested)

{
	unsigned long *table;
	static const int indices[] = {2, 3, 5, 7, 10, 12, 15};
	int i;
	bool ret = true;

	/* if changing between 11b/g and 11a, update the indices registers */

	if (byOldChannel <= CB_MAX_CHANNEL_24G && byNewChannel > CB_MAX_CHANNEL_24G)
		table = dwAL7230InitTableAMode;
	else if (byOldChannel > CB_MAX_CHANNEL_24G && byNewChannel <= CB_MAX_CHANNEL_24G)
		table = dwAL7230InitTable;
	else
		return ret;

	for (i = 0 ; i < ARRAY_SIZE(indices); i++)
		ret &= IFRFbWriteEmbedded(priv, table[indices[i]]);

	return ret;
}