On Thu, 2010-03-18 at 15:26 +0000, Przemo Firszt wrote:
> + unsigned char speed;
Could you add some comments as to what values represent what speed?
> +static ssize_t wacom_store_speed(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device,
> dev);
> + int new_speed;
> +
> + sscanf(buf, "%1d", &new_speed);
You should check the retval of sscanf as well.
> + if (new_speed == 0 || new_speed == 1) {
> + wacom_poke(hdev, new_speed);
> + return strnlen(buf, PAGE_SIZE);
> + } else
> + return -EINVAL;
> +}
>
> +static DEVICE_ATTR(speed, S_IRUGO | S_IWUGO,
> + wacom_show_speed, wacom_store_speed);
If there's only 2 speeds available, and it's actually a boolean, I'd
rather you used a name like "high_speed".
Furthermore, wdata->speed doesn't look like it's initialised.
Did you test whether speed switching works after the device has been
started up?
Cheers
--
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
