All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] input: Add wiichuck driver
@ 2011-05-12  4:29 Grant Likely
  2011-05-12  6:24 ` simon
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Grant Likely @ 2011-05-12  4:29 UTC (permalink / raw)
  To: Dmitry Torokhov, linux-kernel, linux-input

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 <grant.likely@secretlab.ca>
---

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 <grant.likely@secretlab.ca>");
+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);


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

* Re: [RFC PATCH] input: Add wiichuck driver
  2011-05-12  4:29 [RFC PATCH] input: Add wiichuck driver Grant Likely
@ 2011-05-12  6:24 ` simon
  2011-05-15 21:17   ` Grant Likely
  2011-05-14 19:17 ` Oliver Neukum
  2011-05-16 16:44 ` Dmitry Torokhov
  2 siblings, 1 reply; 11+ messages in thread
From: simon @ 2011-05-12  6:24 UTC (permalink / raw)
  To: Grant Likely; +Cc: Dmitry Torokhov, linux-kernel, linux-input

> 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 :-)



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

* Re: [RFC PATCH] input: Add wiichuck driver
  2011-05-12  4:29 [RFC PATCH] input: Add wiichuck driver Grant Likely
  2011-05-12  6:24 ` simon
@ 2011-05-14 19:17 ` Oliver Neukum
  2011-05-14 19:20     ` Grant Likely
  2011-05-16 16:44 ` Dmitry Torokhov
  2 siblings, 1 reply; 11+ messages in thread
From: Oliver Neukum @ 2011-05-14 19:17 UTC (permalink / raw)
  To: Grant Likely; +Cc: Dmitry Torokhov, linux-kernel, linux-input

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

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

* Re: [RFC PATCH] input: Add wiichuck driver
  2011-05-14 19:17 ` Oliver Neukum
@ 2011-05-14 19:20     ` Grant Likely
  0 siblings, 0 replies; 11+ messages in thread
From: Grant Likely @ 2011-05-14 19:20 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: Dmitry Torokhov, linux-kernel, linux-input

On Sat, May 14, 2011 at 9:17 PM, Oliver Neukum <oliver@neukum.org> 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.

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

* Re: [RFC PATCH] input: Add wiichuck driver
@ 2011-05-14 19:20     ` Grant Likely
  0 siblings, 0 replies; 11+ messages in thread
From: Grant Likely @ 2011-05-14 19:20 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: Dmitry Torokhov, linux-kernel, linux-input

On Sat, May 14, 2011 at 9:17 PM, Oliver Neukum <oliver@neukum.org> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [RFC PATCH] input: Add wiichuck driver
  2011-05-12  6:24 ` simon
@ 2011-05-15 21:17   ` Grant Likely
  0 siblings, 0 replies; 11+ messages in thread
From: Grant Likely @ 2011-05-15 21:17 UTC (permalink / raw)
  To: simon; +Cc: Dmitry Torokhov, linux-kernel, linux-input

On Thu, May 12, 2011 at 02:24:59AM -0400, simon@mungewell.org 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.


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

* Re: [RFC PATCH] input: Add wiichuck driver
  2011-05-12  4:29 [RFC PATCH] input: Add wiichuck driver Grant Likely
  2011-05-12  6:24 ` simon
  2011-05-14 19:17 ` Oliver Neukum
@ 2011-05-16 16:44 ` Dmitry Torokhov
  2011-05-16 18:31   ` Grant Likely
  2 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2011-05-16 16:44 UTC (permalink / raw)
  To: Grant Likely; +Cc: linux-kernel, linux-input

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 <grant.likely@secretlab.ca>
> ---
> 
> 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 <grant.likely@secretlab.ca>");
> +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

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

* Re: [RFC PATCH] input: Add wiichuck driver
  2011-05-16 16:44 ` Dmitry Torokhov
@ 2011-05-16 18:31   ` Grant Likely
  2011-05-16 19:44     ` Dmitry Torokhov
  0 siblings, 1 reply; 11+ messages in thread
From: Grant Likely @ 2011-05-16 18:31 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-kernel, linux-input

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 <grant.likely@secretlab.ca>
> > ---
> > 
> > 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.

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

* Re: [RFC PATCH] input: Add wiichuck driver
  2011-05-16 18:31   ` Grant Likely
@ 2011-05-16 19:44     ` Dmitry Torokhov
  2011-05-16 21:01         ` Grant Likely
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2011-05-16 19:44 UTC (permalink / raw)
  To: Grant Likely; +Cc: linux-kernel, linux-input

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 <grant.likely@secretlab.ca>
> > > ---
> > > 
> > > 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

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

* Re: [RFC PATCH] input: Add wiichuck driver
  2011-05-16 19:44     ` Dmitry Torokhov
@ 2011-05-16 21:01         ` Grant Likely
  0 siblings, 0 replies; 11+ messages in thread
From: Grant Likely @ 2011-05-16 21:01 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-kernel, linux-input

On Mon, May 16, 2011 at 1:44 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> 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.

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

* Re: [RFC PATCH] input: Add wiichuck driver
@ 2011-05-16 21:01         ` Grant Likely
  0 siblings, 0 replies; 11+ messages in thread
From: Grant Likely @ 2011-05-16 21:01 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-kernel, linux-input

On Mon, May 16, 2011 at 1:44 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

end of thread, other threads:[~2011-05-16 21:02 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-12  4:29 [RFC PATCH] input: Add wiichuck driver Grant Likely
2011-05-12  6:24 ` simon
2011-05-15 21:17   ` Grant Likely
2011-05-14 19:17 ` Oliver Neukum
2011-05-14 19:20   ` Grant Likely
2011-05-14 19:20     ` Grant Likely
2011-05-16 16:44 ` Dmitry Torokhov
2011-05-16 18:31   ` Grant Likely
2011-05-16 19:44     ` Dmitry Torokhov
2011-05-16 21:01       ` Grant Likely
2011-05-16 21:01         ` Grant Likely

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.