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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <573DA70F.70206@ti.com>
Date:	Thu, 19 May 2016 17:14:15 +0530
From:	Vignesh R <vigneshr@...com>
To:	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Tony Lindgren <tony@...mide.com>
CC:	Jonathan Corbet <corbet@....net>, Johan Hovold <johan@...nel.org>,
	Sylvain Rochet <sylvain.rochet@...secur.com>,
	Masanari Iida <standby24x7@...il.com>,
	Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
	S Twiss <stwiss.opensource@...semi.com>,
	Moritz Fischer <moritz.fischer@...us.com>,
	Arnd Bergmann <arnd@...db.de>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Timo Teras <timo.teras@....fi>,
	Guido Martinez <guido@...guardiasur.com.ar>,
	Clifton Barnes <clifton.a.barnes@...il.com>,
	Uwe Kleine-Konig <u.kleine-koenig@...gutronix.de>,
	<linux-input@...r.kernel.org>, <devicetree@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
	<linux-omap@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute
 encoder

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