Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder

[Vignesh R <vigneshr@ti.com>]

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