Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754001Ab0ADWjj (ORCPT ); Mon, 4 Jan 2010 17:39:39 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753421Ab0ADWjh (ORCPT ); Mon, 4 Jan 2010 17:39:37 -0500 Received: from mail.cs.nmsu.edu ([128.123.64.3]:53076 "EHLO mail.cs.nmsu.edu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752751Ab0ADWjf (ORCPT ); Mon, 4 Jan 2010 17:39:35 -0500 Message-ID: <49c85bdc3cbee3ce495928ad592d4910.squirrel@intranet.cs.nmsu.edu> In-Reply-To: References: <200912142244.nBEMikgN002071@mustang.cs.nmsu.edu> Date: Mon, 4 Jan 2010 15:39:25 -0700 Subject: Re: [PATCH] Logitech G13 driver 0.0.2 From: "Rick L. Vinyard, Jr." To: "Jiri Kosina" Cc: linux-kernel@vger.kernel.org, felipe.balbi@nokia.com, krzysztof.h1@wp.pl, "Andrew Morton" , linux-usb@vger.kernel.org, "Oliver Neukum" , linux-input@vger.kernel.org User-Agent: SquirrelMail/1.4.19 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT X-Priority: 3 (Normal) Importance: Normal Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9021 Lines: 251 Jiri Kosina wrote: > On Wed, 16 Dec 2009, akpm@linux-foundation.org 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/