2007-10-15 14:47:31

by Bryan Wu

[permalink] [raw]
Subject: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver

Subject: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver

[try #2] Changelog:
- Coding style issues fixed, passed checkpatch.pl
- Kill uselss "ad7142_used"
- Move request_irq to probe
- Move i2c_check_functionality to probe
- Error handling added

[try #3] Changelog:
- Use kzalloc
- Use strlcpy
- Use completion instead of wait queue
- Move input_allocate_device and input_register_device to ad7142_probe()
- Fix some issues pointed by LKML

Cc: Andrey Panin <[email protected]>
Cc: Roel Kluin <[email protected]>
Cc: Ahmed S. Darwish <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
drivers/input/joystick/Kconfig | 16 ++
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/ad7142.c | 477 +++++++++++++++++++++++++++++++++++++++
3 files changed, 494 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/joystick/ad7142.c

diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index 7c662ee..aeb7cc9 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -282,4 +282,20 @@ config JOYSTICK_XPAD_LEDS
This option enables support for the LED which surrounds the Big X on
XBox 360 controller.

+config JOYSTICK_AD7142
+ tristate "Analog Devices AD7142 Joystick support"
+ depends on BFIN && I2C
+ help
+ Say Y here if you want to support an AD7142 joystick
+
+
+ config BFIN_JOYSTICK_IRQ_PFX
+ int "GPIO for Interrupt"
+ depends on (BFIN && JOYSTICK_AD7142)
+ range 33 120
+ default "55" if BFIN537_STAMP
+ default "39" if BFIN533_STAMP
+ help
+ Choose an GPIO as Keypad interrupt.[0..15]
+
endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index e855abb..8df388f 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -5,6 +5,7 @@
# Each configuration option enables a list of files.

obj-$(CONFIG_JOYSTICK_A3D) += a3d.o
+obj-$(CONFIG_JOYSTICK_AD7142) += ad7142.o
obj-$(CONFIG_JOYSTICK_ADI) += adi.o
obj-$(CONFIG_JOYSTICK_AMIGA) += amijoy.o
obj-$(CONFIG_JOYSTICK_ANALOG) += analog.o
diff --git a/drivers/input/joystick/ad7142.c b/drivers/input/joystick/ad7142.c
new file mode 100644
index 0000000..d5f3e0f
--- /dev/null
+++ b/drivers/input/joystick/ad7142.c
@@ -0,0 +1,477 @@
+/*
+ * File: drivers/input/joystick/ad7142.c
+ * Based on: drivers/input/joystick/amijoy.c
+ * Original Author: Aubrey Li
+ * Maintained by: Bryan Wu <[email protected]>
+ *
+ * Created: Apr 7th, 2006
+ * Description:
+ *
+ * Modified:
+ * Copyright 2005-2007 Analog Devices Inc.
+ *
+ * Bugs: Enter bugs at http://blackfin.uclinux.org/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; see the file COPYING.
+ * If not, write to the Free Software Foundation,
+ * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/kthread.h>
+#include <linux/uaccess.h>
+#include <linux/irq.h>
+
+#include <asm/blackfin.h>
+
+MODULE_AUTHOR("Aubrey Li, Bryan Wu <[email protected]>");
+MODULE_DESCRIPTION("Driver for AD7142 joysticks");
+MODULE_LICENSE("GPL");
+
+/*
+ * Feeding the output queue to the device is handled by way of a
+ * workqueue.
+ */
+static struct task_struct *ad7142_task;
+
+static struct input_dev *ad7142_dev;
+
+#define AD7142_DRV_NAME "ad7142_js"
+#define AD7142_I2C_ID 0xE622
+#define AD7142_I2C_ADDR 0x2C
+/*
+ * Ram map - these registers are defined as we go along
+ */
+/* RW Power & conversion control */
+#define PWRCONVCTL 0x00
+
+/* RW Ambient compensation control register 0 - 3 */
+#define AMBCOMPCTL_REG0 0x01
+#define AMBCOMPCTL_REG1 0x02
+#define AMBCOMPCTL_REG2 0x03
+#define AMBCOMPCTL_REG3 0x04
+
+/* RW Interrupt enable register 0 - 2 */
+#define INTEN_REG0 0x05
+#define INTEN_REG1 0x06
+#define INTEN_REG2 0x07
+
+/* R Low limit interrupt status register 0 */
+#define INTSTAT_REG0 0x08
+/* R High limit interrupt status register 1 */
+#define INTSTAT_REG1 0x09
+/* R Interrupt status register 2 */
+#define INTSTAT_REG2 0x0A
+
+/* R ADC stage 0 - 11 result (uncompensated) actually located in SRAM */
+#define ADCRESULT_S0 0x0B
+#define ADCRESULT_S1 0x0C
+#define ADCRESULT_S2 0x0D
+#define ADCRESULT_S3 0x0E
+#define ADCRESULT_S4 0x0F
+#define ADCRESULT_S5 0x10
+#define ADCRESULT_S6 0x11
+#define ADCRESULT_S7 0x12
+#define ADCRESULT_S8 0x13
+#define ADCRESULT_S9 0x14
+#define ADCRESULT_S10 0x15
+#define ADCRESULT_S11 0x16
+
+/* R I.D. Register */
+#define DEVID 0x17
+
+/* R Current threshold status register 0, 1 */
+#define THRES_STAT_REG0 0x40
+#define THRES_STAT_REG1 0x41
+/* R Current proximity status register 2 */
+#define PROX_STAT_REG 0x42
+
+#define STAGE0_CONNECTION 0x80
+#define STAGE1_CONNECTION 0x88
+#define STAGE2_CONNECTION 0x90
+#define STAGE3_CONNECTION 0x98
+#define STAGE4_CONNECTION 0xA0
+#define STAGE5_CONNECTION 0xA8
+#define STAGE6_CONNECTION 0xB0
+#define STAGE7_CONNECTION 0xB8
+#define STAGE8_CONNECTION 0xC0
+#define STAGE9_CONNECTION 0xC8
+#define STAGE10_CONNECTION 0xD0
+#define STAGE11_CONNECTION 0xD8
+
+/*
+ * STAGE0: Button1 <----> CIN6(+) Button2 <----> CIN5(-)
+ * STAGE1: Button3 <----> CIN4(-) Button4 <----> CIN3(+)
+ * STAGE2: Axes.Left <----> CIN11(-) Axes.Right <----> CIN13(+)
+ * STAGE3: Axes.Up <----> CIN12(-) Axes.Down <----> CIN10(+)
+ */
+static unsigned short stage[5][8] = {
+ {0xE7FF, 0x3FFF, 0x0005, 0x2626, 0x01F4, 0x01F4, 0x028A, 0x028A},
+ {0xFDBF, 0x3FFF, 0x0001, 0x2626, 0x01F4, 0x01F4, 0x028A, 0x028A},
+ {0xFFFF, 0x2DFF, 0x0001, 0x2626, 0x01F4, 0x01F4, 0x028A, 0x028A},
+ {0xFFFF, 0x37BF, 0x0001, 0x2626, 0x01F4, 0x01F4, 0x028A, 0x028A},
+ {0xFFFF, 0x3FFF, 0x0000, 0x0606, 0x01F4, 0x01F4, 0x0320, 0x0320},
+};
+
+static struct i2c_driver ad7142_driver;
+static struct i2c_client *ad7142_client;
+
+static unsigned short ignore[] = { I2C_CLIENT_END };
+static unsigned short normal_addr[] = { AD7142_I2C_ADDR, I2C_CLIENT_END };
+
+static unsigned short old_status_low, old_status_high;
+
+static completion ad7142_completion;
+
+static struct i2c_client_address_data addr_data = {
+ .normal_i2c = normal_addr,
+ .probe = ignore,
+ .ignore = ignore,
+};
+
+static irqreturn_t ad7142_interrupt(int irq, void *dummy)
+{
+ disable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
+ complete(&ad7142_completion);
+ return IRQ_HANDLED;
+}
+
+static int ad7142_probe(struct i2c_adapter *adap, int addr, int kind)
+{
+ struct i2c_client *client;
+ int rc;
+
+ client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
+ if (!client)
+ return -ENOMEM;
+
+ strlcpy(client->name, AD7142_DRV_NAME, I2C_NAME_SIZE);
+ client->addr = addr;
+ client->adapter = adap;
+ client->driver = &ad7142_driver;
+
+ rc = i2c_attach_client(client);
+ if (rc) {
+ printk(KERN_ERR "i2c_attach_client fail: %d\n", rc);
+ goto fail_attach;
+ }
+
+ /*
+ * The ADV7142 has an autoincrement function,
+ * use it if the adapter understands raw I2C
+ */
+ rc = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
+ if (!rc) {
+ printk(KERN_ERR
+ "AD7142: i2c bus doesn't support raw I2C operation\n");
+ rc = -ENOSYS;
+ goto fail_check;
+ }
+
+ init_completion(&ad7142_completion);
+
+ rc = request_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt,
+ IRQF_TRIGGER_LOW, "ad7142_joy", ad7142_interrupt);
+ if (rc) {
+ printk(KERN_ERR "AD7142: Can't allocate irq %d\n",
+ CONFIG_BFIN_JOYSTICK_IRQ_PFX);
+ goto fail_check;
+ }
+
+ ad7142_client = client;
+ printk(KERN_INFO "%s_attach: at 0x%02x\n",
+ client->name, client->addr << 1);
+
+ /* Allocate and register AD7142 input device */
+ ad7142_dev = input_allocate_device();
+ if (!ad7142_dev) {
+ printk(KERN_ERR "AD7142: Can't allocate input device\n");
+ rc = -ENOMEM;
+ goto fail_allocate;
+ }
+
+ ad7142_dev->open = ad7142_open;
+ ad7142_dev->close = ad7142_close;
+ ad7142_dev->evbit[0] = BIT(EV_KEY);
+ ad7142_dev->keybit[LONG(BTN_BASE)] = BIT(BTN_BASE) | BIT(BTN_BASE2) |
+ BIT(BTN_BASE3) | BIT(BTN_BASE4);
+ ad7142_dev->keybit[LONG(KEY_UP)] |= BIT(KEY_UP) | BIT(KEY_DOWN) |
+ BIT(KEY_LEFT) | BIT(KEY_RIGHT);
+
+ ad7142_dev->name = "ad7142 joystick";
+ ad7142_dev->phys = "ad7142/input0";
+ ad7142_dev->id.bustype = BUS_I2C;
+ ad7142_dev->id.vendor = 0x0001;
+ ad7142_dev->id.product = 0x0001;
+ ad7142_dev->id.version = 0x0100;
+
+ ret = input_register_device(ad7142_dev);
+ if (ret) {
+ printk(KERN_ERR "Failed to register AD7142 input device!\n");
+ goto fail_register;
+ }
+
+ return 0;
+
+fail_register:
+ input_free_device(ad7142_dev);
+fail_allocate:
+ free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
+fail_check:
+ i2c_detach_client(client);
+fail_attach:
+ kfree(client);
+ return rc;
+}
+
+static int ad7142_i2c_write(struct i2c_client *client, unsigned short offset,
+ unsigned short *data, unsigned int len)
+{
+ int ret = -1;
+ int i;
+ u8 block_data[34];
+
+ if (len < 1 || len > 16) {
+ printk(KERN_ERR "AD7142: Write data length error\n");
+ return ret;
+ }
+
+ /* Do raw I2C, not smbus compatible */
+ block_data[0] = (offset & 0xFF00) >> 8;
+ block_data[1] = (offset & 0x00FF);
+
+ for (i = 0; i < len; i++) {
+ block_data[2 * i + 2] = (*data & 0xFF00) >> 8;
+ block_data[2 * i + 3] = *data++ & 0x00FF;
+ }
+
+ ret = i2c_master_send(client, block_data, (len * 2 + 2));
+ if (ret < 0) {
+ printk(KERN_ERR "AD7142: I2C write error\n");
+ return ret;
+ }
+
+ return ret;
+}
+
+static int ad7142_i2c_read(struct i2c_client *client, unsigned short offset,
+ unsigned short *data, unsigned int len)
+{
+ int ret = -1;
+ int i;
+ u8 block_data[32];
+
+ if (len < 1 || len > 16) {
+ printk(KERN_ERR "AD7142: read data length error\n");
+ return ret;
+ }
+
+ /* Do raw I2C, not smbus compatible */
+ block_data[0] = (offset & 0xFF00) >> 8;
+ block_data[1] = (offset & 0x00FF);
+
+ ret = i2c_master_send(client, block_data, 2);
+ if (ret < 0) {
+ printk(KERN_ERR "AD7142: I2C read error\n");
+ return ret;
+ }
+
+ ret = i2c_master_recv(client, block_data, len * 2);
+ if (ret < 0) {
+ printk(KERN_ERR "AD7142: I2C transfer error\n");
+ return ret;
+ }
+
+ for (i = 0; i < len; i++) {
+ unsigned short temp;
+ temp = block_data[2 * i];
+ temp = (temp << 8) & 0xFF00;
+ *data++ = temp | block_data[2 * i + 1];
+ }
+
+ return ret;
+}
+
+static int
+ad7142_attach(struct i2c_adapter *adap)
+{
+ return i2c_probe(adap, &addr_data, &ad7142_probe);
+}
+
+static int
+ad7142_detach_client(struct i2c_client *client)
+{
+ int rc;
+
+ rc = i2c_detach_client(client);
+ if (!rc)
+ kfree(i2c_get_clientdata(client));
+ return rc;
+}
+
+static struct i2c_driver ad7142_driver = {
+ .driver = {
+ .name = AD7142_DRV_NAME,
+ },
+ .id = AD7142_I2C_ID,
+ .attach_adapter = ad7142_attach,
+ .detach_client = ad7142_detach_client,
+};
+
+static void ad7142_decode(void)
+{
+ unsigned short irqno_low, irqno_high;
+ unsigned short temp;
+
+ ad7142_i2c_read(ad7142_client, INTSTAT_REG0, &irqno_low, 1);
+ temp = irqno_low ^ old_status_low;
+ switch (temp) {
+ case 0x0001:
+ input_report_key(ad7142_dev, BTN_BASE, (irqno_low & 0x0001));
+ break;
+ case 0x0002:
+ input_report_key(ad7142_dev, BTN_BASE4,
+ ((irqno_low & 0x0002) >> 1));
+ break;
+ case 0x0004:
+ input_report_key(ad7142_dev, KEY_UP,
+ ((irqno_low & 0x0004) >> 2));
+ break;
+ case 0x0008:
+ input_report_key(ad7142_dev, KEY_RIGHT,
+ ((irqno_low & 0x0008) >> 3));
+ break;
+ }
+ old_status_low = irqno_low;
+
+ ad7142_i2c_read(ad7142_client, INTSTAT_REG1, &irqno_high, 1);
+ temp = irqno_high ^ old_status_high;
+ switch (temp) {
+ case 0x0001:
+ input_report_key(ad7142_dev, BTN_BASE2, irqno_high & 0x0001);
+ break;
+ case 0x0002:
+ input_report_key(ad7142_dev, BTN_BASE3,
+ ((irqno_high & 0x0002) >> 1));
+ break;
+ case 0x0004:
+ input_report_key(ad7142_dev, KEY_DOWN,
+ ((irqno_high & 0x0004) >> 2));
+ break;
+ case 0x0008:
+ input_report_key(ad7142_dev, KEY_LEFT,
+ ((irqno_high & 0x0008) >> 3));
+ break;
+ }
+ old_status_high = irqno_high;
+
+ input_sync(ad7142_dev);
+}
+
+static int ad7142_thread(void *nothing)
+{
+ do {
+ wait_for_completion(&ad7142_completion);
+ ad7142_decode();
+ enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
+ } while (!kthread_should_stop());
+
+ pr_debug("ad7142: kthread exiting\n");
+
+ return 0;
+}
+
+static int ad7142_open(struct input_dev *dev)
+{
+ unsigned short id, value;
+
+ ad7142_i2c_read(ad7142_client, DEVID, &id, 1);
+ if (id != AD7142_I2C_ID) {
+ printk(KERN_ERR "Open AD7142 error\n");
+ return -ENODEV;
+ }
+
+ ad7142_i2c_write(ad7142_client, STAGE0_CONNECTION, stage[0], 8);
+ ad7142_i2c_write(ad7142_client, STAGE1_CONNECTION, stage[1], 8);
+ ad7142_i2c_write(ad7142_client, STAGE2_CONNECTION, stage[2], 8);
+ ad7142_i2c_write(ad7142_client, STAGE3_CONNECTION, stage[3], 8);
+ ad7142_i2c_write(ad7142_client, STAGE4_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE5_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE6_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE7_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE8_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE9_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE10_CONNECTION, stage[4], 8);
+ ad7142_i2c_write(ad7142_client, STAGE11_CONNECTION, stage[4], 8);
+
+ value = 0x00B0;
+ ad7142_i2c_write(ad7142_client, PWRCONVCTL, &value, 1);
+
+ value = 0x0690;
+ ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG1, &value, 1);
+
+ value = 0x0664;
+ ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG2, &value, 1);
+
+ value = 0x290F;
+ ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG3, &value, 1);
+
+ value = 0x000F;
+ ad7142_i2c_write(ad7142_client, INTEN_REG0, &value, 1);
+ ad7142_i2c_write(ad7142_client, INTEN_REG1, &value, 1);
+
+ value = 0x0000;
+ ad7142_i2c_write(ad7142_client, INTEN_REG2, &value, 1);
+
+ ad7142_i2c_read(ad7142_client, AMBCOMPCTL_REG1, &value, 1);
+
+ value = 0x000F;
+ ad7142_i2c_write(ad7142_client, AMBCOMPCTL_REG0, &value, 1);
+
+ ad7142_task = kthread_run(ad7142_thread, NULL, "ad7142_task");
+ if (IS_ERR(ad7142_task)) {
+ printk(KERN_ERR "serio: Failed to start kseriod\n");
+ return PTR_ERR(ad7142_task);
+ }
+ return 0;
+}
+
+static void ad7142_close(struct input_dev *dev)
+{
+ free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
+ kthread_stop(ad7142_task);
+}
+
+static int __init ad7142_init(void)
+{
+ return i2c_add_driver(&ad7142_driver);
+}
+
+static void __exit ad7142_exit(void)
+{
+ i2c_del_driver(&ad7142_driver);
+ input_unregister_device(ad7142_dev);
+}
+
+module_init(ad7142_init);
+module_exit(ad7142_exit);
--
1.5.3.4


