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

[Greg KH <gregkh@linuxfoundation.org>]

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.
> >>
> >> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
> >> ---
> >>  drivers/staging/rts5208/sd.c | 11 ++++-------
> >>  1 file changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
> >> index d6c4982..4269be9 100644
> >> --- a/drivers/staging/rts5208/sd.c
> >> +++ b/drivers/staging/rts5208/sd.c
> >> @@ -2707,17 +2707,14 @@ RTY_SD_RST:
> >>               return STATUS_FAIL;
> >>       }
> >>
> >> +     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?
> >
> > thanks,
> >
> > greg k-h
> 
> Logic remains fine, I think. support_1v8 statement was common to if
> and else so it is taken out. Also, it can be noted that programmer
> wants to set sd_card value only if hi_cap_flow is not null and (rsp[1]
> & 0x40) is true. For rest of the cases, programmer wants to clear the
> sd_card.
> I think I'm sure about the changes made.

But now you always call CLR_SD_HCXC() where before there was one code
path that did not make that call.  Are you sure it is safe to call
CLR_SD_HCXC() before SET_SD_HCXC()?

thanks,

greg k-h