LM8333 uses gpio interrupt line which is activated by falling edge.
When button is pressed before driver is loaded,
driver will miss the edge and never respond again.
To fix this we clear the interrupt via i2c after registering IRQ.

Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
---
 drivers/input/keyboard/lm8333.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
index 7457c3220..9a810ca00 100644
--- a/drivers/input/keyboard/lm8333.c
+++ b/drivers/input/keyboard/lm8333.c
@@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
 	if (err)
 		goto free_mem;
 
+	lm8333_read8(lm8333, LM8333_READ_INT);
+
 	err = input_register_device(input);
 	if (err)
 		goto free_irq;
-- 
2.40.0

Hi Tomas,

On Tue, Apr 25, 2023 at 06:49:03PM +0200, Tomas Mudrunka wrote:
> LM8333 uses gpio interrupt line which is activated by falling edge.
> When button is pressed before driver is loaded,
> driver will miss the edge and never respond again.
> To fix this we clear the interrupt via i2c after registering IRQ.
> 
> Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> ---
>  drivers/input/keyboard/lm8333.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> index 7457c3220..9a810ca00 100644
> --- a/drivers/input/keyboard/lm8333.c
> +++ b/drivers/input/keyboard/lm8333.c
> @@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
>  	if (err)
>  		goto free_mem;
>  
> +	lm8333_read8(lm8333, LM8333_READ_INT);
> +

This is the right idea. I am sort of splitting hairs here, however I
think it makes sense to place this read before the IRQ is requested
and not after.

As written, there is room for an ever-so-tiny race condition wherein
the IRQ is asserted just after it is requested. Before the threaded
handler has run however, the new read in probe swallows the IRQ status
before the threaded handler can read it and react to errors.

Also, I think you should at least capture and evaluate lm8333_read8()'s
return value as is already done for the calls to lm8333_write8().

>  	err = input_register_device(input);
>  	if (err)
>  		goto free_irq;
> -- 
> 2.40.0

Kind regards,
Jeff LaBundy

Hello, thanks for your notes.

> This is the right idea. I am sort of splitting hairs here, however I
> think it makes sense to place this read before the IRQ is requested
> and not after.
>
>
> As written, there is room for an ever-so-tiny race condition wherein
> the IRQ is asserted just after it is requested. Before the threaded
> handler has run however, the new read in probe swallows the IRQ status
> before the threaded handler can read it and react to errors.

In fact i believe quite the opposite case to be true.
If i read before registering IRQ there will be ever-so-tiny race condition that
would allow to miss the edge (exactly the bug this patch is fixing,
but limited).

In the case you describe the worst scenario is likely that the interrupt handler
will be called only to re-read status and immediately return on this condition:

if (!status) return IRQ_NONE;

> Also, I think you should at least capture and evaluate lm8333_read8()'s
> return value as is already done for the calls to lm8333_write8().

Well. If you think this will bring any benefits, i might as well just call
lm8333_irq_thread() instead of lm8333_read8()
Would that be acceptable solution?

Tom.

Hi Tomas,

On Thu, Apr 27, 2023 at 10:13:22AM +0200, Tomáš Mudruňka wrote:
> Hello, thanks for your notes.
> 
> > This is the right idea. I am sort of splitting hairs here, however I
> > think it makes sense to place this read before the IRQ is requested
> > and not after.
> >
> >
> > As written, there is room for an ever-so-tiny race condition wherein
> > the IRQ is asserted just after it is requested. Before the threaded
> > handler has run however, the new read in probe swallows the IRQ status
> > before the threaded handler can read it and react to errors.
> 
> In fact i believe quite the opposite case to be true.
> If i read before registering IRQ there will be ever-so-tiny race condition that
> would allow to miss the edge (exactly the bug this patch is fixing,
> but limited).

I thought the original problem is that the IRQ is already low by the time
the driver loads. Since a high-to-low transition (i.e. falling edge) is
never witnessed, the handler is never called to read the status and allow
the IRQ to go high again. Therefore, key events are gone forever.

The concern you mention is simply that of not responding to key events
until the interrupt handler is registered; there is no way around that.
Any event that occurs before then is off the table. Instead, we can only
make sure that none of those prior events place us in a bad state.

> 
> In the case you describe the worst scenario is likely that the interrupt handler
> will be called only to re-read status and immediately return on this condition:
> 
> if (!status) return IRQ_NONE;

Right, I am simply saying this is one key press that could have been
preserved. As a matter of principle, once the interrupt handler is live,
you should not disturb the precious read-on-clear registers on your own
without concurrency protection. It is much more common to clear suprious
interrupts and _then_ make the handler go live.

> 
> > Also, I think you should at least capture and evaluate lm8333_read8()'s
> > return value as is already done for the calls to lm8333_write8().
> 
> Well. If you think this will bring any benefits, i might as well just call
> lm8333_irq_thread() instead of lm8333_read8()
> Would that be acceptable solution?

Looking at the datasheet, it seems this devices builds up scan codes in
a FIFO. To protect against the rare case in which this dummy read includes
actual data, perhaps it is better to call lm8333_irq_thread() instead of
lm8333_read8() so that the FIFO is flushed.

> 
> Tom.

Kind regards,
Jeff LaBundy

LM8333 uses gpio interrupt line which is triggered by falling edge.
When button is pressed before driver is loaded,
driver will miss the edge and never respond again.
To fix this we run the interrupt handler after registering IRQ
to clear the interrupt via i2c command.

Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
---
 drivers/input/keyboard/lm8333.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
index 7457c3220..9a810ca00 100644
--- a/drivers/input/keyboard/lm8333.c
+++ b/drivers/input/keyboard/lm8333.c
@@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
 	if (err)
 		goto free_mem;
 
+	lm8333_irq_thread(client->irq, lm8333);
+
 	err = input_register_device(input);
 	if (err)
 		goto free_irq;
-- 
2.40.0

Hi Tomas,

On Fri, Apr 28, 2023 at 12:20:15PM +0200, Tomas Mudrunka wrote:
> LM8333 uses gpio interrupt line which is triggered by falling edge.
> When button is pressed before driver is loaded,
> driver will miss the edge and never respond again.
> To fix this we run the interrupt handler after registering IRQ
> to clear the interrupt via i2c command.
> 
> Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> ---
>  drivers/input/keyboard/lm8333.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> index 7457c3220..9a810ca00 100644
> --- a/drivers/input/keyboard/lm8333.c
> +++ b/drivers/input/keyboard/lm8333.c
> @@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
>  	if (err)
>  		goto free_mem;
>  
> +	lm8333_irq_thread(client->irq, lm8333);

Just to clarify, my stance is that this call should go _before_ the handler
is registered. Your earlier statement that doing so would steal any pending
status from the handler is correct; however, it is a moot point because the
handler cannot do anything with that status until the input device has been
registered anyway.

Any events that come before then are off the table, and this is OK because
user space isn't going to start consuming key events until well after this
driver has probed anyway.

The reason behind my assertion is that as a matter of best practice, you
should not have two asynchronous threads that can in theory access the same
register. You are correct that the handler would simply return IRQ_NONE in
such a race, but it sets a bad precedent and opens room for bugs in case
this driver is modified in the future. It also creates one unnecessary I2C
read.

This is why it is much more common to register the handler _after_ manually
accessing read-to-clear registers; the register access remains synchronous.
In case you feel I have misunderstood, please let me know.

> +
>  	err = input_register_device(input);
>  	if (err)
>  		goto free_irq;
> -- 
> 2.40.0

Kind regards,
Jeff LaBundy

> Just to clarify, my stance is that this call should go _before_ the handler
> is registered.

Ok, i will fix the patch later today.

> Any events that come before then are off the table, and this is OK because
> user space isn't going to start consuming key events until well after this
> driver has probed anyway.

Well, that was never my point. I don't care about capturing events
that happen before driver was properly loaded.
My only concern was to limit possibility of deadlock which happened previously.
Because that makes device unusable till the IC is power cycled.
Which might be especially annoying on devices that have power button
implemented using this exact IC :-)

> The reason behind my assertion is that as a matter of best practice, you
> should not have two asynchronous threads that can in theory access the same
> register.

Yeah, this makes bit more sense now. Didn't realized IRQ might
interrupt that lm8333_irq_thread() call immediately.
While not very likely to cause problems like deadlock of the driver, i
think it's a valid point.

After all this is what happens with IRQF_ONESHOT anyway right? Each
time the IRQ is triggered it's disabled, lm8333_irq_thread() is run
and then it's enabled immediately after that. So i guess the behaviour
on each keypress is very similar to calling lm8333_irq_thread() before
registering the IRQ handler, which gives me some confidence there
might not be huge chance for deadlock under normal circumstances.

Though i wonder what would happen if some EMI burst would create rapid
train of randomly timed keypresses, that might just hit the
unfortunate sweetspot after while... Might test that later in the lab,
since i really need the software to remain operational after such
condition had passed.

