Re: input_polling_devices design question + adxl34x polling mode patch

[<paul.chavent@fnac.net>]

[-- 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);