Re: [Outreachy kernel] [PATCH] Staging: rts5208: sd: Remove redundant code

[Arnd Bergmann <arnd@arndb.de>]

On Wednesday 14 October 2015 05:15:12 Shivani Bhardwaj wrote:
> On Wed, Oct 14, 2015 at 5:09 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Oct 14, 2015 at 04:54:45AM +0530, Shivani Bhardwaj wrote:
> >> On Wed, Oct 14, 2015 at 4:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> > On Wed, Oct 14, 2015 at 04:30:10AM +0530, Shivani Bhardwaj wrote:
> >> >> Change the order of statements and remove extra code.
> >> >> +     CLR_SD_HCXC(sd_card);
> >> >> +
> >> >>       if (hi_cap_flow) {
> >> >>               if (rsp[1] & 0x40)
> >> >>                       SET_SD_HCXC(sd_card);
> >> >> -             else
> >> >> -                     CLR_SD_HCXC(sd_card);
> >> >> -
> >> >> -             support_1v8 = false;
> >> >> -     } else {
> >> >> -             CLR_SD_HCXC(sd_card);
> >> >> -             support_1v8 = false;
> >> >>       }
> >> >> +
> >> >> +     support_1v8 = false;
> >> >>       dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
> >> >>
> >> >>       if (support_1v8) {
> >> >
> >> > Are you sure you didn't change the logic of what is happening here?
> >> >
> >> > Why are you making this change?  What prompted it?
...

> I don't think it should be a problem because that is what we
> ultimately want in our code - to clear the memory card subject to
> constraints that one of the two if conditions is wrong. Now, if both
> the if statements, that is,
> if (hi_cap_flow) and if (rsp[1] & 0x40)
> 
> turn out to be true, they set the cleared sd_card again.
> 
> That is what we wanted. If both the if statements are true - set the
> sd_card else clear it up. 

I guess this would be easier to verify if it were not obscured by the
SET_SD_HCXC/CLR_SD_HCXC macros ;-)

Your change looks like a correct transformation, and it would be logical
to remove the support_1v8 variable next, as it can never be true.

However, I wonder whether that is just a bug in the driver, as UHS-1
cards are required to support 1.8v operation, and there is clearly
code to handle it here. Maybe one of them should actually say
"support_1v8 = true".

	Arnd