Hi Rick, On Thu, Jan 07, 2010 at 09:23:24AM -0700, Rick L. Vinyard Jr. wrote: > > This is a driver for the Logitech G13 gamepad, and contains three key > parts. Through the USB reports the device identifies itself as a HID, and as > a result this driver is under the HID framework. > > There are two primary sub-components to this driver; an input device and a > framebuffer device. > > Although identified as a HID, the device does not support standard HID > input messages. As a result, a sub-input device is allocated and > registered separately in g13_probe(). The raw events are monitored and > key presses/joystick activity is reported through the input device after > referencing an indexed keymap. Finally had some time to look through the patch, comments below. > > Additionally, this device contains a 160x43 monochrome LCD display. A > registered framebuffer device manages this display. The design of this > portion of the driver was based on the design of the hecubafb driver with > deferred framebuffer I/O since there is no real memory to map. > > Changes since 0.0.2: > - Moved the module out of the logitech objs to build on its own > > - Added dependency on FB_DEFFERRED_IO > > - Added explanation as to why the load image is used and how to view it > outside the code. > > - Changed the load image to text "G13" with default penguins resized, > cleaned up and inverted from framebuffer monochrome penguin logo. > > - Cleaned up the raw event callback by moving the key event processing to a > separate function. > > - Removed several of the line breaks introduced to accomodate 80-column lines > to improve readability. > > - Reordeded event processing and utility functions to eliminate the need for > a g13_set_mled() prototype. > > - Removed nonstd flag from framebuffer screeninfo structure > > Signed-off-by: Rick L. Vinyard, Jr <rvinyard@xxxxxxxxxxx> > --- > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 24d90ea..548dd4a 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -183,6 +183,20 @@ config LOGIRUMBLEPAD2_FF > Say Y here if you want to enable force feedback support for Logitech > Rumblepad 2 devices. > > +config HID_LOGITECH_G13 > + tristate "Logitech G13 gameboard support" > + depends on FB > + depends on FB_DEFERRED_IO > + select FB_SYS_FILLRECT > + select FB_SYS_COPYAREA > + select FB_SYS_IMAGEBLIT > + select FB_SYS_FOPS > + help > + This provides support for Logitech G13 gameboard > + devices. This includes support for the device > + as a keypad input with mappable keys as well as > + a framebuffer for the LCD display. > + > config HID_MICROSOFT > tristate "Microsoft" if EMBEDDED > depends on USB_HID > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile > index 0de2dff..d39dc5b 100644 > --- a/drivers/hid/Makefile > +++ b/drivers/hid/Makefile > @@ -31,6 +31,7 @@ obj-$(CONFIG_HID_GYRATION) += hid-gyration.o > obj-$(CONFIG_HID_KENSINGTON) += hid-kensington.o > obj-$(CONFIG_HID_KYE) += hid-kye.o > obj-$(CONFIG_HID_LOGITECH) += hid-logitech.o > +obj-$(CONFIG_HID_LOGITECH_G13) += hid-g13.o > obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o > obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o > obj-$(CONFIG_HID_NTRIG) += hid-ntrig.o > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 80792d3..eeae383 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -1325,6 +1325,7 @@ static const struct hid_device_id hid_blacklist[] = { > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOMO_WHEEL2) }, > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G25_WHEEL) }, > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2) }, > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G13) }, > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACETRAVELLER) }, > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACENAVIGATOR) }, > { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV) }, > diff --git a/drivers/hid/hid-g13.c b/drivers/hid/hid-g13.c > new file mode 100644 > index 0000000..63c3044 > --- /dev/null > +++ b/drivers/hid/hid-g13.c > @@ -0,0 +1,1598 @@ > +/*************************************************************************** > + * Copyright (C) 2009 by Rick L. Vinyard, Jr. * > + * rvinyard@xxxxxxxxxxx * > + * * > + * 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 driver 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 software. If not see <http://www.gnu.org/licenses/>. * > + ***************************************************************************/ > +#include <linux/fb.h> > +#include <linux/hid.h> > +#include <linux/init.h> > +#include <linux/input.h> > +#include <linux/mm.h> > +#include <linux/sysfs.h> > +#include <linux/uaccess.h> > +#include <linux/usb.h> > +#include <linux/vmalloc.h> > + > +#include "hid-ids.h" > +#include "usbhid/usbhid.h" > + > +#define G13_NAME "Logitech G13" > + > +/* Key defines */ > +#define G13_KEYS 35 > +#define G13_KEYMAP_SIZE (G13_KEYS*3) > + > +/* G1-G22 indices */ > +#define G13_G1 0 > +#define G13_G2 1 > +#define G13_G3 2 > +#define G13_G4 3 > +#define G13_G5 4 > +#define G13_G6 5 > +#define G13_G7 6 > +#define G13_G8 7 > +#define G13_G9 8 > +#define G13_G10 9 > +#define G13_G11 10 > +#define G13_G12 11 > +#define G13_G13 12 > +#define G13_G14 13 > +#define G13_G15 14 > +#define G13_G16 15 > +#define G13_G17 16 > +#define G13_G18 17 > +#define G13_G19 18 > +#define G13_G20 19 > +#define G13_G21 20 > +#define G13_G22 21 > +#define G13_FUNC 22 > +#define G13_LCD1 23 > +#define G13_LCD2 24 > +#define G13_LCD3 25 > +#define G13_LCD4 26 > +#define G13_M1 27 > +#define G13_M2 28 > +#define G13_M3 29 > +#define G13_MR 30 > +#define G13_BTN_LEFT 31 > +#define G13_BTN_DOWN 32 > +#define G13_BTN_STICK 33 > +#define G13_LIGHT 34 > + > +/* Framebuffer defines */ > +#define G13FB_NAME "g13fb" > +#define G13FB_WIDTH (160) > +#define G13FB_LINE_LENGTH (160/8) > +#define G13FB_HEIGHT (43) > +#define G13FB_SIZE (G13FB_LINE_LENGTH*G13FB_HEIGHT) > + > +#define G13FB_UPDATE_RATE_LIMIT (20) > +#define G13FB_UPDATE_RATE_DEFAULT (10) > + > +/* > + * The native G13 format uses vertical bits. Therefore the number of bytes > + * needed to represent the first column is 43/8 (rows/bits) rounded up. > + * Additionally, the format requires a padding of 32 bits in front of the > + * image data. > + * > + * Therefore the vbitmap size must be: > + * 160 * ceil(43/8) + 32 = 160 * 6 + 32 = 992 > + */ > +#define G13_VBITMAP_SIZE (992) > + > +/* Backlight defaults */ > +#define G13_DEFAULT_RED (0) > +#define G13_DEFAULT_GREEN (255) > +#define G13_DEFAULT_BLUE (0) > + > +/* Per device data structure */ > +struct g13_data { > + /* HID reports */ > + struct hid_device *hdev; > + struct hid_report *backlight_report; > + struct hid_report *start_input_report; > + struct hid_report *report_4; > + struct hid_report *mled_report; > + struct input_dev *input_dev; > + > + char *name; > + int ready; > + int ready2; > + u32 keycode[G13_KEYMAP_SIZE]; > + u8 rgb[3]; > + u8 mled; > + u8 curkeymap; > + u8 emit_msc_raw; > + u8 keymap_switching; > + > + /* Framebuffer stuff */ > + u8 fb_update_rate; > + u8 *fb_vbitmap; > + u8 *fb_bitmap; > + struct fb_info *fb_info; > + struct fb_deferred_io fb_defio; > + > + /* Housekeeping stuff */ > + rwlock_t lock; > +}; > + > +/* Convenience macros */ > +#define hid_get_g13data(hdev) \ > + ((hdev == NULL) ? NULL : (struct g13_data *)(hid_get_drvdata(hdev))) > + > +#define input_get_hdev(idev) \ > + ((idev == NULL) ? NULL : (struct hid_device *)(input_get_drvdata(idev))) > + > +#define input_get_g13data(idev) (hid_get_g13data(input_get_hdev(idev))) I wonder whether these NULL tests are helpful or if they will simply mask potential bugs in the driver. For example - when (in this particular driver) input_get_drvdata would return NULL? You set it before registering the device so any input device method will have it set. I'd remove them. > + > +static unsigned int g13_default_key_map[G13_KEYS] = { const. > + /* first row g1 - g7 */ > + KEY_F1, KEY_F2, KEY_F3, KEY_F4, KEY_F5, KEY_F6, KEY_F7, > + /* second row g8 - g11 */ > + KEY_UNKNOWN, KEY_UNKNOWN, KEY_BACK, KEY_UP, > + /* second row g12 - g13 */ > + KEY_FORWARD, KEY_UNKNOWN, KEY_UNKNOWN, > + /* third row g15 - g19 */ > + KEY_UNKNOWN, KEY_LEFT, KEY_DOWN, KEY_RIGHT, KEY_UNKNOWN, > + /* fourth row g20 - g22 */ > + KEY_BACKSPACE, KEY_ENTER, KEY_SPACE, > + /* next, light left, light center left, light center right, light right */ > + BTN_0, BTN_1, BTN_2, BTN_3, BTN_4, > + /* M1, M2, M3, MR */ > + KEY_RESERVED, KEY_RESERVED, KEY_RESERVED, KEY_RESERVED, > + /* button left, button down, button stick, light */ > + BTN_LEFT, BTN_RIGHT, BTN_MIDDLE, KEY_RESERVED, > +}; > + > +/* Framebuffer visual structures */ > +static struct fb_fix_screeninfo g13fb_fix = { > + .id = G13FB_NAME, > + .type = FB_TYPE_PACKED_PIXELS, > + .visual = FB_VISUAL_MONO01, > + .xpanstep = 0, > + .ypanstep = 0, > + .ywrapstep = 0, > + .line_length = G13FB_LINE_LENGTH, > + .accel = FB_ACCEL_NONE, > +}; > + > +static struct fb_var_screeninfo g13fb_var = { > + .xres = G13FB_WIDTH, > + .yres = G13FB_HEIGHT, > + .xres_virtual = G13FB_WIDTH, > + .yres_virtual = G13FB_HEIGHT, > + .bits_per_pixel = 1, > +}; > + > +/* > + * This is a default logo to be loaded upon driver initialization > + * replacing the default Logitech G13 image loaded on device > + * initialization. This is to provide the user a cue that the > + * Linux driver is loaded and ready. > + * > + * This logo contains the text G13 in the center with two penguins > + * on each side of the text. The penguins are a 40x40 rendition of > + * the default framebuffer 80x80 monochrome image scaled down and > + * cleaned up to retain something that looks a little better than > + * a simple scaling. > + * > + * This logo is a simple xbm image created in GIMP and exported. > + * To view the image copy the following two #defines plus the > + * g13_lcd_bits to an ASCII text file and save with extension > + * .xbm, then open with GIMP or any other graphical editor > + * such as eog that recognizes the .xbm format. > + */ > +#define g13_lcd_width 160 > +#define g13_lcd_height 43 > +static unsigned char g13_lcd_bits[] = { > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0f, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x3c, 0x00, 0x00, 0x00, 0xc0, 0x70, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xc3, 0x01, 0x00, > + 0x00, 0x20, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x80, 0x00, 0x02, 0x00, 0x00, 0x20, 0x10, 0x01, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x80, 0x40, 0x04, 0x00, 0x00, 0x10, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x40, 0x00, 0x04, 0x00, > + 0x00, 0x10, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x40, 0x00, 0x08, 0x00, 0x00, 0xd0, 0x18, 0x02, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x40, 0x63, 0x08, 0x00, 0x00, 0xd0, 0x1c, 0x02, 0x00, 0xf0, 0xff, 0xff, > + 0x83, 0xff, 0x01, 0xf8, 0xff, 0xff, 0x03, 0x00, 0x40, 0x73, 0x08, 0x00, > + 0x00, 0x10, 0x25, 0x02, 0x00, 0xf8, 0xff, 0xff, 0x83, 0xff, 0x01, 0xf8, > + 0xff, 0xff, 0x07, 0x00, 0x40, 0x94, 0x08, 0x00, 0x00, 0x10, 0x20, 0x02, > + 0x00, 0xfc, 0xff, 0xff, 0x83, 0xff, 0x01, 0xf8, 0xff, 0xff, 0x0f, 0x00, > + 0x40, 0x80, 0x08, 0x00, 0x00, 0xd0, 0x1e, 0x02, 0x00, 0xfe, 0xff, 0xff, > + 0x83, 0xff, 0x01, 0xf8, 0xff, 0xff, 0x1f, 0x00, 0x40, 0x7b, 0x08, 0x00, > + 0x00, 0xd0, 0x27, 0x02, 0x00, 0xfe, 0xff, 0xff, 0x83, 0xff, 0x01, 0xf8, > + 0xff, 0xff, 0x1f, 0x00, 0x40, 0x9f, 0x08, 0x00, 0x00, 0x90, 0x1b, 0x02, > + 0x00, 0x3e, 0x00, 0x00, 0x00, 0xf0, 0x01, 0x00, 0x00, 0x00, 0x1f, 0x00, > + 0x40, 0x6e, 0x08, 0x00, 0x00, 0x10, 0x24, 0x02, 0x00, 0x3e, 0x00, 0x00, > + 0x00, 0xf0, 0x01, 0x00, 0x00, 0x00, 0x1f, 0x00, 0x40, 0x90, 0x08, 0x00, > + 0x00, 0x48, 0x3b, 0x05, 0x00, 0x3e, 0x00, 0x00, 0x00, 0xf0, 0x01, 0x00, > + 0x00, 0x00, 0x1f, 0x00, 0x20, 0xed, 0x14, 0x00, 0x00, 0xc8, 0x7c, 0x08, > + 0x00, 0x3e, 0x00, 0x00, 0x00, 0xf0, 0x01, 0x00, 0x00, 0x00, 0x1f, 0x00, > + 0x20, 0xf3, 0x21, 0x00, 0x00, 0xe4, 0x7f, 0x10, 0x00, 0x3e, 0x00, 0x00, > + 0x00, 0xf0, 0x01, 0x00, 0x00, 0x00, 0x1f, 0x00, 0x90, 0xff, 0x41, 0x00, > + 0x00, 0xe2, 0xff, 0x20, 0x00, 0x3e, 0x00, 0x00, 0x00, 0xf0, 0x01, 0x00, > + 0x00, 0x00, 0x1f, 0x00, 0x88, 0xff, 0x83, 0x00, 0x00, 0xc2, 0x9c, 0x40, > + 0x00, 0x3e, 0xf8, 0xff, 0x07, 0xf0, 0x01, 0xf8, 0xff, 0xff, 0x1f, 0x00, > + 0x08, 0x73, 0x02, 0x01, 0x00, 0xf1, 0x3e, 0x41, 0x00, 0x3e, 0xf8, 0xff, > + 0x07, 0xf0, 0x01, 0xf8, 0xff, 0xff, 0x0f, 0x00, 0xc4, 0xfb, 0x04, 0x01, > + 0x00, 0xf1, 0x7f, 0x40, 0x00, 0x3e, 0xf8, 0xff, 0x07, 0xf0, 0x01, 0xf8, > + 0xff, 0xff, 0x07, 0x00, 0xc4, 0xff, 0x01, 0x01, 0x80, 0xf8, 0xff, 0x89, > + 0x00, 0x3e, 0xf8, 0xff, 0x07, 0xf0, 0x01, 0xf8, 0xff, 0xff, 0x0f, 0x00, > + 0xe2, 0xff, 0x27, 0x02, 0x80, 0xf8, 0xff, 0x93, 0x00, 0x3e, 0xf8, 0xff, > + 0x07, 0xf0, 0x01, 0xf8, 0xff, 0xff, 0x0f, 0x00, 0xe2, 0xff, 0x4f, 0x02, > + 0x40, 0xfc, 0xff, 0x93, 0x00, 0x3e, 0x00, 0xc0, 0x07, 0xf0, 0x01, 0x00, > + 0x00, 0x00, 0x1f, 0x00, 0xf1, 0xff, 0x4f, 0x02, 0x40, 0xfc, 0xff, 0x13, > + 0x01, 0x3e, 0x00, 0xc0, 0x07, 0xf0, 0x01, 0x00, 0x00, 0x00, 0x1f, 0x00, > + 0xf1, 0xff, 0x4f, 0x04, 0x20, 0x7c, 0xff, 0x13, 0x01, 0x3e, 0x00, 0xc0, > + 0x07, 0xf0, 0x01, 0x00, 0x00, 0x00, 0x1f, 0x80, 0xf0, 0xfd, 0x4f, 0x04, > + 0x20, 0x7c, 0xff, 0x13, 0x01, 0x3e, 0x00, 0xc0, 0x07, 0xf0, 0x01, 0x00, > + 0x00, 0x00, 0x1f, 0x80, 0xf0, 0xfd, 0x4f, 0x04, 0x20, 0x7c, 0xff, 0x3b, > + 0x01, 0x3e, 0x00, 0xc0, 0x07, 0xf0, 0x01, 0x00, 0x00, 0x00, 0x1f, 0x80, > + 0xf0, 0xfd, 0xef, 0x04, 0xa0, 0xfc, 0xff, 0x00, 0x01, 0x3e, 0x00, 0xc0, > + 0x07, 0xf0, 0x01, 0x00, 0x00, 0x00, 0x1f, 0x80, 0xf2, 0xff, 0x03, 0x04, > + 0xc0, 0xfb, 0x7f, 0x83, 0x00, 0xfe, 0xff, 0xff, 0x87, 0xff, 0x7f, 0xf8, > + 0xff, 0xff, 0x1f, 0x00, 0xef, 0xff, 0x0d, 0x02, 0xe0, 0xe7, 0x7f, 0xc7, > + 0x00, 0xfe, 0xff, 0xff, 0x87, 0xff, 0x7f, 0xf8, 0xff, 0xff, 0x1f, 0x80, > + 0x9f, 0xff, 0x1d, 0x03, 0xf0, 0xc7, 0x7f, 0xff, 0x00, 0xfc, 0xff, 0xff, > + 0x87, 0xff, 0x7f, 0xf8, 0xff, 0xff, 0x0f, 0xc0, 0x1f, 0xff, 0xfd, 0x03, > + 0xf8, 0xcf, 0x7f, 0xff, 0x03, 0xfc, 0xff, 0xff, 0x87, 0xff, 0x7f, 0xf8, > + 0xff, 0xff, 0x0f, 0xe0, 0x3f, 0xff, 0xfd, 0x0f, 0xf8, 0xcf, 0x7f, 0xff, > + 0x03, 0xf0, 0xff, 0xff, 0x87, 0xff, 0x7f, 0xf8, 0xff, 0xff, 0x03, 0xe0, > + 0x3f, 0xff, 0xfd, 0x0f, 0xfc, 0xef, 0x7f, 0xff, 0x07, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xf0, 0xbf, 0xff, 0xfd, 0x1f, > + 0xfc, 0xdf, 0x9f, 0xff, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0xf0, 0x7f, 0x7f, 0xfe, 0x0f, 0xfc, 0x3f, 0x80, 0xff, > + 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xf0, > + 0xff, 0x00, 0xfe, 0x07, 0xf8, 0x3f, 0x80, 0x7f, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe0, 0xff, 0x00, 0xfe, 0x01, > + 0xc0, 0xdf, 0x7f, 0x3f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x7f, 0xff, 0xfd, 0x00, 0x00, 0x1c, 0x00, 0x1f, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x70, 0x00, 0x7c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }; > + > + > +/* Send the current framebuffer vbitmap as an interrupt message */ > +static int g13_fb_vbitmap_send(struct hid_device *hdev) > +{ > + struct usb_interface *intf; > + struct usb_device *usbdev; > + struct g13_data *data = hid_get_g13data(hdev); > + > + /* Get the usb device to send the image on */ > + intf = to_usb_interface(hdev->dev.parent); > + usbdev = interface_to_usbdev(intf); > + > + return usb_interrupt_msg(usbdev, usb_sndintpipe(usbdev, 0x02), > + data->fb_vbitmap, G13_VBITMAP_SIZE, > + NULL, USB_CTRL_SET_TIMEOUT*2); > +} > + > +/* Update fb_vbitmap from the screen_base and send to the device */ > +static void g13_fb_update(struct g13_data *data) > +{ > + int row; > + int col; > + int bit; > + u8 *u; > + size_t offset; > + u8 temp; > + > + /* Clear the vbitmap and set the necessary magic number */ > + memset(data->fb_vbitmap, 0x00, G13_VBITMAP_SIZE); > + data->fb_vbitmap[0] = 0x03; > + > + /* > + * Translate the XBM format screen_base into the format needed by the > + * G13. This format places the pixels in a vertical rather than > + * horizontal format. Assuming a grid with 0,0 in the upper left corner > + * and 159,42 in the lower right corner, the first byte contains the > + * pixels 0,0 through 0,7 and the second byte contains the pixels 1,0 > + * through 1,7. Within the byte, bit 0 represents 0,0; bit 1 0,1; etc. > + * > + * This loop operates in reverse to shift the lower bits into their > + * respective positions, shifting the lower rows into the higher bits. > + * > + * The offset is calculated for every 8 rows and is adjusted by 32 since > + * that is what the G13 image message expects. > + */ > + for (row = G13FB_HEIGHT-1; row >= 0; row--) { > + offset = 32 + row/8 * G13FB_WIDTH; > + u = data->fb_vbitmap + offset; > + /* > + * Iterate across the screen_base columns to get the > + * individual bits > + */ > + for (col = 0; col < G13FB_LINE_LENGTH; col++) { > + /* > + * We will work with a temporary value since we don't > + * want to modify screen_base as we shift each bit > + * downward. > + */ > + temp = data->fb_bitmap[row * G13FB_LINE_LENGTH + col]; > + > + /* > + * For each bit in the pixel row we will shift it onto > + * the appropriate by by shift the g13 byte up by 1 and > + * simply doing a bitwise or of the low byte > + */ > + for (bit = 0; bit < 8; bit++) { > + /*Shift the g13 byte up by 1 for this new row*/ > + u[bit] <<= 1; > + /* Bring in the new pixel of temp */ > + u[bit] |= (temp & 0x01); > + /* > + * Shift temp down so the low pixel is ready > + * for the next byte > + */ > + temp >>= 1; > + } > + > + /* > + * The last byte represented 8 vertical pixels so we'll > + * jump ahead 8 > + */ > + u += 8; > + } > + } > + > + /* > + * Now that we have translated screen_base into a format expected by > + * the g13 let's send out the vbitmap > + */ > + g13_fb_vbitmap_send(data->hdev); > + > +} > + > +/* Callback from deferred IO workqueue */ > +static void g13_fb_deferred_io(struct fb_info *info, struct list_head *pagelist) > +{ > + g13_fb_update(info->par); > +} > + > +/* Stub to call the system default and update the image on the g13 */ > +static void g13_fb_fillrect(struct fb_info *info, > + const struct fb_fillrect *rect) > +{ > + struct g13_data *par = info->par; > + > + sys_fillrect(info, rect); > + > + g13_fb_update(par); > +} > + > +/* Stub to call the system default and update the image on the g13 */ > +static void g13_fb_copyarea(struct fb_info *info, > + const struct fb_copyarea *area) > +{ > + struct g13_data *par = info->par; > + > + sys_copyarea(info, area); > + > + g13_fb_update(par); > +} > + > +/* Stub to call the system default and update the image on the g13 */ > +static void g13_fb_imageblit(struct fb_info *info, const struct fb_image *image) > +{ > + struct g13_data *par = info->par; > + > + sys_imageblit(info, image); > + > + g13_fb_update(par); > +} > + > +/* > + * this is the slow path from userspace. they can seek and write to > + * the fb. it's inefficient to do anything less than a full screen draw > + */ > +static ssize_t g13_fb_write(struct fb_info *info, const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + struct g13_data *par = info->par; > + unsigned long p = *ppos; > + void *dst; > + int err = 0; > + unsigned long total_size; > + > + if (info->state != FBINFO_STATE_RUNNING) > + return -EPERM; > + > + total_size = info->fix.smem_len; > + > + if (p > total_size) > + return -EFBIG; > + > + if (count > total_size) { > + err = -EFBIG; > + count = total_size; > + } > + > + if (count + p > total_size) { > + if (!err) > + err = -ENOSPC; > + > + count = total_size - p; > + } > + > + dst = (void __force *)(info->screen_base + p); > + > + if (copy_from_user(dst, buf, count)) > + err = -EFAULT; > + > + if (!err) > + *ppos += count; > + > + g13_fb_update(par); > + > + return (err) ? err : count; > +} > + > +static struct fb_ops g13fb_ops = { const? > + .owner = THIS_MODULE, > + .fb_read = fb_sys_read, > + .fb_write = g13_fb_write, > + .fb_fillrect = g13_fb_fillrect, > + .fb_copyarea = g13_fb_copyarea, > + .fb_imageblit = g13_fb_imageblit, > +}; > + > +/* > + * The "fb_node" attribute > + */ > +static ssize_t g13_fb_node_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + unsigned fb_node; > + struct g13_data *data = dev_get_drvdata(dev); > + > + if (data == NULL) > + return -ENODATA; > + > + fb_node = data->fb_info->node; > + > + return sprintf(buf, "%u\n", fb_node); > +} > + > +static DEVICE_ATTR(fb_node, 0444, g13_fb_node_show, NULL); > + > + > +/* > + * The "fb_update_rate" attribute > + */ > +static ssize_t g13_fb_update_rate_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + unsigned fb_update_rate; > + struct g13_data *data = dev_get_drvdata(dev); > + > + if (data == NULL) > + return -ENODATA; > + > + fb_update_rate = data->fb_update_rate; > + > + return sprintf(buf, "%u\n", fb_update_rate); > +} > + > +static ssize_t g13_set_fb_update_rate(struct hid_device *hdev, > + unsigned fb_update_rate) > +{ > + struct g13_data *data = hid_get_g13data(hdev); > + > + if (data == NULL) > + return -ENODATA; > + > + if (fb_update_rate > G13FB_UPDATE_RATE_LIMIT) > + data->fb_update_rate = G13FB_UPDATE_RATE_LIMIT; > + else if (fb_update_rate == 0) > + data->fb_update_rate = 1; > + else > + data->fb_update_rate = fb_update_rate; > + > + data->fb_defio.delay = HZ / data->fb_update_rate; > + > + return 0; > +} > + > +static ssize_t g13_fb_update_rate_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct hid_device *hdev; > + int i; > + unsigned u; > + ssize_t set_result; > + > + /* Get the hid associated with the device */ > + hdev = container_of(dev, struct hid_device, dev); > + > + /* If we have an invalid pointer we'll return ENODATA */ > + if (hdev == NULL || &(hdev->dev) != dev) > + return -ENODATA; > + > + i = sscanf(buf, "%u", &u); > + if (i != 1) { > + printk(KERN_ERR "unrecognized input: %s", buf); > + return -1; > + } > + > + set_result = g13_set_fb_update_rate(hdev, u); > + > + if (set_result < 0) > + return set_result; > + > + return count; > +} > + > +static DEVICE_ATTR(fb_update_rate, 0666, > + g13_fb_update_rate_show, > + g13_fb_update_rate_store); > + > + > +/* > + * The "mled" attribute > + * on or off for each of the four M led's (M1 M2 M3 MR) > + */ > +static ssize_t g13_mled_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + unsigned mled; > + struct g13_data *data = dev_get_drvdata(dev); > + > + if (data == NULL) > + return -ENODATA; > + > + mled = data->mled; > + > + return sprintf(buf, "%u\n", mled); > +} > + > +static ssize_t g13_set_mled(struct hid_device *hdev, unsigned mled) int. It does not return size but error code. > +{ > + struct g13_data *data = hid_get_g13data(hdev); > + > + if (data == NULL || data->mled_report == NULL) > + return -ENODATA; > + > + data->mled_report->field[0]->value[0] = mled&0x0F; > + data->mled_report->field[0]->value[1] = 0x00; > + data->mled_report->field[0]->value[2] = 0x00; > + data->mled_report->field[0]->value[3] = 0x00; > + > + usbhid_submit_report(hdev, data->mled_report, USB_DIR_OUT); > + > + data->mled = mled; > + > + return 0; > +} > + > +static ssize_t g13_mled_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct hid_device *hdev; > + int i; > + unsigned m[4]; > + unsigned mled; > + ssize_t set_result; > + > + /* Get the hid associated with the device */ > + hdev = container_of(dev, struct hid_device, dev); > + > + /* If we have an invalid pointer we'll return ENODATA */ > + if (hdev == NULL || &(hdev->dev) != dev) > + return -ENODATA; > + > + i = sscanf(buf, "%u %u %u %u", m, m+1, m+2, m+3); > + if (!(i == 4 || i == 1)) { > + printk(KERN_ERR "unrecognized input: %s", buf); > + return -1; > + } > + > + if (i == 1) > + mled = m[0]; > + else > + mled = (m[0] ? 1 : 0) | (m[1] ? 2 : 0) | > + (m[2] ? 4 : 0) | (m[3] ? 8 : 0); > + > + set_result = g13_set_mled(hdev, mled); > + > + if (set_result < 0) > + return set_result; > + > + return count; > +} > + > +static DEVICE_ATTR(mled, 0666, g13_mled_show, g13_mled_store); Like other people said, you should be using LED framework - uiserspace can still control LED state, but in a standard way. > + > +static int g13_input_setkeycode(struct input_dev *dev, > + int scancode, > + int keycode) > +{ > + int old_keycode; > + int i; > + struct g13_data *data = input_get_g13data(dev); > + > + if (data == NULL) > + return -EINVAL; > + > + if (scancode >= dev->keycodemax) > + return -EINVAL; > + > + if (!dev->keycodesize) > + return -EINVAL; > + > + if (dev->keycodesize < sizeof(keycode) && > + (keycode >> (dev->keycodesize * 8))) > + return -EINVAL; > + > + write_lock(&data->lock); > + > + old_keycode = data->keycode[scancode]; > + data->keycode[scancode] = keycode; > + > + clear_bit(old_keycode, dev->keybit); > + set_bit(keycode, dev->keybit); > + > + for (i = 0; i < dev->keycodemax; i++) { > + if (data->keycode[i] == old_keycode) { > + set_bit(old_keycode, dev->keybit); > + break; /* Setting the bit twice is useless, so break */ > + } > + } > + > + write_unlock(&data->lock); > + > + return 0; > +} > + > +static int g13_input_getkeycode(struct input_dev *dev, > + int scancode, > + int *keycode) > +{ > + struct g13_data *data = input_get_g13data(dev); > + > + if (!dev->keycodesize) > + return -EINVAL; > + > + if (scancode >= dev->keycodemax) > + return -EINVAL; > + > + read_lock(&data->lock); > + > + *keycode = data->keycode[scancode]; > + > + read_unlock(&data->lock); > + > + return 0; > +} Default input core methods should cover this, no? > + > + > +/* > + * The "keymap" attribute > + */ > +static ssize_t g13_keymap_index_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct g13_data *data = dev_get_drvdata(dev); > + > + if (data == NULL) > + return -ENODATA; > + > + return sprintf(buf, "%u\n", data->curkeymap); > +} > + > +static ssize_t g13_set_keymap_index(struct hid_device *hdev, unsigned k) > +{ > + struct g13_data *data = hid_get_g13data(hdev); > + > + if (data == NULL) > + return -ENODATA; > + > + if (k > 2) > + return -EINVAL; > + > + data->curkeymap = k; What if you change keymap while a key is pressed and new keymap does not have anything in that position? The key may get "stuck" for a bit... > + > + if (data->keymap_switching) > + g13_set_mled(hdev, 1<<k); > + > + return 0; > +} > + > +static ssize_t g13_keymap_index_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct hid_device *hdev; > + int i; > + unsigned k; > + ssize_t set_result; > + > + /* Get the hid associated with the device */ > + hdev = container_of(dev, struct hid_device, dev); > + > + /* If we have an invalid pointer we'll return ENODATA */ > + if (hdev == NULL || &(hdev->dev) != dev) > + return -ENODATA; > + > + i = sscanf(buf, "%u", &k); > + if (i != 1) { > + printk(KERN_ERR "unrecognized input: %s", buf); > + return -1; > + } > + > + set_result = g13_set_keymap_index(hdev, k); > + > + if (set_result < 0) > + return set_result; > + > + return count; > +} > + > +static DEVICE_ATTR(keymap_index, 0666, > + g13_keymap_index_show, > + g13_keymap_index_store); > + > +/* > + * The "keycode" attribute > + */ > +static ssize_t g13_keymap_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int i; > + int offset = 0; > + int result; > + > + struct g13_data *data = dev_get_drvdata(dev); > + > + if (data == NULL) > + return -ENODATA; > + > + for (i = 0; i < G13_KEYMAP_SIZE; i++) { > + result = sprintf(buf+offset, > + "0x%03x 0x%04x\n", > + i, data->keycode[i]); > + if (result < 0) > + return -EINVAL; > + offset += result; > + } > + > + return offset+1; > +} > + > +static ssize_t g13_keymap_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct hid_device *hdev; > + int scanned; > + int consumed; > + int scancd; > + int keycd; > + int set_result; > + int set = 0; > + int gkey; > + int index; > + int good; > + struct g13_data *data; > + > + /* Get the hid associated with the device */ > + hdev = container_of(dev, struct hid_device, dev); > + > + /* If we have an invalid pointer we'll return ENODATA */ > + if (hdev == NULL || &(hdev->dev) != dev) > + return -ENODATA; > + > + /* Now, let's get the data structure */ > + data = hid_get_g13data(hdev); > + if (data == NULL) > + return -ENODATA; > + > + do { > + good = 0; > + > + /* Look for scancode keycode pair in hex */ > + scanned = sscanf(buf, "%x %x%n", &scancd, &keycd, &consumed); > + if (scanned == 2) { > + buf += consumed; > + set_result = g13_input_setkeycode(data->input_dev, scancd, keycd); > + if (set_result < 0) > + return set_result; > + set++; > + good = 1; > + } else { > + /* > + * Look for Gkey keycode pair and assign to current > + * keymap > + */ > + scanned = sscanf(buf, "G%d %x%n", &gkey, &keycd, &consumed); > + if (scanned == 2 && gkey > 0 && gkey <= G13_KEYS) { > + buf += consumed; > + scancd = data->curkeymap * G13_KEYS + gkey - 1; > + set_result = g13_input_setkeycode(data->input_dev, scancd, keycd); > + if (set_result < 0) > + return set_result; > + set++; > + good = 1; > + } else { > + /* > + * Look for Gkey-index keycode pair and assign > + * to indexed keymap > + */ > + scanned = sscanf(buf, "G%d-%d %x%n", &gkey, &index, &keycd, &consumed); > + if (scanned == 3 && > + gkey > 0 && gkey <= G13_KEYS && > + index >= 0 && index <= 2) { > + buf += consumed; > + scancd = index * G13_KEYS + gkey - 1; > + set_result = g13_input_setkeycode(data->input_dev, scancd, keycd); > + if (set_result < 0) > + return set_result; > + set++; > + good = 1; > + } > + } > + } > + > + } while (good); > + > + if (set == 0) { > + printk(KERN_ERR "unrecognized keycode input: %s", buf); > + return -1; > + } > + > + return count; > +} > + > +static DEVICE_ATTR(keymap, 0666, g13_keymap_show, g13_keymap_store); > No. Just use EVIOCSKEYCODE. > +/* > + * The "emit_msc_raw" attribute > + */ > +static ssize_t g13_emit_msc_raw_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct g13_data *data = dev_get_drvdata(dev); > + > + if (data == NULL) > + return -ENODATA; > + > + return sprintf(buf, "%u\n", data->emit_msc_raw); > +} > + > +static ssize_t g13_set_emit_msc_raw(struct hid_device *hdev, unsigned k) > +{ > + struct g13_data *data = hid_get_g13data(hdev); > + > + if (data == NULL) > + return -ENODATA; > + > + data->emit_msc_raw = k; > + > + return 0; > +} > + > +static ssize_t g13_emit_msc_raw_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct hid_device *hdev; > + int i; > + unsigned k; > + ssize_t set_result; > + > + /* Get the hid associated with the device */ > + hdev = container_of(dev, struct hid_device, dev); > + > + /* If we have an invalid pointer we'll return ENODATA */ > + if (hdev == NULL || &(hdev->dev) != dev) > + return -ENODATA; > + > + i = sscanf(buf, "%u", &k); > + if (i != 1) { > + printk(KERN_ERR "unrecognized input: %s", buf); > + return -1; > + } > + > + set_result = g13_set_emit_msc_raw(hdev, k); > + > + if (set_result < 0) > + return set_result; > + > + return count; > +} > + > +static DEVICE_ATTR(emit_msc_raw, 0666, > + g13_emit_msc_raw_show, > + g13_emit_msc_raw_store); > + I do no see the need for this attribute, simply emit MSC_RAW always. > + > +/* > + * The "keymap_switching" attribute > + */ > +static ssize_t g13_keymap_switching_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct g13_data *data = dev_get_drvdata(dev); > + > + if (data == NULL) > + return -ENODATA; > + > + return sprintf(buf, "%u\n", data->keymap_switching); > +} > + > +static ssize_t g13_set_keymap_switching(struct hid_device *hdev, unsigned k) > +{ > + struct g13_data *data = hid_get_g13data(hdev); > + > + if (data == NULL) > + return -ENODATA; > + > + data->keymap_switching = k; > + > + if (data->keymap_switching) > + g13_set_mled(hdev, 1<<(data->curkeymap)); > + > + return 0; > +} > + > +static ssize_t g13_keymap_switching_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct hid_device *hdev; > + int i; > + unsigned k; > + ssize_t set_result; > + > + /* Get the hid associated with the device */ > + hdev = container_of(dev, struct hid_device, dev); > + > + /* If we have an invalid pointer we'll return ENODATA */ > + if (hdev == NULL || &(hdev->dev) != dev) > + return -ENODATA; > + > + i = sscanf(buf, "%u", &k); > + if (i != 1) { > + printk(KERN_ERR "unrecognized input: %s", buf); > + return -1; > + } > + > + set_result = g13_set_keymap_switching(hdev, k); > + > + if (set_result < 0) > + return set_result; > + > + return count; > +} > + > +static DEVICE_ATTR(keymap_switching, 0666, > + g13_keymap_switching_show, > + g13_keymap_switching_store); Hmm, attributes that are changing device state are usually 0644. > + > + > +static ssize_t g13_name_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct g13_data *data = dev_get_drvdata(dev); > + int result; > + > + if (data == NULL) > + return -ENODATA; > + > + if (data->name == NULL) { > + buf[0] = 0x00; > + return 1; > + } > + > + read_lock(&data->lock); > + result = sprintf(buf, "%s", data->name); > + read_unlock(&data->lock); > + > + return result; > +} > + > +static ssize_t g13_name_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct g13_data *data = dev_get_drvdata(dev); > + size_t limit = count; > + char *end; > + > + if (data == NULL) > + return -ENODATA; > + > + write_lock(&data->lock); > + > + if (data->name != NULL) { > + kfree(data->name); > + data->name = NULL; > + } > + > + end = strpbrk(buf, "\n\r"); > + if (end != NULL) > + limit = end - buf; > + > + if (end != buf) { > + > + if (limit > 100) > + limit = 100; > + > + data->name = kzalloc(limit+1, GFP_KERNEL); > + > + strncpy(data->name, buf, limit); > + } > + > + write_unlock(&data->lock); > + > + return count; > +} > + > +static DEVICE_ATTR(name, 0666, g13_name_show, g13_name_store); What this attribute is for? > + > +/* > + * The "rgb" attribute > + * red green blue > + * each with values 0 - 255 (black - full intensity) > + */ > +static ssize_t g13_rgb_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + unsigned r, g, b; > + struct g13_data *data = dev_get_drvdata(dev); > + > + if (data == NULL) > + return -ENODATA; > + > + r = data->rgb[0]; > + g = data->rgb[1]; > + b = data->rgb[2]; > + > + return sprintf(buf, "%u %u %u\n", r, g, b); > +} > + > +static ssize_t g13_set_rgb(struct hid_device *hdev, > + unsigned r, unsigned g, unsigned b) > +{ > + struct g13_data *data = hid_get_g13data(hdev); > + > + if (data == NULL || data->backlight_report == NULL) > + return -ENODATA; > + > + data->backlight_report->field[0]->value[0] = r; > + data->backlight_report->field[0]->value[1] = g; > + data->backlight_report->field[0]->value[2] = b; > + data->backlight_report->field[0]->value[3] = 0x00; > + > + usbhid_submit_report(hdev, data->backlight_report, USB_DIR_OUT); > + > + data->rgb[0] = r; > + data->rgb[1] = g; > + data->rgb[2] = b; > + > + return 0; > +} > + > +static ssize_t g13_rgb_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct hid_device *hdev; > + int i; > + unsigned r; > + unsigned g; > + unsigned b; > + ssize_t set_result; > + > + /* Get the hid associated with the device */ > + hdev = container_of(dev, struct hid_device, dev); > + > + /* If we have an invalid pointer we'll return ENODATA */ > + if (hdev == NULL || &(hdev->dev) != dev) > + return -ENODATA; > + > + i = sscanf(buf, "%u %u %u", &r, &g, &b); > + if (i != 3) { > + printk(KERN_ERR "unrecognized input: %s", buf); > + return -1; > + } > + > + set_result = g13_set_rgb(hdev, r, g, b); > + > + if (set_result < 0) > + return set_result; > + > + return count; > +} > + > +static DEVICE_ATTR(rgb, 0666, g13_rgb_show, g13_rgb_store); > + > +/* > + * Create a group of attributes so that we can create and destroy them all > + * at once. > + */ > +static struct attribute *g13_attrs[] = { > + &dev_attr_name.attr, > + &dev_attr_rgb.attr, > + &dev_attr_mled.attr, > + &dev_attr_keymap_index.attr, > + &dev_attr_emit_msc_raw.attr, > + &dev_attr_keymap_switching.attr, > + &dev_attr_keymap.attr, > + &dev_attr_fb_update_rate.attr, > + &dev_attr_fb_node.attr, > + NULL, /* need to NULL terminate the list of attributes */ > +}; > + > +/* > + * An unnamed attribute group will put all of the attributes directly in > + * the kobject directory. If we specify a name, a subdirectory will be > + * created for the attributes with the directory being the name of the > + * attribute group. > + */ > +static struct attribute_group g13_attr_group = { > + .attrs = g13_attrs, > +}; > + > +static struct fb_deferred_io g13_fb_defio = { > + .delay = HZ / G13FB_UPDATE_RATE_DEFAULT, > + .deferred_io = g13_fb_deferred_io, > +}; > + > +static void g13_raw_event_process_input(struct hid_device *hdev, > + struct g13_data *g13data, > + u8 *raw_data) > +{ > + struct input_dev *idev = g13data->input_dev; > + unsigned int code; > + int value; > + int i; > + int mask; > + int offset; > + u8 val; > + > + g13data->ready2 = 1; > + > + if (idev == NULL) > + return; > + > + if (g13data->curkeymap < 3) > + offset = G13_KEYS * g13data->curkeymap; > + else > + offset = 0; > + > + for (i = 0, mask = 0x01; i < 8; i++, mask <<= 1) { > + /* Keys G1 through G8 */ > + code = g13data->keycode[i+offset]; > + value = raw_data[3] & mask; > + if (g13data->keycode[i] != KEY_RESERVED) > + input_report_key(idev, code, value); > + input_event(idev, EV_MSC, MSC_SCAN, i); That means these MSC_SCAN events are emitted _always_. Not sure if that is too useful. If you were to detect the state change and emit MSC_SCAN for changed keys, that would be better. > + > + /* Keys G9 through G16 */ > + code = g13data->keycode[i+8+offset]; > + value = raw_data[4] & mask; > + if (g13data->keycode[i+8] != KEY_RESERVED) > + input_report_key(idev, code, value); > + input_event(idev, EV_MSC, MSC_SCAN, i+8); > + > + /* Keys G17 through G22 */ > + code = g13data->keycode[i+16+offset]; > + value = raw_data[5] & mask; > + if (i <= 5 && g13data->keycode[i+16] != KEY_RESERVED) > + input_report_key(idev, code, value); > + input_event(idev, EV_MSC, MSC_SCAN, i+16); > + > + /* Keys FUNC through M3 */ > + code = g13data->keycode[i+22+offset]; > + value = raw_data[6] & mask; > + if (g13data->keycode[i+22] != KEY_RESERVED) > + input_report_key(idev, code, value); > + input_event(idev, EV_MSC, MSC_SCAN, i+22); > + > + /* Keys MR through LIGHT */ > + code = g13data->keycode[i+30+offset]; > + value = raw_data[7] & mask; > + if (i <= 4 && g13data->keycode[i+30] != KEY_RESERVED) > + input_report_key(idev, code, value); > + input_event(idev, EV_MSC, MSC_SCAN, i+30); > + } > + > + if (g13data->emit_msc_raw) { > + /* > + * Outputs an MSC_RAW value with the low four > + * bits = M1-MR, Low bit = M1 > + */ > + val = raw_data[6] >> 5; > + val |= (raw_data[7] & 0x01 << 3); > + input_event(idev, EV_MSC, MSC_RAW, val); > + } > + > + if (g13data->keymap_switching) { > + if (raw_data[6] & 0x20) { > + g13data->curkeymap = 0; > + g13_set_mled(hdev, 0x01); > + } else if (raw_data[6] & 0x40) { > + g13data->curkeymap = 1; > + g13_set_mled(hdev, 0x02); > + } else if (raw_data[6] & 0x80) { > + g13data->curkeymap = 2; > + g13_set_mled(hdev, 0x04); > + } > + } > + > + input_report_abs(idev, ABS_X, raw_data[1]); > + input_report_abs(idev, ABS_Y, raw_data[2]); > + input_sync(idev); > +} > + > +static int g13_raw_event(struct hid_device *hdev, > + struct hid_report *report, > + u8 *raw_data, int size) > +{ > + /* > + * On initialization receive a 258 byte message with > + * data = 6 0 255 255 255 255 255 255 255 255 ... > + */ > + struct g13_data *g13data; > + g13data = dev_get_drvdata(&hdev->dev); > + > + if (g13data == NULL) > + return 1; > + > + switch (report->id) { > + case 6: > + g13data->ready = 1; > + break; > + case 1: > + g13_raw_event_process_input(hdev, g13data, raw_data); > + break; > + default: > + return 0; > + } > + > + return 1; > +} > + > +static void g13_initialize_keymap(struct g13_data *data) > +{ > + int i; > + > + write_lock(&data->lock); Why do you take this lock? What else could be accessing the structure at this point? > + > + for (i = 0; i < G13_KEYS; i++) { > + data->keycode[i] = g13_default_key_map[i]; > + set_bit(data->keycode[i], data->input_dev->keybit); > + } > + > + clear_bit(0, data->input_dev->keybit); Use KEY_RESERVED instead of 0 so readers know what is going on. Also no need to use locked variants, __set_bit and __clear_bit should suffice. > + > + write_unlock(&data->lock); > + > +} > + > +static int g13_probe(struct hid_device *hdev, > + const struct hid_device_id *id) > +{ > + int error; > + struct g13_data *data; > + int i; > + struct list_head *feature_report_list = > + &hdev->report_enum[HID_FEATURE_REPORT].report_list; > + struct hid_report *report; > + > + dev_dbg(&hdev->dev, "Logitech G13 HID hardware probe..."); > + > + /* > + * Let's allocate the g13 data structure, set some reasonable > + * defaults, and associate it with the device > + */ > + data = kzalloc(sizeof(struct g13_data), GFP_KERNEL); > + if (data == NULL) { > + dev_err(&hdev->dev, "can't allocate space for Logitech G13 device attributes\n"); > + error = -ENOMEM; > + goto err_no_cleanup; > + } > + > + rwlock_init(&data->lock); rwlock is usually slower than spinlock... There is a handful of places where use of rwlock is warranted. > + > + data->hdev = hdev; > + > + data->fb_bitmap = vmalloc(G13FB_SIZE); > + if (data->fb_bitmap == NULL) { > + dev_err(&hdev->dev, G13_NAME ": ERROR: can't get a free page for framebuffer\n"); > + error = -ENOMEM; > + goto err_cleanup_data; > + } > + memcpy(data->fb_bitmap, g13_lcd_bits, G13FB_SIZE); > + > + data->fb_vbitmap = kmalloc(sizeof(u8) * G13_VBITMAP_SIZE, GFP_KERNEL); > + if (data->fb_vbitmap == NULL) { > + dev_err(&hdev->dev, G13_NAME ": ERROR: can't alloc vbitmap image buffer\n"); > + error = -ENOMEM; > + goto err_cleanup_fb_bitmap; > + } > + > + hid_set_drvdata(hdev, data); > + > + dbg_hid("Preparing to parse " G13_NAME " hid reports\n"); > + > + /* Parse the device reports and start it up */ > + error = hid_parse(hdev); > + if (error) { > + dev_err(&hdev->dev, G13_NAME " device report parse failed\n"); > + error = -EINVAL; > + goto err_cleanup_fb_vbitmap; > + } > + > + mdelay(10); Why is this needed? > + > + error = hid_hw_start(hdev, HID_CONNECT_DEFAULT | HID_CONNECT_HIDINPUT_FORCE); > + if (error) { > + dev_err(&hdev->dev, G13_NAME " hardware start failed\n"); > + error = -EINVAL; > + goto err_cleanup_fb_vbitmap; > + } > + > + dbg_hid(G13_NAME " claimed: %d\n", hdev->claimed); > + > + error = hdev->ll_driver->open(hdev); > + if (error) { > + dev_err(&hdev->dev, G13_NAME " failed to open input interrupt pipe for key and joystick events\n"); > + error = -EINVAL; > + goto err_cleanup_fb_vbitmap; > + } > + > + /* Set up the input device for the key I/O */ > + data->input_dev = input_allocate_device(); > + if (data->input_dev == NULL) { > + dev_err(&hdev->dev, G13_NAME " error initializing the input device"); > + error = -ENOMEM; > + goto err_cleanup_fb_vbitmap; > + } > + > + input_set_drvdata(data->input_dev, hdev); > + > + data->input_dev->name = G13_NAME; > + data->input_dev->phys = hdev->phys; > + data->input_dev->uniq = hdev->uniq; > + data->input_dev->id.bustype = hdev->bus; > + data->input_dev->id.vendor = hdev->vendor; > + data->input_dev->id.product = hdev->product; > + data->input_dev->id.version = hdev->version; > + data->input_dev->dev.parent = hdev->dev.parent; > + data->input_dev->keycode = data->keycode; > + data->input_dev->keycodemax = G13_KEYMAP_SIZE; > + data->input_dev->keycodesize = sizeof(u32); > + data->input_dev->setkeycode = g13_input_setkeycode; > + data->input_dev->getkeycode = g13_input_getkeycode; > + > + input_set_capability(data->input_dev, EV_ABS, ABS_X); > + input_set_capability(data->input_dev, EV_ABS, ABS_Y); > + input_set_capability(data->input_dev, EV_MSC, MSC_SCAN); > + input_set_capability(data->input_dev, EV_KEY, KEY_UNKNOWN); > + data->input_dev->evbit[0] |= BIT_MASK(EV_REP); > + > + /* 4 center values */ > + input_set_abs_params(data->input_dev, ABS_X, 0, 0xff, 0, 4); > + input_set_abs_params(data->input_dev, ABS_Y, 0, 0xff, 0, 4); > + > + g13_initialize_keymap(data); > + > + error = input_register_device(data->input_dev); > + if (error) { > + dev_err(&hdev->dev, G13_NAME " error registering the input device"); > + error = -EINVAL; > + goto err_cleanup_input_dev; > + } > + > + /* Set up the framebuffer device */ > + data->fb_update_rate = G13FB_UPDATE_RATE_DEFAULT; > + data->fb_info = framebuffer_alloc(0, &hdev->dev); > + if (data->fb_info == NULL) { > + dev_err(&hdev->dev, G13_NAME " failed to allocate a framebuffer\n"); > + goto err_cleanup_input_dev; > + } > + > + dbg_hid(KERN_INFO G13_NAME " allocated framebuffer\n"); > + > + data->fb_defio = g13_fb_defio; > + data->fb_info->fbdefio = &data->fb_defio; > + > + dbg_hid(KERN_INFO G13_NAME " allocated deferred IO structure\n"); > + > + data->fb_info->screen_base = (char __force __iomem *) data->fb_bitmap; > + data->fb_info->fbops = &g13fb_ops; > + data->fb_info->var = g13fb_var; > + data->fb_info->fix = g13fb_fix; > + data->fb_info->fix.smem_len = G13FB_SIZE; > + data->fb_info->par = data; > + data->fb_info->flags = FBINFO_FLAG_DEFAULT; > + > + fb_deferred_io_init(data->fb_info); > + > + if (register_framebuffer(data->fb_info) < 0) > + goto err_cleanup_fb; > + > + /* Add the sysfs attributes */ > + error = sysfs_create_group(&(hdev->dev.kobj), &g13_attr_group); > + if (error) { > + dev_err(&hdev->dev, "Logitech G13 failed to create sysfs group attributes\n"); > + goto err_cleanup_fb; > + } > + > + dbg_hid("Waiting for G13 to activate\n"); > + > + if (list_empty(feature_report_list)) { > + dev_err(&hdev->dev, "no feature report found\n"); > + error = -ENODEV; > + goto err_cleanup_fb; > + } > + dbg_hid("G13 feature report found\n"); > + > + list_for_each_entry(report, feature_report_list, list) { > + switch (report->id) { > + case 0x04: > + data->report_4 = report; > + break; > + case 0x05: > + data->mled_report = report; > + break; > + case 0x06: > + data->start_input_report = report; > + break; > + case 0x07: > + data->backlight_report = report; > + break; > + default: > + break; > + } > + dbg_hid("G13 Feature report: id=%u type=%u size=%u maxfield=%u report_count=%u\n", > + report->id, report->type, report->size, > + report->maxfield, report->field[0]->report_count); > + } > + > + dbg_hid("Found all reports\n"); > + > + for (i = 0; i < 20; i++) { > + if (data->ready && data->ready2) > + break; > + mdelay(10); > + } Consider using completion? > + > + if (!(data->ready && data->ready2)) > + printk(KERN_ERR "G13 hasn't responded yet, forging ahead with initialization\n"); > + else > + dbg_hid("G13 initialized\n"); > + > + /* > + * Set the initial color and load the linux logo > + * We're going to ignore the error values. If there is an error at this > + * point we'll forge ahead. > + */ > + > + dbg_hid("Set default color\n"); > + > + error = g13_set_rgb(hdev, G13_DEFAULT_RED, G13_DEFAULT_GREEN, G13_DEFAULT_BLUE); And...? > + > + usbhid_submit_report(hdev, data->start_input_report, USB_DIR_IN); > + > + g13_fb_update(data); > + > + dbg_hid("G13 activated and initialized\n"); > + > + /* Everything went well */ > + return 0; > + > +err_cleanup_fb: > + framebuffer_release(data->fb_info); input device was registered here so you need to unregister it (and suppress call to input_free_device below). > + > +err_cleanup_input_dev: > + input_free_device(data->input_dev); > + > +err_cleanup_fb_vbitmap: > + kfree(data->fb_vbitmap); > + > +err_cleanup_fb_bitmap: > + vfree(data->fb_bitmap); > + > +err_cleanup_data: > + /* Make sure we clean up the allocated data structure */ > + kfree(data); > + > +err_no_cleanup: > + > + hid_set_drvdata(hdev, NULL); > + I do not see sysfs group being destroyed. > + return error; > +} > + > +static void g13_remove(struct hid_device *hdev) > +{ > + struct g13_data *data; > + > + hdev->ll_driver->close(hdev); > + > + hid_hw_stop(hdev); > + > + /* Get the internal g13 data buffer */ > + data = hid_get_drvdata(hdev); > + > + /* Clean up the buffer */ > + if (data != NULL) { Can it ever be NULL? > + write_lock(&data->lock); Why? > + input_unregister_device(data->input_dev); > + kfree(data->name); > + write_unlock(&data->lock); > + if (data->fb_info != NULL) { > + fb_deferred_io_cleanup(data->fb_info); > + unregister_framebuffer(data->fb_info); > + framebuffer_release(data->fb_info); > + } > + vfree(data->fb_bitmap); > + kfree(data->fb_vbitmap); > + kfree(data); > + } > + I do not see sysfs group being destroyed. > +} > + > +static const struct hid_device_id g13_devices[] = { > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G13) > + }, > + { } > +}; > +MODULE_DEVICE_TABLE(hid, g13_devices); > + > +static struct hid_driver g13_driver = { > + .name = "hid-g13", > + .id_table = g13_devices, > + .probe = g13_probe, > + .remove = g13_remove, > + .raw_event = g13_raw_event, > +}; > + > +static int __init g13_init(void) > +{ > + return hid_register_driver(&g13_driver); > +} > + > +static void __exit g13_exit(void) > +{ > + hid_unregister_driver(&g13_driver); > +} > + > +module_init(g13_init); > +module_exit(g13_exit); > +MODULE_DESCRIPTION("Logitech G13 HID Driver"); > +MODULE_AUTHOR("Rick L Vinyard Jr (rvinyard@xxxxxxxxxxx)"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > index 3839340..f3e27d3 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -295,6 +295,7 @@ > #define USB_DEVICE_ID_LOGITECH_EXTREME_3D 0xc215 > #define USB_DEVICE_ID_LOGITECH_RUMBLEPAD2 0xc218 > #define USB_DEVICE_ID_LOGITECH_RUMBLEPAD2_2 0xc219 > +#define USB_DEVICE_ID_LOGITECH_G13 0xc21c > #define USB_DEVICE_ID_LOGITECH_WINGMAN_F3D 0xc283 > #define USB_DEVICE_ID_LOGITECH_FORCE3D_PRO 0xc286 > #define USB_DEVICE_ID_LOGITECH_WHEEL 0xc294 Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html