Re: [RFC PATCH] input: Add wiichuck driver

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:

quoted

Hi Grant,

On Thu, May 12, 2011 at 06:29:44AM +0200, Grant Likely wrote:

quoted

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 <redacted>
---

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.

quoted

quoted

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.

quoted

quoted

+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.

quoted

quoted

+	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.

quoted

quoted

+	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