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@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_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@xxxxxxxxxxx>
> "lm8333", lm8333);
> if (err)
> goto free_mem;
> --
> 2.40.0
Kind regards,
Jeff LaBundy
