| Message ID | 1406225645-32141-7-git-send-email-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Benjamin, On Thu, Jul 24, 2014 at 02:14:01PM -0400, Benjamin Tissoires wrote: > Now that wacom is a hid driver, there is no point in having a separate > driver for bluetooth devices. > This patch prepares the common paths of Bluetooth devices in the > common wacom driver. > It also adds the sysfs file "speed" used by Bluetooth devices. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > --- > > new in v2 > > drivers/hid/wacom_sys.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++--- > drivers/hid/wacom_wac.h | 2 ++ > 2 files changed, 69 insertions(+), 3 deletions(-) > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > index d0d06b8..add76ec 100644 > --- a/drivers/hid/wacom_sys.c > +++ b/drivers/hid/wacom_sys.c > @@ -262,6 +262,12 @@ static int wacom_set_device_mode(struct hid_device *hdev, int report_id, > return error < 0 ? error : 0; > } > > +static int wacom_bt_query_tablet_data(struct hid_device *hdev, u8 speed, > + struct wacom_features *features) > +{ > + return 0; > +} > + > /* > * Switch the tablet into its most-capable mode. Wacom tablets are > * typically configured to power-up in a mode which sends mouse-like > @@ -272,6 +278,9 @@ static int wacom_set_device_mode(struct hid_device *hdev, int report_id, > static int wacom_query_tablet_data(struct hid_device *hdev, > struct wacom_features *features) > { > + if (hdev->bus == BUS_BLUETOOTH) > + return wacom_bt_query_tablet_data(hdev, 1, features); > + > if (features->device_type == BTN_TOOL_FINGER) { > if (features->type > TABLETPC) { > /* MT Tablet PC touch */ > @@ -890,6 +899,38 @@ static void wacom_destroy_battery(struct wacom *wacom) > } > } > > +static ssize_t wacom_show_speed(struct device *dev, > + struct device_attribute > + *attr, char *buf) > +{ > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > + struct wacom *wacom = hid_get_drvdata(hdev); > + > + return snprintf(buf, PAGE_SIZE, "%i\n", wacom->wacom_wac.bt_high_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); > + struct wacom *wacom = hid_get_drvdata(hdev); > + int new_speed; > + > + if (sscanf(buf, "%1d", &new_speed ) != 1) Checkpach is unhappy with ')' placement and I agree with it. > + return -EINVAL; kstrtou8? > + > + if (new_speed == 0 || new_speed == 1) { > + wacom_bt_query_tablet_data(hdev, new_speed, > + &wacom->wacom_wac.features); > + return strnlen(buf, PAGE_SIZE); This is weird. Normally you want to return count since you should refuse input with excessive data. > + } else > + return -EINVAL; Need braces on both branches. Thanks.
On Jul 25 2014 or thereabouts, Dmitry Torokhov wrote: > Hi Benjamin, > > On Thu, Jul 24, 2014 at 02:14:01PM -0400, Benjamin Tissoires wrote: > > Now that wacom is a hid driver, there is no point in having a separate > > driver for bluetooth devices. > > This patch prepares the common paths of Bluetooth devices in the > > common wacom driver. > > It also adds the sysfs file "speed" used by Bluetooth devices. > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > > > new in v2 > > > > drivers/hid/wacom_sys.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++--- > > drivers/hid/wacom_wac.h | 2 ++ > > 2 files changed, 69 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > > index d0d06b8..add76ec 100644 > > --- a/drivers/hid/wacom_sys.c > > +++ b/drivers/hid/wacom_sys.c > > @@ -262,6 +262,12 @@ static int wacom_set_device_mode(struct hid_device *hdev, int report_id, > > return error < 0 ? error : 0; > > } > > > > +static int wacom_bt_query_tablet_data(struct hid_device *hdev, u8 speed, > > + struct wacom_features *features) > > +{ > > + return 0; > > +} > > + > > /* > > * Switch the tablet into its most-capable mode. Wacom tablets are > > * typically configured to power-up in a mode which sends mouse-like > > @@ -272,6 +278,9 @@ static int wacom_set_device_mode(struct hid_device *hdev, int report_id, > > static int wacom_query_tablet_data(struct hid_device *hdev, > > struct wacom_features *features) > > { > > + if (hdev->bus == BUS_BLUETOOTH) > > + return wacom_bt_query_tablet_data(hdev, 1, features); > > + > > if (features->device_type == BTN_TOOL_FINGER) { > > if (features->type > TABLETPC) { > > /* MT Tablet PC touch */ > > @@ -890,6 +899,38 @@ static void wacom_destroy_battery(struct wacom *wacom) > > } > > } > > > > +static ssize_t wacom_show_speed(struct device *dev, > > + struct device_attribute > > + *attr, char *buf) > > +{ > > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > > + struct wacom *wacom = hid_get_drvdata(hdev); > > + > > + return snprintf(buf, PAGE_SIZE, "%i\n", wacom->wacom_wac.bt_high_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); > > + struct wacom *wacom = hid_get_drvdata(hdev); > > + int new_speed; > > + > > + if (sscanf(buf, "%1d", &new_speed ) != 1) > > Checkpach is unhappy with ')' placement and I agree with it. > ouch > > + return -EINVAL; > > kstrtou8? re-ouch > > > + > > + if (new_speed == 0 || new_speed == 1) { > > + wacom_bt_query_tablet_data(hdev, new_speed, > > + &wacom->wacom_wac.features); > > + return strnlen(buf, PAGE_SIZE); > > This is weird. Normally you want to return count since you should refuse > input with excessive data. indeed > > > + } else > > + return -EINVAL; > > Need braces on both branches. > Grmblmbl. I should not have blinded copied the code from one driver to one other. I will send a v3 of the rest of the series on top of your wacom branch, at some point next week. Other people can still try to find out other mistakes meanwhile ;) Thanks for the review. Cheers, Benjamin -- 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
On Fri, Jul 25, 2014 at 11:25:17PM -0400, Benjamin Tissoires wrote: > On Jul 25 2014 or thereabouts, Dmitry Torokhov wrote: > > Hi Benjamin, > > > > On Thu, Jul 24, 2014 at 02:14:01PM -0400, Benjamin Tissoires wrote: > > > Now that wacom is a hid driver, there is no point in having a separate > > > driver for bluetooth devices. > > > This patch prepares the common paths of Bluetooth devices in the > > > common wacom driver. > > > It also adds the sysfs file "speed" used by Bluetooth devices. > > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > --- > > > > > > new in v2 > > > > > > drivers/hid/wacom_sys.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++--- > > > drivers/hid/wacom_wac.h | 2 ++ > > > 2 files changed, 69 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > > > index d0d06b8..add76ec 100644 > > > --- a/drivers/hid/wacom_sys.c > > > +++ b/drivers/hid/wacom_sys.c > > > @@ -262,6 +262,12 @@ static int wacom_set_device_mode(struct hid_device *hdev, int report_id, > > > return error < 0 ? error : 0; > > > } > > > > > > +static int wacom_bt_query_tablet_data(struct hid_device *hdev, u8 speed, > > > + struct wacom_features *features) > > > +{ > > > + return 0; > > > +} > > > + > > > /* > > > * Switch the tablet into its most-capable mode. Wacom tablets are > > > * typically configured to power-up in a mode which sends mouse-like > > > @@ -272,6 +278,9 @@ static int wacom_set_device_mode(struct hid_device *hdev, int report_id, > > > static int wacom_query_tablet_data(struct hid_device *hdev, > > > struct wacom_features *features) > > > { > > > + if (hdev->bus == BUS_BLUETOOTH) > > > + return wacom_bt_query_tablet_data(hdev, 1, features); > > > + > > > if (features->device_type == BTN_TOOL_FINGER) { > > > if (features->type > TABLETPC) { > > > /* MT Tablet PC touch */ > > > @@ -890,6 +899,38 @@ static void wacom_destroy_battery(struct wacom *wacom) > > > } > > > } > > > > > > +static ssize_t wacom_show_speed(struct device *dev, > > > + struct device_attribute > > > + *attr, char *buf) > > > +{ > > > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > > > + struct wacom *wacom = hid_get_drvdata(hdev); > > > + > > > + return snprintf(buf, PAGE_SIZE, "%i\n", wacom->wacom_wac.bt_high_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); > > > + struct wacom *wacom = hid_get_drvdata(hdev); > > > + int new_speed; > > > + > > > + if (sscanf(buf, "%1d", &new_speed ) != 1) > > > > Checkpach is unhappy with ')' placement and I agree with it. > > > > ouch > > > > + return -EINVAL; > > > > kstrtou8? > > re-ouch > > > > > > + > > > + if (new_speed == 0 || new_speed == 1) { > > > + wacom_bt_query_tablet_data(hdev, new_speed, > > > + &wacom->wacom_wac.features); > > > + return strnlen(buf, PAGE_SIZE); > > > > This is weird. Normally you want to return count since you should refuse > > input with excessive data. > > indeed > > > > > > + } else > > > + return -EINVAL; > > > > Need braces on both branches. > > > > Grmblmbl. I should not have blinded copied the code from one driver to > one other. I will send a v3 of the rest of the series on top of your > wacom branch, at some point next week. Other people can still try to > find out other mistakes meanwhile ;) Great, will be waiting for it and then will merge everything into next for 3.17. Thanks.
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index d0d06b8..add76ec 100644 --- a/drivers/hid/wacom_sys.c +++ b/drivers/hid/wacom_sys.c @@ -262,6 +262,12 @@ static int wacom_set_device_mode(struct hid_device *hdev, int report_id, return error < 0 ? error : 0; } +static int wacom_bt_query_tablet_data(struct hid_device *hdev, u8 speed, + struct wacom_features *features) +{ + return 0; +} + /* * Switch the tablet into its most-capable mode. Wacom tablets are * typically configured to power-up in a mode which sends mouse-like @@ -272,6 +278,9 @@ static int wacom_set_device_mode(struct hid_device *hdev, int report_id, static int wacom_query_tablet_data(struct hid_device *hdev, struct wacom_features *features) { + if (hdev->bus == BUS_BLUETOOTH) + return wacom_bt_query_tablet_data(hdev, 1, features); + if (features->device_type == BTN_TOOL_FINGER) { if (features->type > TABLETPC) { /* MT Tablet PC touch */ @@ -890,6 +899,38 @@ static void wacom_destroy_battery(struct wacom *wacom) } } +static ssize_t wacom_show_speed(struct device *dev, + struct device_attribute + *attr, char *buf) +{ + struct hid_device *hdev = container_of(dev, struct hid_device, dev); + struct wacom *wacom = hid_get_drvdata(hdev); + + return snprintf(buf, PAGE_SIZE, "%i\n", wacom->wacom_wac.bt_high_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); + struct wacom *wacom = hid_get_drvdata(hdev); + int new_speed; + + if (sscanf(buf, "%1d", &new_speed ) != 1) + return -EINVAL; + + if (new_speed == 0 || new_speed == 1) { + wacom_bt_query_tablet_data(hdev, new_speed, + &wacom->wacom_wac.features); + return strnlen(buf, PAGE_SIZE); + } else + return -EINVAL; +} + +static DEVICE_ATTR(speed, S_IRUGO | S_IWUSR | S_IWGRP, + wacom_show_speed, wacom_store_speed); + static struct input_dev *wacom_allocate_input(struct wacom *wacom) { struct input_dev *input_dev; @@ -1210,6 +1251,9 @@ static int wacom_probe(struct hid_device *hdev, features->y_max = 4096; } + if (hdev->bus == BUS_BLUETOOTH) + features->quirks |= WACOM_QUIRK_BATTERY; + wacom_setup_device_quirks(features); /* set unit to "100th of a mm" for devices not reported by HID */ @@ -1241,10 +1285,25 @@ static int wacom_probe(struct hid_device *hdev, if (error) goto fail2; + if (!(features->quirks & WACOM_QUIRK_MONITOR) && + (features->quirks & WACOM_QUIRK_BATTERY)) { + error = wacom_initialize_battery(wacom); + if (error) + goto fail3; + } + if (!(features->quirks & WACOM_QUIRK_NO_INPUT)) { error = wacom_register_inputs(wacom); if (error) - goto fail3; + goto fail4; + } + + if (hdev->bus == BUS_BLUETOOTH) { + error = device_create_file(&hdev->dev, &dev_attr_speed); + if (error) + hid_warn(hdev, + "can't create sysfs speed attribute err: %d\n", + error); } /* Note that if query fails it is not a hard failure */ @@ -1254,7 +1313,7 @@ static int wacom_probe(struct hid_device *hdev, error = hid_hw_start(hdev, HID_CONNECT_HIDRAW); if (error) { hid_err(hdev, "hw start failed\n"); - goto fail4; + goto fail5; } if (features->quirks & WACOM_QUIRK_MONITOR) @@ -1267,7 +1326,10 @@ static int wacom_probe(struct hid_device *hdev, return 0; - fail4: wacom_unregister_inputs(wacom); + fail5: if (hdev->bus == BUS_BLUETOOTH) + device_remove_file(&hdev->dev, &dev_attr_speed); + wacom_unregister_inputs(wacom); + fail4: wacom_destroy_battery(wacom); fail3: wacom_destroy_leds(wacom); fail2: wacom_remove_shared_data(wacom_wac); fail1: kfree(wacom); @@ -1283,6 +1345,8 @@ static void wacom_remove(struct hid_device *hdev) cancel_work_sync(&wacom->work); wacom_unregister_inputs(wacom); + if (hdev->bus == BUS_BLUETOOTH) + device_remove_file(&hdev->dev, &dev_attr_speed); wacom_destroy_battery(wacom); wacom_destroy_leds(wacom); wacom_remove_shared_data(&wacom->wacom_wac); diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h index 3433a0e..3aa55d9 100644 --- a/drivers/hid/wacom_wac.h +++ b/drivers/hid/wacom_wac.h @@ -169,6 +169,8 @@ struct wacom_wac { int num_contacts_left; int bat_charging; int ps_connected; + __u8 bt_features; + __u8 bt_high_speed; }; #endif
Now that wacom is a hid driver, there is no point in having a separate driver for bluetooth devices. This patch prepares the common paths of Bluetooth devices in the common wacom driver. It also adds the sysfs file "speed" used by Bluetooth devices. Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- new in v2 drivers/hid/wacom_sys.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++--- drivers/hid/wacom_wac.h | 2 ++ 2 files changed, 69 insertions(+), 3 deletions(-)