Messages in this thread | | | Date | Wed, 9 Jul 2014 17:28:59 +0900 | From | James Ban <> | Subject | Re: [PATCH V4] regulator: DA9211 : new regulator driver |
| |
> -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Wednesday, July 09, 2014 4:57 PM > To: Opensource [James Seong-Won Ban] > Cc: Liam Girdwood; Support Opensource; LKML; David Dajun Chen > Subject: Re: your mail > > On Wed, Jul 09, 2014 at 10:03:32AM +0900, James Ban wrote: > > > > > + ret = regmap_read(chip->regmap, DA9211_REG_EVENT_B, ®_val); > > > > + if (ret < 0) > > > > + goto error_i2c; > > > > > + if (reg_val & DA9211_E_OV_CURR_A) { > > > > > + if (reg_val & DA9211_E_OV_CURR_B) { > > > > > + return IRQ_HANDLED; > > > > This is buggy - the driver should only return IRQ_HANDLED if it > > > handled the interrupt somehow, otherwise it should return IRQ_NONE > > > and let the interrupt core handle things. This is especially > > > important since the device appears to require that interrupts are > > > explicitly acknoweldged so if something is flagged but not handled the > interrupt will just sit constantly asserted. > > > Basically all interrupts are masked when the chip wakes up. > > Only two interrupts are unmasked at the start of driver like below. > > I know that's the intention but the code should still be written robustly - > something might go wrong somewhere which causes another interrupt to be > enabled, or we might even gain support for shared threaded interrupts in the > interrupt core and someone could then try to use that in a system. How about below code for proper handle of interrupt? static irqreturn_t da9211_irq_handler(int irq, void *data) { struct da9211 *chip = data; int reg_val, ret;
ret = regmap_read(chip->regmap, DA9211_REG_EVENT_B, ®_val); if (ret < 0) goto error_i2c;
if (reg_val & DA9211_E_OV_CURR_A) { regulator_notifier_call_chain(chip->rdev[0], REGULATOR_EVENT_OVER_CURRENT, rdev_get_drvdata(chip->rdev[0]));
ret = regmap_write(chip->regmap, DA9211_REG_EVENT_B, DA9211_E_OV_CURR_A); if (ret < 0) goto error_i2c;
return IRQ_HANDLED; } else if (reg_val & DA9211_E_OV_CURR_B) { regulator_notifier_call_chain(chip->rdev[1], REGULATOR_EVENT_OVER_CURRENT, rdev_get_drvdata(chip->rdev[1]));
ret = regmap_write(chip->regmap, DA9211_REG_EVENT_B, DA9211_E_OV_CURR_B); if (ret < 0) goto error_i2c;
return IRQ_HANDLED; } else return IRQ_NONE;
error_i2c: dev_err(chip->dev, "I2C error : %d\n", ret); return IRQ_NONE; }
| |