linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* input_polling_devices design question + adxl34x polling mode patch
@ 2011-04-11 16:21 paul.chavent
  2011-04-12  6:23 ` Dmitry Torokhov
  0 siblings, 1 reply; 5+ messages in thread
From: paul.chavent @ 2011-04-11 16:21 UTC (permalink / raw)
  To: michael.hennerich; +Cc: dmitry.torokhov, device-driver-devel, linux-input

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

Hi.

I have  a board with an adxl34x (an accelerometer) on a two wire bus. Its interrupt line aren't routed.

So i would like to use the driver in polling mode.

I submit this patch as a beta version of my work.

I tried to reuse the input_polling structure but I'm facing some problems.

The driver has a "rate" attribute that i would like to control when i setup the "interval" attribute of the input_pollling.

And vice versa, when i setup the "interval" attribute i would like to setup the "rate".

So my questions :
 - Is it possible to reimplement a workqueue for this driver only ? As it seems to have been done yet in other drivers, i wonder if it's acceptable, or if we should avoid this practice.
 - I think it would be complicated to have hooks in input and input_polling for calling each other. I wonder if i haven't any design problem.

I would need an advice in order to cleaning this patch please.

Thank for your help.

Paul.



[-- Attachment #2: 0001-Polling-option-for-adxl34x-input-driver.patch --]
[-- Type: application/octet-stream, Size: 6773 bytes --]

From a03ffc414bff628317d191fde4a9e8fe8b6e669c Mon Sep 17 00:00:00 2001
From: Paul Chavent <paul.chavent@fnac.net>
Date: Mon, 11 Apr 2011 18:01:33 +0200
Subject: [PATCH] Polling option for adxl34x input driver.

---
 drivers/input/misc/adxl34x.c |  138 +++++++++++++++++++++++++++++++++++++-----
 1 files changed, 122 insertions(+), 16 deletions(-)

diff --git a/drivers/input/misc/adxl34x.c b/drivers/input/misc/adxl34x.c
index 144ddbd..88b99f4 100644
--- a/drivers/input/misc/adxl34x.c
+++ b/drivers/input/misc/adxl34x.c
@@ -11,6 +11,9 @@
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/input.h>
+#if defined(CONFIG_INPUT_POLLDEV)
+#include <linux/input-polldev.h>
+#endif /* defined(CONFIG_INPUT_POLLDEV) */
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/slab.h>
@@ -190,6 +193,9 @@ struct axis_triple {
 struct adxl34x {
 	struct device *dev;
 	struct input_dev *input;
+#if defined(CONFIG_INPUT_POLLDEV)
+	struct input_polled_dev *input_polled;
+#endif /* defined(CONFIG_INPUT_POLLDEV) */
 	struct mutex mutex;	/* reentrant protection for struct */
 	struct adxl34x_platform_data pdata;
 	struct axis_triple swcal;
@@ -688,27 +694,75 @@ static void adxl34x_input_close(struct input_dev *input)
 	mutex_unlock(&ac->mutex);
 }
 
+#if defined(CONFIG_INPUT_POLLDEV)
+
+static void adxl34x_input_polled_open(struct input_polled_dev *dev)
+{
+	adxl34x_input_open(dev->input);
+}
+
+static void adxl34x_input_polled_close(struct input_polled_dev *dev)
+{
+	adxl34x_input_close(dev->input);
+}
+
+static void adxl34x_input_polled_poll(struct input_polled_dev *dev)
+{
+	struct adxl34x *ac = input_get_drvdata(dev->input);
+
+	adxl34x_irq(ac->irq, ac);
+}
+
+#endif /* defined(CONFIG_INPUT_POLLDEV) */
+
 struct adxl34x *adxl34x_probe(struct device *dev, int irq,
 			      bool fifo_delay_default,
 			      const struct adxl34x_bus_ops *bops)
 {
 	struct adxl34x *ac;
 	struct input_dev *input_dev;
+#if defined(CONFIG_INPUT_POLLDEV)
+	struct input_polled_dev *input_polled_dev;
+#endif /* defined(CONFIG_INPUT_POLLDEV) */
 	const struct adxl34x_platform_data *pdata;
 	int err, range, i;
 	unsigned char revid;
 
-	if (!irq) {
-		dev_err(dev, "no IRQ?\n");
-		err = -ENODEV;
+
+	ac = kzalloc(sizeof(*ac), GFP_KERNEL);
+	if (!ac) {
+		err = -ENOMEM;
 		goto err_out;
 	}
 
-	ac = kzalloc(sizeof(*ac), GFP_KERNEL);
+	ac->irq = irq;
+
+	if (ac->irq) {
+#if defined(CONFIG_INPUT_POLLDEV)
+		input_polled_dev = 0;
+		ac->input_polled = input_polled_dev;
+#endif /* defined(CONFIG_INPUT_POLLDEV) */
+
 	input_dev = input_allocate_device();
-	if (!ac || !input_dev) {
+		if (!input_dev) {
 		err = -ENOMEM;
-		goto err_free_mem;
+			goto err_free_ac;
+		}
+	} else {
+#if defined(CONFIG_INPUT_POLLDEV)
+		input_polled_dev = input_allocate_polled_device();
+		if (!input_polled_dev) {
+			err = -ENOMEM;
+			goto err_free_ac;
+		}
+		ac->input_polled = input_polled_dev;
+
+		input_dev = input_polled_dev->input;
+#else
+		dev_err(dev, "no IRQ? (perhaps you need to set CONFIG_INPUT_POLLDEV=y)\n");
+		err = -ENODEV;
+		goto err_free_ac;
+#endif /* defined(CONFIG_INPUT_POLLDEV) */
 	}
 
 	ac->fifo_delay = fifo_delay_default;
@@ -725,7 +779,6 @@ struct adxl34x *adxl34x_probe(struct device *dev, int irq,
 
 	ac->input = input_dev;
 	ac->dev = dev;
-	ac->irq = irq;
 	ac->bops = bops;
 
 	mutex_init(&ac->mutex);
@@ -743,7 +796,7 @@ struct adxl34x *adxl34x_probe(struct device *dev, int irq,
 	default:
 		dev_err(dev, "Failed to probe %s\n", input_dev->name);
 		err = -ENODEV;
-		goto err_free_mem;
+		goto err_free_dev;
 	}
 
 	snprintf(ac->phys, sizeof(ac->phys), "%s/input0", dev_name(dev));
@@ -752,8 +805,15 @@ struct adxl34x *adxl34x_probe(struct device *dev, int irq,
 	input_dev->dev.parent = dev;
 	input_dev->id.product = ac->model;
 	input_dev->id.bustype = bops->bustype;
+	if (ac->irq) {
 	input_dev->open = adxl34x_input_open;
 	input_dev->close = adxl34x_input_close;
+	} else {
+#if defined(CONFIG_INPUT_POLLDEV)
+		input_polled_dev->open = adxl34x_input_polled_open;
+		input_polled_dev->close = adxl34x_input_polled_close;
+#endif /* defined(CONFIG_INPUT_POLLDEV) */
+	}
 
 	input_set_drvdata(input_dev, ac);
 
@@ -810,21 +870,44 @@ struct adxl34x *adxl34x_probe(struct device *dev, int irq,
 
 	ac->bops->write(dev, POWER_CTL, 0);
 
+	if (ac->irq) {
 	err = request_threaded_irq(ac->irq, NULL, adxl34x_irq,
 				   IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
 				   dev_name(dev), ac);
+
 	if (err) {
 		dev_err(dev, "irq %d busy?\n", ac->irq);
-		goto err_free_mem;
+			goto err_free_dev;
 	}
 
-	err = sysfs_create_group(&dev->kobj, &adxl34x_attr_group);
+		err = input_register_device(input_dev);
 	if (err)
 		goto err_free_irq;
 
-	err = input_register_device(input_dev);
+		err = sysfs_create_group(&dev->kobj, &adxl34x_attr_group);
+		if (err)
+			goto err_unregister_dev;
+	} else {
+#if defined(CONFIG_INPUT_POLLDEV)
+		input_polled_dev->poll = adxl34x_input_polled_poll;
+
+		err = input_register_polled_device(input_polled_dev);
+		if (err)
+			goto err_free_dev;
+
+#if 0
+		err = sysfs_merge_group(&dev->kobj, &adxl34x_attr_group);
 	if (err)
-		goto err_remove_attr;
+			goto err_unregister_dev;
+#else
+#warning "FIXME : test with > 2.6.36"
+		err = sysfs_create_group(&dev->kobj, &adxl34x_attr_group);
+		if (err)
+			goto err_unregister_dev;
+#endif
+#endif /* defined(CONFIG_INPUT_POLLDEV) */
+	}
+
 
 	AC_WRITE(ac, THRESH_TAP, pdata->tap_threshold);
 	AC_WRITE(ac, OFSX, pdata->x_axis_offset);
@@ -885,12 +968,27 @@ struct adxl34x *adxl34x_probe(struct device *dev, int irq,
 
 	return ac;
 
- err_remove_attr:
-	sysfs_remove_group(&dev->kobj, &adxl34x_attr_group);
+ err_unregister_dev:
+	if (ac->irq) {
+		input_unregister_device(input_dev);
+	} else {
+#if defined(CONFIG_INPUT_POLLDEV)
+		input_unregister_polled_device(input_polled_dev);
+#endif /* defined(CONFIG_INPUT_POLLDEV) */
+	}
  err_free_irq:
+	if (ac->irq) {
 	free_irq(ac->irq, ac);
- err_free_mem:
+	}
+ err_free_dev:
+	if (ac->irq) {
 	input_free_device(input_dev);
+	} else {
+#if defined(CONFIG_INPUT_POLLDEV)
+		input_free_polled_device(input_polled_dev);
+#endif /* defined(CONFIG_INPUT_POLLDEV) */
+	}
+ err_free_ac:
 	kfree(ac);
  err_out:
 	return ERR_PTR(err);
@@ -899,9 +997,17 @@ EXPORT_SYMBOL_GPL(adxl34x_probe);
 
 int adxl34x_remove(struct adxl34x *ac)
 {
+	if (ac->irq) {
 	sysfs_remove_group(&ac->dev->kobj, &adxl34x_attr_group);
-	free_irq(ac->irq, ac);
 	input_unregister_device(ac->input);
+		free_irq(ac->irq, ac);
+		input_free_device(ac->input);
+	} else {
+#if defined(CONFIG_INPUT_POLLDEV)
+		input_unregister_polled_device(ac->input_polled);
+		input_free_polled_device(ac->input_polled);
+#endif /* defined(CONFIG_INPUT_POLLDEV) */
+	}
 	dev_dbg(ac->dev, "unregistered accelerometer\n");
 	kfree(ac);
 
-- 
1.6.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: input_polling_devices design question + adxl34x polling mode patch
@ 2011-04-12  7:22 paul.chavent
  0 siblings, 0 replies; 5+ messages in thread
From: paul.chavent @ 2011-04-12  7:22 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: michael.hennerich, device-driver-devel, linux-input

Hi Dimitry,

>Yes, just use the system workqueue

Ok. I won't use the polled input anymore.

>Note that I normally oppose supporting polling mode for devices that may be interrupt driven, since this normally increases power consumption.

Yes, but in my case i have no choice (the hardware don't allow to use interrupts). So The polled mode could be an option ?

Are you definitively opposed to this options ? 

Paul. 


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: input_polling_devices design question + adxl34x polling mode patch
@ 2011-04-12 16:57 paul.chavent
  2011-04-13  8:39 ` Hennerich, Michael
  0 siblings, 1 reply; 5+ messages in thread
