Re: [PATCH 09/18] input: add input driver for polled GPIO buttons

[Gabor Juhos <juhosg@openwrt.org>]

Hi Ben,

> <...>
> I've tested this driver with the da850-evm pushbuttons and switches
> connected to i2c gpio expanders. It works well. The changes to the
> patch series were straightforward: .config, "gpio-keys" ->
> "gpio-buttons", struct gpio_key -> struct gpio_button etc.
> 
> I do have some comments about this patch. But the new driver is
> functional as-is.
> 
> Tested-by: Ben Gardiner <bengardiner@nanometrics.ca>

Thanks!

>><...>
> 
> Since the new gpio_buttons.c driver presents the same input event
> device as the gpio_keys.c driver, I think it should also be a
> drivers/input/keys device.

Makes sense.

> 
>>  [...]
>> diff --git a/drivers/input/misc/gpio_buttons.c b/drivers/input/misc/gpio_buttons.c
> 
> When I was converting the da850-evm platform code to use the new
> driver I felt that the changes did not indicate a switch to a polled
> driver as seems to be the intent with the introduction of a separate
> driver. All that was different in the platform code was 'button' where
> there use to be 'key' and button does not itself convey the knowledge
> that it is a polled input device.
> 
> I know names of drivers can be contentions but I will propose
> regardless that this driver be called gpio-polled-keys /
> gpio_polled_keys.c

I agree, this would be more informative.

>> <...>
>> +       for (i = 0; i < bdev->pdata->nbuttons; i++) {
>> +               struct gpio_button *button = &pdata->buttons[i];
>> +               struct gpio_button_data *bdata = &bdev->data[i];
>> +
>> +               if (bdata->count < button->threshold)
>> +                       bdata->count++;
>> +               else
>> +                       gpio_buttons_check_state(input, button, bdata);
> 
> I think that a count-theshold can still be performed here, but using
> the debounce_interval and polling_interval field specified in the
> gpio_button and gpio_buttons_platform_data structures, respectively,
> to calculate a threshold value.

Good idea. We don't even have to compute a threshold value, we can use the
debounce_interval and poll_interval fields directly. I mean something similar to
this:

<...>
	if (bdata->interval < button->debounce_interval)
		bdata->interval += pdata->poll_interval;
	else
		gpio_buttons_check_state(input, button, bdata);
<...>

> In this way the gpio_button and gpio_keys_button structs are made more
> similar -- differing only in the presence of .wakeup in
> gpio_keys_button but not in gpio_button. Which may make it possible to
> re-use the gpio_keys_button structure.
> 
>> <...>
>> +struct gpio_button {
>> +       int     gpio;           /* GPIO line number */
>> +       int     active_low;
>> +       char    *desc;          /* button description */
>> +       int     type;           /* input event type (EV_KEY, EV_SW) */
>> +       int     code;           /* input event code (KEY_*, SW_*) */
>> +       int     threshold;      /* count threshold */
> 
> Could we instead use the existing struct gpio_keys_button; we could
> transform debounce_interval into a threshold as described above

Sure, we can use that...

> and add an error when a gpio_button_probe() sees a gpio_key with .wakeup ==
> TRUE?

I don't think we should check that, we can simply ignore this field. Maybe we
should add a comment to the .wakeup field to state that the polled driver does
not use that.

> It seems that this structure duplicates alot of the gpio_keys_button
> structure.

Yes, it is almost the same.

>> [...]
>> +struct gpio_buttons_platform_data {
>> +       struct gpio_button *buttons;
>> +       int     nbuttons;               /* number of buttons */
>> +       int     poll_interval;          /* polling interval */
>> +};
> 
> I think the units of poll_interval should be included in the comment.
> i.e. /* polling interval in msecs */

Yes, you are right. Additionally, we should move this structure into
gpio_keys.h, so we can get rid of the gpio_buttons.h file.

Thank you for the valuable comments. I will create a new patch.

Regards,
Gabor