LM8333 uses gpio interrupt line which is triggered by falling edge.
When button is pressed before driver is loaded,
driver will miss the edge and never respond again.
To fix this we run the interrupt handler before registering IRQ
to clear the interrupt via i2c command.

Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
---
 drivers/input/keyboard/lm8333.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
index 7457c3220..52108c370 100644
--- a/drivers/input/keyboard/lm8333.c
+++ b/drivers/input/keyboard/lm8333.c
@@ -178,6 +178,8 @@ static int lm8333_probe(struct i2c_client *client)
 			dev_warn(&client->dev, "Unable to set active time\n");
 	}
 
+	lm8333_irq_thread(client->irq, lm8333);
+
 	err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread,
 				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				   "lm8333", lm8333);
-- 
2.40.1

Hi Tomas,

On Wed, May 03, 2023 at 05:32:31PM +0200, Tomas Mudrunka wrote:
> LM8333 uses gpio interrupt line which is triggered by falling edge.
> When button is pressed before driver is loaded,
> driver will miss the edge and never respond again.
> To fix this we run the interrupt handler before registering IRQ
> to clear the interrupt via i2c command.
> 
> Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> ---

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

>  drivers/input/keyboard/lm8333.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> index 7457c3220..52108c370 100644
> --- a/drivers/input/keyboard/lm8333.c
> +++ b/drivers/input/keyboard/lm8333.c
> @@ -178,6 +178,8 @@ static int lm8333_probe(struct i2c_client *client)
>  			dev_warn(&client->dev, "Unable to set active time\n");
>  	}
>  
> +	lm8333_irq_thread(client->irq, lm8333);
> +
>  	err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread,
>  				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  				   "lm8333", lm8333);
> -- 
> 2.40.1

Thank you for the productive discussion.

Kind regards,
Jeff LaBundy

On Wed, May 03, 2023 at 08:44:06PM -0500, Jeff LaBundy wrote:
> Hi Tomas,
> 
> On Wed, May 03, 2023 at 05:32:31PM +0200, Tomas Mudrunka wrote:
> > LM8333 uses gpio interrupt line which is triggered by falling edge.
> > When button is pressed before driver is loaded,
> > driver will miss the edge and never respond again.
> > To fix this we run the interrupt handler before registering IRQ
> > to clear the interrupt via i2c command.
> > 
> > Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> > ---
> 
> Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> 
> >  drivers/input/keyboard/lm8333.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> > index 7457c3220..52108c370 100644
> > --- a/drivers/input/keyboard/lm8333.c
> > +++ b/drivers/input/keyboard/lm8333.c
> > @@ -178,6 +178,8 @@ static int lm8333_probe(struct i2c_client *client)
> >  			dev_warn(&client->dev, "Unable to set active time\n");
> >  	}
> >  
> > +	lm8333_irq_thread(client->irq, lm8333);

So this is still racy, isn't it? The interrupt may come after read is
done, but before we register the handler.

> > +
> >  	err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread,
> >  				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> >  				   "lm8333", lm8333);
> > -- 
> > 2.40.1
> 

Thanks.

-- 
Dmitry

> So this is still racy, isn't it? The interrupt may come after read is
> done, but before we register the handler.

Well. It is. But please see the rest of the thread, where we've
already discussed this.

Every time the interrupt handler runs, the interrupt is disabled and
then reenabled after i2c communication is done. Which means this exact
thing happens on each keypress anyway. So i don't think it's a
necessarily huge deal. It might not be perfect solution, but it makes
things much better. window in which the deadlock condition can happen
is now in range of few ms (or us), instead of ~10 seconds (previously
it included bootloader and basicaly any moment from power up to driver
load)

Another solution would be to trigger on LOW instead of FALLING as
proposed in initial version of the patch. That would be safer in terms
of lm8333 deadlock, but Jeff was concerned about possibility of
interrupt storm taking down whole system in case the IRQ line gets
stuck in LOW for some reason...

Tom

