Hi,
On 05/19/2016 04:55 PM, Krzysztof Kozlowski wrote:
[...]
> On 05/19/2016 11:04 AM, Vignesh R wrote:
>> diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
>> index c7fc8d4fb080..8f60289fb6cd 100644
>> --- a/drivers/input/misc/rotary_encoder.c
>> +++ b/drivers/input/misc/rotary_encoder.c
>> @@ -25,24 +25,29 @@
>> #include <linux/of.h>
>> #include <linux/pm.h>
>> #include <linux/property.h>
>> +#include <linux/input-polldev.h>
>>
>> #define DRV_NAME "rotary-encoder"
>>
>> struct rotary_encoder {
>> struct input_dev *input;
>> -
>> +#ifdef CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT
>
> Ifdefs look dirty.
I will drop this.
>
>> + struct input_polled_dev *poll_dev;
>> +#endif
>> struct mutex access_mutex;
>>
>> u32 steps;
>> u32 axis;
>> bool relative_axis;
>> bool rollover;
>> + bool absolute_encoder;
>>
>> unsigned int pos;
>>
>> struct gpio_descs *gpios;
>> + struct device *dev;
>>
>> - unsigned int *irq;
>> + int *irq;
>>
>> bool armed;
>> signed char dir; /* 1 - clockwise, -1 - CCW */
>> @@ -67,6 +72,21 @@ static unsigned int rotary_encoder_get_state(struct rotary_encoder *encoder)
>> return ret & 3;
>> }
>>
>> +static unsigned int rotary_encoder_get_gpios_state(struct rotary_encoder
>> + *encoder)
>> +{
>> + int i;
>> + unsigned int ret = 0;
>> +
>> + for (i = 0; i < encoder->gpios->ndescs; ++i) {
>> + int val = gpiod_get_value_cansleep(encoder->gpios->desc[i]);
>> +
>> + ret = ret << 1 | val;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> static void rotary_encoder_report_event(struct rotary_encoder *encoder)
>> {
>> if (encoder->relative_axis) {
>> @@ -178,6 +198,72 @@ out:
>> return IRQ_HANDLED;
>> }
>>
>> +static void rotary_encoder_setup_input_params(struct rotary_encoder *encoder)
>> +{
>> + struct input_dev *input = encoder->input;
>> + struct platform_device *pdev = to_platform_device(encoder->dev);
>> +
>> + input->name = pdev->name;
>> + input->id.bustype = BUS_HOST;
>> + input->dev.parent = encoder->dev;
>> +
>> + if (encoder->relative_axis)
>> + input_set_capability(input, EV_REL, encoder->axis);
>> + else
>> + input_set_abs_params(input,
>> + encoder->axis, 0, encoder->steps, 0, 1);
>> +}
>> +
>> +static irqreturn_t rotary_absolute_encoder_irq(int irq, void *dev_id)
>> +{
>> + struct rotary_encoder *encoder = dev_id;
>> + unsigned int state;
>> +
>> + mutex_lock(&encoder->access_mutex);
>> +
>> + state = rotary_encoder_get_gpios_state(encoder);
>> + if (state != encoder->last_stable) {
>> + input_report_abs(encoder->input, encoder->axis, state);
>> + input_sync(encoder->input);
>> + encoder->last_stable = state;
>> + }
>> +
>> + mutex_lock(&encoder->access_mutex);
>
> 1. Double mutex_lock()? Oh, that is weird. Please document it at least
> in form of lockdep annotations.
>
> 2. Maybe that should be unlock? Did you test this code?
>
Sorry... this should have been unlock.
I dont have a h/w with rotary-encoder hooked to IRQ line. It would be
much appreciated if someone with rotary-encoder hooked up to h/w IRQ
line test this patch.
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +#ifdef CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT
>
> Same here and later in the code, ifdefs look dirty.
Ok, I will remove #ifdefs and directly select INPUT_POLLDEV when
INPUT_GPIO_ROTARY_ENCODER is selected.
--
Regards
Vignesh
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
