Hi Antonio,
On Tuesday 06 May 2014 12:06:16 Antonio Ospite wrote:
> On recent kernels the hid-sony driver exposes leds class entries in
> sysfs for setting the Sixaxis LEDs, use this interface and fall back to
> hidraw in case using sysfs fails (e.g. on older hid-sony versions).
>
> Setting the LEDs via sysfs is the preferred way on newer kernels, the
> rationale behind that is:
>
> 1. the Sixaxis uses the same HID output report for setting both LEDs
> and rumble effects;
> 2. hid-sony remembers the state of LEDs in order to preserve them when
> setting rumble effects;
> 3. when the LEDs are set via hidraw hid-sony has no way to know the
> new LEDs state and thus can change the LEDs in an inconsistent way
> when setting rumble effects later.
> ---
> plugins/sixaxis.c | 77
> ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73
> insertions(+), 4 deletions(-)
>
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index 2b35d1c..446d499 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -185,10 +185,40 @@ static gboolean set_leds_hidraw(int fd, uint8_t
> leds_bitmap) return FALSE;
> }
>
> +static gboolean set_leds_sysfs(struct leds_data *data)
Make it return bool.
> +{
> + int i;
> +
> + for (i = 0; i < 4; i++) {
> + char path[PATH_MAX] = { 0 };
> + char buf[2] = { 0 };
> + int fd;
> + int ret;
> +
> + if (data->syspaths[i] == NULL)
if (!data->..)
> + return FALSE;
> +
> + snprintf(path, PATH_MAX, "%s/brightness", data->syspaths[i]);
> + fd = open(path, O_WRONLY);
> + if (fd < 0) {
> + error("Cannot open %s (%s)", path, strerror(errno));
Prefix error message with "sixaxis:"
> + return FALSE;
> + }
> +
> + /* i+1 because leds start from 1, LED0 is never used */
> + buf[0] = '0' + !!(data->bitmap & (1 << (i+1)));
Spaces around + in i+1.
> + ret = write(fd, buf, sizeof(buf));
> + close(fd);
> + if (ret != sizeof(buf))
> + return FALSE;
> + }
> +
> + return TRUE;
> +}
> +
> static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
> gpointer user_data)
> {
> - int fd;
> int i;
> int ret;
> struct leds_data *data = (struct leds_data *)user_data;
> @@ -199,9 +229,11 @@ static gboolean setup_leds(GIOChannel *channel,
> GIOCondition cond, if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> goto out;
>
> - fd = g_io_channel_unix_get_fd(channel);
> -
> - set_leds_hidraw(fd, data->bitmap);
> + ret = set_leds_sysfs(data);
> + if (ret == FALSE) {
if (!set_leds_sysfs())
> + int fd = g_io_channel_unix_get_fd(channel);
> + set_leds_hidraw(fd, data->bitmap);
> + }
>
> out:
> for (i = 0; i < 4; i++)
> @@ -324,6 +356,39 @@ static int get_js_number(struct udev_device *udevice)
> return number;
> }
>
> +static int get_leds_devices(struct udev_device *udevice, char *paths[4])
> +{
> + struct udev_list_entry *devices, *dev_list_entry;
> + struct udev_enumerate *enumerate;
> + struct udev_device *hid_parent;
> + int index = 0;
> + int ret;
> +
> + hid_parent = udev_device_get_parent_with_subsystem_devtype(udevice,
> + "hid", NULL);
> +
> + enumerate = udev_enumerate_new(udev_device_get_udev(udevice));
> + udev_enumerate_add_match_parent(enumerate, hid_parent);
> + udev_enumerate_add_match_subsystem(enumerate, "leds");
> + udev_enumerate_scan_devices(enumerate);
> + devices = udev_enumerate_get_list_entry(enumerate);
> +
> + if (devices == NULL) {
if (!devices)..
Also this check should follow devices assignment without extra empty line.
> + ret = FALSE;
> + goto out;
> + }
> +
> + udev_list_entry_foreach(dev_list_entry, devices) {
> + const char *syspath = udev_list_entry_get_name(dev_list_entry);
> + paths[index++] = strdup(syspath);
> + }
Is this guaranteed that this loop will iterate only 4 times? If yes please add
comment, if not break loop to avoid writing on random memory.
> +
> + ret = TRUE;
> +out:
> + udev_enumerate_unref(enumerate);
> + return ret;
> +}
> +
> static struct leds_data *get_leds_data(struct udev_device *udevice)
> {
> struct leds_data *data;
> @@ -344,6 +409,10 @@ static struct leds_data *get_leds_data(struct
> udev_device *udevice) if (ret == FALSE)
> goto err;
>
> + /* it's OK if this fails, set_leds_hidraw() will be used if
> + * any of the values in data->syspaths is not defined */
Multi line comment should be like that: (there is still some inconsistency
about that in BlueZ code, but lets keep new code right)
/*
* foo
* bar
*/
> + get_leds_devices(udevice, data->syspaths);
Since return code is not important in this function why not just make it
return void?
> +
> return data;
>
> err:
--
Szymon K. Janc
szymon.janc@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