From: paul.chavent @ 2011-04-12 16:57 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: michael.hennerich, device-driver-devel, linux-input

[-- Attachment #1: Type: text/plain, Size: 2173 bytes --]

Hi.

This patch allow to use the device even if its interrupts lines aren't routed.

I tried to minimize the impact of the patch on the original code, and if the CONFIG_INPUT_ADXL34X_ALLOW_POLLING option is disabled, the code should be unchanged.

Please give me your opinion.

Thanks.

Paul.

========================================
Message du 12/04/11 08:23
De : "Dmitry Torokhov" 
A : paul.chavent@fnac.net
Copie à : michael.hennerich@analog.com, device-driver-devel@blackfin.uclinux.org, linux-input@vger.kernel.org
Objet : Re: input_polling_devices design question + adxl34x polling mode patch


Hi Paul,

On Mon, Apr 11, 2011 at 06:21:02PM +0200, paul.chavent@fnac.net wrote:
> Hi.
> 
> I have  a board with an adxl34x (an accelerometer) on a two wire bus.
> Its interrupt line aren't routed.
> 
> So i would like to use the driver in polling mode.
> 
> I submit this patch as a beta version of my work.
> 
> I tried to reuse the input_polling structure but I'm facing some
> problems.
> 
> The driver has a "rate" attribute that i would like to control when i
> setup the "interval" attribute of the input_pollling.
> 
> And vice versa, when i setup the "interval" attribute i would like to
> setup the "rate".
> 
> So my questions :  - Is it possible to reimplement a workqueue for
> this driver only ? As it seems to have been done yet in other drivers,
> i wonder if it's acceptable, or if we should avoid this practice.   -
> I think it would be complicated to have hooks in input and
> input_polling for calling each other. I wonder if i haven't any design
> problem.
> 
> I would need an advice in order to cleaning this patch please.
> 

Yes, just use the system workqueue (now even dedicated workqueue is not
really needed) and leave input_polldev alone - it makes sense to use for
devices that are purely polled ones. The devices that may or may not be
interrupt driven I think we should just hook workqueue handling in the
driver.

Note that I normally oppose supporting polling mode for devices that may
be interrupt driven, since this normally increases power consumption.

-- 
Dmitry


[-- Attachment #2: linux-2.6.38.2-adxl.patch --]
[-- Type: application/octet-stream, Size: 4836 bytes --]

Only in linux-2.6.38.2.mod/drivers/input/misc/: .adxl34x-i2c.o.cmd
Only in linux-2.6.38.2.mod/drivers/input/misc/: .adxl34x.o.cmd
Only in linux-2.6.38.2.mod/drivers/input/misc/: .built-in.o.cmd
diff -x 'linux-2.6.38.2-adxl.patch~' -u linux-2.6.38.2.orig/drivers/input/misc/Kconfig linux-2.6.38.2.mod/drivers/input/misc/Kconfig
--- linux-2.6.38.2.orig/drivers/input/misc/Kconfig	2011-03-27 20:37:20.000000000 +0200
+++ linux-2.6.38.2.mod/drivers/input/misc/Kconfig	2011-04-12 18:08:43.530000058 +0200
@@ -430,6 +430,13 @@
 	  To compile this driver as a module, choose M here: the
 	  module will be called adxl34x-spi.
 
+config INPUT_ADXL34X_ALLOW_POLLING
+	bool "allow to fallback to polling mode"
+	depends on INPUT_ADXL34X
+	default n
+	help
+	  Say Y here if the int lines of your ADXL345/6 are unused.
+
 config INPUT_CMA3000
 	tristate "VTI CMA3000 Tri-axis accelerometer"
 	help
Only in linux-2.6.38.2.mod/drivers/input/misc/: Kconfig~
Only in linux-2.6.38.2.mod/drivers/input/misc/: adxl34x-i2c.o
diff -x 'linux-2.6.38.2-adxl.patch~' -u linux-2.6.38.2.orig/drivers/input/misc/adxl34x.c linux-2.6.38.2.mod/drivers/input/misc/adxl34x.c
--- linux-2.6.38.2.orig/drivers/input/misc/adxl34x.c	2011-03-27 20:37:20.000000000 +0200
+++ linux-2.6.38.2.mod/drivers/input/misc/adxl34x.c	2011-04-12 18:47:54.966000059 +0200
@@ -98,6 +98,7 @@
 /* BW_RATE Bits */
 #define LOW_POWER	(1 << 4)
 #define RATE(x)		((x) & 0xF)
+#define RATE2USEC(x)    (10240000 >> RATE(x))
 
 /* POWER_CTL Bits */
 #define PCTL_LINK	(1 << 5)
@@ -205,6 +206,9 @@
 	int irq;
 	unsigned model;
 	unsigned int_mask;
+#if defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING)
+	struct delayed_work work;
+#endif /* defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING) */
 
 	const struct adxl34x_bus_ops *bops;
 };
@@ -398,6 +402,34 @@
 	return IRQ_HANDLED;
 }
 
