[PATCH 00/27] HID: wacom: cleanup/EKR/LED

[PATCH 00/27] HID: wacom: cleanup/EKR/LED

[Benjamin Tissoires]

Hi,

Initially, I thought the series would have been short because it was driven
by 23/27 and 26/27. We wanted to have a common API for the LED in the kernel
and also let the kernel switches the LED for us so that libinput doesn't need
to have root access. The LED kernel API has kernel triggers which allows users
to enable/disable automatic LED switching.

Well, the plan did not go so well, and I ended up cleaning up the Wacom driver
one more time (Daft Punk's song complimentary). I finally made use of devres
management in this driver which allows to simplify the cleanups of the various
resources allocated here and there. I had to resort most of the time to devres
groups because of the 2 wireless receiver Wacom has. With these 2 receivers
(Intuos Pro/Bamboos and the EKR), input nodes can be created/destroyed at
will, without having the parent HID device removed.

The plan was also to provide a common API for the EKR and the rest of the
Intuos/Cintiqs. And working on this reminded me that we never finished providing
proper support of the Wacom ExpressKey Remote (one input node per remote,
one power_supply per remote).

So, here is the result of this past few weeks. I tested this on the following
devices: Intuos Pro, Bamboo 16 fg Pen/Touch, Bamboo Pad, Cintiq 13HD and
the EKR.
Peter tested an earlier series on the 21UX2 and I hope I fixed the bugs he
noted, but I would love the Wacom guys to test the series on the 21UX2 (again)
and the 24HD(T). I was only able to test those 2 devices through uhid, and
manually checked the raw commands were set properly, but an actual test would be
better.

Happy reviewing :)

Cheers,
Benjamin


Benjamin Tissoires (27):
  HID: wacom: actually report the battery level for wireless connected
  HID: wacom: store the type in wacom->shared for INTUOSHT and INTUOSHT2
  HID: wacom: remove cleanup of wacom->remote_dir from
    wacom_clean_inputs()
  HID: wacom: untie leds from inputs
  HID: wacom: use one work queue per task
  HID: wacom: switch battery to devres
  HID: wacom: switch inputs to devres
  HID: wacom: put the managed resources in a group
  HID: wacom: convert LEDs to devres
  HID: wacom: use devm_kasprintf for allocating the name of the remote
  HID: wacom: use devres to allocate driver data
  HID: wacom: devres manage the shared data too
  HID: wacom: leds: dynamically allocate LED groups
  HID: wacom: EKR: add a worker to add/remove resources on
    addition/removal
  HID: wacom: EKR: have the wacom resources dynamically allocated
  HID: wacom: rework fail path in probe() and parse_and_register()
  HID: wacom: EKR: have proper allocator and destructor
  HID: wacom: EKR: use devres groups to manage resources
  HID: wacom: EKR: have one array of struct remotes instead of many
    arrays
  HID: wacom: EKR: allocate one input node per remote
  HID: wacom: EKR: have one power_supply per remote
  HID: wacom: EKR: attach the power_supply on first connection
  HID: wacom: leds: use the ledclass instead of custom made sysfs files
  HID: wacom: leds: actually release the LEDs on disconnect
  HID: wacom: leds: fix ordering of LED banks
  HID: wacom: leds: handle the switch of the LEDs directly in the kernel
  HID: wacom: leds: handle Cintiq 24HD leds buttons

 Documentation/ABI/testing/sysfs-driver-wacom |    5 +
 drivers/hid/wacom.h                          |   98 ++-
 drivers/hid/wacom_sys.c                      | 1104 +++++++++++++++++++-------
 drivers/hid/wacom_wac.c                      |  238 ++++--
 drivers/hid/wacom_wac.h                      |   18 +-
 5 files changed, 1073 insertions(+), 390 deletions(-)

-- 
2.5.5

[PATCH 01/27] HID: wacom: actually report the battery level for wireless connected

[Benjamin Tissoires]

Since fd5f92b ("HID: wacom: reuse wacom_parse_and_register() in
wireless_work"), wacom->shared->type is not set.
Send the information of the battery if we have one.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_wac.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 1eae13c..d6746ca 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -2125,6 +2125,7 @@ static int wacom_bamboo_pad_irq(struct wacom_wac *wacom, size_t len)
 
 static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
 {
+	struct wacom *w = container_of(wacom, struct wacom, wacom_wac);
 	unsigned char *data = wacom->data;
 	int connected;
 
@@ -2152,7 +2153,7 @@ static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
 			wacom_schedule_work(wacom);
 		}
 
-		if (wacom->shared->type)
+		if (w->battery)
 			wacom_notify_battery(wacom, battery, charging, 1, 0);
 
 	} else if (wacom->pid != 0) {
-- 
2.5.5

[PATCH 02/27] HID: wacom: store the type in wacom->shared for INTUOSHT and INTUOSHT2

[Benjamin Tissoires]

The type is never set but we check for it in wacom_wireless_irq().
It looks like this is a big hack from the beginning, so fill in the gap
only.

Untested.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 499cc82..9e283aa 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1720,9 +1720,10 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 		error = hid_hw_open(hdev);
 
 	if ((wacom_wac->features.type == INTUOSHT ||
-	    wacom_wac->features.type == INTUOSHT2) &&
+	     wacom_wac->features.type == INTUOSHT2) &&
 	    (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)) {
-			wacom_wac->shared->touch_input = wacom_wac->touch_input;
+		wacom_wac->shared->type = wacom_wac->features.type;
+		wacom_wac->shared->touch_input = wacom_wac->touch_input;
 	}
 
 	return 0;
-- 
2.5.5

[PATCH 03/27] HID: wacom: remove cleanup of wacom->remote_dir from wacom_clean_inputs()

[Benjamin Tissoires]

wacom->remote_dir has nothing to do with inputs, so better not magically
removing it when cleaning inputs.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 9e283aa..5dd2640 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1397,7 +1397,6 @@ static void wacom_clean_inputs(struct wacom *wacom)
 		else
 			input_free_device(wacom->wacom_wac.pad_input);
 	}
-	kobject_put(wacom->remote_dir);
 	wacom->wacom_wac.pen_input = NULL;
 	wacom->wacom_wac.touch_input = NULL;
 	wacom->wacom_wac.pad_input = NULL;
@@ -1480,16 +1479,10 @@ static int wacom_register_inputs(struct wacom *wacom)
 		error = wacom_initialize_leds(wacom);
 		if (error)
 			goto fail_leds;
-
-		error = wacom_initialize_remote(wacom);
-		if (error)
-			goto fail_remote;
 	}
 
 	return 0;
 
-fail_remote:
-	wacom_destroy_leds(wacom);
 fail_leds:
 	input_unregister_device(pad_input_dev);
 	pad_input_dev = NULL;
@@ -1686,6 +1679,12 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 	if (error)
 		goto fail_register_inputs;
 
+	if (wacom->wacom_wac.features.device_type & WACOM_DEVICETYPE_PAD) {
+		error = wacom_initialize_remote(wacom);
+		if (error)
+			goto fail_remote;
+	}
+
 	if (features->type == HID_GENERIC)
 		connect_mask |= HID_CONNECT_DRIVER;
 
@@ -1705,7 +1704,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 	if ((features->type == BAMBOO_TOUCH) &&
 	    (features->device_type & WACOM_DEVICETYPE_PEN)) {
 		error = -ENODEV;
-		goto fail_hw_start;
+		goto fail_quirks;
 	}
 
 	/* pen only Bamboo neither support touch nor pad */
@@ -1713,7 +1712,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 	    ((features->device_type & WACOM_DEVICETYPE_TOUCH) ||
 	    (features->device_type & WACOM_DEVICETYPE_PAD))) {
 		error = -ENODEV;
-		goto fail_hw_start;
+		goto fail_quirks;
 	}
 
 	if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR)
@@ -1728,10 +1727,13 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 
 	return 0;
 
-fail_hw_start:
+fail_quirks:
 	hid_hw_stop(hdev);
-fail_register_inputs:
+fail_hw_start:
+	kobject_put(wacom->remote_dir);
+fail_remote:
 	wacom_clean_inputs(wacom);
+fail_register_inputs:
 	wacom_destroy_battery(wacom);
 fail_battery:
 	wacom_remove_shared_data(wacom);
@@ -1910,6 +1912,7 @@ static void wacom_remove(struct hid_device *hdev)
 	hid_hw_stop(hdev);
 
 	cancel_work_sync(&wacom->work);
+	kobject_put(wacom->remote_dir);
 	wacom_clean_inputs(wacom);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
-- 
2.5.5

[PATCH 04/27] HID: wacom: untie leds from inputs

[Benjamin Tissoires]

Like remotes, LEDs should be handled by themself, not magically behind
the inputs as they have a complete different life.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 5dd2640..af2801d 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1403,7 +1403,6 @@ static void wacom_clean_inputs(struct wacom *wacom)
 	wacom->wacom_wac.pen_registered = false;
 	wacom->wacom_wac.touch_registered = false;
 	wacom->wacom_wac.pad_registered = false;
-	wacom_destroy_leds(wacom);
 }
 
 static int wacom_allocate_inputs(struct wacom *wacom)
@@ -1475,18 +1474,10 @@ static int wacom_register_inputs(struct wacom *wacom)
 		if (error)
 			goto fail_register_pad_input;
 		wacom_wac->pad_registered = true;
-
-		error = wacom_initialize_leds(wacom);
-		if (error)
-			goto fail_leds;
 	}
 
 	return 0;
 
-fail_leds:
-	input_unregister_device(pad_input_dev);
-	pad_input_dev = NULL;
-	wacom_wac->pad_registered = false;
 fail_register_pad_input:
 	if (touch_input_dev)
 		input_unregister_device(touch_input_dev);
