Re: [PATCH] Logitech G13 driver 0.0.2

From: Rick L. Vinyard, Jr.
Date: Mon Jan 04 2010 - 17:39:43 EST

Next message: Masami Hiramatsu: "Re: [PATCH/resend] Document awk requirements"
Previous message: Christoph Lameter: "[this_cpu_xx V9 3/7] Move local.h include to ringbuffer.c and ring_buffer_benchmark.c"
Next in thread: Andrew Morton: "Re: [PATCH] Logitech G13 driver 0.0.2"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Jiri Kosina wrote:
> On Wed, 16 Dec 2009, akpm@xxxxxxxxxxxxxxxxxxxx wrote:
>
>> +static unsigned char g13_lcd_bits[] = {
>> + 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x28, 0x03, 0x00, 0x40, 0x01, 0x00, 0xc0, 0x3f,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0xa1,
>> + 0x08, 0x00, 0x08, 0x00, 0x00, 0xe0, 0x7f, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x3c, 0x25, 0x00, 0xf3,
>> 0x03,
>> + 0x00, 0xe0, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x0e, 0x05, 0x00, 0x20, 0x16, 0x00, 0xf0, 0xff,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20,
>> 0x09,
>> + 0x00, 0x00, 0x00, 0x42, 0x00, 0xf0, 0xff, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0x05, 0x60, 0x80, 0x00,
>> 0x14,
>> + 0x00, 0x30, 0xe7, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0x00, 0x90, 0x00, 0x00, 0x20, 0x00, 0x10, 0x00, 0x10, 0xe3,
>> 0x01,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x90,
>> 0x02,
>> + 0xe0, 0xdd, 0x03, 0x90, 0x00, 0x50, 0xcb, 0x01, 0x00, 0xfe, 0xff,
>> 0x7f,
>> + 0xf0, 0x3f, 0x00, 0xff, 0xff, 0x7f, 0x80, 0x00, 0xfa, 0xe3, 0x07,
>> 0x38,
>> + 0x00, 0x10, 0xc1, 0x01, 0x00, 0xff, 0xff, 0x7f, 0xf0, 0x3f, 0x00,
>> 0xff,
>> + 0xff, 0xff, 0xd0, 0x00, 0xfc, 0x87, 0x0f, 0x90, 0x00, 0x30, 0xe0,
>> 0x01,
>> + 0x80, 0xff, 0xff, 0x7f, 0xf0, 0x3f, 0x00, 0xff, 0xff, 0xff, 0x81,
>> 0x80,
>> + 0xfe, 0xa7, 0x3f, 0x30, 0x00, 0x30, 0xc0, 0x01, 0xc0, 0xff, 0xff,
>> 0x7f,
>> + 0xf0, 0x3f, 0x00, 0xff, 0xff, 0xff, 0x93, 0x42, 0x0f, 0x08, 0x3a,
>> 0x30,
>> + 0x00, 0x10, 0xe8, 0x01, 0xc0, 0xff, 0xff, 0x7f, 0xf0, 0x3f, 0x00,
>> 0xff,
>> + 0xff, 0xff, 0x93, 0xa4, 0x41, 0x20, 0x20, 0x94, 0x00, 0x30, 0xf4,
>> 0x03,
>> + 0xc0, 0x07, 0x00, 0x00, 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x93,
>> 0x41,
>> + 0x60, 0x48, 0xe1, 0x98, 0x00, 0x70, 0xda, 0x07, 0xc0, 0x07, 0x00,
>> 0x00,
>> + 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x83, 0x77, 0x10, 0x82, 0xc5,
>> 0x1f,
>> + 0x00, 0xd0, 0xcc, 0x07, 0xc0, 0x07, 0x00, 0x00, 0x00, 0x3e, 0x00,
>> 0x00,
>> + 0x00, 0xe0, 0x23, 0x3f, 0x00, 0x90, 0xc0, 0x4f, 0x00, 0x98, 0x83,
>> 0x0f,
>> + 0xc0, 0x07, 0x00, 0x00, 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03,
>> 0x3f,
>> + 0x00, 0x85, 0x86, 0x27, 0x00, 0x0c, 0x80, 0x0f, 0xc0, 0x07, 0x00,
>> 0x00,
>> + 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03, 0x3e, 0xc0, 0x83, 0x86,
>> 0x0b,
>> + 0x00, 0x0c, 0x80, 0x1f, 0xc0, 0x07, 0x00, 0x00, 0x00, 0x3e, 0x00,
>> 0x00,
>> + 0x00, 0xe0, 0x03, 0x11, 0x00, 0x00, 0x04, 0x0d, 0x00, 0x0e, 0x00,
>> 0x3f,
>> + 0xc0, 0x07, 0xff, 0xff, 0x00, 0x3e, 0x00, 0xff, 0xff, 0xff, 0x03,
>> 0x0c,
>> + 0x10, 0x00, 0x02, 0x02, 0x00, 0x0f, 0x00, 0x3f, 0xc0, 0x07, 0xff,
>> 0xff,
>> + 0x00, 0x3e, 0x00, 0xff, 0xff, 0xff, 0x01, 0x00, 0x08, 0x00, 0x1c,
>> 0x02,
>> + 0x00, 0x07, 0x00, 0x7e, 0xc0, 0x07, 0xff, 0xff, 0x00, 0x3e, 0x00,
>> 0xff,
>> + 0xff, 0xff, 0x00, 0x00, 0x04, 0x00, 0x28, 0x04, 0x80, 0x03, 0x00,
>> 0x7e,
>> + 0xc0, 0x07, 0xff, 0xff, 0x00, 0x3e, 0x00, 0xff, 0xff, 0xff, 0x01,
>> 0x08,
>> + 0x02, 0x58, 0x08, 0x00, 0x80, 0x03, 0x00, 0xfc, 0xc0, 0x07, 0xff,
>> 0xff,
>> + 0x00, 0x3e, 0x00, 0xff, 0xff, 0xff, 0x01, 0x00, 0x01, 0x08, 0x21,
>> 0x00,
>> + 0x80, 0x03, 0x00, 0xfc, 0xc0, 0x07, 0x00, 0xf8, 0x00, 0x3e, 0x00,
>> 0x00,
>> + 0x00, 0xe0, 0x03, 0xa4, 0x01, 0x28, 0x00, 0x00, 0xc0, 0x01, 0x00,
>> 0xfc,
>> + 0xc0, 0x07, 0x00, 0xf8, 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03,
>> 0x2e,
>> + 0x02, 0x00, 0x00, 0x00, 0xc0, 0x01, 0x00, 0xfc, 0xc1, 0x07, 0x00,
>> 0xf8,
>> + 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03, 0x20, 0x00, 0x02, 0x00,
>> 0x00,
>> + 0xe0, 0x01, 0x00, 0xfc, 0xc1, 0x07, 0x00, 0xf8, 0x00, 0x3e, 0x00,
>> 0x00,
>> + 0x00, 0xe0, 0x03, 0x20, 0x02, 0x00, 0x10, 0x00, 0xe0, 0x01, 0x00,
>> 0xe8,
>> + 0xc1, 0x07, 0x00, 0xf8, 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03,
>> 0x20,
>> + 0x02, 0x04, 0x00, 0x00, 0xe0, 0x00, 0x00, 0xfc, 0xc1, 0x07, 0x00,
>> 0xf8,
>> + 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03, 0x00, 0x0c, 0x00, 0x00,
>> 0x00,
>> + 0xf0, 0x01, 0x00, 0xfe, 0xc0, 0xff, 0xff, 0xff, 0xf0, 0xff, 0x0f,
>> 0xff,
>> + 0xff, 0xff, 0x03, 0x40, 0x08, 0x08, 0x0d, 0x00, 0x58, 0x03, 0x00,
>> 0x7c,
>> + 0xc0, 0xff, 0xff, 0xff, 0xf0, 0xff, 0x0f, 0xff, 0xff, 0xff, 0x03,
>> 0x40,
>> + 0x10, 0xa0, 0x18, 0x00, 0xa8, 0x06, 0x00, 0xb8, 0x81, 0xff, 0xff,
>> 0xff,
>> + 0xf0, 0xff, 0x0f, 0xff, 0xff, 0xff, 0x01, 0x00, 0x10, 0x00, 0x00,
>> 0x00,
>> + 0x5e, 0x1d, 0x00, 0xe8, 0x82, 0xff, 0xff, 0xff, 0xf0, 0xff, 0x0f,
>> 0xff,
>> + 0xff, 0xff, 0x01, 0x00, 0x20, 0xc0, 0x07, 0x00, 0xab, 0x3a, 0x00,
>> 0x54,
>> + 0x03, 0xfe, 0xff, 0xff, 0xf0, 0xff, 0x0f, 0xff, 0xff, 0x7f, 0x00,
>> 0x80,
>> + 0x40, 0x01, 0x01, 0x00, 0x55, 0x3d, 0x00, 0xaa, 0x06, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x81, 0x01, 0x01,
>> 0x00,
>> + 0xab, 0x1a, 0xc0, 0x55, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x00, 0x80, 0x03, 0x00, 0x00, 0x55, 0x35, 0xe0,
>> 0xab,
>> + 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> + 0xc0, 0x43, 0x01, 0x00, 0xab, 0xaa, 0xff, 0x55, 0x03, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xc7, 0x00,
>> 0x00,
>> + 0x57, 0xf5, 0xff, 0xaa, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0xfc, 0x00, 0x00, 0xaa, 0xea, 0xff,
>> 0xd5,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0xfe, 0x03, 0x00, 0x7c, 0x75, 0x80, 0x2b, 0x00, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xf8, 0x03,
>> 0x00,
>> + 0x80, 0x1f, 0x00, 0x1f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0xf0, 0x01, 0x00 };
>
> Probably somme comment explaining what this actually is would be nice
> here.
>
> [ .. snip .. ]
>
> I can't really comment too much on the framebuffer part, so if you could
> get some Ack for this part from someone skilled in writing framebuffer
> drivers, I'd appreciate it.
>
>> +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);
>
> These constructions are ugly (funnily enough, this is currently being
> discussed on lkml -- see http://lkml.org/lkml/2009/12/15/490
>
> Could you perharps split this into some sub-functions, to make it look
> nicer?
>

There are several places where parameter lists cut down to the 80
character limit caused hideously unreadable code. I'd rather be judicious
in breaking the 80 character limit.

When I first submitted the patch I thought there was a hard-core policy on
80 characters, but after reading the thread you mentioned I see it's just
a recommendation.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Masami Hiramatsu: "Re: [PATCH/resend] Document awk requirements"
Previous message: Christoph Lameter: "[this_cpu_xx V9 3/7] Move local.h include to ringbuffer.c and ring_buffer_benchmark.c"
Next in thread: Andrew Morton: "Re: [PATCH] Logitech G13 driver 0.0.2"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]