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

[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

[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