pá 12. 5. 2023 v 1:44 odesílatel Dmitry Torokhov
<dmitry.torokhov@gmail.com> napsal:
>
> On Wed, May 03, 2023 at 08:44:06PM -0500, Jeff LaBundy wrote:
> > Hi Tomas,
> >
> > On Wed, May 03, 2023 at 05:32:31PM +0200, Tomas Mudrunka wrote:
> > > LM8333 uses gpio interrupt line which is triggered by falling edge.
> > > When button is pressed before driver is loaded,
> > > driver will miss the edge and never respond again.
> > > To fix this we run the interrupt handler before registering IRQ
> > > to clear the interrupt via i2c command.
> > >
> > > Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> > > ---
> >
> > Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> >
> > >  drivers/input/keyboard/lm8333.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> > > index 7457c3220..52108c370 100644
> > > --- a/drivers/input/keyboard/lm8333.c
> > > +++ b/drivers/input/keyboard/lm8333.c
> > > @@ -178,6 +178,8 @@ static int lm8333_probe(struct i2c_client *client)
> > >                     dev_warn(&client->dev, "Unable to set active time\n");
> > >     }
> > >
> > > +   lm8333_irq_thread(client->irq, lm8333);
>
> So this is still racy, isn't it? The interrupt may come after read is
> done, but before we register the handler.
>
> > > +
> > >     err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread,
> > >                                IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > >                                "lm8333", lm8333);
> > > --
> > > 2.40.1
> >
>
> Thanks.
>
> --
> Dmitry

Hi Tomas,

On Fri, May 12, 2023 at 06:55:08PM +0200, Tomáš Mudruňka wrote:
> > So this is still racy, isn't it? The interrupt may come after read is
> > done, but before we register the handler.
> 
> Well. It is. But please see the rest of the thread, where we've
> already discussed this.
> 
> Every time the interrupt handler runs, the interrupt is disabled and
> then reenabled after i2c communication is done. Which means this exact
> thing happens on each keypress anyway. So i don't think it's a
> necessarily huge deal. It might not be perfect solution, but it makes
> things much better. window in which the deadlock condition can happen
> is now in range of few ms (or us), instead of ~10 seconds (previously
> it included bootloader and basicaly any moment from power up to driver
> load)

Right, but the point is that there are some alternatives to reduce the
range to zero. You posted one already, but I mistakenly advised against
it due to my own oversight :)

> 
> Another solution would be to trigger on LOW instead of FALLING as
> proposed in initial version of the patch. That would be safer in terms
> of lm8333 deadlock, but Jeff was concerned about possibility of
> interrupt storm taking down whole system in case the IRQ line gets
> stuck in LOW for some reason...

Just to clarify, this is not my concern; all bets are off in case of
gross hardware failure such as this. Rather, my recommendations are:

1. Level (or edge) sensitivity should be specified in dts, not hard-coded
in the driver.

2. If you open support for level-triggered interrupts, you should verify
on a scope whether there is any chance that the IRQ line may still be in
the process of rising at the moment the read is completed. The datasheet
is ambiguous here.

> 
> Tom
> 
> pá 12. 5. 2023 v 1:44 odesílatel Dmitry Torokhov
> <dmitry.torokhov@gmail.com> napsal:
> >
> > On Wed, May 03, 2023 at 08:44:06PM -0500, Jeff LaBundy wrote:
> > > Hi Tomas,
> > >
> > > On Wed, May 03, 2023 at 05:32:31PM +0200, Tomas Mudrunka wrote:
> > > > LM8333 uses gpio interrupt line which is triggered by falling edge.
> > > > When button is pressed before driver is loaded,
> > > > driver will miss the edge and never respond again.
> > > > To fix this we run the interrupt handler before registering IRQ
> > > > to clear the interrupt via i2c command.
> > > >
> > > > Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> > > > ---
> > >
> > > Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> > >
> > > >  drivers/input/keyboard/lm8333.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> > > > index 7457c3220..52108c370 100644
> > > > --- a/drivers/input/keyboard/lm8333.c
> > > > +++ b/drivers/input/keyboard/lm8333.c
> > > > @@ -178,6 +178,8 @@ static int lm8333_probe(struct i2c_client *client)
> > > >                     dev_warn(&client->dev, "Unable to set active time\n");
> > > >     }
> > > >
> > > > +   lm8333_irq_thread(client->irq, lm8333);
> >
> > So this is still racy, isn't it? The interrupt may come after read is
> > done, but before we register the handler.
> >
> > > > +
> > > >     err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread,
> > > >                                IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > > >                                "lm8333", lm8333);
> > > > --
> > > > 2.40.1
> > >
> >
> > Thanks.
> >
> > --
> > Dmitry

Kind regards,
Jeff LaBundy

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 should handle ONESHOT LOW interrupt rather than edge.

Rather than hardcoding this, we simply remove the override from
driver by calling request_threaded_irq() with IRQF_TRIGGER_NONE flag.
This will keep interrupt trigger configuration as per devicetree. eg.:

	lm8333@51 {
		compatible = "ti,lm8333";
		interrupt-parent = <&gpio1>;
		interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
		...
	}

Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
---
 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_NONE | IRQF_ONESHOT,
 				   "lm8333", lm8333);
 	if (err)
 		goto free_mem;
