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

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

[Bryan Wu]

Subject: [PATCH try #4] 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

[try #4] Changelog:
 - Use workqueue instead of completion
 - Shutdown AD7142 in ad7142_close()
 - Remove input_unregister_device() in ad7142_exit()

Cc: Andrey Panin <pazke@donpac.ru>
Cc: Roel Kluin <12o3l@tiscali.nl>
Cc: Ahmed S. Darwish <darwish.07@gmail.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Bryan Wu <bryan.wu@analog.com>
---
 drivers/input/joystick/Kconfig  |   15 ++
 drivers/input/joystick/Makefile |    1 +
 drivers/input/joystick/ad7142.c |  485 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 501 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..5c8c8ca 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -282,4 +282,19 @@ 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..e5d2ccc
--- /dev/null
+++ b/drivers/input/joystick/ad7142.c
@@ -0,0 +1,485 @@
+/*
+ * File:         drivers/input/joystick/ad7142.c
+ * Original Author: Aubrey Li
+ * Maintained by: Bryan Wu <bryan.wu@analog.com>
+ *
+ * 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/uaccess.h>
+#include <linux/irq.h>
+#include <linux/workqueue.h>
+
+#include <asm/blackfin.h>
+
+MODULE_AUTHOR("Aubrey Li, Bryan Wu <bryan.wu@analog.com>");
+MODULE_DESCRIPTION("Driver for AD7142 Joysticks");
+MODULE_LICENSE("GPL");
+
+#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},
+};
+
+struct ad7142_data {
+	struct input_dev *input_dev;
+
+	struct i2c_driver i2c_drv;
+	struct i2c_client client;
+
+	struct work_struct work;
+
+	unsigned short old_status_low;
+	unsigned short old_status_high;
+};
+
+static unsigned short ignore[] = { I2C_CLIENT_END };
+static unsigned short normal_addr[] = { AD7142_I2C_ADDR, I2C_CLIENT_END };
+
+static struct i2c_client_address_data addr_data = {
+	.normal_i2c = normal_addr,
+	.probe = ignore,
+	.ignore = ignore,
+};
+
+static int ad7142_attach(struct i2c_adapter *adap);
+static int ad7142_detach(struct i2c_client *client);
+
+static struct i2c_driver ad7142_driver = {
+	.driver = {
+		.name = AD7142_DRV_NAME,
+	},
+	.id = AD7142_I2C_ID,
+	.attach_adapter = ad7142_attach,
+	.detach_client = ad7142_detach,
+};
+
+static irqreturn_t ad7142_interrupt(int irq, void *_data)
+{
+	struct ad7142_data *data = _data;
+
+	schedule_work(&data->work);
+
+	return IRQ_HANDLED;
+}
+
+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 void ad7142_work(struct work_struct *work)
+{
+	struct ad7142_data *data = container_of(work, struct ad7142_data, work);
+	struct i2c_client *client = &data->client;
+	struct input_dev *input_dev = data->input_dev;
+	unsigned short irqno_low, irqno_high;
+	unsigned short temp;
+
+	ad7142_i2c_read(client, INTSTAT_REG0, &irqno_low, 1);
+	temp = irqno_low ^ data->old_status_low;
+	switch (temp) {
+	case 0x0001:
+		input_report_key(input_dev, BTN_BASE, (irqno_low & 0x0001));
+		break;
+	case 0x0002:
+		input_report_key(input_dev, BTN_BASE4,
+					((irqno_low & 0x0002) >> 1));
+		break;
+	case 0x0004:
+		input_report_key(input_dev, KEY_UP,
+					((irqno_low & 0x0004) >> 2));
+		break;
+	case 0x0008:
+		input_report_key(input_dev, KEY_RIGHT,
+					((irqno_low & 0x0008) >> 3));
+		break;
+	}
+	data->old_status_low = irqno_low;
+
+	ad7142_i2c_read(client, INTSTAT_REG1, &irqno_high, 1);
+	temp = irqno_high ^ data->old_status_high;
+	switch (temp) {
+	case 0x0001:
+		input_report_key(input_dev, BTN_BASE2, irqno_high & 0x0001);
+		break;
+	case 0x0002:
+		input_report_key(input_dev, BTN_BASE3,
+					((irqno_high & 0x0002) >> 1));
+		break;
+	case 0x0004:
+		input_report_key(input_dev, KEY_DOWN,
+					((irqno_high & 0x0004) >> 2));
+		break;
+	case 0x0008:
+		input_report_key(input_dev, KEY_LEFT,
+					((irqno_high & 0x0008) >> 3));
+		break;
+	}
+	data->old_status_high = irqno_high;
+
+	input_sync(input_dev);
+}
+
+static int ad7142_open(struct input_dev *dev)
+{
+	struct ad7142_data *data = input_get_drvdata(dev);
+	struct i2c_client *client = &data->client;
+	unsigned short id, value;
+
+	ad7142_i2c_read(client, DEVID, &id, 1);
+	if (id != AD7142_I2C_ID) {
+		printk(KERN_ERR "Open AD7142 error\n");
+		return -ENODEV;
+	}
+
+	ad7142_i2c_write(client, STAGE0_CONNECTION, stage[0], 8);
+	ad7142_i2c_write(client, STAGE1_CONNECTION, stage[1], 8);
+	ad7142_i2c_write(client, STAGE2_CONNECTION, stage[2], 8);
+	ad7142_i2c_write(client, STAGE3_CONNECTION, stage[3], 8);
+	ad7142_i2c_write(client, STAGE4_CONNECTION, stage[4], 8);
+	ad7142_i2c_write(client, STAGE5_CONNECTION, stage[4], 8);
+	ad7142_i2c_write(client, STAGE6_CONNECTION, stage[4], 8);
+	ad7142_i2c_write(client, STAGE7_CONNECTION, stage[4], 8);
+	ad7142_i2c_write(client, STAGE8_CONNECTION, stage[4], 8);
+	ad7142_i2c_write(client, STAGE9_CONNECTION, stage[4], 8);
+	ad7142_i2c_write(client, STAGE10_CONNECTION, stage[4], 8);
+	ad7142_i2c_write(client, STAGE11_CONNECTION, stage[4], 8);
+
+	/* In full power mode */
+	value = 0x00B0;
+	ad7142_i2c_write(client, PWRCONVCTL, &value, 1);
+
+	value = 0x0690;
+	ad7142_i2c_write(client, AMBCOMPCTL_REG1, &value, 1);
+
+	value = 0x0664;
+	ad7142_i2c_write(client, AMBCOMPCTL_REG2, &value, 1);
+
+	value = 0x290F;
+	ad7142_i2c_write(client, AMBCOMPCTL_REG3, &value, 1);
+
+	value = 0x000F;
+	ad7142_i2c_write(client, INTEN_REG0, &value, 1);
+	ad7142_i2c_write(client, INTEN_REG1, &value, 1);
+
+	value = 0x0000;
+	ad7142_i2c_write(client, INTEN_REG2, &value, 1);
+
+	ad7142_i2c_read(client, AMBCOMPCTL_REG1, &value, 1);
+
+	value = 0x000F;
+	ad7142_i2c_write(client, AMBCOMPCTL_REG0, &value, 1);
+
+	return 0;
+}
+
+static void ad7142_close(struct input_dev *dev)
+{
+	struct ad7142_data *data = input_get_drvdata(dev);
+	struct i2c_client *client = &data->client;
+	unsigned short value;
+
+	flush_scheduled_work();
+
+	/*
+	 * Turn AD7142 to full shutdown mode
+	 * No CDC conversions
+	 */
+	value = 0x0001;
+	ad7142_i2c_write(client, PWRCONVCTL, &value, 1);
+}
+
+static int ad7142_probe(struct i2c_adapter *adap, int addr, int kind)
+{
+	struct ad7142_data *data;
+	struct i2c_client *client;
+	struct input_dev *input_dev;
+	int rc;
+
+	data = kzalloc(sizeof(struct ad7142_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	client = &data->client;
+
+	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;
+	}
+
+	/* Start workqueue for defer message transfer */
+	INIT_WORK(&data->work, ad7142_work);
+
+	rc = request_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt,
+			IRQF_TRIGGER_LOW, "ad7142_joy", data);
+	if (rc) {
+		printk(KERN_ERR "AD7142: Can't allocate irq %d\n",
+			CONFIG_BFIN_JOYSTICK_IRQ_PFX);
+		goto fail_check;
+	}
+
+	printk(KERN_INFO "%s_attach: at 0x%02x\n",
+			client->name, client->addr << 1);
+
+	/* Allocate and register AD7142 input device */
+	data->input_dev = input_allocate_device();
+	if (!data->input_dev) {
+		printk(KERN_ERR "AD7142: Can't allocate input device\n");
+		rc = -ENOMEM;
+		goto fail_allocate;
+	}
+
+	input_dev = data->input_dev;
+	input_dev->open = ad7142_open;
+	input_dev->close = ad7142_close;
+	input_dev->evbit[0] = BIT(EV_KEY);
+	input_dev->keybit[LONG(BTN_BASE)] = BIT(BTN_BASE) | BIT(BTN_BASE2) |
+						BIT(BTN_BASE3) | BIT(BTN_BASE4);
+	input_dev->keybit[LONG(KEY_UP)] |= BIT(KEY_UP) | BIT(KEY_DOWN) |
+						BIT(KEY_LEFT) | BIT(KEY_RIGHT);
+
+	input_dev->name = "ad7142 joystick";
+	input_dev->phys = "ad7142/input0";
+	input_dev->id.bustype = BUS_I2C;
+	input_dev->id.vendor = 0x0001;
+	input_dev->id.product = 0x0001;
+	input_dev->id.version = 0x0100;
+
+	input_set_drvdata(input_dev, data);
+
+	rc = input_register_device(input_dev);
+	if (rc) {
+		printk(KERN_ERR "Failed to register AD7142 input device!\n");
+		goto fail_register;
+	}
+
+	return 0;
+
+fail_register:
+	input_free_device(input_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_attach(struct i2c_adapter *adap)
+{
+	return i2c_probe(adap, &addr_data, &ad7142_probe);
+}
+
+static int ad7142_detach(struct i2c_client *client)
+{
+	int rc;
+
+	free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
+
+	flush_scheduled_work();
+
+	rc = i2c_detach_client(client);
+	if (!rc)
+		kfree(i2c_get_clientdata(client));
+	return rc;
+}
+
+
+static int __init ad7142_init(void)
+{
+	return i2c_add_driver(&ad7142_driver);
+}
+
+static void __exit ad7142_exit(void)
+{
+	i2c_del_driver(&ad7142_driver);
+}
+
+module_init(ad7142_init);
+module_exit(ad7142_exit);
-- 
1.5.3.4

Re: [PATCH try #4] Input/Joystick Driver: add support AD7142 joystick driver

[Jean Delvare]

Hi Bryan,

On Wed, 17 Oct 2007 15:12:00 +0800, Bryan Wu wrote:
> Subject: [PATCH try #4] Input/Joystick Driver: add support AD7142 joystick driver

This driver is a good candidate for the new-style i2c infrastructure.
Instead of letting i2c-core probe for the device on all available I2C
buses, you would explicitly instantiate it from platform data. In
practice this would mean:
* Declare the ad7142 in platform data.
* Call i2c_register_board_info() on that data.
* In your ad7142 driver, implement .probe and .remove instead
  of .attach_adapter and .detach_client.

Of course this assumes that the underlying bus driver is also new-style
ready (i.e. it uses i2c_add_numbered_adapter() instead of
i2c_add_adapter()), and that you know in advance when the AD7142 is
there or not. I hope this is the case?

Using a new-style i2c driver is especially important if the device
can't be detected.

Also see my i2c-related comments inline.

> 
> [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
> 
> [try #4] Changelog:
>  - Use workqueue instead of completion
>  - Shutdown AD7142 in ad7142_close()
>  - Remove input_unregister_device() in ad7142_exit()
> 
> Cc: Andrey Panin <pazke@donpac.ru>
> Cc: Roel Kluin <12o3l@tiscali.nl>
> Cc: Ahmed S. Darwish <darwish.07@gmail.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Bryan Wu <bryan.wu@analog.com>
> ---
>  drivers/input/joystick/Kconfig  |   15 ++
>  drivers/input/joystick/Makefile |    1 +
>  drivers/input/joystick/ad7142.c |  485 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 501 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..5c8c8ca 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -282,4 +282,19 @@ 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)

Parentheses aren't needed.

> +	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..e5d2ccc
> --- /dev/null
> +++ b/drivers/input/joystick/ad7142.c
> @@ -0,0 +1,485 @@
> +/*
> + * File:         drivers/input/joystick/ad7142.c
> + * Original Author: Aubrey Li
> + * Maintained by: Bryan Wu <bryan.wu@analog.com>
> + *
> + * 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/uaccess.h>
> +#include <linux/irq.h>
> +#include <linux/workqueue.h>
> +
> +#include <asm/blackfin.h>
> +
> +MODULE_AUTHOR("Aubrey Li, Bryan Wu <bryan.wu@analog.com>");
> +MODULE_DESCRIPTION("Driver for AD7142 Joysticks");
> +MODULE_LICENSE("GPL");
> +
> +#define AD7142_DRV_NAME		"ad7142_js"
> +#define AD7142_I2C_ID		0xE622

No! I2C driver IDs are defined in <linux/i2c-id.h> _and only there_!

Anyway, using these IDs is deprecated so you should simply not set any
driver ID at all.

> +#define AD7142_I2C_ADDR		0x2C

Not worth a define IMHO, as you use it only once and the context is
completely clear.

> +/*
> + * 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] = {

Could be const, I guess.

> +	{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},
> +};
> +
> +struct ad7142_data {
> +	struct input_dev *input_dev;
> +
> +	struct i2c_driver i2c_drv;
> +	struct i2c_client client;

Embedding a driver inside a device-private data structure doesn't look
right. Something must be wrong, please check.

> +
> +	struct work_struct work;
> +
> +	unsigned short old_status_low;
> +	unsigned short old_status_high;
> +};
> +
> +static unsigned short ignore[] = { I2C_CLIENT_END };
> +static unsigned short normal_addr[] = { AD7142_I2C_ADDR, I2C_CLIENT_END };
> +
> +static struct i2c_client_address_data addr_data = {
> +	.normal_i2c = normal_addr,
> +	.probe = ignore,
> +	.ignore = ignore,
> +};

Why don't you let the user set probe and ignore addresses at driver
load time? Please use I2C_CLIENT_INSMOD_1() instead of hard-coding
empty lists.

Note: if you switch to a new-style driver, that part of the code will
go away anyway so you can ignore this comment.

> +
> +static int ad7142_attach(struct i2c_adapter *adap);
> +static int ad7142_detach(struct i2c_client *client);
> +
> +static struct i2c_driver ad7142_driver = {
> +	.driver = {
> +		.name = AD7142_DRV_NAME,
> +	},
> +	.id = AD7142_I2C_ID,
> +	.attach_adapter = ad7142_attach,
> +	.detach_client = ad7142_detach,
> +};
> +
> +static irqreturn_t ad7142_interrupt(int irq, void *_data)
> +{
> +	struct ad7142_data *data = _data;
> +
> +	schedule_work(&data->work);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ad7142_i2c_write(struct i2c_client *client, unsigned short offset,
> +		unsigned short *data, unsigned int len)

Should be const unsigned short *data.

> +{
> +	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");

Please use dev_err() instead for all the run-time error messages.

> +		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;
> +	}

Doesn't look safe. What happens if there is another call to either
ad7142_i2c_write() or ad7142_i2c_read() in parallel? Corruption could
ensue, methinks.

You need to do one of two things: either protect the I2C accesses with
a per-device mutex; or use i2c_transfer() instead of i2c_master_send()
+ i2c_master_recv() above. The second solution is by far the most
efficient.

> +
> +	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 void ad7142_work(struct work_struct *work)
> +{
> +	struct ad7142_data *data = container_of(work, struct ad7142_data, work);
> +	struct i2c_client *client = &data->client;
> +	struct input_dev *input_dev = data->input_dev;
> +	unsigned short irqno_low, irqno_high;
> +	unsigned short temp;
> +
> +	ad7142_i2c_read(client, INTSTAT_REG0, &irqno_low, 1);
> +	temp = irqno_low ^ data->old_status_low;
> +	switch (temp) {
> +	case 0x0001:
> +		input_report_key(input_dev, BTN_BASE, (irqno_low & 0x0001));
> +		break;
> +	case 0x0002:
> +		input_report_key(input_dev, BTN_BASE4,
> +					((irqno_low & 0x0002) >> 1));
> +		break;
> +	case 0x0004:
> +		input_report_key(input_dev, KEY_UP,
> +					((irqno_low & 0x0004) >> 2));
> +		break;
> +	case 0x0008:
> +		input_report_key(input_dev, KEY_RIGHT,
> +					((irqno_low & 0x0008) >> 3));
> +		break;
> +	}
> +	data->old_status_low = irqno_low;
> +
> +	ad7142_i2c_read(client, INTSTAT_REG1, &irqno_high, 1);
> +	temp = irqno_high ^ data->old_status_high;
> +	switch (temp) {
> +	case 0x0001:
> +		input_report_key(input_dev, BTN_BASE2, irqno_high & 0x0001);
> +		break;
> +	case 0x0002:
> +		input_report_key(input_dev, BTN_BASE3,
> +					((irqno_high & 0x0002) >> 1));
> +		break;
> +	case 0x0004:
> +		input_report_key(input_dev, KEY_DOWN,
> +					((irqno_high & 0x0004) >> 2));
> +		break;
> +	case 0x0008:
> +		input_report_key(input_dev, KEY_LEFT,
> +					((irqno_high & 0x0008) >> 3));
> +		break;
> +	}
> +	data->old_status_high = irqno_high;
> +
> +	input_sync(input_dev);
> +}
> +
> +static int ad7142_open(struct input_dev *dev)
> +{
> +	struct ad7142_data *data = input_get_drvdata(dev);
> +	struct i2c_client *client = &data->client;
> +	unsigned short id, value;
> +
> +	ad7142_i2c_read(client, DEVID, &id, 1);
> +	if (id != AD7142_I2C_ID) {
> +		printk(KERN_ERR "Open AD7142 error\n");
> +		return -ENODEV;
> +	}
> +
> +	ad7142_i2c_write(client, STAGE0_CONNECTION, stage[0], 8);
> +	ad7142_i2c_write(client, STAGE1_CONNECTION, stage[1], 8);
> +	ad7142_i2c_write(client, STAGE2_CONNECTION, stage[2], 8);
> +	ad7142_i2c_write(client, STAGE3_CONNECTION, stage[3], 8);
> +	ad7142_i2c_write(client, STAGE4_CONNECTION, stage[4], 8);
> +	ad7142_i2c_write(client, STAGE5_CONNECTION, stage[4], 8);
> +	ad7142_i2c_write(client, STAGE6_CONNECTION, stage[4], 8);
> +	ad7142_i2c_write(client, STAGE7_CONNECTION, stage[4], 8);
> +	ad7142_i2c_write(client, STAGE8_CONNECTION, stage[4], 8);
> +	ad7142_i2c_write(client, STAGE9_CONNECTION, stage[4], 8);
> +	ad7142_i2c_write(client, STAGE10_CONNECTION, stage[4], 8);
> +	ad7142_i2c_write(client, STAGE11_CONNECTION, stage[4], 8);
> +
> +	/* In full power mode */
> +	value = 0x00B0;
> +	ad7142_i2c_write(client, PWRCONVCTL, &value, 1);
> +
> +	value = 0x0690;
> +	ad7142_i2c_write(client, AMBCOMPCTL_REG1, &value, 1);
> +
> +	value = 0x0664;
> +	ad7142_i2c_write(client, AMBCOMPCTL_REG2, &value, 1);
> +
> +	value = 0x290F;
> +	ad7142_i2c_write(client, AMBCOMPCTL_REG3, &value, 1);
> +
> +	value = 0x000F;
> +	ad7142_i2c_write(client, INTEN_REG0, &value, 1);
> +	ad7142_i2c_write(client, INTEN_REG1, &value, 1);
> +
> +	value = 0x0000;
> +	ad7142_i2c_write(client, INTEN_REG2, &value, 1);
> +
> +	ad7142_i2c_read(client, AMBCOMPCTL_REG1, &value, 1);
> +
> +	value = 0x000F;
> +	ad7142_i2c_write(client, AMBCOMPCTL_REG0, &value, 1);
> +
> +	return 0;
> +}
> +
> +static void ad7142_close(struct input_dev *dev)
> +{
> +	struct ad7142_data *data = input_get_drvdata(dev);
> +	struct i2c_client *client = &data->client;
> +	unsigned short value;
> +
> +	flush_scheduled_work();
> +
> +	/*
> +	 * Turn AD7142 to full shutdown mode
> +	 * No CDC conversions
> +	 */
> +	value = 0x0001;
> +	ad7142_i2c_write(client, PWRCONVCTL, &value, 1);
> +}
> +
> +static int ad7142_probe(struct i2c_adapter *adap, int addr, int kind)
> +{
> +	struct ad7142_data *data;
> +	struct i2c_client *client;
> +	struct input_dev *input_dev;
> +	int rc;
> +
> +	data = kzalloc(sizeof(struct ad7142_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	client = &data->client;
> +
> +	strlcpy(client->name, AD7142_DRV_NAME, I2C_NAME_SIZE);
> +	client->addr = addr;
> +	client->adapter = adap;
> +	client->driver = &ad7142_driver;
> +

This is unsafe: you're not doing any kind of detection here. Address
0x2c is rather popular for hardware monitoring chips, so a misdetection
could easily happen. And given that your driver is writing a lot to the
chip, it could have bad consequences. The risk is mitigated by the fact
that your driver is currently built for Blackfin only, but this might
change in the future. This is a very good reason to switch to a
new-style i2c 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;
> +	}

It would be more efficient to test this _before_ you allocate the
memory and instantiate the i2c client.

> +
> +	/* Start workqueue for defer message transfer */
> +	INIT_WORK(&data->work, ad7142_work);
> +
> +	rc = request_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt,
> +			IRQF_TRIGGER_LOW, "ad7142_joy", data);
> +	if (rc) {
> +		printk(KERN_ERR "AD7142: Can't allocate irq %d\n",
> +			CONFIG_BFIN_JOYSTICK_IRQ_PFX);
> +		goto fail_check;
> +	}
> +
> +	printk(KERN_INFO "%s_attach: at 0x%02x\n",
> +			client->name, client->addr << 1);
> +
> +	/* Allocate and register AD7142 input device */
> +	data->input_dev = input_allocate_device();
> +	if (!data->input_dev) {
> +		printk(KERN_ERR "AD7142: Can't allocate input device\n");
> +		rc = -ENOMEM;
> +		goto fail_allocate;
> +	}
> +
> +	input_dev = data->input_dev;
> +	input_dev->open = ad7142_open;
> +	input_dev->close = ad7142_close;
> +	input_dev->evbit[0] = BIT(EV_KEY);
> +	input_dev->keybit[LONG(BTN_BASE)] = BIT(BTN_BASE) | BIT(BTN_BASE2) |
> +						BIT(BTN_BASE3) | BIT(BTN_BASE4);
> +	input_dev->keybit[LONG(KEY_UP)] |= BIT(KEY_UP) | BIT(KEY_DOWN) |
> +						BIT(KEY_LEFT) | BIT(KEY_RIGHT);
> +
> +	input_dev->name = "ad7142 joystick";
> +	input_dev->phys = "ad7142/input0";
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->id.vendor = 0x0001;
> +	input_dev->id.product = 0x0001;
> +	input_dev->id.version = 0x0100;
> +
> +	input_set_drvdata(input_dev, data);
> +
> +	rc = input_register_device(input_dev);
> +	if (rc) {
> +		printk(KERN_ERR "Failed to register AD7142 input device!\n");
> +		goto fail_register;
> +	}
> +
> +	return 0;
> +
> +fail_register:
> +	input_free_device(input_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_attach(struct i2c_adapter *adap)
> +{
> +	return i2c_probe(adap, &addr_data, &ad7142_probe);
> +}
> +
> +static int ad7142_detach(struct i2c_client *client)
> +{
> +	int rc;
> +
> +	free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
> +
> +	flush_scheduled_work();
> +
> +	rc = i2c_detach_client(client);
> +	if (!rc)
> +		kfree(i2c_get_clientdata(client));
> +	return rc;
> +}
> +
> +
> +static int __init ad7142_init(void)
> +{
> +	return i2c_add_driver(&ad7142_driver);
> +}
> +
> +static void __exit ad7142_exit(void)
> +{
> +	i2c_del_driver(&ad7142_driver);
> +}
> +
> +module_init(ad7142_init);
> +module_exit(ad7142_exit);


-- 
Jean Delvare

Re: [PATCH try #4] Input/Joystick Driver: add support AD7142 joystick driver

[Bryan Wu]

On 10/17/07, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Bryan,
>
> On Wed, 17 Oct 2007 15:12:00 +0800, Bryan Wu wrote:
> > Subject: [PATCH try #4] Input/Joystick Driver: add support AD7142 joystick driver
>
> This driver is a good candidate for the new-style i2c infrastructure.
> Instead of letting i2c-core probe for the device on all available I2C
> buses, you would explicitly instantiate it from platform data. In
> practice this would mean:
> * Declare the ad7142 in platform data.
> * Call i2c_register_board_info() on that data.
> * In your ad7142 driver, implement .probe and .remove instead
>   of .attach_adapter and .detach_client.
>
I would like to rewrite it by using new-style i2c infrastructure.
But it will take some time. If I meet some problem, hope can get some advice
from you and the community.

> Of course this assumes that the underlying bus driver is also new-style
> ready (i.e. it uses i2c_add_numbered_adapter() instead of
> i2c_add_adapter()), and that you know in advance when the AD7142 is
> there or not. I hope this is the case?
>
Oh, currently i2c-bfin-twi driver is using the old-style code.

> Using a new-style i2c driver is especially important if the device
> can't be detected.
>
> Also see my i2c-related comments inline.
>

I will fix all the issues and try to pass our Blackfin AD7142 Joystick testcase.
and resubmit a patch.

Thanks
-Bryan

[!snip!]

Re: [PATCH try #4] Input/Joystick Driver: add support AD7142 joystick driver

[Dmitry Torokhov]

Hi Bryan,

On 10/17/07, Bryan Wu <bryan.wu@analog.com> wrote:
> Subject: [PATCH try #4] Input/Joystick Driver: add support AD7142 joystick driver
>

My comments are in addition to Jean's:

> +
> +static void ad7142_close(struct input_dev *dev)
> +{
> +       struct ad7142_data *data = input_get_drvdata(dev);
> +       struct i2c_client *client = &data->client;
> +       unsigned short value;
> +

I think you need disable_irq() here

> +       flush_scheduled_work();
> +
> +       /*
> +        * Turn AD7142 to full shutdown mode
> +        * No CDC conversions
> +        */
> +       value = 0x0001;
> +       ad7142_i2c_write(client, PWRCONVCTL, &value, 1);

and enable_irq() here. This way you can be sure that you will not
re-arm the work between flush_scheduled_work() and shutting off the
controller.

> +
> +static int ad7142_attach(struct i2c_adapter *adap)
> +{
> +       return i2c_probe(adap, &addr_data, &ad7142_probe);
> +}
> +
> +static int ad7142_detach(struct i2c_client *client)
> +{
> +       int rc;
> +
> +       free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
> +
> +       flush_scheduled_work();
> +
> +       rc = i2c_detach_client(client);
> +       if (!rc)
> +               kfree(i2c_get_clientdata(client));
> +       return rc;
> +}
> +
> +
> +static int __init ad7142_init(void)
> +{
> +       return i2c_add_driver(&ad7142_driver);
> +}
> +
> +static void __exit ad7142_exit(void)
> +{
> +       i2c_del_driver(&ad7142_driver);
> +}
> +
> +module_init(ad7142_init);
> +module_exit(ad7142_exit);

Where did input_unregister_device() go? It still needs to be somewhere...

-- 
Dmitry

Re: [PATCH try #4] Input/Joystick Driver: add support AD7142 joystick driver

[Robin Getz]

On Wed 17 Oct 2007 10:07, Jean Delvare pondered:
> Hi Bryan,
> 
> On Wed, 17 Oct 2007 15:12:00 +0800, Bryan Wu wrote:
> > Subject: [PATCH try #4] Input/Joystick Driver: add support AD7142
> joystick driver
> > +#define AD7142_I2C_ADDR		0x2C
> 
> Not worth a define IMHO, as you use it only once and the context is
> completely clear.

> > +static int ad7142_probe(struct i2c_adapter *adap, int addr, int kind)
> > +{
> > +	struct ad7142_data *data;
> > +	struct i2c_client *client;
> > +	struct input_dev *input_dev;
> > +	int rc;
> > +
> > +	data = kzalloc(sizeof(struct ad7142_data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	client = &data->client;
> > +
> > +	strlcpy(client->name, AD7142_DRV_NAME, I2C_NAME_SIZE);
> > +	client->addr = addr;
> > +	client->adapter = adap;
> > +	client->driver = &ad7142_driver;
> > +
> 
> This is unsafe: you're not doing any kind of detection here. 

I assume that in order to do things "right", all possible addresses should be 
probed (for this part, that is 0x2C, 0x2D, 0x2E, 0x2F)?

And it should read a device ID (register 0x17 == 0xE62n (where n==rev number))

How many values/registers do you need to read before you say "this is the 
one"? rather than a temp sensor with a similar value somewhere. Is there a 
table anywhere that maps this out?

It is pretty difficult just to probe on reset values, since that disallows 
warm resets. This part - like most other I2C parts, has no external reset, 
only software reset.

> The risk is mitigated by the fact
> that your driver is currently built for Blackfin only, but this might
> change in the future. 

The hope is that others can use it - it shouldn't be specific to Blackfin.

> This is a very good reason to switch to a 
> new-style i2c driver.
> 

Re: [PATCH try #4] Input/Joystick Driver: add support AD7142 joystick driver

[Jean Delvare]

Hi Robin,

On Wed, 17 Oct 2007 15:14:40 -0400, Robin Getz wrote:
> On Wed 17 Oct 2007 10:07, Jean Delvare pondered:
> > Hi Bryan,
> > 
> > On Wed, 17 Oct 2007 15:12:00 +0800, Bryan Wu wrote:
> > > Subject: [PATCH try #4] Input/Joystick Driver: add support AD7142
> > joystick driver
> > > +#define AD7142_I2C_ADDR		0x2C
> > 
> > Not worth a define IMHO, as you use it only once and the context is
> > completely clear.
> 
> > > +static int ad7142_probe(struct i2c_adapter *adap, int addr, int kind)
> > > +{
> > > +	struct ad7142_data *data;
> > > +	struct i2c_client *client;
> > > +	struct input_dev *input_dev;
> > > +	int rc;
> > > +
> > > +	data = kzalloc(sizeof(struct ad7142_data), GFP_KERNEL);
> > > +	if (!data)
> > > +		return -ENOMEM;
> > > +
> > > +	client = &data->client;
> > > +
> > > +	strlcpy(client->name, AD7142_DRV_NAME, I2C_NAME_SIZE);
> > > +	client->addr = addr;
> > > +	client->adapter = adap;
> > > +	client->driver = &ad7142_driver;
> > > +
> > 
> > This is unsafe: you're not doing any kind of detection here. 
> 
> I assume that in order to do things "right", all possible addresses should be 
> probed (for this part, that is 0x2C, 0x2D, 0x2E, 0x2F)?

Not necessarily. Given that detecting this chip safely seems to be
difficult (if not impossible), I'd rather only list the addresses which
are actually known to be used.

> And it should read a device ID (register 0x17 == 0xE62n (where n==rev number))

If the device does have an identification register, then yes you should
test here. However, I seem to understand that the AD7142 uses 2-byte
register addressing, while most I2C devices use 1-byte register
addressing (I2C is tricky in that respect). This means that "reading
from a register" for the AD7142 corresponds to "writing to a register"
for other chips. So even just reading from the device ID register in
the probe function isn't safe.

So to be on the safe side, the ad7142 driver should not probe _any_
address by default. Users should use the "probe" command line parameter
to explicitly ask for a given address to be probed.

> How many values/registers do you need to read before you say "this is the 
> one"? rather than a temp sensor with a similar value somewhere. Is there a 
> table anywhere that maps this out?

No, there is no standard at all. Each driver does its best to identify
the chip depending of the specific register map. When the devices don't
have ID registers, we test for unused bits or registers cycling over
arbitrary boundaries.

> It is pretty difficult just to probe on reset values, since that disallows 
> warm resets. This part - like most other I2C parts, has no external reset, 
> only software reset.

Agreed, don't probe on reset values.

> > The risk is mitigated by the fact
> > that your driver is currently built for Blackfin only, but this might
> > change in the future. 
> 
> The hope is that others can use it - it shouldn't be specific to Blackfin.

It the current state it's not acceptable. Too dangerous.

> > This is a very good reason to switch to a 
> > new-style i2c driver.

That's the way to go, really.

-- 
Jean Delvare