Re: [PATCH 4/6] HID: sony: Add blink support to the LEDs

[Frank Praznik <frank.praznik@gmail.com>]

On 3/1/2014 09:20, Antonio Ospite wrote:
> Hi Frank,
>
> On Fri, 28 Feb 2014 22:58:59 -0500
> Frank Praznik <frank.praznik@oh.rr.com> wrote:
>
>> Add support for setting the blink rate of the LEDs.  The Sixaxis allows control
>> over each individual LED, but the Dualshock 4 only has one global control for
>> the light bar so changing any individual color changes them all.
>>
>> Setting the brightness cancels the blinking as per the LED class
>> specifications.
>>
>> The Sixaxis and Dualshock 4 controllers accept delays in decisecond increments
>> from 0 to 255 (2550 milliseconds).
>>
>> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
>> ---
>>   drivers/hid/hid-sony.c | 105 +++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 93 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index dc6e6fa..914a6cc 100644
>> --- a/drivers/hid/hid-sony.c
>> +++ b/drivers/hid/hid-sony.c
>> @@ -741,6 +741,8 @@ struct sony_sc {
>>   	__u8 battery_charging;
>>   	__u8 battery_capacity;
>>   	__u8 led_state[MAX_LEDS];
>> +	__u8 led_blink_on[MAX_LEDS];
>> +	__u8 led_blink_off[MAX_LEDS];
> Are those values meant to be for the duty cycle in deciseconds?
> What about using a more explicative name? leds_blink_on makes me think
> to something boolean (it could be just me), maybe leds_delay_on and
> leds_delay_off?
>
> Also grouping spare arrays into a single array of structs may be worth
> considering:
>
> struct sony_sc {
> 	...
> 	struct {
> 		struct led_classdev *ldev;
> 		__u8 state;
> 		__u8 delay_on;
> 		__u8 delay_off;
> 	} leds[MAX_LEDS];
> 	...
> };
>
> Defining the struct for leds separately if you prefer so.
It would be a little more organized, but it would also add extra padding 
to the struct.  I'll think about this one.
>
>>   	__u8 led_count;
>>   };
>>   
>> @@ -1127,8 +1129,7 @@ static void sony_led_set_brightness(struct led_classdev *led,
>>   	struct device *dev = led->dev->parent;
>>   	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>>   	struct sony_sc *drv_data;
>> -
>> -	int n;
>> +	int n, blink_index = 0;
>>   
>>   	drv_data = hid_get_drvdata(hdev);
>>   	if (!drv_data) {
>> @@ -1136,14 +1137,30 @@ static void sony_led_set_brightness(struct led_classdev *led,
>>   		return;
>>   	}
>>   
>> +	/* Get the index of the LED */
>>   	for (n = 0; n < drv_data->led_count; n++) {
>> -		if (led == drv_data->leds[n]) {
>> -			if (value != drv_data->led_state[n]) {
>> -				drv_data->led_state[n] = value;
>> -				sony_set_leds(drv_data, drv_data->led_state, drv_data->led_count);
>> -			}
>> +		if (led == drv_data->leds[n])
>>   			break;
>> -		}
>> +	}
>> +
>> +	/* This LED is not registered on this device */
>> +	if (n >= drv_data->led_count)
>> +		return;
>> +
>> +	/* The DualShock 4 has a global LED and always uses index 0 */
>> +	if (!(drv_data->quirks & DUALSHOCK4_CONTROLLER))
>> +		blink_index = n;
>> +
> If you feel the need for a comment here, what about not initializing
> blink_index to 0 before and add an else block here, this way the code
> itself is more explicit, and more symmetric too.
Good idea, I'll fix it in v2.
>
>> +	if ((value != drv_data->led_state[n])   ||
>> +		drv_data->led_blink_on[blink_index] ||
>> +		drv_data->led_blink_off[blink_index]) {
>> +		drv_data->led_state[n] = value;
>> +
>> +		/* Setting the brightness stops the blinking */
>> +		drv_data->led_blink_on[blink_index] = 0;
>> +		drv_data->led_blink_off[blink_index] = 0;
>> +
>> +		sony_set_leds(drv_data, drv_data->led_state, drv_data->led_count);
>>   	}
>>   }
>>   
>> @@ -1169,6 +1186,56 @@ static enum led_brightness sony_led_get_brightness(struct led_classdev *led)
>>   	return LED_OFF;
>>   }
>>   
>> +static int sony_blink_set(struct led_classdev *led, unsigned long *delay_on,
>> +				unsigned long *delay_off)
>> +{
>> +	struct device *dev = led->dev->parent;
>> +	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> +	struct sony_sc *drv_data = hid_get_drvdata(hdev);
>> +	int n = 0;
>> +	__u8 new_on, new_off;
>> +
>> +	if (!drv_data) {
>> +		hid_err(hdev, "No device data\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Max delay is 255 deciseconds or 2550 milliseconds */
>> +	if (*delay_on > 2550)
>> +		*delay_on = 2550;
>> +	if (*delay_off > 2550)
>> +		*delay_off = 2550;
>> +
>> +	/* Blink at 1 Hz if both values are zero */
>> +	if (!*delay_on && !*delay_off)
>> +		*delay_on = *delay_off = 1000;
>> +
>> +	new_on = *delay_on / 10;
>> +	new_off = *delay_off / 10;
>> +
>> +	/* The blink controls are global on the DualShock 4 */
>> +	if (!(drv_data->quirks & DUALSHOCK4_CONTROLLER)) {
>> +		for (n = 0; n < drv_data->led_count; n++) {
>> +			if (led == drv_data->leds[n])
>> +				break;
>> +		}
>> +	}
>> +
>> +	/* This LED is not registered on this device */
>> +	if (n >= drv_data->led_count)
>> +		return -EINVAL;
>> +
>> +	/* Don't schedule work if the values didn't change */
>> +	if (new_on != drv_data->led_blink_on[n] ||
>> +		new_off != drv_data->led_blink_off[n]) {
>> +		drv_data->led_blink_on[n] = new_on;
>> +		drv_data->led_blink_off[n] = new_off;
>> +		schedule_work(&drv_data->state_worker);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static void sony_leds_remove(struct sony_sc *sc)
>>   {
>>   	struct led_classdev *led;
>> @@ -1259,6 +1326,9 @@ static int sony_leds_init(struct sony_sc *sc)
>>   		led->brightness_get = sony_led_get_brightness;
>>   		led->brightness_set = sony_led_set_brightness;
>>   
>> +		if (!(sc->quirks & BUZZ_CONTROLLER))
>> +			led->blink_set = sony_blink_set;
>> +
>>   		ret = led_classdev_register(&hdev->dev, led);
>>   		if (ret) {
>>   			hid_err(hdev, "Failed to register LED %d\n", n);
>> @@ -1280,14 +1350,15 @@ error_leds:
>>   static void sixaxis_state_worker(struct work_struct *work)
>>   {
>>   	struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
>> +	int n;
>>   	unsigned char buf[] = {
>>   		0x01,
>>   		0x00, 0xff, 0x00, 0xff, 0x00,
>>   		0x00, 0x00, 0x00, 0x00, 0x00,
>> -		0xff, 0x27, 0x10, 0x00, 0x32,
>> -		0xff, 0x27, 0x10, 0x00, 0x32,
>> -		0xff, 0x27, 0x10, 0x00, 0x32,
>> -		0xff, 0x27, 0x10, 0x00, 0x32,
>> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 4 */
>> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 3 */
>> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 2 */
>> +		0xff, 0x27, 0x10, 0x00, 0x32, /* LED 1 */
>>   		0x00, 0x00, 0x00, 0x00, 0x00
>>   	};
>>   
>> @@ -1301,6 +1372,13 @@ static void sixaxis_state_worker(struct work_struct *work)
>>   	buf[10] |= sc->led_state[2] << 3;
>>   	buf[10] |= sc->led_state[3] << 4;
>>   
>> +	for (n = 0; n < 4; n++) {
>> +		if (sc->led_blink_on[n] || sc->led_blink_off[n]) {
>> +			buf[29-(n*5)] = sc->led_blink_off[n];
>> +			buf[30-(n*5)] = sc->led_blink_on[n];
>                              ^^^^^^^^
> Kernel coding style prefers spaces around operators.
> I see that scripts/checkpatch.pl does not warn about this, but it's in
> Documentation/CodingStyle.
>
> However the calculations here made me wonder if it's the case to go
> semantic and represent the output report with a struct instead of an
> array (maybe even using a union), so you can access the individual
> fields in a more meaningful, and less bug prone, way.
>
> For example (untested):
>
> struct sixaxis_led {
> 	uint8_t time_enabled; /* the total time the led is active (0xff means forever) */
> 	uint8_t duty_length;   /* how long a cycle is in deciseconds (0 means "really fast") */
> 	uint8_t enabled;
> 	uint8_t duty_off; /* % of duty_length the led is off (0xff means 100%) */
> 	uint8_t duty_on; /* % of duty_length the led is on (0xff mean 100%) */
>
> } __attribute__ ((packed));
>
> struct sixaxis_output_report {
> 	uint8_t report_id;
> 	uint8_t rumble[5]; /* TODO: add the rumble bits here... */
> 	uint8_t padding[4];
> 	uint8_t leds_bitmap; /* bitmap of enabled LEDs: LED_1 = 0x02, LED_2 = 0x04, ... */
> 	struct sixaxis_led led[4]; /* LEDx at (4 - x), add a macro? */
> 	struct sixaxis_led _reserved; /* LED5, not actually soldered */
> }; __attribute__ ((packed));
>
> union output_report_01 {
> 	struct sixaxis_output_report data;
> 	uint8_t buf[36];
> };
>
> I had the snippet above buried somewhere and I don't remember where
> all the info came from.
Nice, this will be much clearer.  I'll tidy it up and add this as a 
separate refactor patch in v2.  Thanks for the snippet.
>> +		}
>> +	}
>> +
>>   	if (sc->quirks & SIXAXIS_CONTROLLER_USB)
>>   		hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
>>   	else
>> @@ -1338,6 +1416,9 @@ static void dualshock4_state_worker(struct work_struct *work)
>>   	buf[offset++] = sc->led_state[1];
>>   	buf[offset++] = sc->led_state[2];
>>   
>> +	buf[offset++] = sc->led_blink_on[0];
>> +	buf[offset++] = sc->led_blink_off[0];
>> +
>>   	if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
>>   		hid_hw_output_report(hdev, buf, 32);
>>   	else
>> -- 
>> 1.8.5.3
>>
>> --
>> 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
>