+#if defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING)
+static void adxl34x_poll_reschedule(struct adxl34x *ac)
+{
+	unsigned long delay_usecs;
+	unsigned long delay_jiffies;
+
+	delay_usecs = RATE2USEC(ac->pdata.data_rate);
+	delay_jiffies = usecs_to_jiffies(delay_usecs);
+	if (delay_jiffies >= HZ)
+		delay_jiffies = round_jiffies_relative(delay_jiffies);
+	schedule_delayed_work(&ac->work, delay_jiffies);
+}
+static void adxl34x_poll_cancel(struct adxl34x *ac)
+{
+	cancel_delayed_work(&ac->work);
+}
+
+static void adxl34x_poll(struct work_struct *work)
+{
+	struct delayed_work *dw = container_of(work, struct delayed_work, work);
+	struct adxl34x *ac = container_of(dw, struct adxl34x, work);
+
+	adxl34x_irq(ac->irq, ac);
+
+	adxl34x_poll_reschedule(ac);
+}
+#endif /* defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING) */
+
 static void __adxl34x_disable(struct adxl34x *ac)
 {
 	/*
@@ -671,6 +703,12 @@
 
 	mutex_unlock(&ac->mutex);
 
+#if defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING)
+	if (!ac->irq) {
+		adxl34x_poll_reschedule(ac);
+	}
+#endif /* defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING) */
+
 	return 0;
 }
 
