Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition
On 03/16, Dan Carpenter wrote: > Generally, it's better to fix the bug in the existing code, and then > do the cleanup later. That way the fixes can be backported to stable > kernels more easily. Hummm... the backport argue make a lot of sense for me. I will generate a v2 with the bug fixes in the beginning. Thanks > I don't know this subsystem very well. Perhaps Jonathan doesn't care > for one reason or another (like maybe he's not going to back port the > fix). So it might not matter how you do it. > > regards, > dan carpenter > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition
Generally, it's better to fix the bug in the existing code, and then do the cleanup later. That way the fixes can be backported to stable kernels more easily. I don't know this subsystem very well. Perhaps Jonathan doesn't care for one reason or another (like maybe he's not going to back port the fix). So it might not matter how you do it. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition
On 03/16, Dan Carpenter wrote:
> You're right that there is a bug but this is not the right fix.
>
> The ade7854_i2c_write_reg_32() function returns 6 on success which makes
> no sense. It should be zero or negative error codes. All the write_reg
> functions in drivers/staging/iio/meter/ade7854-i2c.c have the same bug.
>
> Please, fix that instead and leave ade7854_initial_setup() alone.
I see. However, I think the following steps could be better:
1) Update ade7854_i2c_write_reg() in order to return 0
The primary objective of this patchset it removes the duplications
related to write_reg_* and read_reg_*. As a result, in the first patch
create the function ade7854_i2c_write_reg(). I will add the following
code to fix the problem that we are discussing:
ade7854_i2c_write_reg() {
...
ret = i2c_master_send(st->i2c, st->tx, count);
...
return ret < 0 ? ret : 0;
}
With this, I do not need to touch any of the
ade7854_i2c_write_reg_(8|16|24|32) functions.
2) Drop out the last patch from the patchset
Is that ok? If so, I will send the V2 today.
> regards,
> dan carpenter
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition
You're right that there is a bug but this is not the right fix. The ade7854_i2c_write_reg_32() function returns 6 on success which makes no sense. It should be zero or negative error codes. All the write_reg functions in drivers/staging/iio/meter/ade7854-i2c.c have the same bug. Please, fix that instead and leave ade7854_initial_setup() alone. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition
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
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 7/7] staging:iio:ade7854: Add proper error handling condition
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.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 7/7] staging:iio:ade7854: Add proper error handling condition
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;
}
@@ -544,7 +544,7 @@ int ade7854_probe(struct iio_dev *indio_dev, struct device
*dev)
indio_dev->modes = INDIO_DIRECT_MODE;
ret = devm_iio_device_register(dev, indio_dev);
- if (ret)
+ if (ret < 0)
return ret;
/* Get the device into a sane initial state */
--
2.16.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
