| 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
| ||
|
Message-ID: <174828e8a25d1e607eb91ab963396b2a.squirrel@intranet.cs.nmsu.edu>
Date: Fri, 8 Jan 2010 11:32:29 -0700
From: "Rick L. Vinyard, Jr." <rvinyard@...nmsu.edu>
To: "Dmitry Torokhov" <dmitry.torokhov@...il.com>
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
Dmitry Torokhov wrote:
> 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.
>
You're right. I don't think they're needed anymore. I had them in there in
early development.
>> +
>> +static unsigned int g13_default_key_map[G13_KEYS] = {
>
> const.
>
Fixed.
>> + /* 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?
>
The framebuffer member that the address of this struct is assigned to is
nonconst.
>> + .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.
>
I'm looking at the effect of adding the LED class to the driver. If I can
access the settings through sysfs I'll just drop this code related to the
mleds and sysfs. No need to duplicate.
>> +{
>> + 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.
>
Working on it.
>> +
>> +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?
>
I couldn't find this exposed from input core through sysfs anywhere. From
userspace I could access it from an ioctl, but I'd prefer to allow
userspace to do everything from libsysfs rather than a mixture of libsysfs
and ioctls.
I did make sure the ioctls are still supported by providing functions to
input_dev->setkeycode and input_dev->getkeycode.
>> +
>> +
>> +/*
>> + * 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...
>
I see what you mean. I think it would also get stuck if the keymap had a
different key in that position.
I'll add a keymap switch function that issues a key up for all the keys
that are no longer mapped to the same slot.
>> +
>> + 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.
>
I'd really prefer to provide a sysfs interface as opposed to ioctls.
>> +/*
>> + * 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.
>
Will do.
>> +
>> +/*
>> + * 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.
>
Fixed.
>> +
>> +
>> +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?
>
To provide a mnemonic identifier for the device that can be shared across
applications. It also allows a userspace application to lookup a device by
name through sysfs.
>> +
>> +/*
>> + * 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.
>
I couldn't find anything that really explained the purpose of MSC_SCAN.
Can I use it just to report scancodes?
In particular can I selectively emit MSC_SCAN when I only want to report
specific events such as an unmapped key or a special key such as the
backlight or one of the M* keys?
>> +
>> + /* 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?
>
Removed. I don't remember why I had it there at first...
>> +
>> + 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.
>
I wasn't aware of the difference. Fixed it.
>> +
>> + 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.
>
I just took another look at it and I can change it to a spinlock. I was
using the rwlock for the image buffer prior to using the fbdefio approach
for the LCD image, but after the change to the framebuffer with the cached
region it's no longer necessary.
>> +
>> + 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?
>
>From a hardware standpoint I don't know why. All I know is that if it
isn't there the initial LCD image sometimes doesn't load properly. It's
like the device says it's initialized but isn't quite ready yet.
I'll add a comment.
>> +
>> + 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?
>
I'm not sure what you mean.
>> +
>> + 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...?
>
I had an error message before, but took it out. Missed taking out the
"error =" since at this point we'll just forge ahead.
Although failing to set the backlight color at this point could indicate
some I/O issue it's not critical enough to fail driver initialization.
>> +
>> + 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).
>
Thanks for catching that one.
>> +
>> +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.
>
I reordered the code a bit, so there shouldn't be a need for the sysfs
group to be destroyed in g13_probe(). It's the last item now, so if it
succeeds g13_probe() returns 0. I added the sysfs_remove_group() to
g13_remove().
>> + 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?
>
I thought g13_remove() was still called even when g13_probe() returned a
non-zero value. I see now that it isn't.
>> + write_lock(&data->lock);
>
> Why?
>
I missed the sysfs_remove_group() and wasn't sure when sysfs shut down. It
isn't necessary now that I know.
>> + 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.
>
Added.
>> +}
>> +
>> +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
>
Thanks for the feedback,
---
Rick
--
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