-- 
2.40.0

Hi Tomas,

On Tue, Nov 14, 2023 at 01:30:23PM +0100, 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 should handle ONESHOT LOW interrupt rather than edge.
> 
> Rather than hardcoding this, we simply remove the override from
> driver by calling request_threaded_irq() with IRQF_TRIGGER_NONE flag.
> This will keep interrupt trigger configuration as per devicetree. eg.:
> 
> 	lm8333@51 {
> 		compatible = "ti,lm8333";
> 		interrupt-parent = <&gpio1>;
> 		interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
> 		...
> 	}
> 
> Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> ---
>  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_NONE | IRQF_ONESHOT,

This seems like the best approach; it solves the original problem, and
adopts the correct design pattern of allowing the dts to specify details
about the interrupt polarity and sensitivity.

My only feedback is that I think you can simply drop IRQF_TRIGGER_FALLING
altogether instead of replacing it with IRQF_TRIGGER_NONE; it is pointless
to bitwise OR against zero, and almost no drivers do this. It really should
only be used unless there are quite literally no flags to use. Passing only
IRQF_ONESHOT is sufficient here.

Assuming you agree with this change, please feel free to add the following
for v7:

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

>  				   "lm8333", lm8333);
>  	if (err)
>  		goto free_mem;
> -- 
> 2.40.0

Kind regards,
Jeff LaBundy

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 should handle ONESHOT LOW interrupt rather than edge.

Rather than hardcoding this, we simply remove the override from
driver by calling request_threaded_irq() without specifying trigger.
This will keep interrupt trigger configuration as per devicetree. eg.:

	lm8333@51 {
		compatible = "ti,lm8333";
		interrupt-parent = <&gpio1>;
		interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
		...
	}

Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
---
 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_ONESHOT,
 				   "lm8333", lm8333);
 	if (err)
 		goto free_mem;
-- 
2.40.0

Hi guys! Been a while. Is there anything else blocking this
from being merged?

Best regards. Tom

Hi Dmitry,

On Thu, May 11, 2023 at 04:44:08PM -0700, Dmitry Torokhov wrote:
> On Wed, May 03, 2023 at 08:44:06PM -0500, Jeff LaBundy wrote:
> > Hi Tomas,
> > 
> > On Wed, May 03, 2023 at 05:32:31PM +0200, Tomas Mudrunka wrote:
> > > LM8333 uses gpio interrupt line which is triggered by falling edge.
> > > When button is pressed before driver is loaded,
> > > driver will miss the edge and never respond again.
> > > To fix this we run the interrupt handler before registering IRQ
> > > to clear the interrupt via i2c command.
> > > 
> > > Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> > > ---
> > 
> > Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> > 
> > >  drivers/input/keyboard/lm8333.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> > > index 7457c3220..52108c370 100644
> > > --- a/drivers/input/keyboard/lm8333.c
> > > +++ b/drivers/input/keyboard/lm8333.c
> > > @@ -178,6 +178,8 @@ static int lm8333_probe(struct i2c_client *client)
> > >  			dev_warn(&client->dev, "Unable to set active time\n");
> > >  	}
> > >  
> > > +	lm8333_irq_thread(client->irq, lm8333);
> 
> So this is still racy, isn't it? The interrupt may come after read is
> done, but before we register the handler.

You're absolutely correct; I had not considered this corner case. Apologies
for the churn Tomas.

In that case, it seems the solution is to either move the dummy read after
the handler is registered as in v4, or remove the hard-coded flag and allow
dts to specify level sensitivity.

> 
> > > +
> > >  	err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread,
> > >  				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > >  				   "lm8333", lm8333);
> > > -- 
> > > 2.40.1
> > 
> 
> Thanks.
> 
> -- 
> Dmitry

Kind regards,
Jeff LaBundy

LM8333 uses gpio interrupt line which is triggered by falling edge.
When button is pressed before driver is loaded,
driver will miss the edge and never respond again.
To fix this we run the interrupt handler after registering IRQ
to clear the interrupt via i2c command.

Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
---
 drivers/input/keyboard/lm8333.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
index 7457c3220..9a810ca00 100644
--- a/drivers/input/keyboard/lm8333.c
+++ b/drivers/input/keyboard/lm8333.c
@@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
 	if (err)
 		goto free_mem;
 
+	lm8333_irq_thread(client->irq, (void *) lm8333);
+
 	err = input_register_device(input);
 	if (err)
 		goto free_irq;
-- 
2.40.0