lkml.org 
[lkml]   [2011]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH] input: Add wiichuck driver
    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.


    \
     
     \ /
      Last update: 2011-05-16 20:33    [W:0.024 / U:60.292 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site