On Sat, Apr 21 2018, Christian Luetke wrote: > Am 21.04.2018 00:22, schrieb NeilBrown: >> On Wed, Apr 18 2018, Christian Lütke-Stetzkamp wrote: >> >>> Fix checkpatch: multiple assignments should be avoided, to improve >>> readability. >>> >>> Signed-off-by: Christian Lütke-Stetzkamp <christian@xxxxxxxx> >>> --- >>> drivers/staging/mt7621-mmc/sd.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/staging/mt7621-mmc/sd.c >>> b/drivers/staging/mt7621-mmc/sd.c >>> index 3871602b4651..357c10551773 100644 >>> --- a/drivers/staging/mt7621-mmc/sd.c >>> +++ b/drivers/staging/mt7621-mmc/sd.c >>> @@ -1473,11 +1473,12 @@ static int msdc_do_request(struct mmc_host >>> *mmc, struct mmc_request *mrq) >>> >>> /* deside the transfer mode */ >>> if (drv_mode[host->id] == MODE_PIO) >>> - host->dma_xfer = dma = 0; >>> + host->dma_xfer = 0; >>> else if (drv_mode[host->id] == MODE_DMA) >>> - host->dma_xfer = dma = 1; >>> + host->dma_xfer = 1; >>> else if (drv_mode[host->id] == MODE_SIZE_DEP) >>> - host->dma_xfer = dma = ((host->xfer_size >= dma_size[host->id]) ? >>> 1 : 0); >>> + host->dma_xfer = ((host->xfer_size >= dma_size[host->id]) ? 1 : >>> 0); >>> + dma = host->dma_xfer; >> >> You've changed behaviour here. >> Previously, if none of the 3 conditions were true, dma would have the >> value of 0 that it was initialized to. >> No it will take the value of host->dma_xfer from the previous transfer. >> I doubt that is correct. >> >> I would change the if branches to assign to dma, not to host->dma_xfer. >> Then "host->dma_xfer = dma;". This isn't *quite* what the original >> code >> does, but I think it is close enough. > > But drv_mode has the type msdc_mode, that is an enum that has only the > elements MODE_PIO, MODE_DMA and MODE_SIZE_DEP. So the original code is > not > optimal, maybe the last else if should be an else. Shall I change the > patch to > respect that? Or should I change it to a switch case? (Also this code > will be > removed in the next series, as you suggested, what I already thought of > - > removing the non DMA mode. ) The patch worried me because it claimed to just fix a check-patch warning, but it appears to change behaviour. That happens too easily and can be very problematic. So I jumped on it. I would be happy if you: - explained in the patch description why the code is actually correct, even though it isn't obvious. - changed the last "else if" to "else" - again it requires an explanation - changed to a switch - with explanation. Given that we are likely to discard the code, I really don't care which. If we were going to keep the code, I would prefer the switch. Thanks, NeilBrown > > Thanks for the review. > > Regards, > Christian
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