2007-10-15 15:48:28

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver

Hi Bryan,

On 10/15/07, Bryan Wu <[email protected]> wrote:
> +
> +static int ad7142_thread(void *nothing)
> +{
> + do {
> + wait_for_completion(&ad7142_completion);
> + ad7142_decode();
> + enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
> + } while (!kthread_should_stop());
> +

No, this is not going to work well:
- you at least need to reinitialize the completion before enabling
IRQ, otherwise you will spin in a very tight loop
- if noone would touch the joystick ad7142_clsoe would() block
infinitely because noone would signal the completion and
ad7142_thread() would never stop.

Completion is just not a good abstraction here... Please use work
abstraction and possibly a separate workqueue.

> +
> + ad7142_task = kthread_run(ad7142_thread, NULL, "ad7142_task");
> + if (IS_ERR(ad7142_task)) {
> + printk(KERN_ERR "serio: Failed to start kseriod\n");

kseriod?

> + return PTR_ERR(ad7142_task);
> + }
> + return 0;
> +}
> +
> +static void ad7142_close(struct input_dev *dev)
> +{

Don't you need to write something over i2c to tell the device to shut
down? As it is now I expect the device to continue raising its IRQ
until kernel decides that it is unhandled and should be ignored.

> + free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);

Ok, so you freeing IRQ here, but it is allocated in ad7142_probe().
What happen if you try to open device after it was closed?

