This patch adds a driver for the Nintendo Nunchuck (wiimote accessory)
attached to an i2c bus via something like a wiichuck adapter board
from Sparkfun.
Signed-off-by: Grant Likely <[email protected]>
---
This is RFC because the driver doesn't completely work yet. The
joystick and buttons work fine. There is a bug with the accelerometer
reporting though (nothing gets reported), and I'm not even sure I'm
using the right input type for reporting accelerometer values.
Comments welcome.
drivers/input/joystick/Kconfig | 8 +
drivers/input/joystick/Makefile | 1
drivers/input/joystick/wiichuck.c | 225 +++++++++++++++++++++++++++++++++++++
3 files changed, 234 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/joystick/wiichuck.c
diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index 56eb471..79f18db 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -193,6 +193,14 @@ config JOYSTICK_TWIDJOY
To compile this driver as a module, choose M here: the
module will be called twidjoy.
+config JOYSTICK_WIICHUCK
+ tristate "Nintendo Nunchuck on i2c bus"
+ depends on I2C
+ select INPUT_POLLDEV
+ help
+ Say Y here if you have a Nintendo Nunchuck directly attached to
+ the machine's i2c bus.
+
config JOYSTICK_ZHENHUA
tristate "5-byte Zhenhua RC transmitter"
select SERIO
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 92dc0de..78466d6 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_JOYSTICK_TMDC) += tmdc.o
obj-$(CONFIG_JOYSTICK_TURBOGRAFX) += turbografx.o
obj-$(CONFIG_JOYSTICK_TWIDJOY) += twidjoy.o
obj-$(CONFIG_JOYSTICK_WARRIOR) += warrior.o
+obj-$(CONFIG_JOYSTICK_WIICHUCK) += wiichuck.o
obj-$(CONFIG_JOYSTICK_XPAD) += xpad.o
obj-$(CONFIG_JOYSTICK_ZHENHUA) += zhenhua.o
obj-$(CONFIG_JOYSTICK_WALKERA0701) += walkera0701.o
diff --git a/drivers/input/joystick/wiichuck.c b/drivers/input/joystick/wiichuck.c
new file mode 100644
index 0000000..2afc274
--- /dev/null
+++ b/drivers/input/joystick/wiichuck.c
@@ -0,0 +1,225 @@
+/*
+ * Nintendo Nunchuck driver for i2c connection.
+ *
+ * Copyright (c) 2011 Secret Lab Technologies Ltd.
+ *
+ * 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 of the License, 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; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/input-polldev.h>
+#include <linux/mod_devicetable.h>
+#include <linux/slab.h>
+
+MODULE_AUTHOR("Grant Likely <[email protected]>");
+MODULE_DESCRIPTION("Nintendo Nunchuck driver");
+MODULE_LICENSE("GPL");
+
+#define WIICHUCK_JOY_MAX_AXIS 220
+#define WIICHUCK_JOY_MIN_AXIS 30
+#define WIICHUCK_JOY_FUZZ 4
+#define WIICHUCK_JOY_FLAT 8
+
+struct wiichuck_device {
+ struct input_polled_dev *poll_dev;
+ struct i2c_client *i2c_client;
+ int state;
+};
+
+static void wiichuck_poll(struct input_polled_dev *poll_dev)
+{
+ struct wiichuck_device *wiichuck = poll_dev->private;
+ struct i2c_client *i2c = wiichuck->i2c_client;
+ static uint8_t cmd_byte = 0;
+ struct i2c_msg cmd_msg =
+ { .addr = i2c->addr, .len = 1, .buf = &cmd_byte };
+ uint8_t b[6];
+ struct i2c_msg data_msg =
+ { .addr = i2c->addr, .flags = I2C_M_RD, .len = 6, .buf = b };
+ int jx, jy, ax, ay, az;
+ bool c, z;
+
+ switch (wiichuck->state) {
+ case 0:
+ i2c_transfer(i2c->adapter, &cmd_msg, 1);
+ wiichuck->state = 1;
+ break;
+
+ case 1:
+ i2c_transfer(i2c->adapter, &data_msg, 1);
+
+ jx = b[0];
+ jy = b[1];
+ ax = (b[2] << 2) & ((b[5] >> 2) & 0x3);
+ ay = (b[3] << 2) & ((b[5] >> 4) & 0x3);
+ az = (b[4] << 2) & ((b[5] >> 6) & 0x3);
+ z = !(b[5] & 1);
+ c = !(b[5] & 2);
+
+ input_report_abs(poll_dev->input, ABS_X, jx);
+ input_report_abs(poll_dev->input, ABS_Y, jy);
+ input_report_abs(poll_dev->input, ABS_RX, ax);
+ input_report_abs(poll_dev->input, ABS_RY, ax);
+ input_report_abs(poll_dev->input, ABS_RZ, ay);
+ input_report_key(poll_dev->input, BTN_C, c);
+ input_report_key(poll_dev->input, BTN_Z, z);
+ input_sync(poll_dev->input);
+
+ wiichuck->state = 0;
+ dev_dbg(&i2c->dev, "wiichuck: j=%.3i,%.3i a=%.3x,%.3x,%.3x %c%c\n",
+ jx,jy, ax,ay,az, c ? 'C' : 'c', z ? 'Z' : 'z');
+ break;
+
+ default:
+ wiichuck->state = 0;
+ }
+}
+
+static void wiichuck_open(struct input_polled_dev *poll_dev)
+{
+ struct wiichuck_device *wiichuck = poll_dev->private;
+ struct i2c_client *i2c = wiichuck->i2c_client;
+ static uint8_t data1[2] = { 0xf0, 0x55 };
+ static uint8_t data2[2] = { 0xfb, 0x00 };
+ struct i2c_msg msg1 = { .addr = i2c->addr, .len = 2, .buf = data1 };
+ struct i2c_msg msg2 = { .addr = i2c->addr, .len = 2, .buf = data2 };
+
+ i2c_transfer(i2c->adapter, &msg1, 1);
+ i2c_transfer(i2c->adapter, &msg2, 1);
+ wiichuck->state = 0;
+
+ dev_dbg(&i2c->dev, "wiichuck open()\n");
+}
+
+static int __devinit wiichuck_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct wiichuck_device *wiichuck;
+ struct input_polled_dev *poll_dev;
+ struct input_dev *input_dev;
+ int rc;
+
+ wiichuck = kzalloc(sizeof(*wiichuck), GFP_KERNEL);
+ if (!wiichuck)
+ return -ENOMEM;
+
+ poll_dev = input_allocate_polled_device();
+ if (!poll_dev) {
+ rc = -ENOMEM;
+ goto err_alloc;
+ }
+
+ wiichuck->i2c_client = client;
+ wiichuck->poll_dev = poll_dev;
+
+ poll_dev->private = wiichuck;
+ poll_dev->poll = wiichuck_poll;
+ poll_dev->poll_interval = 50; /* Poll every 50ms */
+ poll_dev->open = wiichuck_open;
+
+ input_dev = poll_dev->input;
+ input_dev->name = "Nintendo Nunchuck";
+ input_dev->id.bustype = BUS_I2C;
+ input_dev->dev.parent = &client->dev;
+
+ /* Declare the events generated by this driver */
+ set_bit(EV_ABS, input_dev->evbit);
+ set_bit(ABS_X, input_dev->absbit); /* joystick */
+ set_bit(ABS_Y, input_dev->absbit);
+ set_bit(ABS_RX, input_dev->absbit); /* accelerometer */
+ set_bit(ABS_RY, input_dev->absbit);
+ set_bit(ABS_RZ, input_dev->absbit);
+
+ set_bit(EV_KEY, input_dev->evbit);
+ set_bit(BTN_C, input_dev->keybit); /* buttons */
+ set_bit(BTN_Z, input_dev->keybit);
+
+ input_set_abs_params(input_dev, ABS_X, 30, 220, 4, 8);
+ input_set_abs_params(input_dev, ABS_Y, 40, 200, 4, 8);
+ input_set_abs_params(input_dev, ABS_RX, 0, 0x3ff, 4, 8);
+ input_set_abs_params(input_dev, ABS_RY, 0, 0x3ff, 4, 8);
+ input_set_abs_params(input_dev, ABS_RZ, 0, 0x3ff, 4, 8);
+
+ rc = input_register_polled_device(wiichuck->poll_dev);
+ if (rc) {
+ dev_err(&client->dev, "Failed to register input device\n");
+ goto err_register;
+ }
+
+ i2c_set_clientdata(client, wiichuck);
+
+ return 0;
+
+ err_register:
+ input_free_polled_device(poll_dev);
+ err_alloc:
+ kfree(wiichuck);
+
+ return rc;
+}
+
+static int __devexit wiichuck_remove(struct i2c_client *client)
+{
+ struct wiichuck_device *wiichuck = i2c_get_clientdata(client);
+
+ i2c_set_clientdata(client, NULL);
+ input_unregister_polled_device(wiichuck->poll_dev);
+ input_free_polled_device(wiichuck->poll_dev);
+ kfree(wiichuck);
+
+ return 0;
+}
+
+static const struct i2c_device_id wiichuck_id[] = {
+ { "nunchuck", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, wiichuck_id);
+
+#ifdef CONFIG_OF
+static struct of_device_id wiichuck_match_table[] __devinitdata = {
+ { .compatible = "nintendo,nunchuck", },
+ { }
+};
+#else
+#define wiichuck_match_table NULL
+#endif
+
+static struct i2c_driver wiichuck_driver = {
+ .driver = {
+ .name = "nunchuck",
+ .owner = THIS_MODULE,
+ .of_match_table = wiichuck_match_table,
+ },
+ .probe = wiichuck_probe,
+ .remove = __devexit_p(wiichuck_remove),
+ .id_table = wiichuck_id,
+};
+
+static int __init wiichuck_init(void)
+{
+ return i2c_add_driver(&wiichuck_driver);
+}
+module_init(wiichuck_init);
+
+static void __exit wiichuck_exit(void)
+{
+ i2c_del_driver(&wiichuck_driver);
+}
+module_exit(wiichuck_exit);
> This patch adds a driver for the Nintendo Nunchuck (wiimote accessory)
> attached to an i2c bus via something like a wiichuck adapter board
> from Sparkfun.
>
I have not tested your driver, will try to find some time next week.
There are also a handful of devices which can be connected as such:
http://www.wiibrew.org/wiki/Wiimote/Extension_Controllers
You might want to consider the possibility that the Nunchuck/Others are
connected via a MotionPlus, in which case the data is interleaved with
Gyro data.
Simon.
PS. If you're really cheap you can (ab)use the I2C DDC link of the VGA
port to connect devices.... it's what I do :-)
Am Donnerstag, 12. Mai 2011, 06:29:44 schrieb Grant Likely:
> +static void wiichuck_poll(struct input_polled_dev *poll_dev)
> +{
> + struct wiichuck_device *wiichuck = poll_dev->private;
> + struct i2c_client *i2c = wiichuck->i2c_client;
> + static uint8_t cmd_byte = 0;
> + struct i2c_msg cmd_msg =
> + { .addr = i2c->addr, .len = 1, .buf = &cmd_byte };
> + uint8_t b[6];
> + struct i2c_msg data_msg =
> + { .addr = i2c->addr, .flags = I2C_M_RD, .len = 6, .buf = b };
Do you need these buffers to be capable of DMA?
Regards
Oliver
On Sat, May 14, 2011 at 9:17 PM, Oliver Neukum <[email protected]> wrote:
> Am Donnerstag, 12. Mai 2011, 06:29:44 schrieb Grant Likely:
>> +static void wiichuck_poll(struct input_polled_dev *poll_dev)
>> +{
>> + ? ? ? struct wiichuck_device *wiichuck = poll_dev->private;
>> + ? ? ? struct i2c_client *i2c = wiichuck->i2c_client;
>> + ? ? ? static uint8_t cmd_byte = 0;
>> + ? ? ? struct i2c_msg cmd_msg =
>> + ? ? ? ? ? ? ? { .addr = i2c->addr, .len = 1, .buf = &cmd_byte };
>> + ? ? ? uint8_t b[6];
>> + ? ? ? struct i2c_msg data_msg =
>> + ? ? ? ? ? ? ? { .addr = i2c->addr, .flags = I2C_M_RD, .len = 6, .buf = b };
>
>
> Do you need these buffers to be capable of DMA?
Good point. Yes, these buffers should be DMA capable. I'll pull them
off the stack.
g.
On Thu, May 12, 2011 at 02:24:59AM -0400, [email protected] wrote:
> > This patch adds a driver for the Nintendo Nunchuck (wiimote accessory)
> > attached to an i2c bus via something like a wiichuck adapter board
> > from Sparkfun.
> >
>
> I have not tested your driver, will try to find some time next week.
>
> There are also a handful of devices which can be connected as such:
> http://www.wiibrew.org/wiki/Wiimote/Extension_Controllers
>
> You might want to consider the possibility that the Nunchuck/Others are
> connected via a MotionPlus, in which case the data is interleaved with
> Gyro data.
>
> Simon.
>
> PS. If you're really cheap you can (ab)use the I2C DDC link of the VGA
> port to connect devices.... it's what I do :-)
Heh. I've actually got it wired up to the i2c bus on a BeagleBoard from TI.
I've got a new version of the driver which I'll post when I get back
home and have a chance to properly test it. I've implemented Nunchuck
and Classic Controller support so I could play with implementing
multiple protocols in a clean way (even though I don't actually have a
classic controller yet). Motion Plus should be straight forward to
add, but I'll probably leave that as an exercise for somebody else.
g.
Hi Grant,
On Thu, May 12, 2011 at 06:29:44AM +0200, Grant Likely wrote:
> This patch adds a driver for the Nintendo Nunchuck (wiimote accessory)
> attached to an i2c bus via something like a wiichuck adapter board
> from Sparkfun.
>
> Signed-off-by: Grant Likely <[email protected]>
> ---
>
> This is RFC because the driver doesn't completely work yet. The
> joystick and buttons work fine. There is a bug with the accelerometer
> reporting though (nothing gets reported), and I'm not even sure I'm
> using the right input type for reporting accelerometer values.
Looks quite reasonable and I think that ABS_Rx is reasonable events for
accelerometer in this particular case.
A few more comments below.
> Comments welcome.
>
> drivers/input/joystick/Kconfig | 8 +
> drivers/input/joystick/Makefile | 1
> drivers/input/joystick/wiichuck.c | 225 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 234 insertions(+), 0 deletions(-)
> create mode 100644 drivers/input/joystick/wiichuck.c
>
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index 56eb471..79f18db 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -193,6 +193,14 @@ config JOYSTICK_TWIDJOY
> To compile this driver as a module, choose M here: the
> module will be called twidjoy.
>
> +config JOYSTICK_WIICHUCK
> + tristate "Nintendo Nunchuck on i2c bus"
> + depends on I2C
> + select INPUT_POLLDEV
> + help
> + Say Y here if you have a Nintendo Nunchuck directly attached to
> + the machine's i2c bus.
> +
To compile this driver as a module, ...
> config JOYSTICK_ZHENHUA
> tristate "5-byte Zhenhua RC transmitter"
> select SERIO
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 92dc0de..78466d6 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_JOYSTICK_TMDC) += tmdc.o
> obj-$(CONFIG_JOYSTICK_TURBOGRAFX) += turbografx.o
> obj-$(CONFIG_JOYSTICK_TWIDJOY) += twidjoy.o
> obj-$(CONFIG_JOYSTICK_WARRIOR) += warrior.o
> +obj-$(CONFIG_JOYSTICK_WIICHUCK) += wiichuck.o
> obj-$(CONFIG_JOYSTICK_XPAD) += xpad.o
> obj-$(CONFIG_JOYSTICK_ZHENHUA) += zhenhua.o
> obj-$(CONFIG_JOYSTICK_WALKERA0701) += walkera0701.o
> diff --git a/drivers/input/joystick/wiichuck.c b/drivers/input/joystick/wiichuck.c
> new file mode 100644
> index 0000000..2afc274
> --- /dev/null
> +++ b/drivers/input/joystick/wiichuck.c
> @@ -0,0 +1,225 @@
> +/*
> + * Nintendo Nunchuck driver for i2c connection.
> + *
> + * Copyright (c) 2011 Secret Lab Technologies Ltd.
> + *
> + * 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 of the License, 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; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/input-polldev.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/slab.h>
> +
> +MODULE_AUTHOR("Grant Likely <[email protected]>");
> +MODULE_DESCRIPTION("Nintendo Nunchuck driver");
> +MODULE_LICENSE("GPL");
> +
> +#define WIICHUCK_JOY_MAX_AXIS 220
> +#define WIICHUCK_JOY_MIN_AXIS 30
> +#define WIICHUCK_JOY_FUZZ 4
> +#define WIICHUCK_JOY_FLAT 8
> +
> +struct wiichuck_device {
> + struct input_polled_dev *poll_dev;
> + struct i2c_client *i2c_client;
> + int state;
> +};
> +
> +static void wiichuck_poll(struct input_polled_dev *poll_dev)
> +{
> + struct wiichuck_device *wiichuck = poll_dev->private;
> + struct i2c_client *i2c = wiichuck->i2c_client;
> + static uint8_t cmd_byte = 0;
> + struct i2c_msg cmd_msg =
> + { .addr = i2c->addr, .len = 1, .buf = &cmd_byte };
> + uint8_t b[6];
As mentioned by others these buffers should not be on stack.
> + struct i2c_msg data_msg =
> + { .addr = i2c->addr, .flags = I2C_M_RD, .len = 6, .buf = b };
> + int jx, jy, ax, ay, az;
> + bool c, z;
> +
> + switch (wiichuck->state) {
> + case 0:
> + i2c_transfer(i2c->adapter, &cmd_msg, 1);
> + wiichuck->state = 1;
Do you really need to have a state machine here? Why not do both
transfers in one poll invocation?
> + break;
> +
> + case 1:
> + i2c_transfer(i2c->adapter, &data_msg, 1);
> +
> + jx = b[0];
> + jy = b[1];
> + ax = (b[2] << 2) & ((b[5] >> 2) & 0x3);
> + ay = (b[3] << 2) & ((b[5] >> 4) & 0x3);
> + az = (b[4] << 2) & ((b[5] >> 6) & 0x3);
> + z = !(b[5] & 1);
> + c = !(b[5] & 2);
> +
> + input_report_abs(poll_dev->input, ABS_X, jx);
> + input_report_abs(poll_dev->input, ABS_Y, jy);
> + input_report_abs(poll_dev->input, ABS_RX, ax);
> + input_report_abs(poll_dev->input, ABS_RY, ax);
> + input_report_abs(poll_dev->input, ABS_RZ, ay);
> + input_report_key(poll_dev->input, BTN_C, c);
> + input_report_key(poll_dev->input, BTN_Z, z);
> + input_sync(poll_dev->input);
> +
> + wiichuck->state = 0;
> + dev_dbg(&i2c->dev, "wiichuck: j=%.3i,%.3i a=%.3x,%.3x,%.3x %c%c\n",
> + jx,jy, ax,ay,az, c ? 'C' : 'c', z ? 'Z' : 'z');
> + break;
> +
> + default:
> + wiichuck->state = 0;
> + }
> +}
> +
> +static void wiichuck_open(struct input_polled_dev *poll_dev)
> +{
> + struct wiichuck_device *wiichuck = poll_dev->private;
> + struct i2c_client *i2c = wiichuck->i2c_client;
> + static uint8_t data1[2] = { 0xf0, 0x55 };
> + static uint8_t data2[2] = { 0xfb, 0x00 };
> + struct i2c_msg msg1 = { .addr = i2c->addr, .len = 2, .buf = data1 };
> + struct i2c_msg msg2 = { .addr = i2c->addr, .len = 2, .buf = data2 };
> +
> + i2c_transfer(i2c->adapter, &msg1, 1);
> + i2c_transfer(i2c->adapter, &msg2, 1);
> + wiichuck->state = 0;
> +
> + dev_dbg(&i2c->dev, "wiichuck open()\n");
> +}
> +
> +static int __devinit wiichuck_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct wiichuck_device *wiichuck;
> + struct input_polled_dev *poll_dev;
> + struct input_dev *input_dev;
> + int rc;
> +
> + wiichuck = kzalloc(sizeof(*wiichuck), GFP_KERNEL);
> + if (!wiichuck)
> + return -ENOMEM;
> +
> + poll_dev = input_allocate_polled_device();
> + if (!poll_dev) {
> + rc = -ENOMEM;
> + goto err_alloc;
> + }
> +
> + wiichuck->i2c_client = client;
> + wiichuck->poll_dev = poll_dev;
> +
> + poll_dev->private = wiichuck;
> + poll_dev->poll = wiichuck_poll;
> + poll_dev->poll_interval = 50; /* Poll every 50ms */
> + poll_dev->open = wiichuck_open;
> +
> + input_dev = poll_dev->input;
> + input_dev->name = "Nintendo Nunchuck";
> + input_dev->id.bustype = BUS_I2C;
> + input_dev->dev.parent = &client->dev;
> +
> + /* Declare the events generated by this driver */
> + set_bit(EV_ABS, input_dev->evbit);
> + set_bit(ABS_X, input_dev->absbit); /* joystick */
> + set_bit(ABS_Y, input_dev->absbit);
> + set_bit(ABS_RX, input_dev->absbit); /* accelerometer */
> + set_bit(ABS_RY, input_dev->absbit);
> + set_bit(ABS_RZ, input_dev->absbit);
> +
> + set_bit(EV_KEY, input_dev->evbit);
> + set_bit(BTN_C, input_dev->keybit); /* buttons */
> + set_bit(BTN_Z, input_dev->keybit);
I prefer __set_bit() here since there is no concurrency.
> +
> + input_set_abs_params(input_dev, ABS_X, 30, 220, 4, 8);
> + input_set_abs_params(input_dev, ABS_Y, 40, 200, 4, 8);
> + input_set_abs_params(input_dev, ABS_RX, 0, 0x3ff, 4, 8);
> + input_set_abs_params(input_dev, ABS_RY, 0, 0x3ff, 4, 8);
> + input_set_abs_params(input_dev, ABS_RZ, 0, 0x3ff, 4, 8);
> +
> + rc = input_register_polled_device(wiichuck->poll_dev);
> + if (rc) {
> + dev_err(&client->dev, "Failed to register input device\n");
> + goto err_register;
> + }
> +
> + i2c_set_clientdata(client, wiichuck);
> +
> + return 0;
> +
> + err_register:
> + input_free_polled_device(poll_dev);
> + err_alloc:
> + kfree(wiichuck);
> +
> + return rc;
> +}
> +
> +static int __devexit wiichuck_remove(struct i2c_client *client)
> +{
> + struct wiichuck_device *wiichuck = i2c_get_clientdata(client);
> +
> + i2c_set_clientdata(client, NULL);
> + input_unregister_polled_device(wiichuck->poll_dev);
> + input_free_polled_device(wiichuck->poll_dev);
> + kfree(wiichuck);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id wiichuck_id[] = {
> + { "nunchuck", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, wiichuck_id);
> +
> +#ifdef CONFIG_OF
> +static struct of_device_id wiichuck_match_table[] __devinitdata = {
> + { .compatible = "nintendo,nunchuck", },
> + { }
> +};
> +#else
> +#define wiichuck_match_table NULL
> +#endif
> +
> +static struct i2c_driver wiichuck_driver = {
> + .driver = {
> + .name = "nunchuck",
> + .owner = THIS_MODULE,
> + .of_match_table = wiichuck_match_table,
> + },
> + .probe = wiichuck_probe,
> + .remove = __devexit_p(wiichuck_remove),
> + .id_table = wiichuck_id,
> +};
> +
> +static int __init wiichuck_init(void)
> +{
> + return i2c_add_driver(&wiichuck_driver);
> +}
> +module_init(wiichuck_init);
> +
> +static void __exit wiichuck_exit(void)
> +{
> + i2c_del_driver(&wiichuck_driver);
> +}
> +module_exit(wiichuck_exit);
>
Thanks.
--
Dmitry
On Mon, May 16, 2011 at 09:44:27AM -0700, Dmitry Torokhov wrote:
> Hi Grant,
>
> On Thu, May 12, 2011 at 06:29:44AM +0200, Grant Likely wrote:
> > This patch adds a driver for the Nintendo Nunchuck (wiimote accessory)
> > attached to an i2c bus via something like a wiichuck adapter board
> > from Sparkfun.
> >
> > Signed-off-by: Grant Likely <[email protected]>
> > ---
> >
> > This is RFC because the driver doesn't completely work yet. The
> > joystick and buttons work fine. There is a bug with the accelerometer
> > reporting though (nothing gets reported), and I'm not even sure I'm
> > using the right input type for reporting accelerometer values.
>
> Looks quite reasonable and I think that ABS_Rx is reasonable events for
> accelerometer in this particular case.
>
> A few more comments below.
Hi Dmitry, thanks for the review. Replies below.
>
> > Comments welcome.
> >
> > drivers/input/joystick/Kconfig | 8 +
> > drivers/input/joystick/Makefile | 1
> > drivers/input/joystick/wiichuck.c | 225 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 234 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/input/joystick/wiichuck.c
> >
> > diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> > index 56eb471..79f18db 100644
> > --- a/drivers/input/joystick/Kconfig
> > +++ b/drivers/input/joystick/Kconfig
> > @@ -193,6 +193,14 @@ config JOYSTICK_TWIDJOY
> > To compile this driver as a module, choose M here: the
> > module will be called twidjoy.
> >
> > +config JOYSTICK_WIICHUCK
> > + tristate "Nintendo Nunchuck on i2c bus"
> > + depends on I2C
> > + select INPUT_POLLDEV
> > + help
> > + Say Y here if you have a Nintendo Nunchuck directly attached to
> > + the machine's i2c bus.
> > +
>
> To compile this driver as a module, ...
That ends up being pretty useless boilerplate. I've stopped adding
that line to any of the Kconfig text I maintain.
> > +static void wiichuck_poll(struct input_polled_dev *poll_dev)
> > +{
> > + struct wiichuck_device *wiichuck = poll_dev->private;
> > + struct i2c_client *i2c = wiichuck->i2c_client;
> > + static uint8_t cmd_byte = 0;
> > + struct i2c_msg cmd_msg =
> > + { .addr = i2c->addr, .len = 1, .buf = &cmd_byte };
> > + uint8_t b[6];
>
> As mentioned by others these buffers should not be on stack.
Fixed.
>
> > + struct i2c_msg data_msg =
> > + { .addr = i2c->addr, .flags = I2C_M_RD, .len = 6, .buf = b };
> > + int jx, jy, ax, ay, az;
> > + bool c, z;
> > +
> > + switch (wiichuck->state) {
> > + case 0:
> > + i2c_transfer(i2c->adapter, &cmd_msg, 1);
> > + wiichuck->state = 1;
>
> Do you really need to have a state machine here? Why not do both
> transfers in one poll invocation?
Mostly because there needs to a gap between setting up the data
capture and reading the data back. I could flip things around to
setup the next transfer after reading the previous, but in my current
version I also add state for handling hotplug of the wiimote
accessories, so I think the state machine is still warranted.
> > + set_bit(EV_KEY, input_dev->evbit);
> > + set_bit(BTN_C, input_dev->keybit); /* buttons */
> > + set_bit(BTN_Z, input_dev->keybit);
>
> I prefer __set_bit() here since there is no concurrency.
In general, I avoid using __* versions of functions unless it is
actually important that I do so. I'm not concerned about slightly
higher overhead here, and I'd rather my code set a good example.
g.
On Mon, May 16, 2011 at 12:31:24PM -0600, Grant Likely wrote:
> On Mon, May 16, 2011 at 09:44:27AM -0700, Dmitry Torokhov wrote:
> > Hi Grant,
> >
> > On Thu, May 12, 2011 at 06:29:44AM +0200, Grant Likely wrote:
> > > This patch adds a driver for the Nintendo Nunchuck (wiimote accessory)
> > > attached to an i2c bus via something like a wiichuck adapter board
> > > from Sparkfun.
> > >
> > > Signed-off-by: Grant Likely <[email protected]>
> > > ---
> > >
> > > This is RFC because the driver doesn't completely work yet. The
> > > joystick and buttons work fine. There is a bug with the accelerometer
> > > reporting though (nothing gets reported), and I'm not even sure I'm
> > > using the right input type for reporting accelerometer values.
> >
> > Looks quite reasonable and I think that ABS_Rx is reasonable events for
> > accelerometer in this particular case.
> >
> > A few more comments below.
>
> Hi Dmitry, thanks for the review. Replies below.
>
> >
> > > Comments welcome.
> > >
> > > drivers/input/joystick/Kconfig | 8 +
> > > drivers/input/joystick/Makefile | 1
> > > drivers/input/joystick/wiichuck.c | 225 +++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 234 insertions(+), 0 deletions(-)
> > > create mode 100644 drivers/input/joystick/wiichuck.c
> > >
> > > diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> > > index 56eb471..79f18db 100644
> > > --- a/drivers/input/joystick/Kconfig
> > > +++ b/drivers/input/joystick/Kconfig
> > > @@ -193,6 +193,14 @@ config JOYSTICK_TWIDJOY
> > > To compile this driver as a module, choose M here: the
> > > module will be called twidjoy.
> > >
> > > +config JOYSTICK_WIICHUCK
> > > + tristate "Nintendo Nunchuck on i2c bus"
> > > + depends on I2C
> > > + select INPUT_POLLDEV
> > > + help
> > > + Say Y here if you have a Nintendo Nunchuck directly attached to
> > > + the machine's i2c bus.
> > > +
> >
> > To compile this driver as a module, ...
>
> That ends up being pretty useless boilerplate. I've stopped adding
> that line to any of the Kconfig text I maintain.
>
This tells the user name of the module, that is the reason I have all
input drivers use this boilerplate.
> > > +static void wiichuck_poll(struct input_polled_dev *poll_dev)
> > > +{
> > > + struct wiichuck_device *wiichuck = poll_dev->private;
> > > + struct i2c_client *i2c = wiichuck->i2c_client;
> > > + static uint8_t cmd_byte = 0;
> > > + struct i2c_msg cmd_msg =
> > > + { .addr = i2c->addr, .len = 1, .buf = &cmd_byte };
> > > + uint8_t b[6];
> >
> > As mentioned by others these buffers should not be on stack.
>
> Fixed.
>
> >
> > > + struct i2c_msg data_msg =
> > > + { .addr = i2c->addr, .flags = I2C_M_RD, .len = 6, .buf = b };
> > > + int jx, jy, ax, ay, az;
> > > + bool c, z;
> > > +
> > > + switch (wiichuck->state) {
> > > + case 0:
> > > + i2c_transfer(i2c->adapter, &cmd_msg, 1);
> > > + wiichuck->state = 1;
> >
> > Do you really need to have a state machine here? Why not do both
> > transfers in one poll invocation?
>
> Mostly because there needs to a gap between setting up the data
> capture and reading the data back.
How long is the gap? You should be able simply sleep in the poll().
> I could flip things around to
> setup the next transfer after reading the previous, but in my current
> version I also add state for handling hotplug of the wiimote
> accessories, so I think the state machine is still warranted.
>
> > > + set_bit(EV_KEY, input_dev->evbit);
> > > + set_bit(BTN_C, input_dev->keybit); /* buttons */
> > > + set_bit(BTN_Z, input_dev->keybit);
> >
> > I prefer __set_bit() here since there is no concurrency.
>
> In general, I avoid using __* versions of functions unless it is
> actually important that I do so. I'm not concerned about slightly
> higher overhead here, and I'd rather my code set a good example.
In case of xxx_bit() __xxx_bit() is not an "internal, call only if you
are really sure" version but regular, non-atomic one. Unfortunately the
naming chosen used double underscores for them.
I prefer drivers using set_bit() and clear_bit() only if atomicity is
important, in all other cases underscored versions are more appropriate.
Thanks.
--
Dmitry
On Mon, May 16, 2011 at 1:44 PM, Dmitry Torokhov
<[email protected]> wrote:
> On Mon, May 16, 2011 at 12:31:24PM -0600, Grant Likely wrote:
>> > > + struct i2c_msg data_msg =
>> > > + ? ? ? ? { .addr = i2c->addr, .flags = I2C_M_RD, .len = 6, .buf = b };
>> > > + int jx, jy, ax, ay, az;
>> > > + bool c, z;
>> > > +
>> > > + switch (wiichuck->state) {
>> > > + case 0:
>> > > + ? ? ? ? i2c_transfer(i2c->adapter, &cmd_msg, 1);
>> > > + ? ? ? ? wiichuck->state = 1;
>> >
>> > Do you really need to have a state machine here? Why not do both
>> > transfers in one poll invocation?
>>
>> Mostly because there needs to a gap between setting up the data
>> capture and reading the data back.
>
> How long is the gap? You should be able simply sleep in the poll().
50ms (from what information I've been able to gleen; it might be
faster). Since I'm already polling the controller at 100ms, a 50ms
sleep in the poll routine would pretty much completely consume the
workqueue if 2 or more wii extension devices were attached to the
system. Voluntarily giving it up is the right thing to do.
That said, the problem is moot since I've modified the read state to
setup the next transfer immediately after reading the data. :-)
Revised patch posted shortly...
g.