On Mon, Dec 04, 2017 at 12:19:02PM +0100, Nguyen Phan Quang Minh wrote: > On Mon, Dec 04, 2017 at 11:51:32AM +0300, Dan Carpenter wrote: > > On Sun, Dec 03, 2017 at 01:09:49AM +0100, Nguyen Phan Quang Minh wrote: > > > @@ -335,6 +406,7 @@ pi433_receive(void *data) > > > struct spi_device *spi = dev->spi; /* needed for SET_CHECKED */ > > > int bytes_to_read, bytes_total; > > > int retval; > > > + int ret; > > > > > > > You might have to write a v2 depending on what order Greg applies > > patches and if so then don't introduce a new "ret" variable when we > > already have "retval". Otherwise this looks good. Thanks for doing it. > > > > regards, > > dan carpenter > > > > I use another variable because the author had a SET_CHECKED right above > a return retval so I suspect they wanted to use that retval value and do > not want to overwrite it. Sorry, I dropped the Greg and list from the CC because I didn't want you to have to redo the patch if no one else had an issue... Very sneaky of me. But actually, you probably should redo the patch. I bet we should call wake_up_interruptible(&dev->tx_wait_queue); even if the rf69_set_mode(dev->spi, standby) call fails. The original code looks like this. SET_CHECKED() has a hidden return: drivers/staging/pi433/pi433_if.c 468 /* rx done, wait was interrupted or error occurred */ 469 abort: 470 dev->interrupt_rx_allowed = true; 471 SET_CHECKED(rf69_set_mode(dev->spi, standby)); 472 wake_up_interruptible(&dev->tx_wait_queue); 473 474 if (retval) 475 return retval; 476 else 477 return bytes_total; The awkward thing is that there are piles of patches to pi433 floating around and they all step on each other's toes a bit. regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