> + kthread_stop(ad7142_task);
> +}
> +
> +static int __init ad7142_init(void)
> +{
> + return i2c_add_driver(&ad7142_driver);
> +}
> +
> +static void __exit ad7142_exit(void)
> +{
> + i2c_del_driver(&ad7142_driver);
> + input_unregister_device(ad7142_dev);

input_unregister_device() should be in ad7142_detach_client? I am not
sure i2c - there seems to be 2 interface styles and you probably need
to use the new one. I am CC-inj Jean on this.

> +}
> +
> +module_init(ad7142_init);
> +module_exit(ad7142_exit);
> --
> 1.5.3.4
>


--
Dmitry

2007-10-15 17:24:40

by Bryan Wu

[permalink] [raw]
Subject: Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver

On 10/15/07, Dmitry Torokhov <[email protected]> wrote:
> Hi Bryan,
>
> On 10/15/07, Bryan Wu <[email protected]> wrote:
> > +
> > +static int ad7142_thread(void *nothing)
> > +{
> > + do {
> > + wait_for_completion(&ad7142_completion);
> > + ad7142_decode();
> > + enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
> > + } while (!kthread_should_stop());
> > +
>
> No, this is not going to work well:
> - you at least need to reinitialize the completion before enabling
> IRQ, otherwise you will spin in a very tight loop
> - if noone would touch the joystick ad7142_clsoe would() block
> infinitely because noone would signal the completion and
> ad7142_thread() would never stop.
>
> Completion is just not a good abstraction here... Please use work
> abstraction and possibly a separate workqueue.

Yes, I agree with you now, although I have a little concern about the
possibility of big delay introduced by workqueue.

>
> > +
> > + ad7142_task = kthread_run(ad7142_thread, NULL, "ad7142_task");
> > + if (IS_ERR(ad7142_task)) {
> > + printk(KERN_ERR "serio: Failed to start kseriod\n");
>
> kseriod?

My fault, I did't notice this copy words from other driver.

>
> > + return PTR_ERR(ad7142_task);
> > + }
> > + return 0;
> > +}
> > +
> > +static void ad7142_close(struct input_dev *dev)
> > +{
>
> Don't you need to write something over i2c to tell the device to shut
> down? As it is now I expect the device to continue raising its IRQ
> until kernel decides that it is unhandled and should be ignored.
>
I am not sure here, should do some investigate of the hardware datasheet.


> > + free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
>
> Ok, so you freeing IRQ here, but it is allocated in ad7142_probe().
> What happen if you try to open device after it was closed?
>
Yes, it should be move to ad7142_detach_client()

> > + kthread_stop(ad7142_task);
> > +}
> > +
> > +static int __init ad7142_init(void)
> > +{
> > + return i2c_add_driver(&ad7142_driver);
> > +}
> > +
> > +static void __exit ad7142_exit(void)
> > +{
> > + i2c_del_driver(&ad7142_driver);
> > + input_unregister_device(ad7142_dev);
>
> input_unregister_device() should be in ad7142_detach_client? I am not
> sure i2c - there seems to be 2 interface styles and you probably need
> to use the new one. I am CC-inj Jean on this.
>
Yes, no need input_unregister_device() here.

Thanks a lot for you kindly review.
I will resend update patch later.

Best Regards,
-Bryan Wu

2007-10-15 17:40:55

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver

On 10/15/07, Bryan Wu <[email protected]> wrote:
> On 10/15/07, Dmitry Torokhov <[email protected]> wrote:
> >
> > Completion is just not a good abstraction here... Please use work
> > abstraction and possibly a separate workqueue.
>
> Yes, I agree with you now, although I have a little concern about the
> possibility of big delay introduced by workqueue.
>

Having a separate workqueue should isolate the driver from users
hogging keventd. Otherwise the speed should be pretty much the same as
with a kthread.

>
> Thanks a lot for you kindly review.
> I will resend update patch later.

Thank you for not getting frustrated with all my change requests. Btw,
blackfin keypad driver is in my tree and should be in mainline once
Linus does the pull I requested.

--
Dmitry

2007-10-15 18:28:22

by Ahmed S. Darwish

[permalink] [raw]
Subject: Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver

On Mon, Oct 15, 2007 at 11:48:17AM -0400, Dmitry Torokhov wrote:
> Hi Bryan,
>
> On 10/15/07, Bryan Wu <[email protected]> wrote:
> > +
> > +static int ad7142_thread(void *nothing)
> > +{
> > + do {
> > + wait_for_completion(&ad7142_completion);
> > + ad7142_decode();
> > + enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
> > + } while (!kthread_should_stop());
> > +
>
> No, this is not going to work well:
> - you at least need to reinitialize the completion before enabling
> IRQ, otherwise you will spin in a very tight loop
> - if noone would touch the joystick ad7142_clsoe would() block
> infinitely because noone would signal the completion and
> ad7142_thread() would never stop.
>
> Completion is just not a good abstraction here... Please use work
> abstraction and possibly a separate workqueue.
>

Bryan, I'm very interested in the technical advantage of using a completion
here.

In my _not-experienced_ opinion, I remember completions was created mainly for
"create_task, wait till task got finished, go on" case. Why using it in a
different context while workqueues was created for a similar situation to
ad7142 one (non-irq context bottom-half) ?

Regards,

--
Ahmed S. Darwish
HomePage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

2007-10-15 19:35:52

by Jean Delvare

[permalink] [raw]
Subject: Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver

On Mon, 15 Oct 2007 11:48:17 -0400, Dmitry Torokhov wrote:
> On 10/15/07, Bryan Wu <[email protected]> wrote:
> > +static int __init ad7142_init(void)
> > +{
> > + return i2c_add_driver(&ad7142_driver);
> > +}
> > +
> > +static void __exit ad7142_exit(void)
> > +{
> > + i2c_del_driver(&ad7142_driver);
> > + input_unregister_device(ad7142_dev);
>
> input_unregister_device() should be in ad7142_detach_client? I am not
> sure i2c - there seems to be 2 interface styles and you probably need
> to use the new one. I am CC-inj Jean on this.

The new style is preferred, yes.

--
Jean Delvare

2007-10-16 05:52:10

by Bryan Wu

[permalink] [raw]
Subject: Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver

On 10/16/07, Dmitry Torokhov <[email protected]> wrote:
> On 10/15/07, Bryan Wu <[email protected]> wrote:
> > On 10/15/07, Dmitry Torokhov <[email protected]> wrote:
> > >
> > > Completion is just not a good abstraction here... Please use work
> > > abstraction and possibly a separate workqueue.
> >
> > Yes, I agree with you now, although I have a little concern about the
> > possibility of big delay introduced by workqueue.
> >
>
> Having a separate workqueue should isolate the driver from users
> hogging keventd. Otherwise the speed should be pretty much the same as
> with a kthread.
>

Does this driver need the create a new kthread instead of keventd?
I think keventd might be sufficient for this driver.

> >
> > Thanks a lot for you kindly review.
> > I will resend update patch later.
>
> Thank you for not getting frustrated with all my change requests.

Oh, your help is very useful. It encourages us to send out our drivers
to LKML.

> Btw,
> blackfin keypad driver is in my tree and should be in mainline once
> Linus does the pull I requested.
>

Thanks again, I noticed it was merged already.

-Bryan Wu

2007-10-16 06:08:22

by Bryan Wu

[permalink] [raw]
Subject: Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver

On 10/16/07, Ahmed S. Darwish <[email protected]> wrote:
> On Mon, Oct 15, 2007 at 11:48:17AM -0400, Dmitry Torokhov wrote:
> > Hi Bryan,
> >
> > On 10/15/07, Bryan Wu <[email protected]> wrote:
> > > +
> > > +static int ad7142_thread(void *nothing)
> > > +{
> > > + do {
> > > + wait_for_completion(&ad7142_completion);
> > > + ad7142_decode();
> > > + enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
> > > + } while (!kthread_should_stop());
> > > +
> >
> > No, this is not going to work well:
> > - you at least need to reinitialize the completion before enabling
> > IRQ, otherwise you will spin in a very tight loop
> > - if noone would touch the joystick ad7142_clsoe would() block
> > infinitely because noone would signal the completion and
> > ad7142_thread() would never stop.
> >
> > Completion is just not a good abstraction here... Please use work
> > abstraction and possibly a separate workqueue.
> >
>
> Bryan, I'm very interested in the technical advantage of using a completion
> here.
>
You are welcome, I'd like to discuss these things here.

> In my _not-experienced_ opinion, I remember completions was created mainly for
> "create_task, wait till task got finished, go on" case. Why using it in a
> different context while workqueues was created for a similar situation to
> ad7142 one (non-irq context bottom-half) ?

I like completion because it is simple to use and understand. Your
understanding is right. But there is no limit for using different
context with completion. completion is a wrapper of waitqueue+done
flag. For some drivers, in process context call
wait_for_completetion(), then schedule out and in irq handler call
complete(). This is very simple and helpful for driver design (For
example, you call dma function to transfer data, then you schedule out
and then DMA IRQ handler will call complete() to wakeup you).

But in this driver, a) can not call ad7142_decode() in IRQ handler,
because it will sleep in IRQ context by calling some i2c API, b) in
original design, creating a new kthread and using some waitqueue API
is the same way as using workqueue, c) cannot use completion as Dmitry
said.

I am going to use workqueue here.

Any idea?

Thanks
-Bryan Wu

2007-10-16 16:41:17

by Ahmed S. Darwish

[permalink] [raw]
Subject: Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver

On Tue, Oct 16, 2007 at 02:08:04PM +0800, Bryan Wu wrote:
> On 10/16/07, Ahmed S. Darwish <[email protected]> wrote:
> > On Mon, Oct 15, 2007 at 11:48:17AM -0400, Dmitry Torokhov wrote:
> > > Hi Bryan,
> > >
> > > On 10/15/07, Bryan Wu <[email protected]> wrote:
> > > > +
> > > > +static int ad7142_thread(void *nothing)
> > > > +{
> > > > + do {
> > > > + wait_for_completion(&ad7142_completion);
> > > > + ad7142_decode();
> > > > + enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
> > > > + } while (!kthread_should_stop());
> > > > +
> > >
> > > No, this is not going to work well:
> > > - you at least need to reinitialize the completion before enabling
> > > IRQ, otherwise you will spin in a very tight loop
> > > - if noone would touch the joystick ad7142_clsoe would() block
> > > infinitely because noone would signal the completion and
> > > ad7142_thread() would never stop.
> > >
> > > Completion is just not a good abstraction here... Please use work
> > > abstraction and possibly a separate workqueue.
> > >
> >
> > Bryan, I'm very interested in the technical advantage of using a completion
> > here.
> >
> You are welcome, I'd like to discuss these things here.
>
> > In my _not-experienced_ opinion, I remember completions was created mainly for
> > "create_task, wait till task got finished, go on" case. Why using it in a
> > different context while workqueues was created for a similar situation to
> > ad7142 one (non-irq context bottom-half) ?
>
> I like completion because it is simple to use and understand. Your
> understanding is right. But there is no limit for using different
> context with completion. completion is a wrapper of waitqueue+done
> flag. For some drivers, in process context call
> wait_for_completetion(), then schedule out and in irq handler call
> complete(). This is very simple and helpful for driver design (For
> example, you call dma function to transfer data, then you schedule out
> and then DMA IRQ handler will call complete() to wakeup you).
>

Thank you for such a useful information.

> But in this driver, a) can not call ad7142_decode() in IRQ handler,
> because it will sleep in IRQ context by calling some i2c API, b) in
> original design, creating a new kthread and using some waitqueue API
> is the same way as using workqueue, c) cannot use completion as Dmitry
> said.
>
> I am going to use workqueue here.
>
> Any idea?
>

I have no better thoughts than the ones provided by Dmitry actually.

> Thanks
> -Bryan Wu

Regards :),

--
Ahmed S. Darwish
HomePage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

2007-10-16 16:53:39

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver

On 10/16/07, Bryan Wu <[email protected]> wrote:
> On 10/16/07, Dmitry Torokhov <[email protected]> wrote:
> > On 10/15/07, Bryan Wu <[email protected]> wrote:
> > > On 10/15/07, Dmitry Torokhov <[email protected]> wrote:
> > > >
> > > > Completion is just not a good abstraction here... Please use work
> > > > abstraction and possibly a separate workqueue.
> > >
> > > Yes, I agree with you now, although I have a little concern about the
> > > possibility of big delay introduced by workqueue.
> > >
> >
> > Having a separate workqueue should isolate the driver from users
> > hogging keventd. Otherwise the speed should be pretty much the same as
> > with a kthread.
> >
>
> Does this driver need the create a new kthread instead of keventd?
> I think keventd might be sufficient for this driver.
>

No it does not have to start a new workqueue. I'd start with keventd
and only implement a separate workqueue later if I saw the driver
being starved by other keventd users.

--
Dmitry