[PATCH v2] HID: lg4ff: Take advantage of private driver data

[PATCH v2] HID: lg4ff: Take advantage of private driver data

[Michal Malý]

[-- Attachment #1: Type: text/plain, Size: 6440 bytes --]

Hi,

this is a revised version of HID: lg4ff: Use Private Data I asked Simon to
submit for me earlier. To recap what the patch is about, it takes advantage
of the changes introduced in my patches from 2011/04/02. lg4ff now calls
hid_get/set_drvdata() to read or store device configuration. The way I understand
it, this is how all HID drivers store their private data. The change from the version
that was submitted earlier are removed all occurences of uninitialized_var() macro.

Please note that this patch depends on
"[PATCH v4] HID: hid-lg: Allow for custom device-specific properties to be stored in private drv data"
"[PATCH v4] HID: lg4ff: Remove sysfs iface before deallocating memory"

Signed-off-by: Michal Malý <madcatxster@gmail.com>

---
 drivers/hid/hid-lg4ff.c |   92 ++++++++++++++++++++++-------------------------
 1 file changed, 42 insertions(+), 50 deletions(-)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index 1145292..32c173f 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -51,10 +51,7 @@ static ssize_t lg4ff_range_store(struct device *dev, struct device_attribute *at
 
 static DEVICE_ATTR(range, S_IRWXU | S_IRWXG | S_IRWXO, lg4ff_range_show, lg4ff_range_store);
 
-static bool list_inited;
-
 struct lg4ff_device_entry {
-	char  *device_id;	/* Use name in respective kobject structure's address as the ID */
 	__u16 range;
 	__u16 min_range;
 	__u16 max_range;
@@ -63,8 +60,6 @@ struct lg4ff_device_entry {
 	void (*set_range)(struct hid_device *hid, u16 range);
 };
 
-static struct lg4ff_device_entry device_list;
-
 static const signed short lg4ff_wheel_effects[] = {
 	FF_CONSTANT,
 	FF_AUTOCENTER,
@@ -285,18 +280,20 @@ static void hid_lg4ff_switch_native(struct hid_device *hid, const struct lg4ff_n
 /* Read current range and display it in terminal */
 static ssize_t lg4ff_range_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	struct lg4ff_device_entry *uninitialized_var(entry);
-	struct list_head *h;
 	struct hid_device *hid = to_hid_device(dev);
+	struct lg4ff_device_entry *entry;
+	struct lg_drv_data *drv_data;
 	size_t count;
 
-	list_for_each(h, &device_list.list) {
-		entry = list_entry(h, struct lg4ff_device_entry, list);
-		if (strcmp(entry->device_id, (&hid->dev)->kobj.name) == 0)
-			break;
+	drv_data = hid_get_drvdata(hid);
+	if (!drv_data) {
+		hid_err(hid, "Private driver data not found!\n");
+		return 0;
 	}
-	if (h == &device_list.list) {
-		dbg_hid("Device not found!");
+
+	entry = drv_data->device_props;
+	if (!entry) {
+		hid_err(hid, "Device properties not found!\n");
 		return 0;
 	}
 
@@ -308,19 +305,21 @@ static ssize_t lg4ff_range_show(struct device *dev, struct device_attribute *att
  * according to the type of the wheel */
 static ssize_t lg4ff_range_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
 {
-	struct lg4ff_device_entry *uninitialized_var(entry);
-	struct list_head *h;
 	struct hid_device *hid = to_hid_device(dev);
+	struct lg4ff_device_entry *entry;
+	struct lg_drv_data *drv_data;
 	__u16 range = simple_strtoul(buf, NULL, 10);
 
-	list_for_each(h, &device_list.list) {
-		entry = list_entry(h, struct lg4ff_device_entry, list);
-		if (strcmp(entry->device_id, (&hid->dev)->kobj.name) == 0)
-			break;
+	drv_data = hid_get_drvdata(hid);
+	if (!drv_data) {
+		hid_err(hid, "Private driver data not found!\n");
+		return 0;
 	}
-	if (h == &device_list.list) {
-		dbg_hid("Device not found!");
-		return count;
+
+	entry = drv_data->device_props;
+	if (!entry) {
+		hid_err(hid, "Device properties not found!\n");
+		return 0;
 	}
 
 	if (range == 0)
@@ -344,6 +343,7 @@ int lg4ff_init(struct hid_device *hid)
 	struct hid_report *report;
 	struct hid_field *field;
 	struct lg4ff_device_entry *entry;
+	struct lg_drv_data *drv_data;
 	struct usb_device_descriptor *udesc;
 	int error, i, j;
 	__u16 bcdDevice, rev_maj, rev_min;
@@ -423,28 +423,24 @@ int lg4ff_init(struct hid_device *hid)
 		dev->ff->set_autocenter(dev, 0);
 	}
 
-		/* Initialize device_list if this is the first device to handle by lg4ff */
-	if (!list_inited) {
-		INIT_LIST_HEAD(&device_list.list);
-		list_inited = 1;
+	/* Get private driver data */
+	drv_data = hid_get_drvdata(hid);
+	if (!drv_data) {
+		hid_err(hid, "Cannot add device, private driver data not allocated\n");
+		return -1;
 	}
 
-	/* Add the device to device_list */
+	/* Initialize device properties */
 	entry = kzalloc(sizeof(struct lg4ff_device_entry), GFP_KERNEL);
 	if (!entry) {
-		hid_err(hid, "Cannot add device, insufficient memory.\n");
-		return -ENOMEM;
-	}
-	entry->device_id = kstrdup((&hid->dev)->kobj.name, GFP_KERNEL);
-	if (!entry->device_id) {
-		hid_err(hid, "Cannot set device_id, insufficient memory.\n");
-		kfree(entry);
+		hid_err(hid, "Cannot add device, insufficient memory to allocate device properties.\n");
 		return -ENOMEM;
 	}
+	drv_data->device_props = entry;
+
 	entry->min_range = lg4ff_devices[i].min_range;
 	entry->max_range = lg4ff_devices[i].max_range;
 	entry->set_range = lg4ff_devices[i].set_range;
-	list_add(&entry->list, &device_list.list);
 
 	/* Create sysfs interface */
 	error = device_create_file(&hid->dev, &dev_attr_range);
@@ -463,27 +459,23 @@ int lg4ff_init(struct hid_device *hid)
 
 int lg4ff_deinit(struct hid_device *hid)
 {
-	bool found = 0;
 	struct lg4ff_device_entry *entry;
-	struct list_head *h, *g;
-	
+	struct lg_drv_data *drv_data;
+
 	device_remove_file(&hid->dev, &dev_attr_range);
 
-	list_for_each_safe(h, g, &device_list.list) {
-		entry = list_entry(h, struct lg4ff_device_entry, list);
-		if (strcmp(entry->device_id, (&hid->dev)->kobj.name) == 0) {
-			list_del(h);
-			kfree(entry->device_id);
-			kfree(entry);
-			found = 1;
-			break;
-		}
+	drv_data = hid_get_drvdata(hid);
+	if (!drv_data) {
+		hid_err(hid, "Error while deinitializing device, no private driver data.\n");
+		return -1;
 	}
-
-	if (!found) {
-		hid_err(hid, "Device entry not found!\n");
+	entry = drv_data->device_props;
+	if (!entry) {
+		hid_err(hid, "Error while deinitializing device, no device properties data.\n");
 		return -1;
 	}
+	/* Deallocate memory */
+	kfree(entry);
 
 	dbg_hid("Device successfully unregistered\n");
 	return 0;
-- 
1.7.9.6


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH v2] HID: lg4ff: Take advantage of private driver data

[simon]

I tested this against a G27 and WiiWheel and it appears to function
correctly.

I'll shortly send another patch on top of this which enables the LEDs on
the G27. Hopefully this will bring to a close this short series of
patches.

Simon.


> Please note that this patch depends on
> "[PATCH v4] HID: hid-lg: Allow for custom device-specific properties to be
> stored in private drv data"
> "[PATCH v4] HID: lg4ff: Remove sysfs iface before deallocating memory"
>
> Signed-off-by: Michal Malý <madcatxster@gmail.com>

tested-by: Simon Wood <simon@mungewell.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] HID: hid-lg4ff: Add support for G27 LEDs

[Simon Wood]

This patch adds supports for controlling the LED 'tachometer' on
the G27 wheel, via the LED subsystem.

The 5 LEDs are arranged from right (1=grn, 2=grn, 3=yel, 4=yel, 5=red)
and 'mirrored' to the left (10 LEDs in total).

Signed-off-by: Simon Wood <simon@mungewell.org>
---
 drivers/hid/hid-lg4ff.c |  149 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 148 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index 32c173f..8960347 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -55,7 +55,8 @@ struct lg4ff_device_entry {
 	__u16 range;
 	__u16 min_range;
 	__u16 max_range;
-	__u8  leds;
+	__u8  led_state;
+	struct led_classdev *led[5];
 	struct list_head list;
 	void (*set_range)(struct hid_device *hid, u16 range);
 };
@@ -335,6 +336,86 @@ static ssize_t lg4ff_range_store(struct device *dev, struct device_attribute *at
 	return count;
 }
 
+static void lg4ff_set_leds(struct hid_device *hid, __u8 leds)
+{
+	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
+	struct hid_report *report = list_entry(report_list->next, struct hid_report, list);
+
+	report->field[0]->value[0] = 0xf8;
+	report->field[0]->value[1] = 0x12;
+	report->field[0]->value[2] = leds;
+	report->field[0]->value[3] = 0x00;
+	report->field[0]->value[4] = 0x00;
+	report->field[0]->value[5] = 0x00;
+	report->field[0]->value[6] = 0x00;
+	usbhid_submit_report(hid, report, USB_DIR_OUT);
+}
+
+static void lg4ff_led_set_brightness(struct led_classdev *led_cdev,
+			enum led_brightness value)
+{
+	struct device *dev = led_cdev->dev->parent;
+	struct hid_device *hid = container_of(dev, struct hid_device, dev);
+	struct lg_drv_data *drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);
+	struct lg4ff_device_entry *entry;
+	int i, state = 0;
+
+	if (!drv_data) {
+		hid_err(hid, "Device data not found.");
+		return;
+	}
+
+	entry = (struct lg4ff_device_entry *)drv_data->device_props;
+
+	if (!entry) {
+		hid_err(hid, "Device properties not found.");
+		return;
+	}
+
+	for (i = 0; i < 5; i++) {
+		if (led_cdev != entry->led[i])
+			continue;
+		state = (entry->led_state >> i) & 1;
+		if (value == LED_OFF && state) {
+			entry->led_state &= ~(1 << i);
+			lg4ff_set_leds(hid, entry->led_state);
+		} else if (value != LED_OFF && !state) {
+			entry->led_state |= 1 << i;
+			lg4ff_set_leds(hid, entry->led_state);
+		}
+		break;
+	}
+}
+
+static enum led_brightness lg4ff_led_get_brightness(struct led_classdev *led_cdev)
+{
+	struct device *dev = led_cdev->dev->parent;
+	struct hid_device *hid = container_of(dev, struct hid_device, dev);
+	struct lg_drv_data *drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);
+	struct lg4ff_device_entry *entry;
+	int i, value = 0;
+
+	if (!drv_data) {
+		hid_err(hid, "Device data not found.");
+		return LED_OFF;
+	}
+
+	entry = (struct lg4ff_device_entry *)drv_data->device_props;
+
+	if (!entry) {
+		hid_err(hid, "Device properties not found.");
+		return LED_OFF;
+	}
+
+	for (i = 0; i < 5; i++)
+		if (led_cdev == entry->led[i]) {
+			value = (entry->led_state >> i) & 1;
+			break;
+		}
+
+	return value ? LED_FULL : LED_OFF;
+}
+
 int lg4ff_init(struct hid_device *hid)
 {
 	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
@@ -347,6 +428,9 @@ int lg4ff_init(struct hid_device *hid)
 	struct usb_device_descriptor *udesc;
 	int error, i, j;
 	__u16 bcdDevice, rev_maj, rev_min;
+	struct led_classdev *led;
+	size_t name_sz;
+	char *name;
 
 	/* Find the report to use */
 	if (list_empty(report_list)) {
@@ -453,14 +537,66 @@ int lg4ff_init(struct hid_device *hid)
 	if (entry->set_range != NULL)
 		entry->set_range(hid, entry->range);
 
+	/* register led subsystem - G27 only */
+	entry->led_state = 0;
+	for (j = 0; j < 5; j++)
+		entry->led[j] = NULL;
+
+	if (lg4ff_devices[i].product_id == USB_DEVICE_ID_LOGITECH_G27_WHEEL) {
+		lg4ff_set_leds(hid, 0);
+
+		name_sz = strlen(dev_name(&hid->dev)) + 8;
+
+		for (j = 0; j < 5; j++) {
+			led = kzalloc(sizeof(struct led_classdev)+name_sz, GFP_KERNEL);
+			if (!led) {
+				hid_err(hid, "can't allocate memory for LED %d\n", j);
+				error = -ENOMEM;
+				goto err;
+			}
+
+			name = (void *)(&led[1]);
+			snprintf(name, name_sz, "%s::RPM%d", dev_name(&hid->dev), j+1);
+			led->name = name;
+			led->brightness = 0;
+			led->max_brightness = 1;
+			led->brightness_get = lg4ff_led_get_brightness;
+			led->brightness_set = lg4ff_led_set_brightness;
+
+			entry->led[j] = led;
+			error = led_classdev_register(&hid->dev, led);
+			if (error) {
+				hid_err(hid, "failed to register LED %d. Aborting.\n", j);
+				goto err;
+			}
+		}
+
+		dbg_hid("sysfs interface created for leds\n");
+	}
+
 	hid_info(hid, "Force feedback for Logitech Speed Force Wireless by Simon Wood <simon@mungewell.org>\n");
 	return 0;
+
+err:
+	/* Deregister LEDs (if any) but let the driver continue */
+	for (j = 0; j < 5; j++) {
+		led = entry->led[j];
+		entry->led[j] = NULL;
+		if (!led)
+			continue;
+		led_classdev_unregister(led);
+		kfree(led);
+	}
+
+	return 0;
 }
 
 int lg4ff_deinit(struct hid_device *hid)
 {
 	struct lg4ff_device_entry *entry;
 	struct lg_drv_data *drv_data;
+	int j;
+	struct led_classdev *led;
 
 	device_remove_file(&hid->dev, &dev_attr_range);
 
@@ -474,6 +610,17 @@ int lg4ff_deinit(struct hid_device *hid)
 		hid_err(hid, "Error while deinitializing device, no device properties data.\n");
 		return -1;
 	}
+
+	/* Deregister LEDs (if any) */
+	for (j = 0; j < 5; j++) {
+		led = entry->led[j];
+		entry->led[j] = NULL;
+		if (!led)
+			continue;
+		led_classdev_unregister(led);
+		kfree(led);
+	}
+
 	/* Deallocate memory */
 	kfree(entry);
 
-- 
1.7.4.1

Re: [PATCH] HID: hid-lg4ff: Add support for G27 LEDs

[Jiri Kosina]

On Mon, 9 Apr 2012, Simon Wood wrote:

> This patch adds supports for controlling the LED 'tachometer' on
> the G27 wheel, via the LED subsystem.
> 
> The 5 LEDs are arranged from right (1=grn, 2=grn, 3=yel, 4=yel, 5=red)
> and 'mirrored' to the left (10 LEDs in total).
> 
> Signed-off-by: Simon Wood <simon@mungewell.org>
> ---
>  drivers/hid/hid-lg4ff.c |  149 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 148 insertions(+), 1 deletions(-)

This patch introduces a clear dependency of hid-lg4ff driver on 
CONFIG_LEDS_CLASS, but the code doesn't reflect it.

So either you have to put #ifdef into source to compile the leds part out 
if leds subsystem is not enabled, or you have to introduce Kconfig rule to 
cover this.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2] HID: lg4ff: Take advantage of private driver data

[Jiri Kosina]

On Mon, 9 Apr 2012, Michal Malý wrote:

> Hi,
> 
> this is a revised version of HID: lg4ff: Use Private Data I asked Simon to
> submit for me earlier. To recap what the patch is about, it takes advantage
> of the changes introduced in my patches from 2011/04/02. lg4ff now calls
> hid_get/set_drvdata() to read or store device configuration. The way I understand
> it, this is how all HID drivers store their private data. The change from the version
> that was submitted earlier are removed all occurences of uninitialized_var() macro.
> 
> Please note that this patch depends on
> "[PATCH v4] HID: hid-lg: Allow for custom device-specific properties to be stored in private drv data"
> "[PATCH v4] HID: lg4ff: Remove sysfs iface before deallocating memory"
> 
> Signed-off-by: Michal Malý <madcatxster@gmail.com>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] HID: hid-lg4ff: Add support for G27 LEDs

[Simon Wood]

This patch adds supports for controlling the LED 'tachometer' on
the G27 wheel, via the LED subsystem.

The 5 LEDs are arranged from right (1=grn, 2=grn, 3=yel, 4=yel, 5=red)
and 'mirrored' to the left (10 LEDs in total).

Signed-off-by: Simon Wood <simon@mungewell.org>
---
 drivers/hid/hid-lg4ff.c |  155 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 154 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index 32c173f..ab5232e 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -55,7 +55,8 @@ struct lg4ff_device_entry {
 	__u16 range;
 	__u16 min_range;
 	__u16 max_range;
-	__u8  leds;
+	__u8  led_state;
+	struct led_classdev *led[5];
 	struct list_head list;
 	void (*set_range)(struct hid_device *hid, u16 range);
 };
@@ -335,6 +336,88 @@ static ssize_t lg4ff_range_store(struct device *dev, struct device_attribute *at
 	return count;
 }
 
+#ifdef CONFIG_LEDS_CLASS
+static void lg4ff_set_leds(struct hid_device *hid, __u8 leds)
+{
+	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
+	struct hid_report *report = list_entry(report_list->next, struct hid_report, list);
+
+	report->field[0]->value[0] = 0xf8;
+	report->field[0]->value[1] = 0x12;
+	report->field[0]->value[2] = leds;
+	report->field[0]->value[3] = 0x00;
+	report->field[0]->value[4] = 0x00;
+	report->field[0]->value[5] = 0x00;
+	report->field[0]->value[6] = 0x00;
+	usbhid_submit_report(hid, report, USB_DIR_OUT);
+}
+
+static void lg4ff_led_set_brightness(struct led_classdev *led_cdev,
+			enum led_brightness value)
+{
+	struct device *dev = led_cdev->dev->parent;
+	struct hid_device *hid = container_of(dev, struct hid_device, dev);
+	struct lg_drv_data *drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);
+	struct lg4ff_device_entry *entry;
+	int i, state = 0;
+
+	if (!drv_data) {
+		hid_err(hid, "Device data not found.");
+		return;
+	}
+
+	entry = (struct lg4ff_device_entry *)drv_data->device_props;
+
+	if (!entry) {
+		hid_err(hid, "Device properties not found.");
+		return;
+	}
+
+	for (i = 0; i < 5; i++) {
+		if (led_cdev != entry->led[i])
+			continue;
+		state = (entry->led_state >> i) & 1;
+		if (value == LED_OFF && state) {
+			entry->led_state &= ~(1 << i);
+			lg4ff_set_leds(hid, entry->led_state);
+		} else if (value != LED_OFF && !state) {
+			entry->led_state |= 1 << i;
+			lg4ff_set_leds(hid, entry->led_state);
+		}
+		break;
+	}
+}
+
+static enum led_brightness lg4ff_led_get_brightness(struct led_classdev *led_cdev)
+{
+	struct device *dev = led_cdev->dev->parent;
+	struct hid_device *hid = container_of(dev, struct hid_device, dev);
+	struct lg_drv_data *drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);
+	struct lg4ff_device_entry *entry;
+	int i, value = 0;
+
+	if (!drv_data) {
+		hid_err(hid, "Device data not found.");
+		return LED_OFF;
+	}
+
+	entry = (struct lg4ff_device_entry *)drv_data->device_props;
+
+	if (!entry) {
+		hid_err(hid, "Device properties not found.");
+		return LED_OFF;
+	}
+
+	for (i = 0; i < 5; i++)
+		if (led_cdev == entry->led[i]) {
+			value = (entry->led_state >> i) & 1;
+			break;
+		}
+
+	return value ? LED_FULL : LED_OFF;
+}
+#endif
+
 int lg4ff_init(struct hid_device *hid)
 {
 	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
@@ -453,6 +536,57 @@ int lg4ff_init(struct hid_device *hid)
 	if (entry->set_range != NULL)
 		entry->set_range(hid, entry->range);
 
+	/* register led subsystem - G27 only */
+	entry->led_state = 0;
+	for (j = 0; j < 5; j++)
+		entry->led[j] = NULL;
+
+#ifdef CONFIG_LEDS_CLASS
+	if (lg4ff_devices[i].product_id == USB_DEVICE_ID_LOGITECH_G27_WHEEL) {
+		struct led_classdev *led;
+		size_t name_sz;
+		char *name;
+
+		lg4ff_set_leds(hid, 0);
+
+		name_sz = strlen(dev_name(&hid->dev)) + 8;
+
+		for (j = 0; j < 5; j++) {
+			led = kzalloc(sizeof(struct led_classdev)+name_sz, GFP_KERNEL);
+			if (!led) {
+				hid_err(hid, "can't allocate memory for LED %d\n", j);
+				goto err;
+			}
+
+			name = (void *)(&led[1]);
+			snprintf(name, name_sz, "%s::RPM%d", dev_name(&hid->dev), j+1);
+			led->name = name;
+			led->brightness = 0;
+			led->max_brightness = 1;
+			led->brightness_get = lg4ff_led_get_brightness;
+			led->brightness_set = lg4ff_led_set_brightness;
+
+			entry->led[j] = led;
+			error = led_classdev_register(&hid->dev, led);
+
+			if (error) {
+				hid_err(hid, "failed to register LED %d. Aborting.\n", j);
+err:
+				/* Deregister LEDs (if any) but let the driver continue */
+				for (j = 0; j < 5; j++) {
+					led = entry->led[j];
+					entry->led[j] = NULL;
+					if (!led)
+						continue;
+					led_classdev_unregister(led);
+				kfree(led);
+				}
+				break;	/* Don't create any more LEDs */
+			}
+		}
+	}
+#endif
+
 	hid_info(hid, "Force feedback for Logitech Speed Force Wireless by Simon Wood <simon@mungewell.org>\n");
 	return 0;
 }
@@ -474,6 +608,25 @@ int lg4ff_deinit(struct hid_device *hid)
 		hid_err(hid, "Error while deinitializing device, no device properties data.\n");
 		return -1;
 	}
+
+#ifdef CONFIG_LEDS_CLASS
+	{
+		int j;
+		struct led_classdev *led;
+
+		/* Deregister LEDs (if any) */
+		for (j = 0; j < 5; j++) {
+
+			led = entry->led[j];
+			entry->led[j] = NULL;
+			if (!led)
+				continue;
+			led_classdev_unregister(led);
+			kfree(led);
+		}
+	}
+#endif
+
 	/* Deallocate memory */
 	kfree(entry);
 
-- 
1.7.4.1

[PATCH 2/2] HID: hid-lg4ff: Updated Comments

[Simon Wood]

Updated comments to say that this driver now supports all Logitech
gaming wheels, and not just the WiiWheel

Signed-off-by: Simon Wood <simon@mungewell.org>
---
 drivers/hid/hid-lg4ff.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index ab5232e..cbafb46 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -1,7 +1,8 @@
 /*
- *  Force feedback support for Logitech Speed Force Wireless
+ *  Force feedback support for Logitech Gaming Wheels
  *
- *  http://wiibrew.org/wiki/Logitech_USB_steering_wheel
+ *  Including G27, G25, DFP, DFGT, FFEX, Momo, Momo2 &
+ *  Wireless Speed Force (WiiWheel)
  *
  *  Copyright (c) 2010 Simon Wood <simon@mungewell.org>
  */
@@ -587,7 +588,7 @@ err:
 	}
 #endif
 
-	hid_info(hid, "Force feedback for Logitech Speed Force Wireless by Simon Wood <simon@mungewell.org>\n");
+	hid_info(hid, "Force feedback for Logitech Gaming Wheels\n");
 	return 0;
 }
 
-- 
1.7.4.1

Re: [PATCH 1/2] HID: hid-lg4ff: Add support for G27 LEDs

[Jiri Kosina]

On Wed, 18 Apr 2012, Simon Wood wrote:

> This patch adds supports for controlling the LED 'tachometer' on
> the G27 wheel, via the LED subsystem.
> 
> The 5 LEDs are arranged from right (1=grn, 2=grn, 3=yel, 4=yel, 5=red)
> and 'mirrored' to the left (10 LEDs in total).
> 
> Signed-off-by: Simon Wood <simon@mungewell.org>
> ---
>  drivers/hid/hid-lg4ff.c |  155 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 154 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
> index 32c173f..ab5232e 100644
> --- a/drivers/hid/hid-lg4ff.c
> +++ b/drivers/hid/hid-lg4ff.c
> @@ -55,7 +55,8 @@ struct lg4ff_device_entry {
>  	__u16 range;
>  	__u16 min_range;
>  	__u16 max_range;
> -	__u8  leds;
> +	__u8  led_state;
> +	struct led_classdev *led[5];

Why not put this under the CONFIG_LEDS_CLASS condition as well?

>  	struct list_head list;
>  	void (*set_range)(struct hid_device *hid, u16 range);
>  };
> @@ -335,6 +336,88 @@ static ssize_t lg4ff_range_store(struct device *dev, struct device_attribute *at
>  	return count;
>  }
>  
> +#ifdef CONFIG_LEDS_CLASS
> +static void lg4ff_set_leds(struct hid_device *hid, __u8 leds)
> +{
> +	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
> +	struct hid_report *report = list_entry(report_list->next, struct hid_report, list);
> +
> +	report->field[0]->value[0] = 0xf8;
> +	report->field[0]->value[1] = 0x12;
> +	report->field[0]->value[2] = leds;
> +	report->field[0]->value[3] = 0x00;
> +	report->field[0]->value[4] = 0x00;
> +	report->field[0]->value[5] = 0x00;
> +	report->field[0]->value[6] = 0x00;
> +	usbhid_submit_report(hid, report, USB_DIR_OUT);
> +}
> +
> +static void lg4ff_led_set_brightness(struct led_classdev *led_cdev,
> +			enum led_brightness value)
> +{
> +	struct device *dev = led_cdev->dev->parent;
> +	struct hid_device *hid = container_of(dev, struct hid_device, dev);
> +	struct lg_drv_data *drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);
> +	struct lg4ff_device_entry *entry;
> +	int i, state = 0;
> +
> +	if (!drv_data) {
> +		hid_err(hid, "Device data not found.");
> +		return;
> +	}
> +
> +	entry = (struct lg4ff_device_entry *)drv_data->device_props;
> +
> +	if (!entry) {
> +		hid_err(hid, "Device properties not found.");
> +		return;
> +	}
> +
> +	for (i = 0; i < 5; i++) {
> +		if (led_cdev != entry->led[i])
> +			continue;
> +		state = (entry->led_state >> i) & 1;
> +		if (value == LED_OFF && state) {
> +			entry->led_state &= ~(1 << i);
> +			lg4ff_set_leds(hid, entry->led_state);
> +		} else if (value != LED_OFF && !state) {
> +			entry->led_state |= 1 << i;
> +			lg4ff_set_leds(hid, entry->led_state);
> +		}
> +		break;
> +	}
> +}
> +
> +static enum led_brightness lg4ff_led_get_brightness(struct led_classdev *led_cdev)
> +{
> +	struct device *dev = led_cdev->dev->parent;
> +	struct hid_device *hid = container_of(dev, struct hid_device, dev);
> +	struct lg_drv_data *drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);
> +	struct lg4ff_device_entry *entry;
> +	int i, value = 0;
> +
> +	if (!drv_data) {
> +		hid_err(hid, "Device data not found.");
> +		return LED_OFF;
> +	}
> +
> +	entry = (struct lg4ff_device_entry *)drv_data->device_props;
> +
> +	if (!entry) {
> +		hid_err(hid, "Device properties not found.");
> +		return LED_OFF;
> +	}
> +
> +	for (i = 0; i < 5; i++)
> +		if (led_cdev == entry->led[i]) {
> +			value = (entry->led_state >> i) & 1;
> +			break;
> +		}
> +
> +	return value ? LED_FULL : LED_OFF;
> +}
> +#endif
> +
>  int lg4ff_init(struct hid_device *hid)
>  {
>  	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
> @@ -453,6 +536,57 @@ int lg4ff_init(struct hid_device *hid)
>  	if (entry->set_range != NULL)
>  		entry->set_range(hid, entry->range);
>  
> +	/* register led subsystem - G27 only */
> +	entry->led_state = 0;
> +	for (j = 0; j < 5; j++)
> +		entry->led[j] = NULL;
> +

The same here.

> +#ifdef CONFIG_LEDS_CLASS
> +	if (lg4ff_devices[i].product_id == USB_DEVICE_ID_LOGITECH_G27_WHEEL) {
> +		struct led_classdev *led;
> +		size_t name_sz;
> +		char *name;
> +
> +		lg4ff_set_leds(hid, 0);
> +
> +		name_sz = strlen(dev_name(&hid->dev)) + 8;
> +
> +		for (j = 0; j < 5; j++) {
> +			led = kzalloc(sizeof(struct led_classdev)+name_sz, GFP_KERNEL);
> +			if (!led) {
> +				hid_err(hid, "can't allocate memory for LED %d\n", j);
> +				goto err;
> +			}
> +
> +			name = (void *)(&led[1]);
> +			snprintf(name, name_sz, "%s::RPM%d", dev_name(&hid->dev), j+1);
> +			led->name = name;
> +			led->brightness = 0;
> +			led->max_brightness = 1;
> +			led->brightness_get = lg4ff_led_get_brightness;
> +			led->brightness_set = lg4ff_led_set_brightness;
> +
> +			entry->led[j] = led;
> +			error = led_classdev_register(&hid->dev, led);
> +
> +			if (error) {
> +				hid_err(hid, "failed to register LED %d. Aborting.\n", j);
> +err:
> +				/* Deregister LEDs (if any) but let the driver continue */
> +				for (j = 0; j < 5; j++) {
> +					led = entry->led[j];
> +					entry->led[j] = NULL;
> +					if (!led)
> +						continue;
> +					led_classdev_unregister(led);
> +				kfree(led);

Indentation seems to be wrong here.

> +				}
> +				break;	/* Don't create any more LEDs */

Putting the whole err: label outside the outer for cycle would make the 
whole thing much more readable (and would save you the break). Could you 
please redo this?

> +			}
> +		}
> +	}
> +#endif
> +
>  	hid_info(hid, "Force feedback for Logitech Speed Force Wireless by Simon Wood <simon@mungewell.org>\n");
>  	return 0;
>  }
> @@ -474,6 +608,25 @@ int lg4ff_deinit(struct hid_device *hid)
>  		hid_err(hid, "Error while deinitializing device, no device properties data.\n");
>  		return -1;
>  	}
> +
> +#ifdef CONFIG_LEDS_CLASS
> +	{
> +		int j;
> +		struct led_classdev *led;
> +
> +		/* Deregister LEDs (if any) */
> +		for (j = 0; j < 5; j++) {
> +
> +			led = entry->led[j];
> +			entry->led[j] = NULL;
> +			if (!led)
> +				continue;
> +			led_classdev_unregister(led);
> +			kfree(led);
> +		}
> +	}
> +#endif
> +
>  	/* Deallocate memory */
>  	kfree(entry);
>  

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] HID: hid-lg4ff: Add support for G27 LEDs

[simon]

>> +	__u8  led_state;
>> +	struct led_classdev *led[5];
>
> Why not put this under the CONFIG_LEDS_CLASS condition as well?

Can do, I don't think these are referenced anywhere else.

>> +	/* register led subsystem - G27 only */
>> +	entry->led_state = 0;
>> +	for (j = 0; j < 5; j++)
>> +		entry->led[j] = NULL;
>> +
>
> The same here.

OK.

>> +			if (error) {
>> +				hid_err(hid, "failed to register LED %d. Aborting.\n", j);
>> +err:
>> +				/* Deregister LEDs (if any) but let the driver continue */
>> +				for (j = 0; j < 5; j++) {
>> +					led = entry->led[j];
>> +					entry->led[j] = NULL;
>> +					if (!led)
>> +						continue;
>> +					led_classdev_unregister(led);
>> +				kfree(led);
>
> Indentation seems to be wrong here.

Bugger...

>
>> +				}
>> +				break;	/* Don't create any more LEDs */
>
> Putting the whole err: label outside the outer for cycle would make the
> whole thing much more readable (and would save you the break). Could you
> please redo this?

The 'break' is actually not needed as 'j' has reached maximum value and
the outer for loop would be at completion. I put the 'break' there just to
highlight this fact.

Maybe just a comment would be OK, such as 'on error fall though to driver
completion'.

The reason I'd like this code here is that it seems traditional to output
the following 'hid_info' line once the driver is fully active.

>>  	hid_info(hid, "Force feedback for Logitech Speed Force Wireless by
>> Simon Wood <simon@mungewell.org>\n");

Moving the 'err:' code would have to:
1). Jump back to hit this hid_info line
2). Duplicate the hid_info line
3). Have err code called as a function

Any major objections to keeping it as is?


Man, it seems to be taking me ages to get this code right....
Simon

Re: [PATCH 1/2] HID: hid-lg4ff: Add support for G27 LEDs

[Jiri Kosina]

On Wed, 18 Apr 2012, simon@mungewell.org wrote:

> > Putting the whole err: label outside the outer for cycle would make the
> > whole thing much more readable (and would save you the break). Could you
> > please redo this?
> 
> The 'break' is actually not needed as 'j' has reached maximum value and
> the outer for loop would be at completion. 

Ah, right, I have even missed this fact, that you are using the same index 
in the inner loop as in the outter loop.

Quite unreadable indeed :)

> I put the 'break' there just to highlight this fact.
> 
> Maybe just a comment would be OK, such as 'on error fall though to driver
> completion'.
> 
> The reason I'd like this code here is that it seems traditional to output
> the following 'hid_info' line once the driver is fully active.
> 
> >>  	hid_info(hid, "Force feedback for Logitech Speed Force Wireless by
> >> Simon Wood <simon@mungewell.org>\n");
> 
> Moving the 'err:' code would have to:
> 1). Jump back to hit this hid_info line
> 2). Duplicate the hid_info line
> 3). Have err code called as a function

I am not getting it. Why can't you do just something like


	for ( .... ) {
		/* outter loop */
		if (error) {
			for ( ... ) {
				/* inner loop */
			}
			goto out;
		}
	}
out:
	hid_info(....);
	return;

-- 
Jiri Kosina
SUSE Labs

[PATCH 1/2] HID: hid-lg4ff: Add support for G27 LEDs

[Simon Wood]

This patch adds supports for controlling the LED 'tachometer' on
the G27 wheel, via the LED subsystem.

The 5 LEDs are arranged from right (1=grn, 2=grn, 3=yel, 4=yel, 5=red)
and 'mirrored' to the left (10 LEDs in total).

Signed-off-by: Simon Wood <simon@mungewell.org>
---
 drivers/hid/hid-lg4ff.c |  158 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 157 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index 32c173f..1e942a0 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -55,7 +55,10 @@ struct lg4ff_device_entry {
 	__u16 range;
 	__u16 min_range;
 	__u16 max_range;
-	__u8  leds;
+#ifdef CONFIG_LEDS_CLASS
+	__u8  led_state;
+	struct led_classdev *led[5];
+#endif
 	struct list_head list;
 	void (*set_range)(struct hid_device *hid, u16 range);
 };
@@ -335,6 +338,88 @@ static ssize_t lg4ff_range_store(struct device *dev, struct device_attribute *at
 	return count;
 }
 
+#ifdef CONFIG_LEDS_CLASS
+static void lg4ff_set_leds(struct hid_device *hid, __u8 leds)
+{
+	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
+	struct hid_report *report = list_entry(report_list->next, struct hid_report, list);
+
+	report->field[0]->value[0] = 0xf8;
+	report->field[0]->value[1] = 0x12;
+	report->field[0]->value[2] = leds;
+	report->field[0]->value[3] = 0x00;
+	report->field[0]->value[4] = 0x00;
+	report->field[0]->value[5] = 0x00;
+	report->field[0]->value[6] = 0x00;
+	usbhid_submit_report(hid, report, USB_DIR_OUT);
+}
+
+static void lg4ff_led_set_brightness(struct led_classdev *led_cdev,
+			enum led_brightness value)
+{
+	struct device *dev = led_cdev->dev->parent;
+	struct hid_device *hid = container_of(dev, struct hid_device, dev);
+	struct lg_drv_data *drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);
+	struct lg4ff_device_entry *entry;
+	int i, state = 0;
+
+	if (!drv_data) {
+		hid_err(hid, "Device data not found.");
+		return;
+	}
+
+	entry = (struct lg4ff_device_entry *)drv_data->device_props;
+
+	if (!entry) {
+		hid_err(hid, "Device properties not found.");
+		return;
+	}
+
+	for (i = 0; i < 5; i++) {
+		if (led_cdev != entry->led[i])
+			continue;
+		state = (entry->led_state >> i) & 1;
+		if (value == LED_OFF && state) {
+			entry->led_state &= ~(1 << i);
+			lg4ff_set_leds(hid, entry->led_state);
+		} else if (value != LED_OFF && !state) {
+			entry->led_state |= 1 << i;
+			lg4ff_set_leds(hid, entry->led_state);
+		}
+		break;
+	}
+}
+
+static enum led_brightness lg4ff_led_get_brightness(struct led_classdev *led_cdev)
+{
+	struct device *dev = led_cdev->dev->parent;
+	struct hid_device *hid = container_of(dev, struct hid_device, dev);
+	struct lg_drv_data *drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);
+	struct lg4ff_device_entry *entry;
+	int i, value = 0;
+
+	if (!drv_data) {
+		hid_err(hid, "Device data not found.");
+		return LED_OFF;
+	}
+
+	entry = (struct lg4ff_device_entry *)drv_data->device_props;
+
+	if (!entry) {
+		hid_err(hid, "Device properties not found.");
+		return LED_OFF;
+	}
+
+	for (i = 0; i < 5; i++)
+		if (led_cdev == entry->led[i]) {
+			value = (entry->led_state >> i) & 1;
+			break;
+		}
+
+	return value ? LED_FULL : LED_OFF;
+}
+#endif
+
 int lg4ff_init(struct hid_device *hid)
 {
 	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
@@ -453,6 +538,58 @@ int lg4ff_init(struct hid_device *hid)
 	if (entry->set_range != NULL)
 		entry->set_range(hid, entry->range);
 
+#ifdef CONFIG_LEDS_CLASS
+	/* register led subsystem - G27 only */
+	entry->led_state = 0;
+	for (j = 0; j < 5; j++)
+		entry->led[j] = NULL;
+
+	if (lg4ff_devices[i].product_id == USB_DEVICE_ID_LOGITECH_G27_WHEEL) {
+		struct led_classdev *led;
+		size_t name_sz;
+		char *name;
+
+		lg4ff_set_leds(hid, 0);
+
+		name_sz = strlen(dev_name(&hid->dev)) + 8;
+
+		for (j = 0; j < 5; j++) {
+			led = kzalloc(sizeof(struct led_classdev)+name_sz, GFP_KERNEL);
+			if (!led) {
+				hid_err(hid, "can't allocate memory for LED %d\n", j);
+				goto err;
+			}
+
+			name = (void *)(&led[1]);
+			snprintf(name, name_sz, "%s::RPM%d", dev_name(&hid->dev), j+1);
+			led->name = name;
+			led->brightness = 0;
+			led->max_brightness = 1;
+			led->brightness_get = lg4ff_led_get_brightness;
+			led->brightness_set = lg4ff_led_set_brightness;
+
+			entry->led[j] = led;
+			error = led_classdev_register(&hid->dev, led);
+
+			if (error) {
+				hid_err(hid, "failed to register LED %d. Aborting.\n", j);
+err:
+				/* Deregister LEDs (if any) */
+				for (j = 0; j < 5; j++) {
+					led = entry->led[j];
+					entry->led[j] = NULL;
+					if (!led)
+						continue;
+					led_classdev_unregister(led);
+					kfree(led);
+				}
+				goto out;	/* Let the driver continue without LEDs */
+			}
+		}
+	}
+#endif
+
+out:
 	hid_info(hid, "Force feedback for Logitech Speed Force Wireless by Simon Wood <simon@mungewell.org>\n");
 	return 0;
 }
@@ -474,6 +611,25 @@ int lg4ff_deinit(struct hid_device *hid)
 		hid_err(hid, "Error while deinitializing device, no device properties data.\n");
 		return -1;
 	}
+
+#ifdef CONFIG_LEDS_CLASS
+	{
+		int j;
+		struct led_classdev *led;
+
+		/* Deregister LEDs (if any) */
+		for (j = 0; j < 5; j++) {
+
+			led = entry->led[j];
+			entry->led[j] = NULL;
+			if (!led)
+				continue;
+			led_classdev_unregister(led);
+			kfree(led);
+		}
+	}
+#endif
+
 	/* Deallocate memory */
 	kfree(entry);
 
-- 
1.7.4.1

[PATCH 2/2] HID: hid-lg4ff: Updated Comments

[Simon Wood]

Updated comments to say that this driver now supports all Logitech
gaming wheels, and not just the WiiWheel.

Signed-off-by: Simon Wood <simon@mungewell.org>
---
 drivers/hid/hid-lg4ff.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index 1e942a0..24cf037 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -1,7 +1,8 @@
 /*
- *  Force feedback support for Logitech Speed Force Wireless
+ *  Force feedback support for Logitech Gaming Wheels
  *
- *  http://wiibrew.org/wiki/Logitech_USB_steering_wheel
+ *  Including G27, G25, DFP, DFGT, FFEX, Momo, Momo2 &
+ *  Speed Force Wireless (WiiWheel)
  *
  *  Copyright (c) 2010 Simon Wood <simon@mungewell.org>
  */
@@ -590,7 +591,7 @@ err:
 #endif
 
 out:
-	hid_info(hid, "Force feedback for Logitech Speed Force Wireless by Simon Wood <simon@mungewell.org>\n");
+	hid_info(hid, "Force feedback support for Logitech Gaming Wheels\n");
 	return 0;
 }
 
-- 
1.7.4.1

Re: [PATCH 1/2] HID: hid-lg4ff: Add support for G27 LEDs

[Jiri Kosina]

On Sat, 21 Apr 2012, Simon Wood wrote:

> This patch adds supports for controlling the LED 'tachometer' on
> the G27 wheel, via the LED subsystem.
> 
> The 5 LEDs are arranged from right (1=grn, 2=grn, 3=yel, 4=yel, 5=red)
> and 'mirrored' to the left (10 LEDs in total).
> 
> Signed-off-by: Simon Wood <simon@mungewell.org>

Both patches applied, thanks.

-- 
Jiri Kosina
SUSE Labs