Re: [PATCH 2/3] auxdisplay: ht16k33: rework input device initialization

[Robin van der Gracht]

On Tue, 31 Jan 2017 12:54:37 -0800
Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:

...

> +
> +static irqreturn_t ht16k33_keypad_irq_thread(int irq, void *dev)
> +{
> +	struct ht16k33_keypad *keypad = dev;
> +
> +	do {
> +		wait_event_timeout(keypad->wait, keypad->stopped,
> +				    msecs_to_jiffies(keypad->debounce_ms));
> +		if (keypad->stopped)
> +			break;
> +	} while (ht16k33_keypad_scan(keypad));

I like how you worked this one out! Its way cleaner and simpler.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ht16k33_keypad_start(struct input_dev *dev)
> +{
> +	struct ht16k33_keypad *keypad = input_get_drvdata(dev);
> +
> +	keypad->stopped = false;
> +	mb();
> +	enable_irq(keypad->client->irq);
> +
> +	return 0;
> +}
> +
> +static void ht16k33_keypad_stop(struct input_dev *dev)
> +{
> +	struct ht16k33_keypad *keypad = input_get_drvdata(dev);
> +
> +	keypad->stopped = true;
> +	mb();
> +	wake_up(&keypad->wait);
> +	disable_irq(keypad->client->irq);
> +}

Nice!

> +
> +static int ht16k33_keypad_probe(struct i2c_client *client,
> +				struct ht16k33_keypad *keypad)
> +{

...

> +
> +	err = devm_request_threaded_irq(&client->dev, client->irq,
> +					NULL, ht16k33_keypad_irq_thread,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +					DRIVER_NAME, keypad);

I believe the interrupt trigger should be switched to IRQF_TRIGGER_HIGH
combined with IRQF_ONESHOT, for this new setup. This is causing a race
condition.

In the previous situation a 'key data' read was triggered by
ht16k33_keypad_start(). This cleared the IRQ and gave a (somewhat racy)
known state. Now, when the IRQ line is HIGH during probe keyscan
wont work. 

Also, when ht16k33_keypad_stop() is run while the irq thread is waiting
for debounce, the scan will be skipped and irq will never be cleared.
This also breaks keyscan.

Regards,
Robin