@@ -686,6 +724,12 @@
 	ac->opened = false;
 
 	mutex_unlock(&ac->mutex);
+
+#if defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING)
+	if (!ac->irq) {
+		adxl34x_poll_cancel(ac);
+	}
+#endif /* defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING) */
 }
 
 struct adxl34x *adxl34x_probe(struct device *dev, int irq,
@@ -699,9 +743,13 @@
 	unsigned char revid;
 
 	if (!irq) {
+#if defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING)
+		dev_dbg(dev, "no IRQ, switch to poll mode\n");
+#else
 		dev_err(dev, "no IRQ?\n");
 		err = -ENODEV;
 		goto err_out;
+#endif /* defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING) */
 	}
 
 	ac = kzalloc(sizeof(*ac), GFP_KERNEL);
@@ -810,13 +858,20 @@
 
 	ac->bops->write(dev, POWER_CTL, 0);
 
-	err = request_threaded_irq(ac->irq, NULL, adxl34x_irq,
-				   IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
-				   dev_name(dev), ac);
-	if (err) {
-		dev_err(dev, "irq %d busy?\n", ac->irq);
-		goto err_free_mem;
+	if (ac->irq) {
+		err = request_threaded_irq(ac->irq, NULL, adxl34x_irq,
+					   IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+					   dev_name(dev), ac);
+		if (err) {
+			dev_err(dev, "irq %d busy?\n", ac->irq);
+			goto err_free_mem;
+		}
+	}
+#if defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING)
+	else {
+		INIT_DELAYED_WORK(&ac->work, &adxl34x_poll);
 	}
+#endif /* defined(CONFIG_INPUT_ADXL34X_ALLOW_POLLING) */
 
 	err = sysfs_create_group(&dev->kobj, &adxl34x_attr_group);
 	if (err)
@@ -888,7 +943,8 @@
  err_remove_attr:
 	sysfs_remove_group(&dev->kobj, &adxl34x_attr_group);
  err_free_irq:
-	free_irq(ac->irq, ac);
+	if (ac->irq)
+		free_irq(ac->irq, ac);
  err_free_mem:
 	input_free_device(input_dev);
 	kfree(ac);
@@ -900,7 +956,8 @@
 int adxl34x_remove(struct adxl34x *ac)
 {
 	sysfs_remove_group(&ac->dev->kobj, &adxl34x_attr_group);
-	free_irq(ac->irq, ac);
+	if (ac->irq)
+		free_irq(ac->irq, ac);
 	input_unregister_device(ac->input);
 	dev_dbg(ac->dev, "unregistered accelerometer\n");
 	kfree(ac);

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2011-04-13  8:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-04-11 16:21 input_polling_devices design question + adxl34x polling mode patch paul.chavent
2011-04-12  6:23 ` Dmitry Torokhov
  -- strict thread matches above, loose matches on Subject: below --
2011-04-12  7:22 paul.chavent
2011-04-12 16:57 paul.chavent
2011-04-13  8:39 ` Hennerich, Michael

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).