Re: [PATCH] Logitech G13 driver 0.0.2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux