On Tue, Nov 16, 2010 at 4:36 AM, Paul Mundt <lethal@xxxxxxxxxxxx> wrote:
> On Mon, Nov 15, 2010 at 02:07:50PM -0500, Ben Gardiner wrote:
>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>> index 6069abe..477802a 100644
>> --- a/drivers/input/keyboard/gpio_keys.c
>> +++ b/drivers/input/keyboard/gpio_keys.c
>> @@ -44,6 +48,26 @@ struct gpio_keys_drvdata {
>> struct gpio_button_data data[0];
>> };
>>
>> +#if !defined(CONFIG_INPUT_POLLDEV) && !defined(CONFIG_INPUT_POLLDEV_MODULE)
>> +inline struct input_polled_dev *input_allocate_polled_device(void)
>> +{
>> + return NULL;
>> +}
>> +
>> +inline void input_free_polled_device(struct input_polled_dev *poll_dev)
>> +{
>> +}
>> +
>> +inline int input_register_polled_device(struct input_polled_dev *dev)
>> +{
>> + return -ENXIO;
>> +}
>> +
>> +inline void input_unregister_polled_device(struct input_polled_dev *poll_dev)
>> +{
>> +}
>> +#endif
>> +
>
> Wouldn't these be better off in linux/input-polldev.h?
I'm not sure.
gpio-keys.c with these proposed changes appears to be the only user of
input-polldev.c that needs to compile and execute when
CONFIG_INPUT_POLLDEV is not defined. Moving these to input-polldev.h
wouldn't have any impact on other in-tree targets but could there be
effects on out-of-tree ones?
How would you feel about a shim layer that does a pass-thru when
CONFIG_INPUT_POLLDEV is defined and does some error checking
otherwise? I'll show you what I mean in the next version of the patch.
>> +#if !defined(CONFIG_INPUT_POLLDEV) && !defined(CONFIG_INPUT_POLLDEV_MODULE)
>> + if (pdata->poll_interval) {
>> + dev_err(dev, "device needs polling (enable INPUT_POLLDEV)\n");
>> + return -EINVAL;
>> + }
>> +#endif
>> +
> This you could probably just shove in to..
>
>> ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
>> pdata->nbuttons * sizeof(struct gpio_button_data),
>> GFP_KERNEL);
>> - input = input_allocate_device();
>> + if (pdata->poll_interval) {
>> + poll_dev = input_allocate_polled_device();
>> + input = poll_dev ? poll_dev->input : 0;
>> + } else
>> + input = input_allocate_device();
>> if (!ddata || !input) {
>> dev_err(dev, "failed to allocate state\n");
>> error = -ENOMEM;
>> goto fail1;
>
> this. Then just get rid of the ifdefs, as the
> input_allocate_polled_device() case will have already NULL'ed out.
I like removing that #ifdef block -- as long as we keep the dev_err
about enabling INPUT_POLLDEV. It is possible for INPUT_POLLDEV to be
defined and an allocation error to occur; I think it is useful to the
clients of gpio_keys -- particularly the board setup developers -- to
differentiate between 'INPUT_POLLDEV needs to be defined' and 'failed
to allocate state', at least by printing an error message as in the
current state of this patch.
> The rest is obviously fine with me!
Alright! :) I will re-spin a version that tries to address your
comments. Thank you for your input so far.
Best Regards,
Ben Gardiner
---
Nanometrics Inc.
http://www.nanometrics.ca
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
