[PATCH 2/6] mfd: mc13xxx-core: ADC conv: wait_for_completion returns a long
From: Uwe Kleine-König <hidden>
Date: 2012-01-30 07:24:53
On Mon, Jan 30, 2012 at 09:33:24AM +1100, Marc Reilly wrote:
Use the correct return type for wait_for_completion, as long may be larger than int.
That's a theoretical problem only because the return value should be in the range -ESOMETHING ... HZ which fits into an int.
quoted hunk ↗ jump to hunk
Signed-off-by: Marc Reilly <redacted> --- drivers/mfd/mc13xxx-core.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-)diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c index 8cb83ef..afff892 100644 --- a/drivers/mfd/mc13xxx-core.c +++ b/drivers/mfd/mc13xxx-core.c@@ -513,6 +513,7 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode, { u32 adc0, adc1, old_adc0; int i, ret; + long timeout; struct mc13xxx_adcdone_data adcdone_data = { .mc13xxx = mc13xxx, };@@ -566,20 +567,23 @@ int mc13xxx_adc_do_conversion(struct mc13xxx *mc13xxx, unsigned int mode, mc13xxx_handler_adcdone, __func__, &adcdone_data); mc13xxx_reg_write(mc13xxx, MC13XXX_ADC0, adc0); - mc13xxx_reg_write(mc13xxx, MC13XXX_ADC1, adc1); + ret = mc13xxx_reg_write(mc13xxx, MC13XXX_ADC1, adc1);
Is this change intended? I guess without it you get a warning that ret is used uninitialized, but if mc13xxx_reg_write fails you should IMHO return at once.
mc13xxx_unlock(mc13xxx);
- ret = wait_for_completion_interruptible_timeout(&adcdone_data.done, HZ);
+ timeout = wait_for_completion_interruptible_timeout(&adcdone_data.done, HZ);
- if (!ret)
+ if (timeout <= 0) {
+ dev_warn(mc13xxx->dev,
+ "timed out waiting for ADC completion\n");
ret = -ETIMEDOUT;
+ }I think this is wrong. wait_for_completion_interruptible_timeout returns -ERESTARTSYS if it was interrupted. That's not a timeout and -ERESTARTSYS should be propagated then. !ret is the correct test for timeout.
mc13xxx_lock(mc13xxx); mc13xxx_irq_free(mc13xxx, MC13XXX_IRQ_ADCDONE, &adcdone_data); - if (ret > 0) + if (!ret)
This is wrong, too, isn't it?
for (i = 0; i < 4; ++i) {
ret = mc13xxx_reg_read(mc13xxx,
MC13XXX_ADC2, &sample[i]);Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |