linux-kernel - Re: [PATCH v2] staging: rts5208: Clean up coding style in rtsx_chip.c to get rid of checkpatch.pl warnings and combine two ifs into one

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1412267991.3247.64.camel@joe-AO725>
Date:	Thu, 02 Oct 2014 09:39:51 -0700
From:	Joe Perches <joe@...ches.com>
To:	Giedrius Statkevicius <giedrius.statkevicius@...il.com>
Cc:	gregkh@...uxfoundation.org, micky_ching@...lsil.com.cn,
	fabio.falzoi84@...il.com, mahati.chamarthy@...il.com,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] staging: rts5208: Clean up coding style in
 rtsx_chip.c to get rid of checkpatch.pl warnings and combine two ifs into
 one

On Thu, 2014-10-02 at 18:25 +0300, Giedrius Statkevicius wrote:
> On 2014.10.02 14:45, Joe Perches wrote:
> > On Thu, 2014-10-02 at 14:13 +0300, Giedrius Statkevicius wrote:
> >> Fix 10 occurences of coding style errors where the line exceeds 80 characters by breaking them into more lines.
> >> Checkpatch.pl reports these errors as: 'WARNING: line over 80 characters'
> >> Also, removes unnecessary returns in two void functions by removing the return statements.
> >> Checkpatch.pl reports these errors as: 'WARNING: void function return statements are not generally useful'
> >> Lastly it combines two if statements into one in rtsx_reset_chip()
> > 
> > Another way to reduce indentation is to write another
> > utility function like the suggested patch at the end
> > of this email.
[]
> > Using ternaries can also reduce line count and
> > indentation:
> > 	if (something)
> > 		foo(bar, 1, ...);
> > 	else
> > 		foo(bar, 2, ...);
> > can be written
> > 	foo(bar, something ? 1 : 2, ...)
> First of all, the purpose of this trivial patch is to clean the
> simple, obvious coding style mistakes reported by checkpatch.pl to get
> my feet wet as I am a kernel newbie.

Hello Giedrius.

By itself, that's fine.  A mechanical change as a first
contribution as a means to do larger changes is very
welcome, as I hope you are welcomed here.

While mechanical changes have some value, the purpose
of a tool like checkpatch isn't any significant
transformational change, it's a trivial oversight minder.

Fixing checkpatch bleats won't make code "better".

checkpatch really only helps code style conformity.
It tries to help make code a bit more like other code
already in the kernel.
 
People play a larger role in looking at code and
making it cleaner, neater, more intelligible, better
structured, smaller, faster, maintainable, etc.

> Obviously, if we take a look at the code more closely there are many
> problems including the ones you've mentioned.
> For example, in rts5208_init() in atleast in 6 places we can use
> ternaries. This is one of them:
> 
> 	if (val & 0x01)
> 		chip->hw_bypass_sd = 1;
> 	else
> 		chip->hw_bypass_sd = 0; 
> 
> A lot of places where we can combine if's. In rtsx_disable_aspm():
> 
> 	if (chip->aspm_l0s_l1_en && chip->dynamic_aspm) {
> 		if (chip->aspm_enabled) {
> 
> So I guess I should make a way bigger patch that cleans more of the
> coding style mistakes if no one wants to take this patch?
> To be honest, I would like these to be two seperate patches as they do
> different things and not just a big one that does both things as I may
> not be able to do that sucessfully.

Have at it.

Start small, learn a bit, do more, repeat ad libitum...

A series of patches that do single things is much
easier for others to validate, accept and apply.

As long as patches are done in sets of single things,
don't fear making larger transformational changes too.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/