Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <2a080d9c5dc6206d9a255410a61d1d9cce26d944.camel@perches.com>
Subject: Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: sdio_halinit:
 Remove unnecessary conditions
From:   Joe Perches <joe@perches.com>
To:     Shreeya Patel <shreeya.patel23498@gmail.com>,
        gregkh@linuxfoundation.org, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org, outreachy-kernel@googlegroups.com,
        sbrivio@redhat.com, daniel.baluta@gmail.com,
        nramas@linux.microsoft.com, hverkuil@xs4all.nl,
        Larry.Finger@lwfinger.net
Date:   Wed, 11 Mar 2020 17:15:14 -0700
In-Reply-To: <1CF27D55-EEB4-4A75-B767-A30845BD5E1B@gmail.com>
References: <20200311133811.2246-1-shreeya.patel23498@gmail.com>
         <0fed25f914c6f39b024dd3bbc4f11892c40f4a60.camel@perches.com>
         <1CF27D55-EEB4-4A75-B767-A30845BD5E1B@gmail.com>
Content-Type: text/plain; charset="ISO-8859-1"
User-Agent: Evolution 3.34.1-2 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 2020-03-12 at 03:58 +0530, Shreeya Patel wrote:
> Hey Joe,
> 
> On March 11, 2020 10:56:29 PM GMT+05:30, Joe Perches <joe@perches.com> wrote:
> > On Wed, 2020-03-11 at 19:08 +0530, Shreeya Patel wrote:
> > > Remove if and else conditions since both are leading to the
> > > initialization of "valueDMATimeout" and "valueDMAPageCount" with
> > > the same value.
> > 
> > You might consider removing the
> > 	/* Timeout value is calculated by 34 / (2^n) */
> > comment entirely as it doesn't make much sense.
> > 
> 
> You want me to remove the other comments as well?
> Since Julia suggested in another email that the comments are not useful if we are removing the condition since they were applied to only one branch ( i.e. "if" branch )

It's not an either/or/both situation.

Code like:

	if (test) {
		[block 1]
	} else {
		[block 2]
	}

where the contents of block 1 and block 2 are identical
exist for many reasons.  All of them need situational
analysis to determine what the right thing to do is.

Sometimes it's a defect from a copy/paste where some other
code was intended on one of the blocks and so that one path
has a defect somewhere and actually needs to be changed.

Sometimes the blocks were originally different, but some
code was removed from one of the blocks that lead to the
blocks being identical.  Those can be consolidated.

Sometimes test can be removed, sometimes not.

For hardware drivers like this, it's sometimes useful to
look at the latest code from the vendor and sometimes it's
better to ask the vendor what the right thing do is.

It does appear in this case that the branches should be
consolidated and comments in both branches removed.