From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-f193.google.com ([209.85.220.193]:36567 "EHLO mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932766AbeCPA2G (ORCPT ); Thu, 15 Mar 2018 20:28:06 -0400 Date: Thu, 15 Mar 2018 21:28:00 -0300 From: Rodrigo Siqueira To: Dan Carpenter Cc: Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Greg Kroah-Hartman , Barry Song <21cnbao@gmail.com>, John Syne , linux-iio@vger.kernel.org, devel@driverdev.osuosl.org, daniel.baluta@nxp.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition Message-ID: <20180316002800.mvy75in42y5j2dpz@smtp.gmail.com> References: <72d5aaef3445edaca08498b4d7aaff19d4ac0232.1521037060.git.rodrigosiqueiramelo@gmail.com> <20180315121115.iwkx2dfikgpjeeec@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <20180315121115.iwkx2dfikgpjeeec@mwanda> Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org On 03/15, Dan Carpenter wrote: > On Wed, Mar 14, 2018 at 03:12:18PM -0300, Rodrigo Siqueira wrote: > > There is some improper error handling for IRQ and device register. This > > patch adds a proper verification. The IRQ correction was extracted from > > John Syne patches. > > > > Signed-off-by: Rodrigo Siqueira > > Signed-off-by: John Syne > > --- > > drivers/staging/iio/meter/ade7854.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c > > index 09fd8c067738..49cbe365e43d 100644 > > --- a/drivers/staging/iio/meter/ade7854.c > > +++ b/drivers/staging/iio/meter/ade7854.c > > @@ -436,7 +436,7 @@ static int ade7854_initial_setup(struct iio_dev *indio_dev) > > > > /* Disable IRQ */ > > ret = ade7854_set_irq(dev, false); > > - if (ret) { > > + if (ret < 0) { > > dev_err(dev, "disable irq failed"); > > goto err_ret; > > } > > Why is the original wrong? It seems fine. Hi, If you look at "drivers/staging/iio/meter/ade7854-(i2c|spi).c", you will find a line like this: " st->write_reg = ade7854_(i2c|spi)_write_reg;". Than, if you go through the "ade7854_i2c_write_reg" (focus only on i2c for while) you will see "i2c_master_send(st->i2c, st->tx, count)", which can returns a positive number that does not necessary means an error. If you find the same for the spi case, you will end up in the function "spi_sync_transfer" that only return 0 for success or negative for failure. So, handling the negative case better fit the i2c and spi case. Thanks > regards, > dan carpenter > >