lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 8 Jan 2010 00:30:20 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	"Rick L. Vinyard Jr." <rvinyard@...nmsu.edu>
Cc:	linux-kernel@...r.kernel.org, felipe.balbi@...ia.com, pavel@....cz,
	jayakumar.lkml@...il.com, linux-fbdev@...r.kernel.org,
	krzysztof.h1@...pl, akpm@...ux-foundation.org,
	linux-usb@...r.kernel.org, oliver@...kum.org,
	linux-input@...r.kernel.org, jkosina@...e.cz
Subject: Re: [PATCH] hid: Logitech G13 driver 0.0.3

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@...nmsu.edu>
> ---
> 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@...nmsu.edu                                                  *
> + *                                                                         *
> + *   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@...nmsu.edu)");
> +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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