@@ -1680,6 +1671,10 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 		goto fail_register_inputs;
 
 	if (wacom->wacom_wac.features.device_type & WACOM_DEVICETYPE_PAD) {
+		error = wacom_initialize_leds(wacom);
+		if (error)
+			goto fail_leds;
+
 		error = wacom_initialize_remote(wacom);
 		if (error)
 			goto fail_remote;
@@ -1732,6 +1727,8 @@ fail_quirks:
 fail_hw_start:
 	kobject_put(wacom->remote_dir);
 fail_remote:
+	wacom_destroy_leds(wacom);
+fail_leds:
 	wacom_clean_inputs(wacom);
 fail_register_inputs:
 	wacom_destroy_battery(wacom);
@@ -1765,12 +1762,14 @@ static void wacom_wireless_work(struct work_struct *work)
 	hdev1 = usb_get_intfdata(usbdev->config->interface[1]);
 	wacom1 = hid_get_drvdata(hdev1);
 	wacom_wac1 = &(wacom1->wacom_wac);
+	wacom_destroy_leds(wacom1);
 	wacom_clean_inputs(wacom1);
 
 	/* Touch interface */
 	hdev2 = usb_get_intfdata(usbdev->config->interface[2]);
 	wacom2 = hid_get_drvdata(hdev2);
 	wacom_wac2 = &(wacom2->wacom_wac);
+	wacom_destroy_leds(wacom2);
 	wacom_clean_inputs(wacom2);
 
 	if (wacom_wac->pid == 0) {
@@ -1825,7 +1824,9 @@ static void wacom_wireless_work(struct work_struct *work)
 	return;
 
 fail:
+	wacom_destroy_leds(wacom1);
 	wacom_clean_inputs(wacom1);
+	wacom_destroy_leds(wacom2);
 	wacom_clean_inputs(wacom2);
 	return;
 }
@@ -1913,6 +1914,7 @@ static void wacom_remove(struct hid_device *hdev)
 
 	cancel_work_sync(&wacom->work);
 	kobject_put(wacom->remote_dir);
+	wacom_destroy_leds(wacom);
 	wacom_clean_inputs(wacom);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
-- 
2.5.5

[PATCH 05/27] HID: wacom: use one work queue per task

[Benjamin Tissoires]

Looks like the battery hijacked the wireless worker. That's not fair so
use a work queue per task.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     | 21 ++++++++++++++++++---
 drivers/hid/wacom_sys.c | 10 ++++++----
 drivers/hid/wacom_wac.c | 13 +++++--------
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 4681a65..a968fbb 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -105,13 +105,19 @@
 #define USB_VENDOR_ID_WACOM	0x056a
 #define USB_VENDOR_ID_LENOVO	0x17ef
 
+enum wacom_worker {
+	WACOM_WORKER_WIRELESS,
+	WACOM_WORKER_BATTERY,
+};
+
 struct wacom {
 	struct usb_device *usbdev;
 	struct usb_interface *intf;
 	struct wacom_wac wacom_wac;
 	struct hid_device *hdev;
 	struct mutex lock;
-	struct work_struct work;
+	struct work_struct wireless_work;
+	struct work_struct battery_work;
 	struct wacom_led {
 		u8 select[5]; /* status led selector (0..3) */
 		u8 llv;       /* status led brightness no button (1..127) */
@@ -127,10 +133,19 @@ struct wacom {
 	struct attribute_group remote_group[5];
 };
 
-static inline void wacom_schedule_work(struct wacom_wac *wacom_wac)
+static inline void wacom_schedule_work(struct wacom_wac *wacom_wac,
+				       enum wacom_worker which)
 {
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
-	schedule_work(&wacom->work);
+
+	switch (which) {
+	case WACOM_WORKER_WIRELESS:
+		schedule_work(&wacom->wireless_work);
+		break;
+	case WACOM_WORKER_BATTERY:
+		schedule_work(&wacom->battery_work);
+		break;
+	}
 }
 
 extern const struct hid_device_id wacom_ids[];
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index af2801d..54f0260 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1527,7 +1527,7 @@ static void wacom_calculate_res(struct wacom_features *features)
 
 void wacom_battery_work(struct work_struct *work)
 {
-	struct wacom *wacom = container_of(work, struct wacom, work);
+	struct wacom *wacom = container_of(work, struct wacom, battery_work);
 
 	if ((wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) &&
 	     !wacom->battery) {
@@ -1743,7 +1743,7 @@ fail_allocate_inputs:
 
 static void wacom_wireless_work(struct work_struct *work)
 {
-	struct wacom *wacom = container_of(work, struct wacom, work);
+	struct wacom *wacom = container_of(work, struct wacom, wireless_work);
 	struct usb_device *usbdev = wacom->usbdev;
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
 	struct hid_device *hdev1, *hdev2;
@@ -1871,7 +1871,8 @@ static int wacom_probe(struct hid_device *hdev,
 	wacom->usbdev = dev;
 	wacom->intf = intf;
 	mutex_init(&wacom->lock);
-	INIT_WORK(&wacom->work, wacom_wireless_work);
+	INIT_WORK(&wacom->wireless_work, wacom_wireless_work);
+	INIT_WORK(&wacom->battery_work, wacom_battery_work);
 
 	/* ask for the report descriptor to be loaded by HID */
 	error = hid_parse(hdev);
@@ -1912,7 +1913,8 @@ static void wacom_remove(struct hid_device *hdev)
 
 	hid_hw_stop(hdev);
 
-	cancel_work_sync(&wacom->work);
+	cancel_work_sync(&wacom->wireless_work);
+	cancel_work_sync(&wacom->battery_work);
 	kobject_put(wacom->remote_dir);
 	wacom_destroy_leds(wacom);
 	wacom_clean_inputs(wacom);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index d6746ca..c7707b2 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -814,8 +814,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 	if (!wacom->battery &&
 	    !(features->quirks & WACOM_QUIRK_BATTERY)) {
 		features->quirks |= WACOM_QUIRK_BATTERY;
-		INIT_WORK(&wacom->work, wacom_battery_work);
-		wacom_schedule_work(wacom_wac);
+		wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
 	}
 
 	wacom_notify_battery(wacom_wac, bat_percent, bat_charging, 1,
@@ -2150,7 +2149,7 @@ static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
 		charging = !!(data[5] & 0x80);
 		if (wacom->pid != pid) {
 			wacom->pid = pid;
-			wacom_schedule_work(wacom);
+			wacom_schedule_work(wacom, WACOM_WORKER_WIRELESS);
 		}
 
 		if (w->battery)
@@ -2159,7 +2158,7 @@ static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
 	} else if (wacom->pid != 0) {
 		/* disconnected while previously connected */
 		wacom->pid = 0;
-		wacom_schedule_work(wacom);
+		wacom_schedule_work(wacom, WACOM_WORKER_WIRELESS);
 		wacom_notify_battery(wacom, 0, 0, 0, 0);
 	}
 
@@ -2194,15 +2193,13 @@ static int wacom_status_irq(struct wacom_wac *wacom_wac, size_t len)
 		if (!wacom->battery &&
 		    !(features->quirks & WACOM_QUIRK_BATTERY)) {
 			features->quirks |= WACOM_QUIRK_BATTERY;
-			INIT_WORK(&wacom->work, wacom_battery_work);
-			wacom_schedule_work(wacom_wac);
+			wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
 		}
 	}
 	else if ((features->quirks & WACOM_QUIRK_BATTERY) &&
 		 wacom->battery) {
 		features->quirks &= ~WACOM_QUIRK_BATTERY;
-		INIT_WORK(&wacom->work, wacom_battery_work);
-		wacom_schedule_work(wacom_wac);
+		wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
 		wacom_notify_battery(wacom_wac, 0, 0, 0, 0);
 	}
 	return 0;
-- 
2.5.5

[PATCH 06/27] HID: wacom: switch battery to devres

[Benjamin Tissoires]

Simplifying the error code paths.
We need to keep wacom_destroy_battery() around for now as the wireless
module and the remotes are using it to dynamically remove the battery
supply on disconnect.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

# Conflicts:
#	drivers/hid/wacom_sys.c
---
 drivers/hid/wacom_sys.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 54f0260..61dcb87e 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1082,11 +1082,16 @@ static int wacom_ac_get_property(struct power_supply *psy,
 static int wacom_initialize_battery(struct wacom *wacom)
 {
 	static atomic_t battery_no = ATOMIC_INIT(0);
+	struct device *dev = &wacom->hdev->dev;
 	struct power_supply_config psy_cfg = { .drv_data = wacom, };
+	struct power_supply_desc *bat_desc = &wacom->battery_desc;
 	unsigned long n;
+	int error;
+
+	if (!devres_open_group(dev, bat_desc, GFP_KERNEL))
+		return -ENOMEM;
 
 	if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
-		struct power_supply_desc *bat_desc = &wacom->battery_desc;
 		struct power_supply_desc *ac_desc = &wacom->ac_desc;
 		n = atomic_inc_return(&battery_no) - 1;
 
@@ -1106,33 +1111,40 @@ static int wacom_initialize_battery(struct wacom *wacom)
 		ac_desc->type = POWER_SUPPLY_TYPE_MAINS;
 		ac_desc->use_for_apm = 0;
 
-		wacom->battery = power_supply_register(&wacom->hdev->dev,
-					      &wacom->battery_desc, &psy_cfg);
-		if (IS_ERR(wacom->battery))
-			return PTR_ERR(wacom->battery);
+		wacom->battery = devm_power_supply_register(dev,
+							   &wacom->battery_desc,
+							   &psy_cfg);
+		if (IS_ERR(wacom->battery)) {
+			error = PTR_ERR(wacom->battery);
+			goto err;
+		}
 
 		power_supply_powers(wacom->battery, &wacom->hdev->dev);
 
-		wacom->ac = power_supply_register(&wacom->hdev->dev,
-						  &wacom->ac_desc,
-						  &psy_cfg);
+		wacom->ac = devm_power_supply_register(dev,
+						       &wacom->ac_desc,
+						       &psy_cfg);
 		if (IS_ERR(wacom->ac)) {
-			power_supply_unregister(wacom->battery);
-			return PTR_ERR(wacom->ac);
+			error = PTR_ERR(wacom->ac);
+			goto err;
 		}
 
 		power_supply_powers(wacom->ac, &wacom->hdev->dev);
 	}
 
+	devres_close_group(dev, bat_desc);
 	return 0;
+
+err:
+	devres_release_group(dev, bat_desc);
+	return error;
 }
 
 static void wacom_destroy_battery(struct wacom *wacom)
 {
 	if (wacom->battery) {
-		power_supply_unregister(wacom->battery);
+		devres_release_group(&wacom->hdev->dev, &wacom->battery_desc);
 		wacom->battery = NULL;
-		power_supply_unregister(wacom->ac);
 		wacom->ac = NULL;
 	}
 }
@@ -1731,7 +1743,6 @@ fail_remote:
 fail_leds:
 	wacom_clean_inputs(wacom);
 fail_register_inputs:
-	wacom_destroy_battery(wacom);
 fail_battery:
 	wacom_remove_shared_data(wacom);
 fail_shared_data:
@@ -1920,7 +1931,6 @@ static void wacom_remove(struct hid_device *hdev)
 	wacom_clean_inputs(wacom);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
-	wacom_destroy_battery(wacom);
 	wacom_remove_shared_data(wacom);
 
 	hid_set_drvdata(hdev, NULL);
-- 
2.5.5

[PATCH 07/27] HID: wacom: switch inputs to devres

[Benjamin Tissoires]

Simplifying the error code paths.
We need to keep wacom_clean_inputs() around for now as the wireless
module is using it to dynamically remove the inputs on disconnect.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 61dcb87e..da2fccf 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -91,7 +91,12 @@ static void wacom_close(struct input_dev *dev)
 {
 	struct wacom *wacom = input_get_drvdata(dev);
 
-	hid_hw_close(wacom->hdev);
+	/*
+	 * wacom->hdev should never be null, but surprisingly, I had the case
+	 * once while unplugging the Wacom Wireless Receiver.
+	 */
+	if (wacom->hdev)
+		hid_hw_close(wacom->hdev);
 }
 
 /*
@@ -1370,7 +1375,7 @@ static struct input_dev *wacom_allocate_input(struct wacom *wacom)
 	struct hid_device *hdev = wacom->hdev;
 	struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
 
-	input_dev = input_allocate_device();
+	input_dev = devm_input_allocate_device(&hdev->dev);
 	if (!input_dev)
 		return NULL;
 
@@ -1424,10 +1429,10 @@ static int wacom_allocate_inputs(struct wacom *wacom)
 	wacom_wac->pen_input = wacom_allocate_input(wacom);
 	wacom_wac->touch_input = wacom_allocate_input(wacom);
 	wacom_wac->pad_input = wacom_allocate_input(wacom);
-	if (!wacom_wac->pen_input || !wacom_wac->touch_input || !wacom_wac->pad_input) {
-		wacom_clean_inputs(wacom);
+	if (!wacom_wac->pen_input ||
+	    !wacom_wac->touch_input ||
+	    !wacom_wac->pad_input)
 		return -ENOMEM;
-	}
 
 	wacom_wac->pen_input->name = wacom_wac->pen_name;
 	wacom_wac->touch_input->name = wacom_wac->touch_name;
@@ -1458,7 +1463,7 @@ static int wacom_register_inputs(struct wacom *wacom)
 	} else {
 		error = input_register_device(pen_input_dev);
 		if (error)
-			goto fail_register_pen_input;
+			goto fail;
 		wacom_wac->pen_registered = true;
 	}
 
@@ -1471,7 +1476,7 @@ static int wacom_register_inputs(struct wacom *wacom)
 	} else {
 		error = input_register_device(touch_input_dev);
 		if (error)
-			goto fail_register_touch_input;
+			goto fail;
 		wacom_wac->touch_registered = true;
 	}
 
@@ -1484,23 +1489,19 @@ static int wacom_register_inputs(struct wacom *wacom)
 	} else {
 		error = input_register_device(pad_input_dev);
 		if (error)
-			goto fail_register_pad_input;
+			goto fail;
 		wacom_wac->pad_registered = true;
 	}
 
 	return 0;
 
-fail_register_pad_input:
-	if (touch_input_dev)
-		input_unregister_device(touch_input_dev);
+fail:
+	wacom_wac->pad_input = NULL;
+	wacom_wac->pad_registered = false;
 	wacom_wac->touch_input = NULL;
 	wacom_wac->touch_registered = false;
-fail_register_touch_input:
-	if (pen_input_dev)
-		input_unregister_device(pen_input_dev);
 	wacom_wac->pen_input = NULL;
 	wacom_wac->pen_registered = false;
-fail_register_pen_input:
 	return error;
 }
 
@@ -1741,14 +1742,12 @@ fail_hw_start:
 fail_remote:
 	wacom_destroy_leds(wacom);
 fail_leds:
-	wacom_clean_inputs(wacom);
 fail_register_inputs:
 fail_battery:
 	wacom_remove_shared_data(wacom);
 fail_shared_data:
 fail_parsed:
 fail_allocate_inputs:
-	wacom_clean_inputs(wacom);
 	return error;
 }
 
@@ -1928,7 +1927,6 @@ static void wacom_remove(struct hid_device *hdev)
 	cancel_work_sync(&wacom->battery_work);
 	kobject_put(wacom->remote_dir);
 	wacom_destroy_leds(wacom);
-	wacom_clean_inputs(wacom);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
 	wacom_remove_shared_data(wacom);
-- 
2.5.5

[PATCH 08/27] HID: wacom: put the managed resources in a group

[Benjamin Tissoires]

We currently have a complex clean_inputs() function while this can be
handled all by devres. Set a group that we can destroy in wireless_work().

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  1 +
 drivers/hid/wacom_sys.c | 69 +++++++++++++++++++++----------------------------
 drivers/hid/wacom_wac.c |  2 +-
 drivers/hid/wacom_wac.h |  3 ---
 4 files changed, 32 insertions(+), 43 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index a968fbb..6f5555f 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -131,6 +131,7 @@ struct wacom {
 	struct power_supply_desc ac_desc;
 	struct kobject *remote_dir;
 	struct attribute_group remote_group[5];
+	bool resources;
 };
 
 static inline void wacom_schedule_work(struct wacom_wac *wacom_wac,
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index da2fccf..08f6d1f 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1394,34 +1394,6 @@ static struct input_dev *wacom_allocate_input(struct wacom *wacom)
 	return input_dev;
 }
 
-static void wacom_clean_inputs(struct wacom *wacom)
-{
-	if (wacom->wacom_wac.pen_input) {
-		if (wacom->wacom_wac.pen_registered)
-			input_unregister_device(wacom->wacom_wac.pen_input);
-		else
-			input_free_device(wacom->wacom_wac.pen_input);
-	}
-	if (wacom->wacom_wac.touch_input) {
-		if (wacom->wacom_wac.touch_registered)
-			input_unregister_device(wacom->wacom_wac.touch_input);
-		else
-			input_free_device(wacom->wacom_wac.touch_input);
-	}
-	if (wacom->wacom_wac.pad_input) {
-		if (wacom->wacom_wac.pad_registered)
-			input_unregister_device(wacom->wacom_wac.pad_input);
-		else
-			input_free_device(wacom->wacom_wac.pad_input);
-	}
-	wacom->wacom_wac.pen_input = NULL;
-	wacom->wacom_wac.touch_input = NULL;
-	wacom->wacom_wac.pad_input = NULL;
-	wacom->wacom_wac.pen_registered = false;
-	wacom->wacom_wac.touch_registered = false;
-	wacom->wacom_wac.pad_registered = false;
-}
-
 static int wacom_allocate_inputs(struct wacom *wacom)
 {
 	struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
@@ -1464,7 +1436,6 @@ static int wacom_register_inputs(struct wacom *wacom)
 		error = input_register_device(pen_input_dev);
 		if (error)
 			goto fail;
-		wacom_wac->pen_registered = true;
 	}
 
 	error = wacom_setup_touch_input_capabilities(touch_input_dev, wacom_wac);
@@ -1477,7 +1448,6 @@ static int wacom_register_inputs(struct wacom *wacom)
 		error = input_register_device(touch_input_dev);
 		if (error)
 			goto fail;
-		wacom_wac->touch_registered = true;
 	}
 
 	error = wacom_setup_pad_input_capabilities(pad_input_dev, wacom_wac);
@@ -1490,18 +1460,14 @@ static int wacom_register_inputs(struct wacom *wacom)
 		error = input_register_device(pad_input_dev);
 		if (error)
 			goto fail;
-		wacom_wac->pad_registered = true;
 	}
 
 	return 0;
 
 fail:
 	wacom_wac->pad_input = NULL;
-	wacom_wac->pad_registered = false;
 	wacom_wac->touch_input = NULL;
-	wacom_wac->touch_registered = false;
 	wacom_wac->pen_input = NULL;
-	wacom_wac->pen_registered = false;
 	return error;
 }
 
@@ -1612,6 +1578,22 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)
 		"%s%s Pad", name, suffix);
 }
 
+static void wacom_release_resources(struct wacom *wacom)
+{
+	struct hid_device *hdev = wacom->hdev;
+
+	if (!wacom->resources)
+		return;
+
+	devres_release_group(&hdev->dev, wacom);
+
+	wacom->resources = false;
+
+	wacom->wacom_wac.pen_input = NULL;
+	wacom->wacom_wac.touch_input = NULL;
+	wacom->wacom_wac.pad_input = NULL;
+}
+
 static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 {
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
@@ -1624,9 +1606,14 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 	if (features->pktlen > WACOM_PKGLEN_MAX)
 		return -EINVAL;
 
+	if (!devres_open_group(&hdev->dev, wacom, GFP_KERNEL))
+		return -ENOMEM;
+
+	wacom->resources = true;
+
 	error = wacom_allocate_inputs(wacom);
 	if (error)
-		return error;
+		goto fail_open_group;
 
 	/*
 	 * Bamboo Pad has a generic hid handling for the Pen, and we switch it
@@ -1733,6 +1720,8 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 		wacom_wac->shared->touch_input = wacom_wac->touch_input;
 	}
 
+	devres_close_group(&hdev->dev, wacom);
+
 	return 0;
 
 fail_quirks:
@@ -1748,6 +1737,8 @@ fail_battery:
 fail_shared_data:
 fail_parsed:
 fail_allocate_inputs:
+fail_open_group:
+	wacom_release_resources(wacom);
 	return error;
 }
 
@@ -1773,14 +1764,14 @@ static void wacom_wireless_work(struct work_struct *work)
 	wacom1 = hid_get_drvdata(hdev1);
 	wacom_wac1 = &(wacom1->wacom_wac);
 	wacom_destroy_leds(wacom1);
-	wacom_clean_inputs(wacom1);
+	wacom_release_resources(wacom1);
 
 	/* Touch interface */
 	hdev2 = usb_get_intfdata(usbdev->config->interface[2]);
 	wacom2 = hid_get_drvdata(hdev2);
 	wacom_wac2 = &(wacom2->wacom_wac);
 	wacom_destroy_leds(wacom2);
-	wacom_clean_inputs(wacom2);
+	wacom_release_resources(wacom2);
 
 	if (wacom_wac->pid == 0) {
 		hid_info(wacom->hdev, "wireless tablet disconnected\n");
@@ -1835,9 +1826,9 @@ static void wacom_wireless_work(struct work_struct *work)
 
 fail:
 	wacom_destroy_leds(wacom1);
-	wacom_clean_inputs(wacom1);
+	wacom_release_resources(wacom1);
 	wacom_destroy_leds(wacom2);
-	wacom_clean_inputs(wacom2);
+	wacom_release_resources(wacom2);
 	return;
 }
 
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index c7707b2..3e2c903 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1941,7 +1941,7 @@ static int wacom_bpt3_touch(struct wacom_wac *wacom)
 	}
 
 	/* only update touch if we actually have a touchpad and touch data changed */
-	if (wacom->touch_registered && touch_changed) {
+	if (wacom->touch_input && touch_changed) {
 		input_mt_sync_frame(wacom->touch_input);
 		wacom->shared->touch_down = wacom_wac_finger_count_touches(wacom);
 	}
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 53d1653..d5b973d 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -234,9 +234,6 @@ struct wacom_wac {
 	struct input_dev *pen_input;
 	struct input_dev *touch_input;
 	struct input_dev *pad_input;
-	bool pen_registered;
-	bool touch_registered;
-	bool pad_registered;
 	int pid;
 	int battery_capacity;
 	int num_contacts_left;
-- 
2.5.5

[PATCH 09/27] HID: wacom: convert LEDs to devres

[Benjamin Tissoires]

Use our own wacom_devm_sysfs_create_group() as there is currently no
generic one. It has been requested at least twice [1][2] but has been
always rejected.
However, in the Wacom case, for the wirelessly connected devices, we need
to be able to release the created sysfs files without removing the parent
kobject.

[1] https://patchwork.kernel.org/patch/7526551/
[2] https://lkml.org/lkml/2013/3/14/728

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  1 -
 drivers/hid/wacom_sys.c | 95 +++++++++++++++++++++++--------------------------
 2 files changed, 45 insertions(+), 51 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 6f5555f..bcfeb51 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -124,7 +124,6 @@ struct wacom {
 		u8 hlv;       /* status led brightness button pressed (1..127) */
 		u8 img_lum;   /* OLED matrix display brightness */
 	} led;
-	bool led_initialized;
 	struct power_supply *battery;
 	struct power_supply *ac;
 	struct power_supply_desc battery_desc;
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 08f6d1f..c08a752 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -909,6 +909,45 @@ static struct attribute_group intuos5_led_attr_group = {
 	.attrs = intuos5_led_attrs,
 };
 
+struct wacom_sysfs_group_devres {
+	struct attribute_group *group;
+	struct kobject *root;
+};
+
+static void wacom_devm_sysfs_group_release(struct device *dev, void *res)
+{
+	struct wacom_sysfs_group_devres *devres = res;
+	struct kobject *kobj = devres->root;
+
+	dev_dbg(dev, "%s: dropping reference to %s\n",
+		__func__, devres->group->name);
+	sysfs_remove_group(kobj, devres->group);
+}
+
+static int wacom_devm_sysfs_create_group(struct wacom *wacom,
+					 struct attribute_group *group)
+{
+	struct wacom_sysfs_group_devres *devres;
+	int error;
+
+	devres = devres_alloc(wacom_devm_sysfs_group_release,
+			      sizeof(struct wacom_sysfs_group_devres),
+			      GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	devres->group = group;
+	devres->root = &wacom->hdev->dev.kobj;
+
+	error = sysfs_create_group(devres->root, group);
+	if (error)
+		return error;
+
+	devres_add(&wacom->hdev->dev, devres);
+
+	return 0;
+}
+
 static int wacom_initialize_leds(struct wacom *wacom)
 {
 	int error;
@@ -927,8 +966,8 @@ static int wacom_initialize_leds(struct wacom *wacom)
 		wacom->led.llv = 10;
 		wacom->led.hlv = 20;
 		wacom->led.img_lum = 10;
-		error = sysfs_create_group(&wacom->hdev->dev.kobj,
-					   &intuos4_led_attr_group);
+		error = wacom_devm_sysfs_create_group(wacom,
+						      &intuos4_led_attr_group);
 		break;
 
 	case WACOM_24HD:
@@ -939,8 +978,8 @@ static int wacom_initialize_leds(struct wacom *wacom)
 		wacom->led.hlv = 0;
 		wacom->led.img_lum = 0;
 
-		error = sysfs_create_group(&wacom->hdev->dev.kobj,
-					   &cintiq_led_attr_group);
+		error = wacom_devm_sysfs_create_group(wacom,
+						      &cintiq_led_attr_group);
 		break;
 
 	case INTUOS5S:
@@ -955,8 +994,8 @@ static int wacom_initialize_leds(struct wacom *wacom)
 		wacom->led.hlv = 0;
 		wacom->led.img_lum = 0;
 
-		error = sysfs_create_group(&wacom->hdev->dev.kobj,
-					  &intuos5_led_attr_group);
+		error = wacom_devm_sysfs_create_group(wacom,
+						      &intuos5_led_attr_group);
 		break;
 
 	default:
@@ -969,48 +1008,10 @@ static int wacom_initialize_leds(struct wacom *wacom)
 		return error;
 	}
 	wacom_led_control(wacom);
-	wacom->led_initialized = true;
 
 	return 0;
 }
 
-static void wacom_destroy_leds(struct wacom *wacom)
-{
-	if (!wacom->led_initialized)
-		return;
-
-	if (!(wacom->wacom_wac.features.device_type & WACOM_DEVICETYPE_PAD))
-		return;
-
-	wacom->led_initialized = false;
-
-	switch (wacom->wacom_wac.features.type) {
-	case INTUOS4S:
-	case INTUOS4:
-	case INTUOS4WL:
-	case INTUOS4L:
-		sysfs_remove_group(&wacom->hdev->dev.kobj,
-				   &intuos4_led_attr_group);
-		break;
-
-	case WACOM_24HD:
-	case WACOM_21UX2:
-		sysfs_remove_group(&wacom->hdev->dev.kobj,
-				   &cintiq_led_attr_group);
-		break;
-
-	case INTUOS5S:
-	case INTUOS5:
-	case INTUOS5L:
-	case INTUOSPS:
-	case INTUOSPM:
-	case INTUOSPL:
-		sysfs_remove_group(&wacom->hdev->dev.kobj,
-				   &intuos5_led_attr_group);
-		break;
-	}
-}
-
 static enum power_supply_property wacom_battery_props[] = {
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_STATUS,
@@ -1729,7 +1730,6 @@ fail_quirks:
 fail_hw_start:
 	kobject_put(wacom->remote_dir);
 fail_remote:
-	wacom_destroy_leds(wacom);
 fail_leds:
 fail_register_inputs:
 fail_battery:
@@ -1763,14 +1763,12 @@ static void wacom_wireless_work(struct work_struct *work)
 	hdev1 = usb_get_intfdata(usbdev->config->interface[1]);
 	wacom1 = hid_get_drvdata(hdev1);
 	wacom_wac1 = &(wacom1->wacom_wac);
-	wacom_destroy_leds(wacom1);
 	wacom_release_resources(wacom1);
 
 	/* Touch interface */
 	hdev2 = usb_get_intfdata(usbdev->config->interface[2]);
 	wacom2 = hid_get_drvdata(hdev2);
 	wacom_wac2 = &(wacom2->wacom_wac);
-	wacom_destroy_leds(wacom2);
 	wacom_release_resources(wacom2);
 
 	if (wacom_wac->pid == 0) {
@@ -1825,9 +1823,7 @@ static void wacom_wireless_work(struct work_struct *work)
 	return;
 
 fail:
-	wacom_destroy_leds(wacom1);
 	wacom_release_resources(wacom1);
-	wacom_destroy_leds(wacom2);
 	wacom_release_resources(wacom2);
 	return;
 }
@@ -1917,7 +1913,6 @@ static void wacom_remove(struct hid_device *hdev)
 	cancel_work_sync(&wacom->wireless_work);
 	cancel_work_sync(&wacom->battery_work);
 	kobject_put(wacom->remote_dir);
-	wacom_destroy_leds(wacom);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
 	wacom_remove_shared_data(wacom);
-- 
2.5.5

[PATCH 10/27] HID: wacom: use devm_kasprintf for allocating the name of the remote

[Benjamin Tissoires]

The sysfs group was indeed removed by kobject_put(wacom->remote_dir) in
wacom_remove(), but the name of the group was never freed.

Also remove the misplaced kobject_put(wacom->remote_dir) in the error
path of wacom_remote_create_attr_group().

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index c08a752..ec088c1 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -25,7 +25,6 @@
 #define WAC_CMD_RETRIES		10
 #define WAC_CMD_DELETE_PAIRING	0x20
 #define WAC_CMD_UNPAIR_ALL	0xFF
-#define WAC_REMOTE_SERIAL_MAX_STRLEN	9
 
 #define DEV_ATTR_RW_PERM (S_IRUGO | S_IWUSR | S_IWGRP)
 #define DEV_ATTR_WO_PERM (S_IWUSR | S_IWGRP)
@@ -1233,23 +1232,21 @@ DEVICE_EKR_ATTR_GROUP(4);
 int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial, int index)
 {
 	int error = 0;
-	char *buf;
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
 
 	wacom_wac->serial[index] = serial;
 
-	buf = kzalloc(WAC_REMOTE_SERIAL_MAX_STRLEN, GFP_KERNEL);
-	if (!buf)
+	wacom->remote_group[index].name = devm_kasprintf(&wacom->hdev->dev,
+							 GFP_KERNEL,
+							 "%d", serial);
+	if (!wacom->remote_group[index].name)
 		return -ENOMEM;
-	snprintf(buf, WAC_REMOTE_SERIAL_MAX_STRLEN, "%d", serial);
-	wacom->remote_group[index].name = buf;
 
 	error = sysfs_create_group(wacom->remote_dir,
 				   &wacom->remote_group[index]);
 	if (error) {
 		hid_err(wacom->hdev,
 			"cannot create sysfs group err: %d\n", error);
-		kobject_put(wacom->remote_dir);
 		return error;
 	}
 
@@ -1271,7 +1268,8 @@ void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial)
 			if (wacom->remote_group[i].name) {
 				sysfs_remove_group(wacom->remote_dir,
 						   &wacom->remote_group[i]);
-				kfree(wacom->remote_group[i].name);
+				devm_kfree(&wacom->hdev->dev,
+					   (char *)wacom->remote_group[i].name);
 				wacom->remote_group[i].name = NULL;
 			}
 		}
-- 
2.5.5

[PATCH 11/27] HID: wacom: use devres to allocate driver data

[Benjamin Tissoires]

We started switching the driver to devres, so we should use it as much
as possible.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index ec088c1..56d62e8 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1844,7 +1844,7 @@ static int wacom_probe(struct hid_device *hdev,
 	/* hid-core sets this quirk for the boot interface */
 	hdev->quirks &= ~HID_QUIRK_NOGET;
 
-	wacom = kzalloc(sizeof(struct wacom), GFP_KERNEL);
+	wacom = devm_kzalloc(&hdev->dev, sizeof(struct wacom), GFP_KERNEL);
 	if (!wacom)
 		return -ENOMEM;
 
@@ -1892,7 +1892,6 @@ static int wacom_probe(struct hid_device *hdev,
 
 fail_type:
 fail_parse:
-	kfree(wacom);
 	hid_set_drvdata(hdev, NULL);
 	return error;
 }
@@ -1916,7 +1915,6 @@ static void wacom_remove(struct hid_device *hdev)
 	wacom_remove_shared_data(wacom);
 
 	hid_set_drvdata(hdev, NULL);
-	kfree(wacom);
 }
 
 #ifdef CONFIG_PM
-- 
2.5.5

[PATCH 12/27] HID: wacom: devres manage the shared data too

[Benjamin Tissoires]

wacom_release_shared_data() and wacom_remove_shared_data() are moved up
so they can be referenced in wacom_add_shared_data().

There is no point in explicitly setting wacom_wac1->shared->type to 0 in
wacom_wireless_work() (plus this would give an oops).

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 73 ++++++++++++++++++++++++++-----------------------
 1 file changed, 39 insertions(+), 34 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 56d62e8..d0f5764 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -567,6 +567,38 @@ static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
 	return NULL;
 }
 
+static void wacom_release_shared_data(struct kref *kref)
+{
+	struct wacom_hdev_data *data =
+		container_of(kref, struct wacom_hdev_data, kref);
+
+	mutex_lock(&wacom_udev_list_lock);
+	list_del(&data->list);
+	mutex_unlock(&wacom_udev_list_lock);
+
+	kfree(data);
+}
+
+static void wacom_remove_shared_data(void *res)
+{
+	struct wacom *wacom = res;
+	struct wacom_hdev_data *data;
+	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+
+	if (wacom_wac->shared) {
+		data = container_of(wacom_wac->shared, struct wacom_hdev_data,
+				    shared);
+
+		if (wacom_wac->shared->touch == wacom->hdev)
+			wacom_wac->shared->touch = NULL;
+		else if (wacom_wac->shared->pen == wacom->hdev)
+			wacom_wac->shared->pen = NULL;
+
+		kref_put(&data->kref, wacom_release_shared_data);
+		wacom_wac->shared = NULL;
+	}
+}
+
 static int wacom_add_shared_data(struct hid_device *hdev)
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
@@ -591,6 +623,13 @@ static int wacom_add_shared_data(struct hid_device *hdev)
 
 	wacom_wac->shared = &data->shared;
 
+	retval = devm_add_action(&hdev->dev, wacom_remove_shared_data, wacom);
+	if (retval) {
+		mutex_unlock(&wacom_udev_list_lock);
+		wacom_remove_shared_data(wacom);
+		return retval;
+	}
+
 	if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
 		wacom_wac->shared->touch = hdev;
 	else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
@@ -601,37 +640,6 @@ out:
 	return retval;
 }
 
-static void wacom_release_shared_data(struct kref *kref)
-{
-	struct wacom_hdev_data *data =
-		container_of(kref, struct wacom_hdev_data, kref);
-
-	mutex_lock(&wacom_udev_list_lock);
-	list_del(&data->list);
-	mutex_unlock(&wacom_udev_list_lock);
-
-	kfree(data);
-}
-
-static void wacom_remove_shared_data(struct wacom *wacom)
-{
-	struct wacom_hdev_data *data;
-	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
-
-	if (wacom_wac->shared) {
-		data = container_of(wacom_wac->shared, struct wacom_hdev_data,
-				    shared);
-
-		if (wacom_wac->shared->touch == wacom->hdev)
-			wacom_wac->shared->touch = NULL;
-		else if (wacom_wac->shared->pen == wacom->hdev)
-			wacom_wac->shared->pen = NULL;
-
-		kref_put(&data->kref, wacom_release_shared_data);
-		wacom_wac->shared = NULL;
-	}
-}
-
 static int wacom_led_control(struct wacom *wacom)
 {
 	unsigned char *buf;
@@ -1731,7 +1739,6 @@ fail_remote:
 fail_leds:
 fail_register_inputs:
 fail_battery:
-	wacom_remove_shared_data(wacom);
 fail_shared_data:
 fail_parsed:
 fail_allocate_inputs:
@@ -1771,7 +1778,6 @@ static void wacom_wireless_work(struct work_struct *work)
 
 	if (wacom_wac->pid == 0) {
 		hid_info(wacom->hdev, "wireless tablet disconnected\n");
-		wacom_wac1->shared->type = 0;
 	} else {
 		const struct hid_device_id *id = wacom_ids;
 
@@ -1912,7 +1918,6 @@ static void wacom_remove(struct hid_device *hdev)
 	kobject_put(wacom->remote_dir);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
-	wacom_remove_shared_data(wacom);
 
 	hid_set_drvdata(hdev, NULL);
 }
-- 
2.5.5

[PATCH 13/27] HID: wacom: leds: dynamically allocate LED groups

[Benjamin Tissoires]

We need to add an action to ensure wacom->led.groups is null when
wacom_led_control() gets called after the resources has been freed.

This also prevents to send a LED command when there is no support
from the device.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  8 +++--
 drivers/hid/wacom_sys.c | 85 +++++++++++++++++++++++++++++++++++++++++--------
 drivers/hid/wacom_wac.c |  2 +-
 3 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index bcfeb51..8ac1eb8 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -110,6 +110,10 @@ enum wacom_worker {
 	WACOM_WORKER_BATTERY,
 };
 
+struct wacom_group_leds {
+	u8 select; /* status led selector (0..3) */
+};
+
 struct wacom {
 	struct usb_device *usbdev;
 	struct usb_interface *intf;
@@ -118,8 +122,8 @@ struct wacom {
 	struct mutex lock;
 	struct work_struct wireless_work;
 	struct work_struct battery_work;
-	struct wacom_led {
-		u8 select[5]; /* status led selector (0..3) */
+	struct wacom_leds {
+		struct wacom_group_leds *groups;
 		u8 llv;       /* status led brightness no button (1..127) */
 		u8 hlv;       /* status led brightness button pressed (1..127) */
 		u8 img_lum;   /* OLED matrix display brightness */
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index d0f5764..4d4c737 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -647,6 +647,9 @@ static int wacom_led_control(struct wacom *wacom)
 	unsigned char report_id = WAC_CMD_LED_CONTROL;
 	int buf_size = 9;
 
+	if (!wacom->led.groups)
+		return -ENOTSUPP;
+
 	if (wacom->wacom_wac.pid) { /* wireless connected */
 		report_id = WAC_CMD_WL_LED_CONTROL;
 		buf_size = 13;
@@ -662,7 +665,7 @@ static int wacom_led_control(struct wacom *wacom)
 		 * one of four values:
 		 *    0 = Low; 1 = Medium; 2 = High; 3 = Off
 		 */
-		int ring_led = wacom->led.select[0] & 0x03;
+		int ring_led = wacom->led.groups[0].select & 0x03;
 		int ring_lum = (((wacom->led.llv & 0x60) >> 5) - 1) & 0x03;
 		int crop_lum = 0;
 		unsigned char led_bits = (crop_lum << 4) | (ring_lum << 2) | (ring_led);
@@ -677,11 +680,11 @@ static int wacom_led_control(struct wacom *wacom)
 			buf[1] = led_bits;
 	}
 	else {
-		int led = wacom->led.select[0] | 0x4;
+		int led = wacom->led.groups[0].select | 0x4;
 
 		if (wacom->wacom_wac.features.type == WACOM_21UX2 ||
 		    wacom->wacom_wac.features.type == WACOM_24HD)
-			led |= (wacom->led.select[1] << 4) | 0x40;
+			led |= (wacom->led.groups[1].select << 4) | 0x40;
 
 		buf[0] = report_id;
 		buf[1] = led;
@@ -753,7 +756,7 @@ static ssize_t wacom_led_select_store(struct device *dev, int set_id,
 
 	mutex_lock(&wacom->lock);
 
-	wacom->led.select[set_id] = id & 0x3;
+	wacom->led.groups[set_id].select = id & 0x3;
 	err = wacom_led_control(wacom);
 
 	mutex_unlock(&wacom->lock);
@@ -773,7 +776,7 @@ static ssize_t wacom_led##SET_ID##_select_show(struct device *dev,	\
 	struct hid_device *hdev = to_hid_device(dev);\
 	struct wacom *wacom = hid_get_drvdata(hdev);			\
 	return scnprintf(buf, PAGE_SIZE, "%d\n",			\
-			 wacom->led.select[SET_ID]);			\
+			 wacom->led.groups[SET_ID].select);		\
 }									\
 static DEVICE_ATTR(status_led##SET_ID##_select, DEV_ATTR_RW_PERM,	\
 		    wacom_led##SET_ID##_select_show,			\
@@ -955,6 +958,35 @@ static int wacom_devm_sysfs_create_group(struct wacom *wacom,
 	return 0;
 }
 
+static void wacom_led_groups_release(void *data)
+{
+	struct wacom *wacom = data;
+
+	wacom->led.groups = NULL;
+}
+
+static int wacom_led_groups_allocate(struct wacom *wacom, int count)
+{
+	struct wacom_group_leds *groups;
+	int error;
+
+	groups = devm_kzalloc(&wacom->hdev->dev,
+			      sizeof(struct wacom_group_leds) * count,
+			      GFP_KERNEL);
+	if (!groups)
+		return -ENOMEM;
+
+	error = devm_add_action_or_reset(&wacom->hdev->dev,
+					 wacom_led_groups_release,
+					 wacom);
+	if (error)
+		return error;
+
+	wacom->led.groups = groups;
+
+	return 0;
+}
+
 static int wacom_initialize_leds(struct wacom *wacom)
 {
 	int error;
@@ -968,23 +1000,34 @@ static int wacom_initialize_leds(struct wacom *wacom)
 	case INTUOS4:
 	case INTUOS4WL:
 	case INTUOS4L:
-		wacom->led.select[0] = 0;
-		wacom->led.select[1] = 0;
 		wacom->led.llv = 10;
 		wacom->led.hlv = 20;
 		wacom->led.img_lum = 10;
+
+		error = wacom_led_groups_allocate(wacom, 1);
+		if (error) {
+			hid_err(wacom->hdev,
+				"cannot create leds err: %d\n", error);
+			return error;
+		}
+
 		error = wacom_devm_sysfs_create_group(wacom,
 						      &intuos4_led_attr_group);
 		break;
 
 	case WACOM_24HD:
 	case WACOM_21UX2:
-		wacom->led.select[0] = 0;
-		wacom->led.select[1] = 0;
 		wacom->led.llv = 0;
 		wacom->led.hlv = 0;
 		wacom->led.img_lum = 0;
 
+		error = wacom_led_groups_allocate(wacom, 2);
+		if (error) {
+			hid_err(wacom->hdev,
+				"cannot create leds err: %d\n", error);
+			return error;
+		}
+
 		error = wacom_devm_sysfs_create_group(wacom,
 						      &cintiq_led_attr_group);
 		break;
@@ -995,16 +1038,30 @@ static int wacom_initialize_leds(struct wacom *wacom)
 	case INTUOSPS:
 	case INTUOSPM:
 	case INTUOSPL:
-		wacom->led.select[0] = 0;
-		wacom->led.select[1] = 0;
 		wacom->led.llv = 32;
 		wacom->led.hlv = 0;
 		wacom->led.img_lum = 0;
 
+		error = wacom_led_groups_allocate(wacom, 1);
+		if (error) {
+			hid_err(wacom->hdev,
+				"cannot create leds err: %d\n", error);
+			return error;
+		}
+
 		error = wacom_devm_sysfs_create_group(wacom,
 						      &intuos5_led_attr_group);
 		break;
 
+	case REMOTE:
+		error = wacom_led_groups_allocate(wacom, 5);
+		if (error) {
+			hid_err(wacom->hdev,
+				"cannot create leds err: %d\n", error);
+			return error;
+		}
+		return 0;
+
 	default:
 		return 0;
 	}
@@ -1204,7 +1261,7 @@ static ssize_t wacom_show_remote_mode(struct kobject *kobj,
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	u8 mode;
 
-	mode = wacom->led.select[index];
+	mode = wacom->led.groups[index].select;
 	if (mode >= 0 && mode < 3)
 		return snprintf(buf, PAGE_SIZE, "%d\n", mode);
 	else
@@ -1272,7 +1329,7 @@ void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial)
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		if (wacom_wac->serial[i] == serial) {
 			wacom_wac->serial[i] = 0;
-			wacom->led.select[i] = WACOM_STATUS_UNKNOWN;
+			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
 			if (wacom->remote_group[i].name) {
 				sysfs_remove_group(wacom->remote_dir,
 						   &wacom->remote_group[i]);
@@ -1369,7 +1426,7 @@ static int wacom_initialize_remote(struct wacom *wacom)
 	}
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		wacom->led.select[i] = WACOM_STATUS_UNKNOWN;
+		wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
 		wacom_wac->serial[i] = 0;
 	}
 
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 3e2c903..932d3ee 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -808,7 +808,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		if (wacom_wac->serial[i] == serial)
-			wacom->led.select[i] = touch_ring_mode;
+			wacom->led.groups[i].select = touch_ring_mode;
 	}
 
 	if (!wacom->battery &&
-- 
2.5.5

[PATCH 14/27] HID: wacom: EKR: add a worker to add/remove resources on addition/removal

[Benjamin Tissoires]

wacom_remote_status_irq() sends information of addition/removal of EKR.
We want to allocate one input node per remote, so better having this
in a separate worker, not handled in the IRQ directly.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     | 11 +++++--
 drivers/hid/wacom_sys.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/hid/wacom_wac.c | 41 +++++++++----------------
 drivers/hid/wacom_wac.h |  7 +++++
 4 files changed, 106 insertions(+), 33 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 8ac1eb8..1797e4b 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -90,6 +90,7 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/hid.h>
+#include <linux/kfifo.h>
 #include <linux/usb/input.h>
 #include <linux/power_supply.h>
 #include <asm/unaligned.h>
@@ -108,6 +109,7 @@
 enum wacom_worker {
 	WACOM_WORKER_WIRELESS,
 	WACOM_WORKER_BATTERY,
+	WACOM_WORKER_REMOTE,
 };
 
 struct wacom_group_leds {
@@ -122,6 +124,9 @@ struct wacom {
 	struct mutex lock;
 	struct work_struct wireless_work;
 	struct work_struct battery_work;
+	struct work_struct remote_work;
+	spinlock_t remote_lock;
+	struct kfifo remote_fifo;
 	struct wacom_leds {
 		struct wacom_group_leds *groups;
 		u8 llv;       /* status led brightness no button (1..127) */
@@ -149,6 +154,9 @@ static inline void wacom_schedule_work(struct wacom_wac *wacom_wac,
 	case WACOM_WORKER_BATTERY:
 		schedule_work(&wacom->battery_work);
 		break;
+	case WACOM_WORKER_REMOTE:
+		schedule_work(&wacom->remote_work);
+		break;
 	}
 }
 
@@ -168,7 +176,4 @@ int wacom_wac_event(struct hid_device *hdev, struct hid_field *field,
 		struct hid_usage *usage, __s32 value);
 void wacom_wac_report(struct hid_device *hdev, struct hid_report *report);
 void wacom_battery_work(struct work_struct *work);
-int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
-				   int index);
-void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial);
 #endif
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 4d4c737..7e01117 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1294,7 +1294,8 @@ DEVICE_EKR_ATTR_GROUP(2);
 DEVICE_EKR_ATTR_GROUP(3);
 DEVICE_EKR_ATTR_GROUP(4);
 
-int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial, int index)
+static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
+					  int index)
 {
 	int error = 0;
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
@@ -1318,7 +1319,7 @@ int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial, int index)
 	return 0;
 }
 
-void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial)
+static void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial)
 {
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
 	int i;
@@ -1889,6 +1890,65 @@ fail:
 	return;
 }
 
+static void wacom_remote_work(struct work_struct *work)
+{
+	struct wacom *wacom = container_of(work, struct wacom, remote_work);
+	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_remote_data data;
+	unsigned long flags;
+	unsigned int count;
+	u32 serial;
+	int i, k;
+
+	spin_lock_irqsave(&wacom->remote_lock, flags);
+
+	count = kfifo_out(&wacom->remote_fifo, &data, sizeof(data));
+
+	if (count != sizeof(data)) {
+		hid_err(wacom->hdev,
+			"workitem triggered without status available\n");
+		spin_unlock_irqrestore(&wacom->remote_lock, flags);
+		return;
+	}
+
+	if (!kfifo_is_empty(&wacom->remote_fifo))
+		wacom_schedule_work(&wacom->wacom_wac, WACOM_WORKER_REMOTE);
+
+	spin_unlock_irqrestore(&wacom->remote_lock, flags);
+
+	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
+		serial = data.remote[i].serial;
+		if (data.remote[i].connected) {
+
+			if (wacom_wac->serial[i] == serial)
+				continue;
+
+			if (wacom_wac->serial[i]) {
+				wacom_remote_destroy_attr_group(wacom,
+							wacom_wac->serial[i]);
+			}
+
+			/* A remote can pair more than once with an EKR,
+			 * check to make sure this serial isn't already paired.
+			 */
+			for (k = 0; k < WACOM_MAX_REMOTES; k++) {
+				if (wacom_wac->serial[k] == serial)
+					break;
+			}
+
+			if (k < WACOM_MAX_REMOTES) {
+				wacom_wac->serial[i] = serial;
+				continue;
+			}
+			wacom_remote_create_attr_group(wacom, serial, i);
+
+		} else if (wacom_wac->serial[i]) {
+			wacom_remote_destroy_attr_group(wacom,
+							wacom_wac->serial[i]);
+		}
+	}
+}
+
 static int wacom_probe(struct hid_device *hdev,
 		const struct hid_device_id *id)
 {
@@ -1931,6 +1991,17 @@ static int wacom_probe(struct hid_device *hdev,
 	mutex_init(&wacom->lock);
 	INIT_WORK(&wacom->wireless_work, wacom_wireless_work);
 	INIT_WORK(&wacom->battery_work, wacom_battery_work);
+	INIT_WORK(&wacom->remote_work, wacom_remote_work);
+	spin_lock_init(&wacom->remote_lock);
+
+	if (kfifo_alloc(&wacom->remote_fifo,
+			5 * sizeof(struct wacom_remote_data),
+			GFP_KERNEL)) {
+		dev_err(&hdev->dev,
+			"%s:failed allocating remote_fifo\n", __func__);
+		error = -ENOMEM;
+		goto fail_type;
+	}
 
 	/* ask for the report descriptor to be loaded by HID */
 	error = hid_parse(hdev);
@@ -1953,8 +2024,9 @@ static int wacom_probe(struct hid_device *hdev,
 
 	return 0;
 
-fail_type:
 fail_parse:
+	kfifo_free(&wacom->remote_fifo);
+fail_type:
 	hid_set_drvdata(hdev, NULL);
 	return error;
 }
@@ -1972,6 +2044,8 @@ static void wacom_remove(struct hid_device *hdev)
 
 	cancel_work_sync(&wacom->wireless_work);
 	cancel_work_sync(&wacom->battery_work);
+	cancel_work_sync(&wacom->remote_work);
+	kfifo_free(&wacom->remote_fifo);
 	kobject_put(wacom->remote_dir);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 932d3ee..4f4a2b9 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -827,7 +827,9 @@ static int wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
 {
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
 	unsigned char *data = wacom_wac->data;
-	int i;
+	struct wacom_remote_data remote_data = {0};
+	unsigned long flags;
+	int i, ret;
 
 	if (data[0] != WACOM_REPORT_DEVICE_LIST)
 		return 0;
@@ -837,36 +839,21 @@ static int wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
 		int serial = (data[j+6] << 16) + (data[j+5] << 8) + data[j+4];
 		bool connected = data[j+2];
 
-		if (connected) {
-			int k;
-
-			if (wacom_wac->serial[i] == serial)
-				continue;
+		remote_data.remote[i].serial = serial;
+		remote_data.remote[i].connected = connected;
+	}
 
-			if (wacom_wac->serial[i]) {
-				wacom_remote_destroy_attr_group(wacom,
-							wacom_wac->serial[i]);
-			}
+	spin_lock_irqsave(&wacom->remote_lock, flags);
 
-			/* A remote can pair more than once with an EKR,
-			 * check to make sure this serial isn't already paired.
-			 */
-			for (k = 0; k < WACOM_MAX_REMOTES; k++) {
-				if (wacom_wac->serial[k] == serial)
-					break;
-			}
+	ret = kfifo_in(&wacom->remote_fifo, &remote_data, sizeof(remote_data));
+	if (ret != sizeof(remote_data)) {
+		hid_err(wacom->hdev, "Can't queue Remote status event.\n");
+		return 0;
+	}
 
-			if (k < WACOM_MAX_REMOTES) {
-				wacom_wac->serial[i] = serial;
-				continue;
-			}
-			wacom_remote_create_attr_group(wacom, serial, i);
+	spin_unlock_irqrestore(&wacom->remote_lock, flags);
 
-		} else if (wacom_wac->serial[i]) {
-			wacom_remote_destroy_attr_group(wacom,
-							wacom_wac->serial[i]);
-		}
-	}
+	wacom_schedule_work(wacom_wac, WACOM_WORKER_REMOTE);
 
 	return 0;
 }
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index d5b973d..c2c65b8 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -218,6 +218,13 @@ struct hid_data {
 	int num_received;
 };
 
+struct wacom_remote_data {
+	struct {
+		u32 serial;
+		bool connected;
+	} remote[WACOM_MAX_REMOTES];
+};
+
 struct wacom_wac {
 	char pen_name[WACOM_NAME_MAX];
 	char touch_name[WACOM_NAME_MAX];
-- 
2.5.5

Re: [PATCH 14/27] HID: wacom: EKR: add a worker to add/remove resources on addition/removal

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2135 bytes --]

Hi,

[auto build test WARNING on hid/for-next]
[also build test WARNING on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Benjamin-Tissoires/HID-wacom-cleanup-EKR-LED/20160705-225431
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next
config: m68k-allyesconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m68k 

All warnings (new ones prefixed by >>):

   drivers/hid/wacom_wac.c: In function 'wacom_remote_status_irq':
>> drivers/hid/wacom_wac.c:830:9: warning: missing braces around initializer [-Wmissing-braces]
     struct wacom_remote_data remote_data = {0};
            ^
   drivers/hid/wacom_wac.c:830:9: warning: (near initialization for 'remote_data.remote') [-Wmissing-braces]

vim +830 drivers/hid/wacom_wac.c

   814		if (!wacom->battery &&
   815		    !(features->quirks & WACOM_QUIRK_BATTERY)) {
   816			features->quirks |= WACOM_QUIRK_BATTERY;
   817			wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
   818		}
   819	
   820		wacom_notify_battery(wacom_wac, bat_percent, bat_charging, 1,
   821				     bat_charging);
   822	
   823		return 1;
   824	}
   825	
   826	static int wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
   827	{
   828		struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
   829		unsigned char *data = wacom_wac->data;
 > 830		struct wacom_remote_data remote_data = {0};
   831		unsigned long flags;
   832		int i, ret;
   833	
   834		if (data[0] != WACOM_REPORT_DEVICE_LIST)
   835			return 0;
   836	
   837		for (i = 0; i < WACOM_MAX_REMOTES; i++) {
   838			int j = i * 6;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 37195 bytes --]

[PATCH 15/27] HID: wacom: EKR: have the wacom resources dynamically allocated

[Benjamin Tissoires]

If we want to have one input device per remote, it's better to have our
own struct wacom_remote which is dynamically allocated.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  13 +++--
 drivers/hid/wacom_sys.c | 133 ++++++++++++++++++++++++++++--------------------
 drivers/hid/wacom_wac.c |  10 ++--
 drivers/hid/wacom_wac.h |   2 +-
 4 files changed, 93 insertions(+), 65 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 1797e4b..110ea67 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -116,6 +116,14 @@ struct wacom_group_leds {
 	u8 select; /* status led selector (0..3) */
 };
 
+struct wacom_remote {
+	spinlock_t remote_lock;
+	struct kfifo remote_fifo;
+	struct kobject *remote_dir;
+	struct attribute_group remote_group[WACOM_MAX_REMOTES];
+	__u32 serial[WACOM_MAX_REMOTES];
+};
+
 struct wacom {
 	struct usb_device *usbdev;
 	struct usb_interface *intf;
@@ -125,8 +133,7 @@ struct wacom {
 	struct work_struct wireless_work;
 	struct work_struct battery_work;
 	struct work_struct remote_work;
-	spinlock_t remote_lock;
-	struct kfifo remote_fifo;
+	struct wacom_remote *remote;
 	struct wacom_leds {
 		struct wacom_group_leds *groups;
 		u8 llv;       /* status led brightness no button (1..127) */
@@ -137,8 +144,6 @@ struct wacom {
 	struct power_supply *ac;
 	struct power_supply_desc battery_desc;
 	struct power_supply_desc ac_desc;
-	struct kobject *remote_dir;
-	struct attribute_group remote_group[5];
 	bool resources;
 };
 
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 7e01117..64acb71 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1298,18 +1298,18 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 					  int index)
 {
 	int error = 0;
-	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_remote *remote = wacom->remote;
 
-	wacom_wac->serial[index] = serial;
+	remote->serial[index] = serial;
 
-	wacom->remote_group[index].name = devm_kasprintf(&wacom->hdev->dev,
-							 GFP_KERNEL,
-							 "%d", serial);
-	if (!wacom->remote_group[index].name)
+	remote->remote_group[index].name = devm_kasprintf(&wacom->hdev->dev,
+							  GFP_KERNEL,
+							  "%d", serial);
+	if (!remote->remote_group[index].name)
 		return -ENOMEM;
 
-	error = sysfs_create_group(wacom->remote_dir,
-				   &wacom->remote_group[index]);
+	error = sysfs_create_group(remote->remote_dir,
+				   &remote->remote_group[index]);
 	if (error) {
 		hid_err(wacom->hdev,
 			"cannot create sysfs group err: %d\n", error);
@@ -1321,22 +1321,22 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 
 static void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial)
 {
-	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_remote *remote = wacom->remote;
 	int i;
 
 	if (!serial)
 		return;
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		if (wacom_wac->serial[i] == serial) {
-			wacom_wac->serial[i] = 0;
+		if (remote->serial[i] == serial) {
+			remote->serial[i] = 0;
 			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
-			if (wacom->remote_group[i].name) {
-				sysfs_remove_group(wacom->remote_dir,
-						   &wacom->remote_group[i]);
+			if (remote->remote_group[i].name) {
+				sysfs_remove_group(remote->remote_dir,
+						   &remote->remote_group[i]);
 				devm_kfree(&wacom->hdev->dev,
-					   (char *)wacom->remote_group[i].name);
-				wacom->remote_group[i].name = NULL;
+					  (char *)remote->remote_group[i].name);
+				remote->remote_group[i].name = NULL;
 			}
 		}
 	}
@@ -1398,27 +1398,57 @@ static const struct attribute *remote_unpair_attrs[] = {
 	NULL
 };
 
-static int wacom_initialize_remote(struct wacom *wacom)
+static void wacom_remotes_destroy(void *data)
+{
+	struct wacom *wacom = data;
+	struct wacom_remote *remote = wacom->remote;
+
+	if (!remote)
+		return;
+
+	kobject_put(remote->remote_dir);
+	kfifo_free(&remote->remote_fifo);
+	wacom->remote = NULL;
+}
+
+static int wacom_initialize_remotes(struct wacom *wacom)
 {
 	int error = 0;
-	struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
+	struct wacom_remote *remote;
 	int i;
 
 	if (wacom->wacom_wac.features.type != REMOTE)
 		return 0;
 
-	wacom->remote_group[0] = remote0_serial_group;
-	wacom->remote_group[1] = remote1_serial_group;
-	wacom->remote_group[2] = remote2_serial_group;
-	wacom->remote_group[3] = remote3_serial_group;
-	wacom->remote_group[4] = remote4_serial_group;
+	remote = devm_kzalloc(&wacom->hdev->dev, sizeof(*wacom->remote),
+			      GFP_KERNEL);
+	if (!remote)
+		return -ENOMEM;
 
-	wacom->remote_dir = kobject_create_and_add("wacom_remote",
-						   &wacom->hdev->dev.kobj);
-	if (!wacom->remote_dir)
+	wacom->remote = remote;
+
+	spin_lock_init(&remote->remote_lock);
+
+	error = kfifo_alloc(&remote->remote_fifo,
+			5 * sizeof(struct wacom_remote_data),
+			GFP_KERNEL);
+	if (error) {
+		hid_err(wacom->hdev, "failed allocating remote_fifo\n");
 		return -ENOMEM;
+	}
 
-	error = sysfs_create_files(wacom->remote_dir, remote_unpair_attrs);
+	remote->remote_group[0] = remote0_serial_group;
+	remote->remote_group[1] = remote1_serial_group;
+	remote->remote_group[2] = remote2_serial_group;
+	remote->remote_group[3] = remote3_serial_group;
+	remote->remote_group[4] = remote4_serial_group;
+
+	remote->remote_dir = kobject_create_and_add("wacom_remote",
+						    &wacom->hdev->dev.kobj);
+	if (!remote->remote_dir)
+		return -ENOMEM;
+
+	error = sysfs_create_files(remote->remote_dir, remote_unpair_attrs);
 
 	if (error) {
 		hid_err(wacom->hdev,
@@ -1428,9 +1458,14 @@ static int wacom_initialize_remote(struct wacom *wacom)
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
-		wacom_wac->serial[i] = 0;
+		remote->serial[i] = 0;
 	}
 
+	error = devm_add_action_or_reset(&wacom->hdev->dev,
+					 wacom_remotes_destroy, wacom);
+	if (error)
+		return error;
+
 	return 0;
 }
 
@@ -1740,7 +1775,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 		if (error)
 			goto fail_leds;
 
-		error = wacom_initialize_remote(wacom);
+		error = wacom_initialize_remotes(wacom);
 		if (error)
 			goto fail_remote;
 	}
@@ -1792,7 +1827,6 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 fail_quirks:
 	hid_hw_stop(hdev);
 fail_hw_start:
-	kobject_put(wacom->remote_dir);
 fail_remote:
 fail_leds:
 fail_register_inputs:
@@ -1893,58 +1927,58 @@ fail:
 static void wacom_remote_work(struct work_struct *work)
 {
 	struct wacom *wacom = container_of(work, struct wacom, remote_work);
-	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_remote *remote = wacom->remote;
 	struct wacom_remote_data data;
 	unsigned long flags;
 	unsigned int count;
 	u32 serial;
 	int i, k;
 
-	spin_lock_irqsave(&wacom->remote_lock, flags);
+	spin_lock_irqsave(&remote->remote_lock, flags);
 
-	count = kfifo_out(&wacom->remote_fifo, &data, sizeof(data));
+	count = kfifo_out(&remote->remote_fifo, &data, sizeof(data));
 
 	if (count != sizeof(data)) {
 		hid_err(wacom->hdev,
 			"workitem triggered without status available\n");
-		spin_unlock_irqrestore(&wacom->remote_lock, flags);
+		spin_unlock_irqrestore(&remote->remote_lock, flags);
 		return;
 	}
 
-	if (!kfifo_is_empty(&wacom->remote_fifo))
+	if (!kfifo_is_empty(&remote->remote_fifo))
 		wacom_schedule_work(&wacom->wacom_wac, WACOM_WORKER_REMOTE);
 
-	spin_unlock_irqrestore(&wacom->remote_lock, flags);
+	spin_unlock_irqrestore(&remote->remote_lock, flags);
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		serial = data.remote[i].serial;
 		if (data.remote[i].connected) {
 
-			if (wacom_wac->serial[i] == serial)
+			if (remote->serial[i] == serial)
 				continue;
 
-			if (wacom_wac->serial[i]) {
+			if (remote->serial[i]) {
 				wacom_remote_destroy_attr_group(wacom,
-							wacom_wac->serial[i]);
+							remote->serial[i]);
 			}
 
 			/* A remote can pair more than once with an EKR,
 			 * check to make sure this serial isn't already paired.
 			 */
 			for (k = 0; k < WACOM_MAX_REMOTES; k++) {
-				if (wacom_wac->serial[k] == serial)
+				if (remote->serial[k] == serial)
 					break;
 			}
 
 			if (k < WACOM_MAX_REMOTES) {
-				wacom_wac->serial[i] = serial;
+				remote->serial[i] = serial;
 				continue;
 			}
 			wacom_remote_create_attr_group(wacom, serial, i);
 
-		} else if (wacom_wac->serial[i]) {
+		} else if (remote->serial[i]) {
 			wacom_remote_destroy_attr_group(wacom,
-							wacom_wac->serial[i]);
+							remote->serial[i]);
 		}
 	}
 }
@@ -1992,16 +2026,6 @@ static int wacom_probe(struct hid_device *hdev,
 	INIT_WORK(&wacom->wireless_work, wacom_wireless_work);
 	INIT_WORK(&wacom->battery_work, wacom_battery_work);
 	INIT_WORK(&wacom->remote_work, wacom_remote_work);
-	spin_lock_init(&wacom->remote_lock);
-
-	if (kfifo_alloc(&wacom->remote_fifo,
-			5 * sizeof(struct wacom_remote_data),
-			GFP_KERNEL)) {
-		dev_err(&hdev->dev,
-			"%s:failed allocating remote_fifo\n", __func__);
-		error = -ENOMEM;
-		goto fail_type;
-	}
 
 	/* ask for the report descriptor to be loaded by HID */
 	error = hid_parse(hdev);
@@ -2025,7 +2049,6 @@ static int wacom_probe(struct hid_device *hdev,
 	return 0;
 
 fail_parse:
-	kfifo_free(&wacom->remote_fifo);
 fail_type:
 	hid_set_drvdata(hdev, NULL);
 	return error;
@@ -2045,8 +2068,6 @@ static void wacom_remove(struct hid_device *hdev)
 	cancel_work_sync(&wacom->wireless_work);
 	cancel_work_sync(&wacom->battery_work);
 	cancel_work_sync(&wacom->remote_work);
-	kfifo_free(&wacom->remote_fifo);
-	kobject_put(wacom->remote_dir);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
 
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 4f4a2b9..d5c180d 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -753,6 +753,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 	unsigned char *data = wacom_wac->data;
 	struct input_dev *input = wacom_wac->pad_input;
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
+	struct wacom_remote *remote = wacom->remote;
 	struct wacom_features *features = &wacom_wac->features;
 	int bat_charging, bat_percent, touch_ring_mode;
 	__u32 serial;
@@ -807,7 +808,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 	touch_ring_mode = (data[11] & 0xC0) >> 6;
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		if (wacom_wac->serial[i] == serial)
+		if (remote->serial[i] == serial)
 			wacom->led.groups[i].select = touch_ring_mode;
 	}
 
@@ -827,6 +828,7 @@ static int wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
 {
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
 	unsigned char *data = wacom_wac->data;
+	struct wacom_remote *remote = wacom->remote;
 	struct wacom_remote_data remote_data = {0};
 	unsigned long flags;
 	int i, ret;
@@ -843,15 +845,15 @@ static int wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
 		remote_data.remote[i].connected = connected;
 	}
 
-	spin_lock_irqsave(&wacom->remote_lock, flags);
+	spin_lock_irqsave(&remote->remote_lock, flags);
 
-	ret = kfifo_in(&wacom->remote_fifo, &remote_data, sizeof(remote_data));
+	ret = kfifo_in(&remote->remote_fifo, &remote_data, sizeof(remote_data));
 	if (ret != sizeof(remote_data)) {
 		hid_err(wacom->hdev, "Can't queue Remote status event.\n");
 		return 0;
 	}
 
-	spin_unlock_irqrestore(&wacom->remote_lock, flags);
+	spin_unlock_irqrestore(&remote->remote_lock, flags);
 
 	wacom_schedule_work(wacom_wac, WACOM_WORKER_REMOTE);
 
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index c2c65b8..6be6cae 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -234,7 +234,7 @@ struct wacom_wac {
 	unsigned char data[WACOM_PKGLEN_MAX];
 	int tool[2];
 	int id[2];
-	__u32 serial[5];
+	__u32 serial[2];
 	bool reporting_data;
 	struct wacom_features features;
 	struct wacom_shared *shared;
-- 
2.5.5

[PATCH 16/27] HID: wacom: rework fail path in probe() and parse_and_register()

[Benjamin Tissoires]

Thanks to devres management, we don't need to remember a lot of failure
path. One or two is enough.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 64acb71..241acee 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1713,7 +1713,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 
 	error = wacom_allocate_inputs(wacom);
 	if (error)
-		goto fail_open_group;
+		goto fail;
 
 	/*
 	 * Bamboo Pad has a generic hid handling for the Pen, and we switch it
@@ -1726,7 +1726,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 		} else if ((features->pktlen != WACOM_PKGLEN_BPAD_TOUCH) &&
 			   (features->pktlen != WACOM_PKGLEN_BPAD_TOUCH_USB)) {
 			error = -ENODEV;
-			goto fail_allocate_inputs;
+			goto fail;
 		}
 	}
 
@@ -1746,7 +1746,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 			 error ? "Ignoring" : "Assuming pen");
 
 		if (error)
-			goto fail_parsed;
+			goto fail;
 
 		features->device_type |= WACOM_DEVICETYPE_PEN;
 	}
@@ -1757,27 +1757,27 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 
 	error = wacom_add_shared_data(hdev);
 	if (error)
-		goto fail_shared_data;
+		goto fail;
 
 	if (!(features->device_type & WACOM_DEVICETYPE_WL_MONITOR) &&
 	     (features->quirks & WACOM_QUIRK_BATTERY)) {
 		error = wacom_initialize_battery(wacom);
 		if (error)
-			goto fail_battery;
+			goto fail;
 	}
 
 	error = wacom_register_inputs(wacom);
 	if (error)
-		goto fail_register_inputs;
+		goto fail;
 
 	if (wacom->wacom_wac.features.device_type & WACOM_DEVICETYPE_PAD) {
 		error = wacom_initialize_leds(wacom);
 		if (error)
-			goto fail_leds;
+			goto fail;
 
 		error = wacom_initialize_remotes(wacom);
 		if (error)
-			goto fail_remote;
+			goto fail;
 	}
 
 	if (features->type == HID_GENERIC)
@@ -1787,7 +1787,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 	error = hid_hw_start(hdev, connect_mask);
 	if (error) {
 		hid_err(hdev, "hw start failed\n");
-		goto fail_hw_start;
+		goto fail;
 	}
 
 	if (!wireless) {
@@ -1826,15 +1826,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 
 fail_quirks:
 	hid_hw_stop(hdev);
-fail_hw_start:
-fail_remote:
-fail_leds:
-fail_register_inputs:
-fail_battery:
-fail_shared_data:
-fail_parsed:
-fail_allocate_inputs:
-fail_open_group:
+fail:
 	wacom_release_resources(wacom);
 	return error;
 }
@@ -2014,7 +2006,7 @@ static int wacom_probe(struct hid_device *hdev,
 
 	if (features->check_for_hid_type && features->hid_type != hdev->type) {
 		error = -ENODEV;
-		goto fail_type;
+		goto fail;
 	}
 
 	wacom_wac->hid_data.inputmode = -1;
@@ -2031,12 +2023,12 @@ static int wacom_probe(struct hid_device *hdev,
 	error = hid_parse(hdev);
 	if (error) {
 		hid_err(hdev, "parse failed\n");
-		goto fail_parse;
+		goto fail;
 	}
 
 	error = wacom_parse_and_register(wacom, false);
 	if (error)
-		goto fail_parse;
+		goto fail;
 
 	if (hdev->bus == BUS_BLUETOOTH) {
 		error = device_create_file(&hdev->dev, &dev_attr_speed);
@@ -2048,8 +2040,7 @@ static int wacom_probe(struct hid_device *hdev,
 
 	return 0;
 
-fail_parse:
-fail_type:
+fail:
 	hid_set_drvdata(hdev, NULL);
 	return error;
 }
-- 
2.5.5

[PATCH 17/27] HID: wacom: EKR: have proper allocator and destructor

[Benjamin Tissoires]

The wacom_remote_create_attr_group() and wacom_remote_destroy_attr_group()
functions were both allocating/destroying the sysfs groups but also
initializing the parameters for the remotes. Have proper functions
that can be called and extended.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 93 ++++++++++++++++++++++++++++---------------------
 1 file changed, 53 insertions(+), 40 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 241acee..27ca147 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1300,8 +1300,6 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 	int error = 0;
 	struct wacom_remote *remote = wacom->remote;
 
-	remote->serial[index] = serial;
-
 	remote->remote_group[index].name = devm_kasprintf(&wacom->hdev->dev,
 							  GFP_KERNEL,
 							  "%d", serial);
@@ -1319,27 +1317,13 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 	return 0;
 }
 
-static void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial)
+static void wacom_remote_destroy_attr_group(struct wacom *wacom, unsigned int i)
 {
 	struct wacom_remote *remote = wacom->remote;
-	int i;
 
-	if (!serial)
-		return;
-
-	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		if (remote->serial[i] == serial) {
-			remote->serial[i] = 0;
-			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
-			if (remote->remote_group[i].name) {
-				sysfs_remove_group(remote->remote_dir,
-						   &remote->remote_group[i]);
-				devm_kfree(&wacom->hdev->dev,
-					  (char *)remote->remote_group[i].name);
-				remote->remote_group[i].name = NULL;
-			}
-		}
-	}
+	sysfs_remove_group(remote->remote_dir, &remote->remote_group[i]);
+	devm_kfree(&wacom->hdev->dev, (char *)remote->remote_group[i].name);
+	remote->remote_group[i].name = NULL;
 }
 
 static int wacom_cmd_unpair_remote(struct wacom *wacom, unsigned char selector)
@@ -1916,6 +1900,50 @@ fail:
 	return;
 }
 
+static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
+{
+	struct wacom_remote *remote = wacom->remote;
+	u32 serial = remote->serial[index];
+	int i;
+
+	wacom_remote_destroy_attr_group(wacom, index);
+
+	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
+		if (remote->serial[i] == serial) {
+			remote->serial[i] = 0;
+			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
+		}
+	}
+}
+
+static int wacom_remote_create_one(struct wacom *wacom, u32 serial,
+				   unsigned int index)
+{
+	struct wacom_remote *remote = wacom->remote;
+	int error, k;
+
+	/* A remote can pair more than once with an EKR,
+	 * check to make sure this serial isn't already paired.
+	 */
+	for (k = 0; k < WACOM_MAX_REMOTES; k++) {
+		if (remote->serial[k] == serial)
+			break;
+	}
+
+	if (k < WACOM_MAX_REMOTES) {
+		remote->serial[index] = serial;
+		return 0;
+	}
+
+	error = wacom_remote_create_attr_group(wacom, serial, index);
+	if (error)
+		return error;
+
+	remote->serial[index] = serial;
+
+	return 0;
+}
+
 static void wacom_remote_work(struct work_struct *work)
 {
 	struct wacom *wacom = container_of(work, struct wacom, remote_work);
@@ -1924,7 +1952,7 @@ static void wacom_remote_work(struct work_struct *work)
 	unsigned long flags;
 	unsigned int count;
 	u32 serial;
-	int i, k;
+	int i;
 
 	spin_lock_irqsave(&remote->remote_lock, flags);
 
@@ -1949,28 +1977,13 @@ static void wacom_remote_work(struct work_struct *work)
 			if (remote->serial[i] == serial)
 				continue;
 
-			if (remote->serial[i]) {
-				wacom_remote_destroy_attr_group(wacom,
-							remote->serial[i]);
-			}
-
-			/* A remote can pair more than once with an EKR,
-			 * check to make sure this serial isn't already paired.
-			 */
-			for (k = 0; k < WACOM_MAX_REMOTES; k++) {
-				if (remote->serial[k] == serial)
-					break;
-			}
+			if (remote->serial[i])
+				wacom_remote_destroy_one(wacom, i);
 
-			if (k < WACOM_MAX_REMOTES) {
-				remote->serial[i] = serial;
-				continue;
-			}
-			wacom_remote_create_attr_group(wacom, serial, i);
+			wacom_remote_create_one(wacom, serial, i);
 
 		} else if (remote->serial[i]) {
-			wacom_remote_destroy_attr_group(wacom,
-							remote->serial[i]);
+			wacom_remote_destroy_one(wacom, i);
 		}
 	}
 }
-- 
2.5.5

[PATCH 18/27] HID: wacom: EKR: use devres groups to manage resources

[Benjamin Tissoires]

This will be useful when each remote will be assigned its own input device.
We won't need to unregister each input and sysfs and other elements one
at a time.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 27ca147..9121e72 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -934,8 +934,9 @@ static void wacom_devm_sysfs_group_release(struct device *dev, void *res)
 	sysfs_remove_group(kobj, devres->group);
 }
 
-static int wacom_devm_sysfs_create_group(struct wacom *wacom,
-					 struct attribute_group *group)
+static int __wacom_devm_sysfs_create_group(struct wacom *wacom,
+					   struct kobject *root,
+					   struct attribute_group *group)
 {
 	struct wacom_sysfs_group_devres *devres;
 	int error;
@@ -947,7 +948,7 @@ static int wacom_devm_sysfs_create_group(struct wacom *wacom,
 		return -ENOMEM;
 
 	devres->group = group;
-	devres->root = &wacom->hdev->dev.kobj;
+	devres->root = root;
 
 	error = sysfs_create_group(devres->root, group);
 	if (error)
@@ -958,6 +959,13 @@ static int wacom_devm_sysfs_create_group(struct wacom *wacom,
 	return 0;
 }
 
+static int wacom_devm_sysfs_create_group(struct wacom *wacom,
+					 struct attribute_group *group)
+{
+	return __wacom_devm_sysfs_create_group(wacom, &wacom->hdev->dev.kobj,
+					       group);
+}
+
 static void wacom_led_groups_release(void *data)
 {
 	struct wacom *wacom = data;
@@ -1306,9 +1314,10 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 	if (!remote->remote_group[index].name)
 		return -ENOMEM;
 
-	error = sysfs_create_group(remote->remote_dir,
-				   &remote->remote_group[index]);
+	error = __wacom_devm_sysfs_create_group(wacom, remote->remote_dir,
+						&remote->remote_group[index]);
 	if (error) {
+		remote->remote_group[index].name = NULL;
 		hid_err(wacom->hdev,
 			"cannot create sysfs group err: %d\n", error);
 		return error;
@@ -1317,15 +1326,6 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 	return 0;
 }
 
-static void wacom_remote_destroy_attr_group(struct wacom *wacom, unsigned int i)
-{
-	struct wacom_remote *remote = wacom->remote;
-
-	sysfs_remove_group(remote->remote_dir, &remote->remote_group[i]);
-	devm_kfree(&wacom->hdev->dev, (char *)remote->remote_group[i].name);
-	remote->remote_group[i].name = NULL;
-}
-
 static int wacom_cmd_unpair_remote(struct wacom *wacom, unsigned char selector)
 {
 	const size_t buf_size = 2;
@@ -1906,11 +1906,13 @@ static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
 	u32 serial = remote->serial[index];
 	int i;
 
-	wacom_remote_destroy_attr_group(wacom, index);
+	if (remote->remote_group[index].name)
+		devres_release_group(&wacom->hdev->dev, &remote->serial[index]);
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		if (remote->serial[i] == serial) {
 			remote->serial[i] = 0;
+			remote->remote_group[i].name = NULL;
 			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
 		}
 	}
@@ -1920,6 +1922,7 @@ static int wacom_remote_create_one(struct wacom *wacom, u32 serial,
 				   unsigned int index)
 {
 	struct wacom_remote *remote = wacom->remote;
+	struct device *dev = &wacom->hdev->dev;
 	int error, k;
 
 	/* A remote can pair more than once with an EKR,
@@ -1935,13 +1938,22 @@ static int wacom_remote_create_one(struct wacom *wacom, u32 serial,
 		return 0;
 	}
 
+	if (!devres_open_group(dev, &remote->serial[index], GFP_KERNEL))
+		return -ENOMEM;
+
 	error = wacom_remote_create_attr_group(wacom, serial, index);
 	if (error)
-		return error;
+		goto fail;
 
 	remote->serial[index] = serial;
 
+	devres_close_group(dev, &remote->serial[index]);
 	return 0;
+
+fail:
+	devres_release_group(dev, &remote->serial[index]);
+	remote->serial[index] = 0;
+	return error;
 }
 
 static void wacom_remote_work(struct work_struct *work)
-- 
2.5.5

[PATCH 19/27] HID: wacom: EKR: have one array of struct remotes instead of many arrays

[Benjamin Tissoires]

No functional changes, just a prep patch for the one after.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  6 ++++--
 drivers/hid/wacom_sys.c | 53 +++++++++++++++++++++++++------------------------
 drivers/hid/wacom_wac.c |  2 +-
 3 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 110ea67..6b8df67 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -120,8 +120,10 @@ struct wacom_remote {
 	spinlock_t remote_lock;
 	struct kfifo remote_fifo;
 	struct kobject *remote_dir;
-	struct attribute_group remote_group[WACOM_MAX_REMOTES];
-	__u32 serial[WACOM_MAX_REMOTES];
+	struct {
+		struct attribute_group group;
+		u32 serial;
+	} remotes[WACOM_MAX_REMOTES];
 };
 
 struct wacom {
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 9121e72..c3b2692 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1308,16 +1308,16 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 	int error = 0;
 	struct wacom_remote *remote = wacom->remote;
 
-	remote->remote_group[index].name = devm_kasprintf(&wacom->hdev->dev,
+	remote->remotes[index].group.name = devm_kasprintf(&wacom->hdev->dev,
 							  GFP_KERNEL,
 							  "%d", serial);
-	if (!remote->remote_group[index].name)
+	if (!remote->remotes[index].group.name)
 		return -ENOMEM;
 
 	error = __wacom_devm_sysfs_create_group(wacom, remote->remote_dir,
-						&remote->remote_group[index]);
+						&remote->remotes[index].group);
 	if (error) {
-		remote->remote_group[index].name = NULL;
+		remote->remotes[index].group.name = NULL;
 		hid_err(wacom->hdev,
 			"cannot create sysfs group err: %d\n", error);
 		return error;
@@ -1421,11 +1421,11 @@ static int wacom_initialize_remotes(struct wacom *wacom)
 		return -ENOMEM;
 	}
 
-	remote->remote_group[0] = remote0_serial_group;
-	remote->remote_group[1] = remote1_serial_group;
-	remote->remote_group[2] = remote2_serial_group;
-	remote->remote_group[3] = remote3_serial_group;
-	remote->remote_group[4] = remote4_serial_group;
+	remote->remotes[0].group = remote0_serial_group;
+	remote->remotes[1].group = remote1_serial_group;
+	remote->remotes[2].group = remote2_serial_group;
+	remote->remotes[3].group = remote3_serial_group;
+	remote->remotes[4].group = remote4_serial_group;
 
 	remote->remote_dir = kobject_create_and_add("wacom_remote",
 						    &wacom->hdev->dev.kobj);
@@ -1442,7 +1442,7 @@ static int wacom_initialize_remotes(struct wacom *wacom)
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
-		remote->serial[i] = 0;
+		remote->remotes[i].serial = 0;
 	}
 
 	error = devm_add_action_or_reset(&wacom->hdev->dev,
@@ -1903,16 +1903,17 @@ fail:
 static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
 {
 	struct wacom_remote *remote = wacom->remote;
-	u32 serial = remote->serial[index];
+	u32 serial = remote->remotes[index].serial;
 	int i;
 
-	if (remote->remote_group[index].name)
-		devres_release_group(&wacom->hdev->dev, &remote->serial[index]);
+	if (remote->remotes[index].group.name)
+		devres_release_group(&wacom->hdev->dev,
+				     &remote->remotes[index]);
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		if (remote->serial[i] == serial) {
-			remote->serial[i] = 0;
-			remote->remote_group[i].name = NULL;
+		if (remote->remotes[i].serial == serial) {
+			remote->remotes[i].serial = 0;
+			remote->remotes[i].group.name = NULL;
 			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
 		}
 	}
@@ -1929,30 +1930,30 @@ static int wacom_remote_create_one(struct wacom *wacom, u32 serial,
 	 * check to make sure this serial isn't already paired.
 	 */
 	for (k = 0; k < WACOM_MAX_REMOTES; k++) {
-		if (remote->serial[k] == serial)
+		if (remote->remotes[k].serial == serial)
 			break;
 	}
 
 	if (k < WACOM_MAX_REMOTES) {
-		remote->serial[index] = serial;
+		remote->remotes[index].serial = serial;
 		return 0;
 	}
 
-	if (!devres_open_group(dev, &remote->serial[index], GFP_KERNEL))
+	if (!devres_open_group(dev, &remote->remotes[index], GFP_KERNEL))
 		return -ENOMEM;
 
 	error = wacom_remote_create_attr_group(wacom, serial, index);
 	if (error)
 		goto fail;
 
-	remote->serial[index] = serial;
+	remote->remotes[index].serial = serial;
 
-	devres_close_group(dev, &remote->serial[index]);
+	devres_close_group(dev, &remote->remotes[index]);
 	return 0;
 
 fail:
-	devres_release_group(dev, &remote->serial[index]);
-	remote->serial[index] = 0;
+	devres_release_group(dev, &remote->remotes[index]);
+	remote->remotes[index].serial = 0;
 	return error;
 }
 
@@ -1986,15 +1987,15 @@ static void wacom_remote_work(struct work_struct *work)
 		serial = data.remote[i].serial;
 		if (data.remote[i].connected) {
 
-			if (remote->serial[i] == serial)
+			if (remote->remotes[i].serial == serial)
 				continue;
 
-			if (remote->serial[i])
+			if (remote->remotes[i].serial)
 				wacom_remote_destroy_one(wacom, i);
 
 			wacom_remote_create_one(wacom, serial, i);
 
-		} else if (remote->serial[i]) {
+		} else if (remote->remotes[i].serial) {
 			wacom_remote_destroy_one(wacom, i);
 		}
 	}
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index d5c180d..1162dc5 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -808,7 +808,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 	touch_ring_mode = (data[11] & 0xC0) >> 6;
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		if (remote->serial[i] == serial)
+		if (remote->remotes[i].serial == serial)
 			wacom->led.groups[i].select = touch_ring_mode;
 	}
 
-- 
2.5.5

[PATCH 20/27] HID: wacom: EKR: allocate one input node per remote

[Benjamin Tissoires]

Thanks to devres, we can now afford to create more than one input node
without having to overload the remove/failure paths. Having one input
node per remote is something which should have been implemented from start
but the probability of having users with several remotes is quite low.
Anyway, still, better looking at the future and implement things properly.

Remote input nodes will be freed/unregistered magically as they are
created in the devres group &remote->remotes[index].

We need to open the hid node now that the remotes are dynamically
allocated.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  2 ++
 drivers/hid/wacom_sys.c | 30 ++++++++++++++++++++++++++++++
 drivers/hid/wacom_wac.c | 35 ++++++++++++++++++++++++++++++-----
 3 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 6b8df67..393b5af 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -123,6 +123,8 @@ struct wacom_remote {
 	struct {
 		struct attribute_group group;
 		u32 serial;
+		struct input_dev *input;
+		bool registered;
 	} remotes[WACOM_MAX_REMOTES];
 };
 
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index c3b2692..f2f5b4b 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1905,6 +1905,11 @@ static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
 	struct wacom_remote *remote = wacom->remote;
 	u32 serial = remote->remotes[index].serial;
 	int i;
+	unsigned long flags;
+
+	spin_lock_irqsave(&remote->remote_lock, flags);
+	remote->remotes[index].registered = false;
+	spin_unlock_irqrestore(&remote->remote_lock, flags);
 
 	if (remote->remotes[index].group.name)
 		devres_release_group(&wacom->hdev->dev,
@@ -1914,6 +1919,7 @@ static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
 		if (remote->remotes[i].serial == serial) {
 			remote->remotes[i].serial = 0;
 			remote->remotes[i].group.name = NULL;
+			remote->remotes[i].registered = false;
 			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
 		}
 	}
@@ -1946,8 +1952,32 @@ static int wacom_remote_create_one(struct wacom *wacom, u32 serial,
 	if (error)
 		goto fail;
 
+	remote->remotes[index].input = wacom_allocate_input(wacom);
+	if (!remote->remotes[index].input) {
+		error = -ENOMEM;
+		goto fail;
+	}
+	remote->remotes[index].input->uniq = remote->remotes[index].group.name;
+	remote->remotes[index].input->name = wacom->wacom_wac.pad_name;
+
+	if (!remote->remotes[index].input->name) {
+		error = -EINVAL;
+		goto fail;
+	}
+
+	error = wacom_setup_pad_input_capabilities(remote->remotes[index].input,
+						   &wacom->wacom_wac);
+	if (error)
+		goto fail;
+
 	remote->remotes[index].serial = serial;
 
+	error = input_register_device(remote->remotes[index].input);
+	if (error)
+		goto fail;
+
+	remote->remotes[index].registered = true;
+
 	devres_close_group(dev, &remote->remotes[index]);
 	return 0;
 
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 1162dc5..d1210d0 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -751,23 +751,38 @@ static int wacom_intuos_inout(struct wacom_wac *wacom)
 static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 {
 	unsigned char *data = wacom_wac->data;
-	struct input_dev *input = wacom_wac->pad_input;
+	struct input_dev *input;
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
 	struct wacom_remote *remote = wacom->remote;
 	struct wacom_features *features = &wacom_wac->features;
 	int bat_charging, bat_percent, touch_ring_mode;
 	__u32 serial;
-	int i;
+	int i, index = -1;
+	unsigned long flags;
 
 	if (data[0] != WACOM_REPORT_REMOTE) {
-		dev_dbg(input->dev.parent,
-			"%s: received unknown report #%d", __func__, data[0]);
+		hid_dbg(wacom->hdev, "%s: received unknown report #%d",
+			__func__, data[0]);
 		return 0;
 	}
 
 	serial = data[3] + (data[4] << 8) + (data[5] << 16);
 	wacom_wac->id[0] = PAD_DEVICE_ID;
 
+	spin_lock_irqsave(&remote->remote_lock, flags);
+
+	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
+		if (remote->remotes[i].serial == serial) {
+			index = i;
+			break;
+		}
+	}
+
+	if (index < 0 || !remote->remotes[index].registered)
+		goto out;
+
+	input = remote->remotes[index].input;
+
 	input_report_key(input, BTN_0, (data[9] & 0x01));
 	input_report_key(input, BTN_1, (data[9] & 0x02));
 	input_report_key(input, BTN_2, (data[9] & 0x04));
@@ -804,6 +819,8 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 
 	input_event(input, EV_MSC, MSC_SERIAL, serial);
 
+	input_sync(input);
+
 	/*Which mode select (LED light) is currently on?*/
 	touch_ring_mode = (data[11] & 0xC0) >> 6;
 
@@ -821,7 +838,9 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 	wacom_notify_battery(wacom_wac, bat_percent, bat_charging, 1,
 			     bat_charging);
 
-	return 1;
+out:
+	spin_unlock_irqrestore(&remote->remote_lock, flags);
+	return 0;
 }
 
 static int wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
@@ -2456,6 +2475,9 @@ void wacom_setup_device_quirks(struct wacom *wacom)
 			features->quirks |= WACOM_QUIRK_BATTERY;
 		}
 	}
+
+	if (features->type == REMOTE)
+		features->device_type |= WACOM_DEVICETYPE_WL_MONITOR;
 }
 
 int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
@@ -2760,6 +2782,9 @@ int wacom_setup_pad_input_capabilities(struct input_dev *input_dev,
 	if (!(features->device_type & WACOM_DEVICETYPE_PAD))
 		return -ENODEV;
 
+	if (features->type == REMOTE && input_dev == wacom_wac->pad_input)
+		return -ENODEV;
+
 	input_dev->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
 
 	/* kept for making legacy xf86-input-wacom working with the wheels */
-- 
2.5.5

[PATCH 21/27] HID: wacom: EKR: have one power_supply per remote

[Benjamin Tissoires]

Previously, all the remotes attached to the same receiver would share the
same power_supply. That's not good as the remotes will constantly change
the battery information according to their own state.

To have something generic enough, we introduce struct wacom_battery
which regroups all the information we need for a battery.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  19 ++++++--
 drivers/hid/wacom_sys.c | 123 ++++++++++++++++++++++++++----------------------
 drivers/hid/wacom_wac.c |  55 ++++++++++++----------
 drivers/hid/wacom_wac.h |   6 ---
 4 files changed, 111 insertions(+), 92 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 393b5af..768d696 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -116,6 +116,19 @@ struct wacom_group_leds {
 	u8 select; /* status led selector (0..3) */
 };
 
+struct wacom_battery {
+	struct power_supply_desc bat_desc;
+	struct power_supply_desc ac_desc;
+	struct power_supply *battery;
+	struct power_supply *ac;
+	char bat_name[WACOM_NAME_MAX];
+	char ac_name[WACOM_NAME_MAX];
+	int battery_capacity;
+	int bat_charging;
+	int bat_connected;
+	int ps_connected;
+};
+
 struct wacom_remote {
 	spinlock_t remote_lock;
 	struct kfifo remote_fifo;
@@ -125,6 +138,7 @@ struct wacom_remote {
 		u32 serial;
 		struct input_dev *input;
 		bool registered;
+		struct wacom_battery battery;
 	} remotes[WACOM_MAX_REMOTES];
 };
 
@@ -144,10 +158,7 @@ struct wacom {
 		u8 hlv;       /* status led brightness button pressed (1..127) */
 		u8 img_lum;   /* OLED matrix display brightness */
 	} led;
-	struct power_supply *battery;
-	struct power_supply *ac;
-	struct power_supply_desc battery_desc;
-	struct power_supply_desc ac_desc;
+	struct wacom_battery battery;
 	bool resources;
 };
 
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index f2f5b4b..04f5c75 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1101,27 +1101,26 @@ static int wacom_battery_get_property(struct power_supply *psy,
 				      enum power_supply_property psp,
 				      union power_supply_propval *val)
 {
-	struct wacom *wacom = power_supply_get_drvdata(psy);
+	struct wacom_battery *battery = power_supply_get_drvdata(psy);
 	int ret = 0;
 
 	switch (psp) {
 		case POWER_SUPPLY_PROP_PRESENT:
-			val->intval = wacom->wacom_wac.bat_connected;
+			val->intval = battery->bat_connected;
 			break;
 		case POWER_SUPPLY_PROP_SCOPE:
 			val->intval = POWER_SUPPLY_SCOPE_DEVICE;
 			break;
 		case POWER_SUPPLY_PROP_CAPACITY:
-			val->intval =
-				wacom->wacom_wac.battery_capacity;
+			val->intval = battery->battery_capacity;
 			break;
 		case POWER_SUPPLY_PROP_STATUS:
-			if (wacom->wacom_wac.bat_charging)
+			if (battery->bat_charging)
 				val->intval = POWER_SUPPLY_STATUS_CHARGING;
-			else if (wacom->wacom_wac.battery_capacity == 100 &&
-				    wacom->wacom_wac.ps_connected)
+			else if (battery->battery_capacity == 100 &&
+				    battery->ps_connected)
 				val->intval = POWER_SUPPLY_STATUS_FULL;
-			else if (wacom->wacom_wac.ps_connected)
+			else if (battery->ps_connected)
 				val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
 			else
 				val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
@@ -1138,14 +1137,14 @@ static int wacom_ac_get_property(struct power_supply *psy,
 				enum power_supply_property psp,
 				union power_supply_propval *val)
 {
-	struct wacom *wacom = power_supply_get_drvdata(psy);
+	struct wacom_battery *battery = power_supply_get_drvdata(psy);
 	int ret = 0;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_PRESENT:
 		/* fall through */
 	case POWER_SUPPLY_PROP_ONLINE:
-		val->intval = wacom->wacom_wac.ps_connected;
+		val->intval = battery->ps_connected;
 		break;
 	case POWER_SUPPLY_PROP_SCOPE:
 		val->intval = POWER_SUPPLY_SCOPE_DEVICE;
@@ -1157,58 +1156,56 @@ static int wacom_ac_get_property(struct power_supply *psy,
 	return ret;
 }
 
-static int wacom_initialize_battery(struct wacom *wacom)
+static int __wacom_initialize_battery(struct wacom *wacom,
+				      struct wacom_battery *battery)
 {
 	static atomic_t battery_no = ATOMIC_INIT(0);
 	struct device *dev = &wacom->hdev->dev;
-	struct power_supply_config psy_cfg = { .drv_data = wacom, };
-	struct power_supply_desc *bat_desc = &wacom->battery_desc;
+	struct power_supply_config psy_cfg = { .drv_data = battery, };
+	struct power_supply *ps_bat, *ps_ac;
+	struct power_supply_desc *bat_desc = &battery->bat_desc;
+	struct power_supply_desc *ac_desc = &battery->ac_desc;
 	unsigned long n;
 	int error;
 
 	if (!devres_open_group(dev, bat_desc, GFP_KERNEL))
 		return -ENOMEM;
 
-	if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
-		struct power_supply_desc *ac_desc = &wacom->ac_desc;
-		n = atomic_inc_return(&battery_no) - 1;
-
-		bat_desc->properties = wacom_battery_props;
-		bat_desc->num_properties = ARRAY_SIZE(wacom_battery_props);
-		bat_desc->get_property = wacom_battery_get_property;
-		sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
-		bat_desc->name = wacom->wacom_wac.bat_name;
-		bat_desc->type = POWER_SUPPLY_TYPE_BATTERY;
-		bat_desc->use_for_apm = 0;
-
-		ac_desc->properties = wacom_ac_props;
-		ac_desc->num_properties = ARRAY_SIZE(wacom_ac_props);
-		ac_desc->get_property = wacom_ac_get_property;
-		sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
-		ac_desc->name = wacom->wacom_wac.ac_name;
-		ac_desc->type = POWER_SUPPLY_TYPE_MAINS;
-		ac_desc->use_for_apm = 0;
-
-		wacom->battery = devm_power_supply_register(dev,
-							   &wacom->battery_desc,
-							   &psy_cfg);
-		if (IS_ERR(wacom->battery)) {
-			error = PTR_ERR(wacom->battery);
-			goto err;
-		}
+	n = atomic_inc_return(&battery_no) - 1;
+
+	bat_desc->properties = wacom_battery_props;
+	bat_desc->num_properties = ARRAY_SIZE(wacom_battery_props);
+	bat_desc->get_property = wacom_battery_get_property;
+	sprintf(battery->bat_name, "wacom_battery_%ld", n);
+	bat_desc->name = battery->bat_name;
+	bat_desc->type = POWER_SUPPLY_TYPE_BATTERY;
+	bat_desc->use_for_apm = 0;
+
+	ac_desc->properties = wacom_ac_props;
+	ac_desc->num_properties = ARRAY_SIZE(wacom_ac_props);
+	ac_desc->get_property = wacom_ac_get_property;
+	sprintf(battery->ac_name, "wacom_ac_%ld", n);
+	ac_desc->name = battery->ac_name;
+	ac_desc->type = POWER_SUPPLY_TYPE_MAINS;
+	ac_desc->use_for_apm = 0;
+
+	ps_bat = devm_power_supply_register(dev, bat_desc, &psy_cfg);
+	if (IS_ERR(ps_bat)) {
+		error = PTR_ERR(ps_bat);
+		goto err;
+	}
 
-		power_supply_powers(wacom->battery, &wacom->hdev->dev);
+	ps_ac = devm_power_supply_register(dev, ac_desc, &psy_cfg);
+	if (IS_ERR(ps_ac)) {
+		error = PTR_ERR(ps_ac);
+		goto err;
+	}
 
-		wacom->ac = devm_power_supply_register(dev,
-						       &wacom->ac_desc,
-						       &psy_cfg);
-		if (IS_ERR(wacom->ac)) {
-			error = PTR_ERR(wacom->ac);
-			goto err;
-		}
+	power_supply_powers(ps_bat, &wacom->hdev->dev);
+	power_supply_powers(ps_ac, &wacom->hdev->dev);
 
-		power_supply_powers(wacom->ac, &wacom->hdev->dev);
-	}
+	battery->battery = ps_bat;
+	battery->ac = ps_ac;
 
 	devres_close_group(dev, bat_desc);
 	return 0;
@@ -1218,12 +1215,21 @@ err:
 	return error;
 }
 
+static int wacom_initialize_battery(struct wacom *wacom)
+{
+	if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY)
+		return __wacom_initialize_battery(wacom, &wacom->battery);
+
+	return 0;
+}
+
 static void wacom_destroy_battery(struct wacom *wacom)
 {
-	if (wacom->battery) {
-		devres_release_group(&wacom->hdev->dev, &wacom->battery_desc);
-		wacom->battery = NULL;
-		wacom->ac = NULL;
+	if (wacom->battery.battery) {
+		devres_release_group(&wacom->hdev->dev,
+				     &wacom->battery.bat_desc);
+		wacom->battery.battery = NULL;
+		wacom->battery.ac = NULL;
 	}
 }
 
@@ -1593,11 +1599,11 @@ void wacom_battery_work(struct work_struct *work)
 	struct wacom *wacom = container_of(work, struct wacom, battery_work);
 
 	if ((wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) &&
-	     !wacom->battery) {
+	     !wacom->battery.battery) {
 		wacom_initialize_battery(wacom);
 	}
 	else if (!(wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) &&
-		 wacom->battery) {
+		 wacom->battery.battery) {
 		wacom_destroy_battery(wacom);
 	}
 }
@@ -1976,6 +1982,11 @@ static int wacom_remote_create_one(struct wacom *wacom, u32 serial,
 	if (error)
 		goto fail;
 
+	error = __wacom_initialize_battery(wacom,
+					   &remote->remotes[index].battery);
+	if (error)
+		goto fail;
+
 	remote->remotes[index].registered = true;
 
 	devres_close_group(dev, &remote->remotes[index]);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index d1210d0..6904e9b 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -48,25 +48,37 @@ static unsigned short batcap_gr[8] = { 1, 15, 25, 35, 50, 70, 100, 100 };
  */
 static unsigned short batcap_i4[8] = { 1, 15, 30, 45, 60, 70, 85, 100 };
 
+static void __wacom_notify_battery(struct wacom_battery *battery,
+				   int bat_capacity, bool bat_charging,
+				   bool bat_connected, bool ps_connected)
+{
+	bool changed = battery->battery_capacity != bat_capacity  ||
+		       battery->bat_charging     != bat_charging  ||
+		       battery->bat_connected    != bat_connected ||
+		       battery->ps_connected     != ps_connected;
+
+	if (changed) {
+		battery->battery_capacity = bat_capacity;
+		battery->bat_charging = bat_charging;
+		battery->bat_connected = bat_connected;
+		battery->ps_connected = ps_connected;
+
+		if (battery->battery)
+			power_supply_changed(battery->battery);
+	}
+}
+
 static void wacom_notify_battery(struct wacom_wac *wacom_wac,
 	int bat_capacity, bool bat_charging, bool bat_connected,
 	bool ps_connected)
 {
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
-	bool changed = wacom_wac->battery_capacity != bat_capacity  ||
-		       wacom_wac->bat_charging     != bat_charging  ||
-		       wacom_wac->bat_connected    != bat_connected ||
-		       wacom_wac->ps_connected     != ps_connected;
 
-	if (changed) {
-		wacom_wac->battery_capacity = bat_capacity;
-		wacom_wac->bat_charging = bat_charging;
-		wacom_wac->bat_connected = bat_connected;
-		wacom_wac->ps_connected = ps_connected;
+	if (!wacom->battery.battery)
+		return;
 
-		if (wacom->battery)
-			power_supply_changed(wacom->battery);
-	}
+	__wacom_notify_battery(&wacom->battery, bat_capacity, bat_charging,
+			       bat_connected, ps_connected);
 }
 
 static int wacom_penpartner_irq(struct wacom_wac *wacom)
@@ -754,7 +766,6 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 	struct input_dev *input;
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
 	struct wacom_remote *remote = wacom->remote;
-	struct wacom_features *features = &wacom_wac->features;
 	int bat_charging, bat_percent, touch_ring_mode;
 	__u32 serial;
 	int i, index = -1;
@@ -829,14 +840,8 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 			wacom->led.groups[i].select = touch_ring_mode;
 	}
 
-	if (!wacom->battery &&
-	    !(features->quirks & WACOM_QUIRK_BATTERY)) {
-		features->quirks |= WACOM_QUIRK_BATTERY;
-		wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
-	}
-
-	wacom_notify_battery(wacom_wac, bat_percent, bat_charging, 1,
-			     bat_charging);
+	__wacom_notify_battery(&remote->remotes[index].battery, bat_percent,
+				bat_charging, 1, bat_charging);
 
 out:
 	spin_unlock_irqrestore(&remote->remote_lock, flags);
@@ -2132,7 +2137,6 @@ static int wacom_bamboo_pad_irq(struct wacom_wac *wacom, size_t len)
 
 static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
 {
-	struct wacom *w = container_of(wacom, struct wacom, wacom_wac);
 	unsigned char *data = wacom->data;
 	int connected;
 
@@ -2160,8 +2164,7 @@ static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
 			wacom_schedule_work(wacom, WACOM_WORKER_WIRELESS);
 		}
 
-		if (w->battery)
-			wacom_notify_battery(wacom, battery, charging, 1, 0);
+		wacom_notify_battery(wacom, battery, charging, 1, 0);
 
 	} else if (wacom->pid != 0) {
 		/* disconnected while previously connected */
@@ -2198,14 +2201,14 @@ static int wacom_status_irq(struct wacom_wac *wacom_wac, size_t len)
 		wacom_notify_battery(wacom_wac, battery, charging,
 				     battery || charging, 1);
 
-		if (!wacom->battery &&
+		if (!wacom->battery.battery &&
 		    !(features->quirks & WACOM_QUIRK_BATTERY)) {
 			features->quirks |= WACOM_QUIRK_BATTERY;
 			wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
 		}
 	}
 	else if ((features->quirks & WACOM_QUIRK_BATTERY) &&
-		 wacom->battery) {
+		 wacom->battery.battery) {
 		features->quirks &= ~WACOM_QUIRK_BATTERY;
 		wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
 		wacom_notify_battery(wacom_wac, 0, 0, 0, 0);
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 6be6cae..8a8974c 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -229,8 +229,6 @@ struct wacom_wac {
 	char pen_name[WACOM_NAME_MAX];
 	char touch_name[WACOM_NAME_MAX];
 	char pad_name[WACOM_NAME_MAX];
-	char bat_name[WACOM_NAME_MAX];
-	char ac_name[WACOM_NAME_MAX];
 	unsigned char data[WACOM_PKGLEN_MAX];
 	int tool[2];
 	int id[2];
@@ -242,11 +240,7 @@ struct wacom_wac {
 	struct input_dev *touch_input;
 	struct input_dev *pad_input;
 	int pid;
-	int battery_capacity;
 	int num_contacts_left;
-	int bat_charging;
-	int bat_connected;
-	int ps_connected;
 	u8 bt_features;
 	u8 bt_high_speed;
 	int mode_report;
-- 
2.5.5

[PATCH 22/27] HID: wacom: EKR: attach the power_supply on first connection

[Benjamin Tissoires]

Or Gnome complains about an empty battery.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 04f5c75..1d79215 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1917,6 +1917,10 @@ static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
 	remote->remotes[index].registered = false;
 	spin_unlock_irqrestore(&remote->remote_lock, flags);
 
+	if (remote->remotes[index].battery.battery)
+		devres_release_group(&wacom->hdev->dev,
+				     &remote->remotes[index].battery.bat_desc);
+
 	if (remote->remotes[index].group.name)
 		devres_release_group(&wacom->hdev->dev,
 				     &remote->remotes[index]);
@@ -1926,6 +1930,7 @@ static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
 			remote->remotes[i].serial = 0;
 			remote->remotes[i].group.name = NULL;
 			remote->remotes[i].registered = false;
+			remote->remotes[i].battery.battery = NULL;
 			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
 		}
 	}
@@ -1982,11 +1987,6 @@ static int wacom_remote_create_one(struct wacom *wacom, u32 serial,
 	if (error)
 		goto fail;
 
-	error = __wacom_initialize_battery(wacom,
-					   &remote->remotes[index].battery);
-	if (error)
-		goto fail;
-
 	remote->remotes[index].registered = true;
 
 	devres_close_group(dev, &remote->remotes[index]);
@@ -1998,6 +1998,28 @@ fail:
 	return error;
 }
 
+static int wacom_remote_attach_battery(struct wacom *wacom, int index)
+{
+	struct wacom_remote *remote = wacom->remote;
+	int error;
+
+	if (!remote->remotes[index].registered)
+		return 0;
+
+	if (remote->remotes[index].battery.battery)
+		return 0;
+
+	if (wacom->led.groups[index].select == WACOM_STATUS_UNKNOWN)
+		return 0;
+
+	error = __wacom_initialize_battery(wacom,
+					&wacom->remote->remotes[index].battery);
+	if (error)
+		return error;
+
+	return 0;
+}
+
 static void wacom_remote_work(struct work_struct *work)
 {
 	struct wacom *wacom = container_of(work, struct wacom, remote_work);
@@ -2028,8 +2050,10 @@ static void wacom_remote_work(struct work_struct *work)
 		serial = data.remote[i].serial;
 		if (data.remote[i].connected) {
 
-			if (remote->remotes[i].serial == serial)
+			if (remote->remotes[i].serial == serial) {
+				wacom_remote_attach_battery(wacom, i);
 				continue;
+			}
 
 			if (remote->remotes[i].serial)
 				wacom_remote_destroy_one(wacom, i);
-- 
2.5.5

Re: [PATCH 22/27] HID: wacom: EKR: attach the power_supply on first connection

[Benjamin Tissoires]

On Jul 08 2016 or thereabouts, Aaron Armstrong Skomra wrote:
> On Tue, Jul 5, 2016 at 7:39 AM, Benjamin Tissoires <
> benjamin.tissoires@redhat.com> wrote:
> 
> > Or Gnome complains about an empty battery.
> >
> > Hi Benjamin,
> 
> I tested this series on the 24HD, 21UX2, Intuos P&T (CTH-680), and with 2
> EKRs.

I somehow missed this email. Sorry for the late answer.

> 
> When I attach (by USB wire) a wireless capable tablet with the rechargeable
> battery and wireless module connected inside the tablet, I get the same
> momentary 0% battery notification from gnome (see attached upower output).

Yes, I saw that, but I think it's not a regression. It's the way it's
already working now (from what I can see), and so it would be nice to
be fixed, but I didn't in this series.

> 
> Aside from that
> 
> Tested-by: Aaron Armstrong Skomra <aaron.skomra@wacom.com>

Thanks!

But given Peter's comments regarding grouping, I think I'll need you to
redo the tests on the 24HD and 21UX2 with the next series as the initial
kernel implementation of LEDs needs to be reverted, or libwacom will
fail :(

Cheers,
Benjamin

> 
> Best,
> Aaron
> 
> 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  drivers/hid/wacom_sys.c | 36 ++++++++++++++++++++++++++++++------
> >  1 file changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index 04f5c75..1d79215 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -1917,6 +1917,10 @@ static void wacom_remote_destroy_one(struct wacom
> > *wacom, unsigned int index)
> >         remote->remotes[index].registered = false;
> >         spin_unlock_irqrestore(&remote->remote_lock, flags);
> >
> > +       if (remote->remotes[index].battery.battery)
> > +               devres_release_group(&wacom->hdev->dev,
> > +
> > &remote->remotes[index].battery.bat_desc);
> > +
> >         if (remote->remotes[index].group.name)
> >                 devres_release_group(&wacom->hdev->dev,
> >                                      &remote->remotes[index]);
> > @@ -1926,6 +1930,7 @@ static void wacom_remote_destroy_one(struct wacom
> > *wacom, unsigned int index)
> >                         remote->remotes[i].serial = 0;
> >                         remote->remotes[i].group.name = NULL;
> >                         remote->remotes[i].registered = false;
> > +                       remote->remotes[i].battery.battery = NULL;
> >                         wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
> >                 }
> >         }
> > @@ -1982,11 +1987,6 @@ static int wacom_remote_create_one(struct wacom
> > *wacom, u32 serial,
> >         if (error)
> >                 goto fail;
> >
> > -       error = __wacom_initialize_battery(wacom,
> > -
> > &remote->remotes[index].battery);
> > -       if (error)
> > -               goto fail;
> > -
> >         remote->remotes[index].registered = true;
> >
> >         devres_close_group(dev, &remote->remotes[index]);
> > @@ -1998,6 +1998,28 @@ fail:
> >         return error;
> >  }
> >
> > +static int wacom_remote_attach_battery(struct wacom *wacom, int index)
> > +{
> > +       struct wacom_remote *remote = wacom->remote;
> > +       int error;
> > +
> > +       if (!remote->remotes[index].registered)
> > +               return 0;
> > +
> > +       if (remote->remotes[index].battery.battery)
> > +               return 0;
> > +
> > +       if (wacom->led.groups[index].select == WACOM_STATUS_UNKNOWN)
> > +               return 0;
> > +
> > +       error = __wacom_initialize_battery(wacom,
> > +
> >  &wacom->remote->remotes[index].battery);
> > +       if (error)
> > +               return error;
> > +
> > +       return 0;
> > +}
> > +
> >  static void wacom_remote_work(struct work_struct *work)
> >  {
> >         struct wacom *wacom = container_of(work, struct wacom,
> > remote_work);
> > @@ -2028,8 +2050,10 @@ static void wacom_remote_work(struct work_struct
> > *work)
> >                 serial = data.remote[i].serial;
> >                 if (data.remote[i].connected) {
> >
> > -                       if (remote->remotes[i].serial == serial)
> > +                       if (remote->remotes[i].serial == serial) {
> > +                               wacom_remote_attach_battery(wacom, i);
> >                                 continue;
> > +                       }
> >
> >                         if (remote->remotes[i].serial)
> >                                 wacom_remote_destroy_one(wacom, i);
> > --
> > 2.5.5
> >
> >

> [wacom@localhost ~]$ upower -d
> Device: /org/freedesktop/UPower/devices/mouse_wacom_battery_7
>   native-path:          wacom_battery_7
>   power supply:         no
>   updated:              Thu 07 Jul 2016 01:55:12 PM PDT (1 seconds ago)
>   has history:          yes
>   has statistics:       yes
>   mouse
>     present:             yes
>     rechargeable:        yes
>     state:               discharging
>     warning-level:       critical
>     percentage:          0%
>     icon-name:          'battery-caution-symbolic'
>   History (charge):
>     1467924912	0.000	unknown
>     1467924886	96.000	charging
>     1467924880	0.000	unknown
>   History (rate):
>     1467924912	0.000	unknown
>     1467924880	0.000	unknown
> 
> Device: /org/freedesktop/UPower/devices/DisplayDevice
>   power supply:         no
>   updated:              Thu 07 Jul 2016 12:09:09 PM PDT (6364 seconds ago)
>   has history:          no
>   has statistics:       no
>   unknown
>     warning-level:       none
>     icon-name:          ''
> 
> Daemon:
>   daemon-version:  0.99.4
>   on-battery:      no
>   lid-is-closed:   no
>   lid-is-present:  no
>   critical-action: PowerOff
> [wacom@localhost ~]$ upower -d
> Device: /org/freedesktop/UPower/devices/mouse_wacom_battery_7
>   native-path:          wacom_battery_7
>   power supply:         no
>   updated:              Thu 07 Jul 2016 01:55:18 PM PDT (3 seconds ago)
>   has history:          yes
>   has statistics:       yes
>   mouse
>     present:             yes
>     rechargeable:        yes
>     state:               charging
>     warning-level:       none
>     percentage:          96%
>     icon-name:          'battery-full-charging-symbolic'
>   History (charge):
>     1467924918	96.000	charging
>     1467924912	0.000	unknown
>     1467924886	96.000	charging
>     1467924880	0.000	unknown
>   History (rate):
>     1467924912	0.000	unknown
>     1467924880	0.000	unknown
> 
> Device: /org/freedesktop/UPower/devices/DisplayDevice
>   power supply:         no
>   updated:              Thu 07 Jul 2016 12:09:09 PM PDT (6372 seconds ago)
>   has history:          no
>   has statistics:       no
>   unknown
>     warning-level:       none
>     icon-name:          ''
> 
> Daemon:
>   daemon-version:  0.99.4
>   on-battery:      no
>   lid-is-closed:   no
>   lid-is-present:  no
>   critical-action: PowerOff
> 

Re: [PATCH 22/27] HID: wacom: EKR: attach the power_supply on first connection

[Benjamin Tissoires]

On Jul 12 2016 or thereabouts, Benjamin Tissoires wrote:
> On Jul 08 2016 or thereabouts, Aaron Armstrong Skomra wrote:
> > On Tue, Jul 5, 2016 at 7:39 AM, Benjamin Tissoires <
> > benjamin.tissoires@redhat.com> wrote:
> > 
> > > Or Gnome complains about an empty battery.
> > >
> > > Hi Benjamin,
> > 
> > I tested this series on the 24HD, 21UX2, Intuos P&T (CTH-680), and with 2
> > EKRs.
> 
> I somehow missed this email. Sorry for the late answer.
> 
> > 
> > When I attach (by USB wire) a wireless capable tablet with the rechargeable
> > battery and wireless module connected inside the tablet, I get the same
> > momentary 0% battery notification from gnome (see attached upower output).
> 
> Yes, I saw that, but I think it's not a regression. It's the way it's
> already working now (from what I can see), and so it would be nice to
> be fixed, but I didn't in this series.

My bad. I found the issue and it was added by my patches :( It's fixed
now and will be in v2 (coming shortly).

Cheers,
Benjamin

> 
> > 
> > Aside from that
> > 
> > Tested-by: Aaron Armstrong Skomra <aaron.skomra@wacom.com>
> 
> Thanks!
> 
> But given Peter's comments regarding grouping, I think I'll need you to
> redo the tests on the 24HD and 21UX2 with the next series as the initial
> kernel implementation of LEDs needs to be reverted, or libwacom will
> fail :(
> 
> Cheers,
> Benjamin
> 
> > 
> > Best,
> > Aaron
> > 
> > 
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > ---
> > >  drivers/hid/wacom_sys.c | 36 ++++++++++++++++++++++++++++++------
> > >  1 file changed, 30 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > > index 04f5c75..1d79215 100644
> > > --- a/drivers/hid/wacom_sys.c
> > > +++ b/drivers/hid/wacom_sys.c
> > > @@ -1917,6 +1917,10 @@ static void wacom_remote_destroy_one(struct wacom
> > > *wacom, unsigned int index)
> > >         remote->remotes[index].registered = false;
> > >         spin_unlock_irqrestore(&remote->remote_lock, flags);
> > >
> > > +       if (remote->remotes[index].battery.battery)
> > > +               devres_release_group(&wacom->hdev->dev,
> > > +
> > > &remote->remotes[index].battery.bat_desc);
> > > +
> > >         if (remote->remotes[index].group.name)
> > >                 devres_release_group(&wacom->hdev->dev,
> > >                                      &remote->remotes[index]);
> > > @@ -1926,6 +1930,7 @@ static void wacom_remote_destroy_one(struct wacom
> > > *wacom, unsigned int index)
> > >                         remote->remotes[i].serial = 0;
> > >                         remote->remotes[i].group.name = NULL;
> > >                         remote->remotes[i].registered = false;
> > > +                       remote->remotes[i].battery.battery = NULL;
> > >                         wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
> > >                 }
> > >         }
> > > @@ -1982,11 +1987,6 @@ static int wacom_remote_create_one(struct wacom
> > > *wacom, u32 serial,
> > >         if (error)
> > >                 goto fail;
> > >
> > > -       error = __wacom_initialize_battery(wacom,
> > > -
> > > &remote->remotes[index].battery);
> > > -       if (error)
> > > -               goto fail;
> > > -
> > >         remote->remotes[index].registered = true;
> > >
> > >         devres_close_group(dev, &remote->remotes[index]);
> > > @@ -1998,6 +1998,28 @@ fail:
> > >         return error;
> > >  }
> > >
> > > +static int wacom_remote_attach_battery(struct wacom *wacom, int index)
> > > +{
> > > +       struct wacom_remote *remote = wacom->remote;
> > > +       int error;
> > > +
> > > +       if (!remote->remotes[index].registered)
> > > +               return 0;
> > > +
> > > +       if (remote->remotes[index].battery.battery)
> > > +               return 0;
> > > +
> > > +       if (wacom->led.groups[index].select == WACOM_STATUS_UNKNOWN)
> > > +               return 0;
> > > +
> > > +       error = __wacom_initialize_battery(wacom,
> > > +
> > >  &wacom->remote->remotes[index].battery);
> > > +       if (error)
> > > +               return error;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  static void wacom_remote_work(struct work_struct *work)
> > >  {
> > >         struct wacom *wacom = container_of(work, struct wacom,
> > > remote_work);
> > > @@ -2028,8 +2050,10 @@ static void wacom_remote_work(struct work_struct
> > > *work)
> > >                 serial = data.remote[i].serial;
> > >                 if (data.remote[i].connected) {
> > >
> > > -                       if (remote->remotes[i].serial == serial)
> > > +                       if (remote->remotes[i].serial == serial) {
> > > +                               wacom_remote_attach_battery(wacom, i);
> > >                                 continue;
> > > +                       }
> > >
> > >                         if (remote->remotes[i].serial)
> > >                                 wacom_remote_destroy_one(wacom, i);
> > > --
> > > 2.5.5
> > >
> > >
> 
> > [wacom@localhost ~]$ upower -d
> > Device: /org/freedesktop/UPower/devices/mouse_wacom_battery_7
> >   native-path:          wacom_battery_7
> >   power supply:         no
> >   updated:              Thu 07 Jul 2016 01:55:12 PM PDT (1 seconds ago)
> >   has history:          yes
> >   has statistics:       yes
> >   mouse
> >     present:             yes
> >     rechargeable:        yes
> >     state:               discharging
> >     warning-level:       critical
> >     percentage:          0%
> >     icon-name:          'battery-caution-symbolic'
> >   History (charge):
> >     1467924912	0.000	unknown
> >     1467924886	96.000	charging
> >     1467924880	0.000	unknown
> >   History (rate):
> >     1467924912	0.000	unknown
> >     1467924880	0.000	unknown
> > 
> > Device: /org/freedesktop/UPower/devices/DisplayDevice
> >   power supply:         no
> >   updated:              Thu 07 Jul 2016 12:09:09 PM PDT (6364 seconds ago)
> >   has history:          no
> >   has statistics:       no
> >   unknown
> >     warning-level:       none
> >     icon-name:          ''
> > 
> > Daemon:
> >   daemon-version:  0.99.4
> >   on-battery:      no
> >   lid-is-closed:   no
> >   lid-is-present:  no
> >   critical-action: PowerOff
> > [wacom@localhost ~]$ upower -d
> > Device: /org/freedesktop/UPower/devices/mouse_wacom_battery_7
> >   native-path:          wacom_battery_7
> >   power supply:         no
> >   updated:              Thu 07 Jul 2016 01:55:18 PM PDT (3 seconds ago)
> >   has history:          yes
> >   has statistics:       yes
> >   mouse
> >     present:             yes
> >     rechargeable:        yes
> >     state:               charging
> >     warning-level:       none
> >     percentage:          96%
> >     icon-name:          'battery-full-charging-symbolic'
> >   History (charge):
> >     1467924918	96.000	charging
> >     1467924912	0.000	unknown
> >     1467924886	96.000	charging
> >     1467924880	0.000	unknown
> >   History (rate):
> >     1467924912	0.000	unknown
> >     1467924880	0.000	unknown
> > 
> > Device: /org/freedesktop/UPower/devices/DisplayDevice
> >   power supply:         no
> >   updated:              Thu 07 Jul 2016 12:09:09 PM PDT (6372 seconds ago)
> >   has history:          no
> >   has statistics:       no
> >   unknown
> >     warning-level:       none
> >     icon-name:          ''
> > 
> > Daemon:
> >   daemon-version:  0.99.4
> >   on-battery:      no
> >   lid-is-closed:   no
> >   lid-is-present:  no
> >   critical-action: PowerOff
> > 
> 

[PATCH 23/27] HID: wacom: leds: use the ledclass instead of custom made sysfs files

[Benjamin Tissoires]

The now obsolete sysfs files for LEDs and EKRemote are kept for backward
compatibility.
Both the EKR (read-only) and the regular Cintiqs and Intuos are now
sharing the same led API.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 Documentation/ABI/testing/sysfs-driver-wacom |   5 +
 drivers/hid/wacom.h                          |  19 +++
 drivers/hid/wacom_sys.c                      | 191 +++++++++++++++++++++++++--
 3 files changed, 205 insertions(+), 10 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-wacom b/Documentation/ABI/testing/sysfs-driver-wacom
index dca4293..2aa5503 100644
--- a/Documentation/ABI/testing/sysfs-driver-wacom
+++ b/Documentation/ABI/testing/sysfs-driver-wacom
@@ -24,6 +24,7 @@ What:		/sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/wacom_led/status0_luminance
 Date:		August 2014
 Contact:	linux-input@vger.kernel.org
 Description:
+		<obsoleted by the LED class API now exported by the driver>
 		Writing to this file sets the status LED luminance (1..127)
 		when the stylus does not touch the tablet surface, and no
 		button is pressed on the stylus. This luminance level is
@@ -33,6 +34,7 @@ What:		/sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/wacom_led/status1_luminance
 Date:		August 2014
 Contact:	linux-input@vger.kernel.org
 Description:
+		<obsoleted by the LED class API now exported by the driver>
 		Writing to this file sets the status LED luminance (1..127)
 		when the stylus touches the tablet surface, or any button is
 		pressed on the stylus.
@@ -41,6 +43,7 @@ What:		/sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/wacom_led/status_led0_select
 Date:		August 2014
 Contact:	linux-input@vger.kernel.org
 Description:
+		<obsoleted by the LED class API now exported by the driver>
 		Writing to this file sets which one of the four (for Intuos 4
 		and Intuos 5) or of the right four (for Cintiq 21UX2 and Cintiq
 		24HD) status LEDs is active (0..3). The other three LEDs on the
@@ -50,6 +53,7 @@ What:		/sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/wacom_led/status_led1_select
 Date:		August 2014
 Contact:	linux-input@vger.kernel.org
 Description:
+		<obsoleted by the LED class API now exported by the driver>
 		Writing to this file sets which one of the left four (for Cintiq 21UX2
 		and Cintiq 24HD) status LEDs is active (0..3). The other three LEDs on
 		the left are always inactive.
@@ -91,6 +95,7 @@ What:		/sys/bus/hid/devices/<bus>:<vid>:<pid>.<n>/wacom_remote/<serial_number>/r
 Date:		July 2015
 Contact:	linux-input@vger.kernel.org
 Description:
+		<obsoleted by the LED class API now exported by the driver>
 		Reading from this file reports the mode status of the
 		remote as indicated by the LED lights on the device. If no
 		reports have been received from the paired device, reading
diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 768d696..0c498c4 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -91,6 +91,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/hid.h>
 #include <linux/kfifo.h>
+#include <linux/leds.h>
 #include <linux/usb/input.h>
 #include <linux/power_supply.h>
 #include <asm/unaligned.h>
@@ -112,8 +113,23 @@ enum wacom_worker {
 	WACOM_WORKER_REMOTE,
 };
 
+struct wacom;
+
+struct wacom_led {
+	struct led_classdev cdev;
+	struct led_trigger trigger;
+	struct wacom *wacom;
+	unsigned int group;
+	unsigned int id;
+	u8 llv;
+	u8 hlv;
+	bool held;
+};
+
 struct wacom_group_leds {
 	u8 select; /* status led selector (0..3) */
+	struct wacom_led *leds;
+	unsigned int count;
 };
 
 struct wacom_battery {
@@ -154,9 +170,12 @@ struct wacom {
 	struct wacom_remote *remote;
 	struct wacom_leds {
 		struct wacom_group_leds *groups;
+		unsigned int count;
 		u8 llv;       /* status led brightness no button (1..127) */
 		u8 hlv;       /* status led brightness button pressed (1..127) */
 		u8 img_lum;   /* OLED matrix display brightness */
+		u8 max_llv;   /* maximum brightness of LED (llv) */
+		u8 max_hlv;   /* maximum brightness of LED (hlv) */
 	} led;
 	struct wacom_battery battery;
 	bool resources;
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 1d79215..2354d73 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -966,31 +966,193 @@ static int wacom_devm_sysfs_create_group(struct wacom *wacom,
 					       group);
 }
 
+static enum led_brightness wacom_leds_brightness_get(struct wacom_led *led)
+{
+	struct wacom *wacom = led->wacom;
+
+	if (wacom->led.max_hlv)
+		return led->hlv * LED_FULL / wacom->led.max_hlv;
+
+	if (wacom->led.max_llv)
+		return led->llv * LED_FULL / wacom->led.max_llv;
+
+	/* device doesn't support brightness tuning */
+	return LED_FULL;
+}
+
+static enum led_brightness __wacom_led_brightness_get(struct led_classdev *cdev)
+{
+	struct wacom_led *led = container_of(cdev, struct wacom_led, cdev);
+	struct wacom *wacom = led->wacom;
+
+	if (wacom->led.groups[led->group].select != led->id)
+		return LED_OFF;
+
+	return wacom_leds_brightness_get(led);
+}
+
+static int wacom_led_brightness_set(struct led_classdev *cdev,
+				    enum led_brightness brightness)
+{
+	struct wacom_led *led = container_of(cdev, struct wacom_led, cdev);
+	struct wacom *wacom = led->wacom;
+	int error;
+
+	mutex_lock(&wacom->lock);
+
+	if (!wacom->led.groups || (brightness == LED_OFF &&
+	    wacom->led.groups[led->group].select != led->id)) {
+		error = 0;
+		goto out;
+	}
+
+	led->llv = wacom->led.llv = wacom->led.max_llv * brightness / LED_FULL;
+	led->hlv = wacom->led.hlv = wacom->led.max_hlv * brightness / LED_FULL;
+
+	wacom->led.groups[led->group].select = led->id;
+
+	error = wacom_led_control(wacom);
+
+out:
+	mutex_unlock(&wacom->lock);
+
+	return error;
+}
+
+static void wacom_led_readonly_brightness_set(struct led_classdev *cdev,
+					       enum led_brightness brightness)
+{
+}
+
+static int wacom_led_register_one(struct device *dev, struct wacom *wacom,
+				  struct wacom_led *led, unsigned int group,
+				  unsigned int id, bool read_only)
+{
+	int error;
+	char *name;
+
+	name = devm_kasprintf(dev, GFP_KERNEL,
+			      "%s::wacom-led_%d.%d",
+			      dev_name(dev),
+			      group,
+			      id);
+	if (!name)
+		return -ENOMEM;
+
+	led->group = group;
+	led->id = id;
+	led->wacom = wacom;
+	led->llv = wacom->led.llv;
+	led->hlv = wacom->led.hlv;
+	led->cdev.name = name;
+	led->cdev.max_brightness = LED_FULL;
+	led->cdev.brightness_get = __wacom_led_brightness_get;
+	if (!read_only)
+		led->cdev.brightness_set_blocking = wacom_led_brightness_set;
+	else
+		led->cdev.brightness_set = wacom_led_readonly_brightness_set;
+
+	error = devm_led_classdev_register(dev, &led->cdev);
+	if (error) {
+		hid_err(wacom->hdev,
+			"failed to register LED %s: %d\n",
+			led->cdev.name, error);
+		led->cdev.name = NULL;
+		return error;
+	}
+
+	return 0;
+}
+
+static int wacom_led_groups_alloc_and_register_one(struct device *dev,
+						   struct wacom *wacom,
+						   int group_id, int count,
+						   bool read_only)
+{
+	struct wacom_led *leds;
+	int i, error;
+
+	if (group_id >= wacom->led.count || count <= 0)
+		return -EINVAL;
+
+	if (!devres_open_group(dev, &wacom->led.groups[group_id], GFP_KERNEL))
+		return -ENOMEM;
+
+	leds = devm_kzalloc(dev, sizeof(struct wacom_led) * count, GFP_KERNEL);
+	if (!leds) {
+		error = -ENOMEM;
+		goto err;
+	}
+
+	wacom->led.groups[group_id].leds = leds;
+	wacom->led.groups[group_id].count = count;
+
+	for (i = 0; i < count; i++) {
+		error = wacom_led_register_one(dev, wacom, &leds[i],
+					       group_id, i, read_only);
+		if (error)
+			goto err;
+	}
+
+	devres_remove_group(dev, &wacom->led.groups[group_id]);
+	return 0;
+
+err:
+	devres_release_group(dev, &wacom->led.groups[group_id]);
+	return error;
+}
+
 static void wacom_led_groups_release(void *data)
 {
 	struct wacom *wacom = data;
 
 	wacom->led.groups = NULL;
+	wacom->led.count = 0;
 }
 
 static int wacom_led_groups_allocate(struct wacom *wacom, int count)
 {
+	struct device *dev = &wacom->hdev->dev;
 	struct wacom_group_leds *groups;
 	int error;
 
-	groups = devm_kzalloc(&wacom->hdev->dev,
-			      sizeof(struct wacom_group_leds) * count,
+	groups = devm_kzalloc(dev, sizeof(struct wacom_group_leds) * count,
 			      GFP_KERNEL);
 	if (!groups)
 		return -ENOMEM;
 
-	error = devm_add_action_or_reset(&wacom->hdev->dev,
-					 wacom_led_groups_release,
-					 wacom);
+	error = devm_add_action_or_reset(dev, wacom_led_groups_release, wacom);
 	if (error)
 		return error;
 
 	wacom->led.groups = groups;
+	wacom->led.count = count;
+
+	return 0;
+}
+
+static int wacom_leds_alloc_and_register(struct wacom *wacom, int group_count,
+					 int led_per_group, bool read_only)
+{
+	struct device *dev;
+	int i, error;
+
+	if (!wacom->wacom_wac.pad_input)
+		return -EINVAL;
+
+	dev = &wacom->wacom_wac.pad_input->dev;
+
+	error = wacom_led_groups_allocate(wacom, group_count);
+	if (error)
+		return error;
+
+	for (i = 0; i < group_count; i++) {
+		error = wacom_led_groups_alloc_and_register_one(dev, wacom, i,
+								led_per_group,
+								read_only);
+		if (error)
+			return error;
+	}
 
 	return 0;
 }
@@ -1010,9 +1172,11 @@ static int wacom_initialize_leds(struct wacom *wacom)
 	case INTUOS4L:
 		wacom->led.llv = 10;
 		wacom->led.hlv = 20;
+		wacom->led.max_llv = 127;
+		wacom->led.max_hlv = 127;
 		wacom->led.img_lum = 10;
 
-		error = wacom_led_groups_allocate(wacom, 1);
+		error = wacom_leds_alloc_and_register(wacom, 1, 4, false);
 		if (error) {
 			hid_err(wacom->hdev,
 				"cannot create leds err: %d\n", error);
@@ -1029,7 +1193,7 @@ static int wacom_initialize_leds(struct wacom *wacom)
 		wacom->led.hlv = 0;
 		wacom->led.img_lum = 0;
 
-		error = wacom_led_groups_allocate(wacom, 2);
+		error = wacom_leds_alloc_and_register(wacom, 2, 4, false);
 		if (error) {
 			hid_err(wacom->hdev,
 				"cannot create leds err: %d\n", error);
@@ -1047,10 +1211,9 @@ static int wacom_initialize_leds(struct wacom *wacom)
 	case INTUOSPM:
 	case INTUOSPL:
 		wacom->led.llv = 32;
-		wacom->led.hlv = 0;
-		wacom->led.img_lum = 0;
+		wacom->led.max_llv = 96;
 
-		error = wacom_led_groups_allocate(wacom, 1);
+		error = wacom_leds_alloc_and_register(wacom, 1, 4, false);
 		if (error) {
 			hid_err(wacom->hdev,
 				"cannot create leds err: %d\n", error);
@@ -1062,6 +1225,8 @@ static int wacom_initialize_leds(struct wacom *wacom)
 		break;
 
 	case REMOTE:
+		wacom->led.llv = 255;
+		wacom->led.max_llv = 255;
 		error = wacom_led_groups_allocate(wacom, 5);
 		if (error) {
 			hid_err(wacom->hdev,
@@ -1987,6 +2152,12 @@ static int wacom_remote_create_one(struct wacom *wacom, u32 serial,
 	if (error)
 		goto fail;
 
+	error = wacom_led_groups_alloc_and_register_one(
+					&remote->remotes[index].input->dev,
+					wacom, index, 3, true);
+	if (error)
+		goto fail;
+
 	remote->remotes[index].registered = true;
 
 	devres_close_group(dev, &remote->remotes[index]);
-- 
2.5.5

Re: [PATCH 23/27] HID: wacom: leds: use the ledclass instead of custom made sysfs files

[Peter Hutterer]

On Tue, Jul 05, 2016 at 04:39:19PM +0200, Benjamin Tissoires wrote:
> The now obsolete sysfs files for LEDs and EKRemote are kept for backward
> compatibility.
> Both the EKR (read-only) and the regular Cintiqs and Intuos are now
> sharing the same led API.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  Documentation/ABI/testing/sysfs-driver-wacom |   5 +
>  drivers/hid/wacom.h                          |  19 +++
>  drivers/hid/wacom_sys.c                      | 191 +++++++++++++++++++++++++--
>  3 files changed, 205 insertions(+), 10 deletions(-)
> 
[...]

> +static int wacom_led_register_one(struct device *dev, struct wacom *wacom,
> +				  struct wacom_led *led, unsigned int group,
> +				  unsigned int id, bool read_only)
> +{
> +	int error;
> +	char *name;
> +
> +	name = devm_kasprintf(dev, GFP_KERNEL,
> +			      "%s::wacom-led_%d.%d",
> +			      dev_name(dev),
> +			      group,
> +			      id);

let's use something other than the - and _ mix please. I'd prefer
wacom-led-0.1 but I'll also accept wacom_led_0.1, the mix is a bit of an
eyesore.

also, arguably you don't need the "led" bit, see e.g.  "input3::capslock" so
even "input123::wacom-0.1" would be ok too.

Cheers,
   Peter

[PATCH 24/27] HID: wacom: leds: actually release the LEDs on disconnect

[Benjamin Tissoires]

There is a bug (?) in devm_led_classdev_register() in which its increments
the refcount of the parent. If the parent is an input device, that means
the ref count never reaches 0 when devm_input_device_release() gets called.
This means that the LEDs and all the devres resources attached to the
input device are not released.

Manually force the release of the group so that the leds are released once
we are done using them.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  1 +
 drivers/hid/wacom_sys.c | 27 ++++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 0c498c4..9159dd3 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -130,6 +130,7 @@ struct wacom_group_leds {
 	u8 select; /* status led selector (0..3) */
 	struct wacom_led *leds;
 	unsigned int count;
+	struct device *dev;
 };
 
 struct wacom_battery {
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 2354d73..b88896c 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1064,6 +1064,13 @@ static int wacom_led_register_one(struct device *dev, struct wacom *wacom,
 	return 0;
 }
 
+static void wacom_led_groups_release_one(void *data)
+{
+	struct wacom_group_leds *group = data;
+
+	devres_release_group(group->dev, group);
+}
+
 static int wacom_led_groups_alloc_and_register_one(struct device *dev,
 						   struct wacom *wacom,
 						   int group_id, int count,
@@ -1094,7 +1101,25 @@ static int wacom_led_groups_alloc_and_register_one(struct device *dev,
 			goto err;
 	}
 
-	devres_remove_group(dev, &wacom->led.groups[group_id]);
+	wacom->led.groups[group_id].dev = dev;
+
+	devres_close_group(dev, &wacom->led.groups[group_id]);
+
+	/*
+	 * There is a bug (?) in devm_led_classdev_register() in which its
+	 * increments the refcount of the parent. If the parent is an input
+	 * device, that means the ref count never reaches 0 when
+	 * devm_input_device_release() gets called.
+	 * This means that the LEDs are still there after disconnect.
+	 * Manually force the release of the group so that the leds are released
+	 * once we are done using them.
+	 */
+	error = devm_add_action_or_reset(&wacom->hdev->dev,
+					 wacom_led_groups_release_one,
+					 &wacom->led.groups[group_id]);
+	if (error)
+		return error;
+
 	return 0;
 
 err:
-- 
2.5.5

[PATCH 25/27] HID: wacom: leds: fix ordering of LED banks

[Benjamin Tissoires]

Historically, 21UX2 and 24HD have the select groups inverted
(0 is the right LED bank, and 1 the left one).

This is not right, so fix that in the new LED API. For backward
compatibility, we keep the wacom_led sysfs ABI stable. We don't
need to care about luminance for these two devices as only the
select sysfs file gets exported (brightness is not configurable).

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index b88896c..153e453 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -683,8 +683,10 @@ static int wacom_led_control(struct wacom *wacom)
 		int led = wacom->led.groups[0].select | 0x4;
 
 		if (wacom->wacom_wac.features.type == WACOM_21UX2 ||
-		    wacom->wacom_wac.features.type == WACOM_24HD)
-			led |= (wacom->led.groups[1].select << 4) | 0x40;
+		    wacom->wacom_wac.features.type == WACOM_24HD) {
+			led <<= 4;
+			led |= wacom->led.groups[1].select | 0x04;
+		}
 
 		buf[0] = report_id;
 		buf[1] = led;
@@ -742,6 +744,19 @@ out:
 	return retval;
 }
 
+static inline int wacom_led_select_get_id(struct wacom *wacom, int set_id)
+{
+	/*
+	 * Historically, 21UX2 and 24HD have the select groups inverted
+	 * (0 is the right LED bank, and 1 the left one)
+	 */
+	if (wacom->wacom_wac.features.type == WACOM_21UX2 ||
+	    wacom->wacom_wac.features.type == WACOM_24HD)
+		return 1 - set_id;
+
+	return set_id;
+}
+
 static ssize_t wacom_led_select_store(struct device *dev, int set_id,
 				      const char *buf, size_t count)
 {
@@ -754,6 +769,8 @@ static ssize_t wacom_led_select_store(struct device *dev, int set_id,
 	if (err)
 		return err;
 
+	set_id = wacom_led_select_get_id(wacom, set_id);
+
 	mutex_lock(&wacom->lock);
 
 	wacom->led.groups[set_id].select = id & 0x3;
@@ -775,8 +792,9 @@ static ssize_t wacom_led##SET_ID##_select_show(struct device *dev,	\
 {									\
 	struct hid_device *hdev = to_hid_device(dev);\
 	struct wacom *wacom = hid_get_drvdata(hdev);			\
+	int set_id = wacom_led_select_get_id(wacom, SET_ID);		\
 	return scnprintf(buf, PAGE_SIZE, "%d\n",			\
-			 wacom->led.groups[SET_ID].select);		\
+			 wacom->led.groups[set_id].select);		\
 }									\
 static DEVICE_ATTR(status_led##SET_ID##_select, DEV_ATTR_RW_PERM,	\
 		    wacom_led##SET_ID##_select_show,			\
-- 
2.5.5

Re: [PATCH 25/27] HID: wacom: leds: fix ordering of LED banks

[Peter Hutterer]

On Tue, Jul 05, 2016 at 04:39:21PM +0200, Benjamin Tissoires wrote:
> Historically, 21UX2 and 24HD have the select groups inverted
> (0 is the right LED bank, and 1 the left one).
> 
> This is not right, so fix that in the new LED API. For backward
> compatibility, we keep the wacom_led sysfs ABI stable. We don't
> need to care about luminance for these two devices as only the
> select sysfs file gets exported (brightness is not configurable).

For the archives:
unfortunately we can't do this, it breaks userspace, sort-of. Due to the
information we require about wacom tablets that isn't accessible from the
device we rely heavily on libwacom. libwacom already has calls to return the
led group based on the button index, changing the order means those values
are now incorrect. And since clients using libwacom don't necessarily have
access to the sysfs or know whether the underlying process uses the old or
new sysfs approach we can't hack around this either.

so we'll have to stick with the inverted ordering of led groups.

Cheers,
   Peter

> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/wacom_sys.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index b88896c..153e453 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -683,8 +683,10 @@ static int wacom_led_control(struct wacom *wacom)
>  		int led = wacom->led.groups[0].select | 0x4;
>  
>  		if (wacom->wacom_wac.features.type == WACOM_21UX2 ||
> -		    wacom->wacom_wac.features.type == WACOM_24HD)
> -			led |= (wacom->led.groups[1].select << 4) | 0x40;
> +		    wacom->wacom_wac.features.type == WACOM_24HD) {
> +			led <<= 4;
> +			led |= wacom->led.groups[1].select | 0x04;
> +		}
>  
>  		buf[0] = report_id;
>  		buf[1] = led;
> @@ -742,6 +744,19 @@ out:
>  	return retval;
>  }
>  
> +static inline int wacom_led_select_get_id(struct wacom *wacom, int set_id)
> +{
> +	/*
> +	 * Historically, 21UX2 and 24HD have the select groups inverted
> +	 * (0 is the right LED bank, and 1 the left one)
> +	 */
> +	if (wacom->wacom_wac.features.type == WACOM_21UX2 ||
> +	    wacom->wacom_wac.features.type == WACOM_24HD)
> +		return 1 - set_id;
> +
> +	return set_id;
> +}
> +
>  static ssize_t wacom_led_select_store(struct device *dev, int set_id,
>  				      const char *buf, size_t count)
>  {
> @@ -754,6 +769,8 @@ static ssize_t wacom_led_select_store(struct device *dev, int set_id,
>  	if (err)
>  		return err;
>  
> +	set_id = wacom_led_select_get_id(wacom, set_id);
> +
>  	mutex_lock(&wacom->lock);
>  
>  	wacom->led.groups[set_id].select = id & 0x3;
> @@ -775,8 +792,9 @@ static ssize_t wacom_led##SET_ID##_select_show(struct device *dev,	\
>  {									\
>  	struct hid_device *hdev = to_hid_device(dev);\
>  	struct wacom *wacom = hid_get_drvdata(hdev);			\
> +	int set_id = wacom_led_select_get_id(wacom, SET_ID);		\
>  	return scnprintf(buf, PAGE_SIZE, "%d\n",			\
> -			 wacom->led.groups[SET_ID].select);		\
> +			 wacom->led.groups[set_id].select);		\
>  }									\
>  static DEVICE_ATTR(status_led##SET_ID##_select, DEV_ATTR_RW_PERM,	\
>  		    wacom_led##SET_ID##_select_show,			\
> -- 
> 2.5.5
> 
> --
> 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
> 

[PATCH 26/27] HID: wacom: leds: handle the switch of the LEDs directly in the kernel

[Benjamin Tissoires]

The EKR switches the LED directly, and there is no point in having
userspace handling the switch it self when it's easy enough to do
in the kernel.

The other benefit is that now userspace does not need to have root access
to the LED but need only to read them with user privileges.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  4 ++++
 drivers/hid/wacom_sys.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++---
 drivers/hid/wacom_wac.c | 53 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 9159dd3..8f8a162 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -216,4 +216,8 @@ int wacom_wac_event(struct hid_device *hdev, struct hid_field *field,
 		struct hid_usage *usage, __s32 value);
 void wacom_wac_report(struct hid_device *hdev, struct hid_report *report);
 void wacom_battery_work(struct work_struct *work);
+enum led_brightness wacom_leds_brightness_get(struct wacom_led *led);
+struct wacom_led *wacom_led_find(struct wacom *wacom, unsigned int group,
+				 unsigned int id);
+struct wacom_led *wacom_led_next(struct wacom *wacom, struct wacom_led *cur);
 #endif
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 153e453..4a0bb6f 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -984,7 +984,7 @@ static int wacom_devm_sysfs_create_group(struct wacom *wacom,
 					       group);
 }
 
-static enum led_brightness wacom_leds_brightness_get(struct wacom_led *led)
+enum led_brightness wacom_leds_brightness_get(struct wacom_led *led)
 {
 	struct wacom *wacom = led->wacom;
 
@@ -1057,6 +1057,17 @@ static int wacom_led_register_one(struct device *dev, struct wacom *wacom,
 	if (!name)
 		return -ENOMEM;
 
+	if (!read_only) {
+		led->trigger.name = name;
+		error = devm_led_trigger_register(dev, &led->trigger);
+		if (error) {
+			hid_err(wacom->hdev,
+				"failed to register LED trigger %s: %d\n",
+				led->cdev.name, error);
+			return error;
+		}
+	}
+
 	led->group = group;
 	led->id = id;
 	led->wacom = wacom;
@@ -1065,10 +1076,12 @@ static int wacom_led_register_one(struct device *dev, struct wacom *wacom,
 	led->cdev.name = name;
 	led->cdev.max_brightness = LED_FULL;
 	led->cdev.brightness_get = __wacom_led_brightness_get;
-	if (!read_only)
+	if (!read_only) {
 		led->cdev.brightness_set_blocking = wacom_led_brightness_set;
-	else
+		led->cdev.default_trigger = led->cdev.name;
+	} else {
 		led->cdev.brightness_set = wacom_led_readonly_brightness_set;
+	}
 
 	error = devm_led_classdev_register(dev, &led->cdev);
 	if (error) {
@@ -1145,6 +1158,50 @@ err:
 	return error;
 }
 
+struct wacom_led *wacom_led_find(struct wacom *wacom, unsigned int group_id,
+				 unsigned int id)
+{
+	struct wacom_group_leds *group;
+
+	if (group_id >= wacom->led.count)
+		return NULL;
+
+	group = &wacom->led.groups[group_id];
+
+	if (!group->leds)
+		return NULL;
+
+	id %= group->count;
+
+	return &group->leds[id];
+}
+
+/**
+ * wacom_led_next: gives the next available led with a wacom trigger.
+ *
+ * returns the next available struct wacom_led which has its default trigger
+ * or the current one if none is available.
+ */
+struct wacom_led *wacom_led_next(struct wacom *wacom, struct wacom_led *cur)
+{
+	struct wacom_led *next_led;
+	int group, next;
+
+	if (!wacom || !cur)
+		return NULL;
+
+	group = cur->group;
+	next = cur->id;
+
+	do {
+		next_led = wacom_led_find(wacom, group, ++next);
+		if (!next_led || next_led == cur)
+			return next_led;
+	} while (next_led->cdev.trigger != &next_led->trigger);
+
+	return next_led;
+}
+
 static void wacom_led_groups_release(void *data)
 {
 	struct wacom *wacom = data;
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 6904e9b..a2b9c09 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -2764,11 +2764,64 @@ static void wacom_setup_numbered_buttons(struct input_dev *input_dev,
 		__set_bit(BTN_BASE + (i-16), input_dev->keybit);
 }
 
+static bool wacom_is_led_toggled(struct wacom *wacom, int button_count,
+				 int mask, int group)
+{
+	int button_per_group;
+
+	button_per_group = button_count/wacom->led.count;
+
+	return mask & (1 << (group * button_per_group));
+}
+
+static void wacom_update_led(struct wacom *wacom, int button_count, int mask,
+			     int group)
+{
+	struct wacom_led *led, *next_led;
+	int cur;
+	bool pressed;
+
+	pressed = wacom_is_led_toggled(wacom, button_count, mask, group);
+	cur = wacom->led.groups[group].select;
+
+	led = wacom_led_find(wacom, group, cur);
+	if (!led) {
+		hid_err(wacom->hdev, "can't find current LED %d in group %d\n",
+			cur, group);
+		return;
+	}
+
+	if (!pressed) {
+		led->held = false;
+		return;
+	}
+
+	if (led->held && pressed)
+		return;
+
+	next_led = wacom_led_next(wacom, led);
+	if (!next_led) {
+		hid_err(wacom->hdev, "can't find next LED in group %d\n",
+			group);
+		return;
+	}
+	if (next_led == led)
+		return;
+
+	next_led->held = true;
+	led_trigger_event(&next_led->trigger,
+			  wacom_leds_brightness_get(next_led));
+}
+
 static void wacom_report_numbered_buttons(struct input_dev *input_dev,
 				int button_count, int mask)
 {
+	struct wacom *wacom = input_get_drvdata(input_dev);
 	int i;
 
+	for (i = 0; i < wacom->led.count; i++)
+		wacom_update_led(wacom,  button_count, mask, i);
+
 	for (i = 0; i < button_count && i < 10; i++)
 		input_report_key(input_dev, BTN_0 + i, mask & (1 << i));
 	for (i = 10; i < button_count && i < 16; i++)
-- 
2.5.5

Re: [PATCH 26/27] HID: wacom: leds: handle the switch of the LEDs directly in the kernel

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6098 bytes --]

Hi,

[auto build test ERROR on hid/for-next]
[also build test ERROR on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Benjamin-Tissoires/HID-wacom-cleanup-EKR-LED/20160705-225431
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next
config: i386-randconfig-a0-201627 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/hid/wacom_sys.c: In function 'wacom_led_register_one':
>> drivers/hid/wacom_sys.c:1061:15: error: 'struct led_trigger' has no member named 'name'
      led->trigger.name = name;
                  ^
>> drivers/hid/wacom_sys.c:1062:11: error: implicit declaration of function 'devm_led_trigger_register' [-Werror=implicit-function-declaration]
      error = devm_led_trigger_register(dev, &led->trigger);
              ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/hid/wacom_sys.c: In function 'wacom_led_next':
>> drivers/hid/wacom_sys.c:1200:25: error: 'struct led_classdev' has no member named 'trigger'
     } while (next_led->cdev.trigger != &next_led->trigger);
                            ^
   cc1: some warnings being treated as errors

vim +1061 drivers/hid/wacom_sys.c

  1055				      group,
  1056				      id);
  1057		if (!name)
  1058			return -ENOMEM;
  1059	
  1060		if (!read_only) {
> 1061			led->trigger.name = name;
> 1062			error = devm_led_trigger_register(dev, &led->trigger);
  1063			if (error) {
  1064				hid_err(wacom->hdev,
  1065					"failed to register LED trigger %s: %d\n",
  1066					led->cdev.name, error);
  1067				return error;
  1068			}
  1069		}
  1070	
  1071		led->group = group;
  1072		led->id = id;
  1073		led->wacom = wacom;
  1074		led->llv = wacom->led.llv;
  1075		led->hlv = wacom->led.hlv;
  1076		led->cdev.name = name;
  1077		led->cdev.max_brightness = LED_FULL;
  1078		led->cdev.brightness_get = __wacom_led_brightness_get;
  1079		if (!read_only) {
  1080			led->cdev.brightness_set_blocking = wacom_led_brightness_set;
  1081			led->cdev.default_trigger = led->cdev.name;
  1082		} else {
  1083			led->cdev.brightness_set = wacom_led_readonly_brightness_set;
  1084		}
  1085	
  1086		error = devm_led_classdev_register(dev, &led->cdev);
  1087		if (error) {
  1088			hid_err(wacom->hdev,
  1089				"failed to register LED %s: %d\n",
  1090				led->cdev.name, error);
  1091			led->cdev.name = NULL;
  1092			return error;
  1093		}
  1094	
  1095		return 0;
  1096	}
  1097	
  1098	static void wacom_led_groups_release_one(void *data)
  1099	{
  1100		struct wacom_group_leds *group = data;
  1101	
  1102		devres_release_group(group->dev, group);
  1103	}
  1104	
  1105	static int wacom_led_groups_alloc_and_register_one(struct device *dev,
  1106							   struct wacom *wacom,
  1107							   int group_id, int count,
  1108							   bool read_only)
  1109	{
  1110		struct wacom_led *leds;
  1111		int i, error;
  1112	
  1113		if (group_id >= wacom->led.count || count <= 0)
  1114			return -EINVAL;
  1115	
  1116		if (!devres_open_group(dev, &wacom->led.groups[group_id], GFP_KERNEL))
  1117			return -ENOMEM;
  1118	
  1119		leds = devm_kzalloc(dev, sizeof(struct wacom_led) * count, GFP_KERNEL);
  1120		if (!leds) {
  1121			error = -ENOMEM;
  1122			goto err;
  1123		}
  1124	
  1125		wacom->led.groups[group_id].leds = leds;
  1126		wacom->led.groups[group_id].count = count;
  1127	
  1128		for (i = 0; i < count; i++) {
  1129			error = wacom_led_register_one(dev, wacom, &leds[i],
  1130						       group_id, i, read_only);
  1131			if (error)
  1132				goto err;
  1133		}
  1134	
  1135		wacom->led.groups[group_id].dev = dev;
  1136	
  1137		devres_close_group(dev, &wacom->led.groups[group_id]);
  1138	
  1139		/*
  1140		 * There is a bug (?) in devm_led_classdev_register() in which its
  1141		 * increments the refcount of the parent. If the parent is an input
  1142		 * device, that means the ref count never reaches 0 when
  1143		 * devm_input_device_release() gets called.
  1144		 * This means that the LEDs are still there after disconnect.
  1145		 * Manually force the release of the group so that the leds are released
  1146		 * once we are done using them.
  1147		 */
  1148		error = devm_add_action_or_reset(&wacom->hdev->dev,
  1149						 wacom_led_groups_release_one,
  1150						 &wacom->led.groups[group_id]);
  1151		if (error)
  1152			return error;
  1153	
  1154		return 0;
  1155	
  1156	err:
  1157		devres_release_group(dev, &wacom->led.groups[group_id]);
  1158		return error;
  1159	}
  1160	
  1161	struct wacom_led *wacom_led_find(struct wacom *wacom, unsigned int group_id,
  1162					 unsigned int id)
  1163	{
  1164		struct wacom_group_leds *group;
  1165	
  1166		if (group_id >= wacom->led.count)
  1167			return NULL;
  1168	
  1169		group = &wacom->led.groups[group_id];
  1170	
  1171		if (!group->leds)
  1172			return NULL;
  1173	
  1174		id %= group->count;
  1175	
  1176		return &group->leds[id];
  1177	}
  1178	
  1179	/**
  1180	 * wacom_led_next: gives the next available led with a wacom trigger.
  1181	 *
  1182	 * returns the next available struct wacom_led which has its default trigger
  1183	 * or the current one if none is available.
  1184	 */
  1185	struct wacom_led *wacom_led_next(struct wacom *wacom, struct wacom_led *cur)
  1186	{
  1187		struct wacom_led *next_led;
  1188		int group, next;
  1189	
  1190		if (!wacom || !cur)
  1191			return NULL;
  1192	
  1193		group = cur->group;
  1194		next = cur->id;
  1195	
  1196		do {
  1197			next_led = wacom_led_find(wacom, group, ++next);
  1198			if (!next_led || next_led == cur)
  1199				return next_led;
> 1200		} while (next_led->cdev.trigger != &next_led->trigger);
  1201	
  1202		return next_led;
  1203	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24153 bytes --]

Re: [PATCH 26/27] HID: wacom: leds: handle the switch of the LEDs directly in the kernel

[Benjamin Tissoires]

Hi,

On Jul 05 2016 or thereabouts, kbuild test robot wrote:
> Hi,
> 
> [auto build test ERROR on hid/for-next]
> [also build test ERROR on v4.7-rc6 next-20160705]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Benjamin-Tissoires/HID-wacom-cleanup-EKR-LED/20160705-225431
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next
> config: i386-randconfig-a0-201627 (attached as .config)
> compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/hid/wacom_sys.c: In function 'wacom_led_register_one':
> >> drivers/hid/wacom_sys.c:1061:15: error: 'struct led_trigger' has no member named 'name'
>       led->trigger.name = name;

Thanks, looks like I forgot to add the Kconfig bits CONFIG_LEDS_TRIGGERS
here (and CONFIG_LEDS_CLASS in 23/27).

Cheers,
Benjamin

>                   ^
> >> drivers/hid/wacom_sys.c:1062:11: error: implicit declaration of function 'devm_led_trigger_register' [-Werror=implicit-function-declaration]
>       error = devm_led_trigger_register(dev, &led->trigger);
>               ^~~~~~~~~~~~~~~~~~~~~~~~~
>    drivers/hid/wacom_sys.c: In function 'wacom_led_next':
> >> drivers/hid/wacom_sys.c:1200:25: error: 'struct led_classdev' has no member named 'trigger'
>      } while (next_led->cdev.trigger != &next_led->trigger);
>                             ^
>    cc1: some warnings being treated as errors
> 
> vim +1061 drivers/hid/wacom_sys.c
> 
>   1055				      group,
>   1056				      id);
>   1057		if (!name)
>   1058			return -ENOMEM;
>   1059	
>   1060		if (!read_only) {
> > 1061			led->trigger.name = name;
> > 1062			error = devm_led_trigger_register(dev, &led->trigger);
>   1063			if (error) {
>   1064				hid_err(wacom->hdev,
>   1065					"failed to register LED trigger %s: %d\n",
>   1066					led->cdev.name, error);
>   1067				return error;
>   1068			}
>   1069		}
>   1070	
>   1071		led->group = group;
>   1072		led->id = id;
>   1073		led->wacom = wacom;
>   1074		led->llv = wacom->led.llv;
>   1075		led->hlv = wacom->led.hlv;
>   1076		led->cdev.name = name;
>   1077		led->cdev.max_brightness = LED_FULL;
>   1078		led->cdev.brightness_get = __wacom_led_brightness_get;
>   1079		if (!read_only) {
>   1080			led->cdev.brightness_set_blocking = wacom_led_brightness_set;
>   1081			led->cdev.default_trigger = led->cdev.name;
>   1082		} else {
>   1083			led->cdev.brightness_set = wacom_led_readonly_brightness_set;
>   1084		}
>   1085	
>   1086		error = devm_led_classdev_register(dev, &led->cdev);
>   1087		if (error) {
>   1088			hid_err(wacom->hdev,
>   1089				"failed to register LED %s: %d\n",
>   1090				led->cdev.name, error);
>   1091			led->cdev.name = NULL;
>   1092			return error;
>   1093		}
>   1094	
>   1095		return 0;
>   1096	}
>   1097	
>   1098	static void wacom_led_groups_release_one(void *data)
>   1099	{
>   1100		struct wacom_group_leds *group = data;
>   1101	
>   1102		devres_release_group(group->dev, group);
>   1103	}
>   1104	
>   1105	static int wacom_led_groups_alloc_and_register_one(struct device *dev,
>   1106							   struct wacom *wacom,
>   1107							   int group_id, int count,
>   1108							   bool read_only)
>   1109	{
>   1110		struct wacom_led *leds;
>   1111		int i, error;
>   1112	
>   1113		if (group_id >= wacom->led.count || count <= 0)
>   1114			return -EINVAL;
>   1115	
>   1116		if (!devres_open_group(dev, &wacom->led.groups[group_id], GFP_KERNEL))
>   1117			return -ENOMEM;
>   1118	
>   1119		leds = devm_kzalloc(dev, sizeof(struct wacom_led) * count, GFP_KERNEL);
>   1120		if (!leds) {
>   1121			error = -ENOMEM;
>   1122			goto err;
>   1123		}
>   1124	
>   1125		wacom->led.groups[group_id].leds = leds;
>   1126		wacom->led.groups[group_id].count = count;
>   1127	
>   1128		for (i = 0; i < count; i++) {
>   1129			error = wacom_led_register_one(dev, wacom, &leds[i],
>   1130						       group_id, i, read_only);
>   1131			if (error)
>   1132				goto err;
>   1133		}
>   1134	
>   1135		wacom->led.groups[group_id].dev = dev;
>   1136	
>   1137		devres_close_group(dev, &wacom->led.groups[group_id]);
>   1138	
>   1139		/*
>   1140		 * There is a bug (?) in devm_led_classdev_register() in which its
>   1141		 * increments the refcount of the parent. If the parent is an input
>   1142		 * device, that means the ref count never reaches 0 when
>   1143		 * devm_input_device_release() gets called.
>   1144		 * This means that the LEDs are still there after disconnect.
>   1145		 * Manually force the release of the group so that the leds are released
>   1146		 * once we are done using them.
>   1147		 */
>   1148		error = devm_add_action_or_reset(&wacom->hdev->dev,
>   1149						 wacom_led_groups_release_one,
>   1150						 &wacom->led.groups[group_id]);
>   1151		if (error)
>   1152			return error;
>   1153	
>   1154		return 0;
>   1155	
>   1156	err:
>   1157		devres_release_group(dev, &wacom->led.groups[group_id]);
>   1158		return error;
>   1159	}
>   1160	
>   1161	struct wacom_led *wacom_led_find(struct wacom *wacom, unsigned int group_id,
>   1162					 unsigned int id)
>   1163	{
>   1164		struct wacom_group_leds *group;
>   1165	
>   1166		if (group_id >= wacom->led.count)
>   1167			return NULL;
>   1168	
>   1169		group = &wacom->led.groups[group_id];
>   1170	
>   1171		if (!group->leds)
>   1172			return NULL;
>   1173	
>   1174		id %= group->count;
>   1175	
>   1176		return &group->leds[id];
>   1177	}
>   1178	
>   1179	/**
>   1180	 * wacom_led_next: gives the next available led with a wacom trigger.
>   1181	 *
>   1182	 * returns the next available struct wacom_led which has its default trigger
>   1183	 * or the current one if none is available.
>   1184	 */
>   1185	struct wacom_led *wacom_led_next(struct wacom *wacom, struct wacom_led *cur)
>   1186	{
>   1187		struct wacom_led *next_led;
>   1188		int group, next;
>   1189	
>   1190		if (!wacom || !cur)
>   1191			return NULL;
>   1192	
>   1193		group = cur->group;
>   1194		next = cur->id;
>   1195	
>   1196		do {
>   1197			next_led = wacom_led_find(wacom, group, ++next);
>   1198			if (!next_led || next_led == cur)
>   1199				return next_led;
> > 1200		} while (next_led->cdev.trigger != &next_led->trigger);
>   1201	
>   1202		return next_led;
>   1203	}
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH 27/27] HID: wacom: leds: handle Cintiq 24HD leds buttons

[Benjamin Tissoires]

The 24HD has 1 button per LED (first three buttons of each group).
We need a special treatment for them as it's not a uniq button that
switches between the LEDs.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_wac.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index a2b9c09..fcf2264 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -2764,6 +2764,31 @@ static void wacom_setup_numbered_buttons(struct input_dev *input_dev,
 		__set_bit(BTN_BASE + (i-16), input_dev->keybit);
 }
 
+static void wacom_24hd_update_leds(struct wacom *wacom, int mask, int group)
+{
+	struct wacom_led *led;
+	int i;
+	bool updated = false;
+
+	if (group > 0)
+		mask >>= 8;
+
+	for (i = 0; i < 3; i++) {
+		led = wacom_led_find(wacom, group, i);
+		if (!led) {
+			hid_err(wacom->hdev, "can't find LED %d in group %d\n",
+				i, group);
+			continue;
+		}
+		if (!updated && mask & BIT(i)) {
+			led->held = true;
+			led_trigger_event(&led->trigger, LED_FULL);
+		} else {
+			led->held = false;
+		}
+	}
+}
+
 static bool wacom_is_led_toggled(struct wacom *wacom, int button_count,
 				 int mask, int group)
 {
@@ -2781,7 +2806,11 @@ static void wacom_update_led(struct wacom *wacom, int button_count, int mask,
 	int cur;
 	bool pressed;
 
+	if (wacom->wacom_wac.features.type == WACOM_24HD)
+		return wacom_24hd_update_leds(wacom, mask, group);
+
 	pressed = wacom_is_led_toggled(wacom, button_count, mask, group);
+
 	cur = wacom->led.groups[group].select;
 
 	led = wacom_led_find(wacom, group, cur);
@@ -2808,6 +2837,13 @@ static void wacom_update_led(struct wacom *wacom, int button_count, int mask,
 	if (next_led == led)
 		return;
 
+	pr_err("%s group: %d led: (%d -> %d) t: %s %s:%d\n", __func__,
+		led->group,
+		led->id,
+		next_led->id,
+		next_led->trigger.name,
+		__FILE__, __LINE__);
+
 	next_led->held = true;
 	led_trigger_event(&next_led->trigger,
 			  wacom_leds_brightness_get(next_led));
-- 
2.5.5

Re: [PATCH 27/27] HID: wacom: leds: handle Cintiq 24HD leds buttons

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

Hi,

[auto build test ERROR on hid/for-next]
[also build test ERROR on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Benjamin-Tissoires/HID-wacom-cleanup-EKR-LED/20160705-225431
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next
config: i386-randconfig-a0-201627 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from include/linux/kernel.h:13:0,
                    from include/asm-generic/bug.h:13,
                    from arch/x86/include/asm/bug.h:35,
                    from include/linux/bug.h:4,
                    from include/linux/mmdebug.h:4,
                    from include/linux/gfp.h:4,
                    from include/linux/slab.h:14,
                    from include/linux/hid.h:30,
                    from drivers/hid/wacom_wac.h:13,
                    from drivers/hid/wacom_wac.c:15:
   drivers/hid/wacom_wac.c: In function 'wacom_update_led':
>> drivers/hid/wacom_wac.c:2844:20: error: 'struct led_trigger' has no member named 'name'
      next_led->trigger.name,
                       ^
   include/linux/printk.h:264:33: note: in definition of macro 'pr_err'
     printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
                                    ^~~~~~~~~~~

vim +2844 drivers/hid/wacom_wac.c

  2838			return;
  2839	
  2840		pr_err("%s group: %d led: (%d -> %d) t: %s %s:%d\n", __func__,
  2841			led->group,
  2842			led->id,
  2843			next_led->id,
> 2844			next_led->trigger.name,
  2845			__FILE__, __LINE__);
  2846	
  2847		next_led->held = true;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24153 bytes --]