Re: [PATCHv2] staging: pi433: pi433_if.c remove SET_CHECKED macro

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux