Hi Jeff, Tomas, On Tue, Apr 25, 2023 at 10:39:49AM -0500, Jeff LaBundy wrote: > Hi Tomas, > > On Tue, Apr 25, 2023 at 03:00:53PM +0200, Tomas Mudrunka wrote: > > LM8333 uses gpio interrupt line which is active-low. > > When interrupt is set to FALLING edge and button is pressed > > before driver loads, driver will miss the edge and never respond. > > To fix this we handle ONESHOT LOW interrupt rather than edge. > > > > Signed-off-by: Tomas Mudrunka <tomas.mudrunka@xxxxxxxxx> > > --- > > drivers/input/keyboard/lm8333.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c > > index 7457c3220..c5770ebb2 100644 > > --- a/drivers/input/keyboard/lm8333.c > > +++ b/drivers/input/keyboard/lm8333.c > > @@ -179,7 +179,7 @@ static int lm8333_probe(struct i2c_client *client) > > } > > > > err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread, > > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > "lm8333", lm8333); > > if (err) > > goto free_mem; > > Thanks for the patch, but this is a NAK in my opinion. > > First of all, we should not be hard-coding interrupt polarity in the > first place; that is an existing piece of technical debt in this driver. Yes, I wonder if the original hardware was limited to the edge interrupts. > > Second, changing from edge-triggered to level-triggered interrupts runs > the risk of creating an interrupt storm depending on the time it takes > the device to deassert the irq following the I2C read and the point at > which the threaded handler returns. Have you measured this? IRQF_ONESHOT ensures that the level interrupt is unmasked only when the threaded handler returns. > > Can we not simply read the interrupt status registers once at start-up > to clear any pending status? This is essentially what your change does > anyway, albeit indirectly. > Thanks. -- Dmitry
