diff mbox series

[v2] hwmon: corsair-cpro: Change to HID driver

Message ID 20200715151419.43134-1-mail@mariuszachmann.de (mailing list archive)
State Superseded
Headers show
Series [v2] hwmon: corsair-cpro: Change to HID driver | expand

Commit Message

Marius Zachmann July 15, 2020, 3:14 p.m. UTC
This changes corsair-cpro to a hid driver using hid reports.

Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>

---
Changes from v1
- add comment at the beginning about using hid and hidraw
- use hwmon_device_register_with_info and hwmon_device_unregister
- use a define for timeout

---
 drivers/hid/hid-quirks.c     |   2 -
 drivers/hwmon/Kconfig        |   2 +-
 drivers/hwmon/corsair-cpro.c | 124 ++++++++++++++++++++++++++---------
 3 files changed, 95 insertions(+), 35 deletions(-)

--
2.27.0

Comments

Guenter Roeck July 15, 2020, 3:35 p.m. UTC | #1
On 7/15/20 8:14 AM, Marius Zachmann wrote:
> This changes corsair-cpro to a hid driver using hid reports.
> 
> Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
> 
> ---

[ ... ]

> -static const struct usb_device_id ccp_devices[] = {
> -	{ USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_COMMANDERPRO) },
> -	{ USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_1000D) },
> +static const struct hid_device_id ccp_devices[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_COMMANDERPRO) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_1000D) },
>  	{ }
>  };
> 
> -static struct usb_driver ccp_driver = {
> +static struct hid_driver ccp_driver = {
>  	.name = "corsair-cpro",
> +	.id_table = ccp_devices,
>  	.probe = ccp_probe,
> -	.disconnect = ccp_disconnect,
> -	.id_table = ccp_devices
> +	.remove = ccp_remove,
> +	.raw_event = ccp_raw_event,
>  };
> 
> -MODULE_DEVICE_TABLE(usb, ccp_devices);
> +MODULE_DEVICE_TABLE(hid, ccp_devices);
>  MODULE_LICENSE("GPL");
> 
> -module_usb_driver(ccp_driver);
> +static int __init ccp_init(void)
> +{
> +	return hid_register_driver(&ccp_driver);
> +}
> +
> +static void __exit ccp_exit(void)
> +{
> +	hid_unregister_driver(&ccp_driver);
> +}
> +
> +/* make sure it is loaded after hid */
> +late_initcall(ccp_init);
> +module_exit(ccp_exit);

Sorry for not noticing before, but can you use module_hid_driver() ?
That seems to work for other hid drivers.

Thanks,
Guenter
Marius Zachmann July 15, 2020, 7:12 p.m. UTC | #2
On 15.07.20 at 17:35:08 CEST, Guenter Roeck wrote
> On 7/15/20 8:14 AM, Marius Zachmann wrote:
> > This changes corsair-cpro to a hid driver using hid reports.
> > 
> > Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
> > 
> > ---
> 
...
> > -module_usb_driver(ccp_driver);
> > +static int __init ccp_init(void)
> > +{
> > +	return hid_register_driver(&ccp_driver);
> > +}
> > +
> > +static void __exit ccp_exit(void)
> > +{
> > +	hid_unregister_driver(&ccp_driver);
> > +}
> > +
> > +/* make sure it is loaded after hid */
> > +late_initcall(ccp_init);
> > +module_exit(ccp_exit);
> 
> Sorry for not noticing before, but can you use module_hid_driver() ?
> That seems to work for other hid drivers.
> 
> Thanks,
> Guenter
> 

When using module_hid_driver() and compiling the driver as built-in
the driver init function will get called before hid and fail registering
the driver. late_initcall forces it to be loaded after hid.

Greetings,
Marius
Guenter Roeck July 15, 2020, 7:49 p.m. UTC | #3
On 7/15/20 12:12 PM, Marius Zachmann wrote:
> On 15.07.20 at 17:35:08 CEST, Guenter Roeck wrote
>> On 7/15/20 8:14 AM, Marius Zachmann wrote:
>>> This changes corsair-cpro to a hid driver using hid reports.
>>>
>>> Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
>>>
>>> ---
>>
> ...
>>> -module_usb_driver(ccp_driver);
>>> +static int __init ccp_init(void)
>>> +{
>>> +	return hid_register_driver(&ccp_driver);
>>> +}
>>> +
>>> +static void __exit ccp_exit(void)
>>> +{
>>> +	hid_unregister_driver(&ccp_driver);
>>> +}
>>> +
>>> +/* make sure it is loaded after hid */
>>> +late_initcall(ccp_init);
>>> +module_exit(ccp_exit);
>>
>> Sorry for not noticing before, but can you use module_hid_driver() ?
>> That seems to work for other hid drivers.
>>
>> Thanks,
>> Guenter
>>
> 
> When using module_hid_driver() and compiling the driver as built-in
> the driver init function will get called before hid and fail registering
> the driver. late_initcall forces it to be loaded after hid.
> 

That seems odd. Why does no other hid driver have that problem ?
Your argument is pretty much that module_hid_driver() would not work,
which seems unlikely.

Thanks,
Guenter
Marius Zachmann July 15, 2020, 7:55 p.m. UTC | #4
On 15.07.20 at 21:49:59 CEST, Guenter Roeck wrote
> On 7/15/20 12:12 PM, Marius Zachmann wrote:
> > On 15.07.20 at 17:35:08 CEST, Guenter Roeck wrote
> >> On 7/15/20 8:14 AM, Marius Zachmann wrote:
> >>> This changes corsair-cpro to a hid driver using hid reports.
> >>>
> >>> Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
> >>>
> >>> ---
> >>
> > ...
> >>> -module_usb_driver(ccp_driver);
> >>> +static int __init ccp_init(void)
> >>> +{
> >>> +	return hid_register_driver(&ccp_driver);
> >>> +}
> >>> +
> >>> +static void __exit ccp_exit(void)
> >>> +{
> >>> +	hid_unregister_driver(&ccp_driver);
> >>> +}
> >>> +
> >>> +/* make sure it is loaded after hid */
> >>> +late_initcall(ccp_init);
> >>> +module_exit(ccp_exit);
> >>
> >> Sorry for not noticing before, but can you use module_hid_driver() ?
> >> That seems to work for other hid drivers.
> >>
> >> Thanks,
> >> Guenter
> >>
> > 
> > When using module_hid_driver() and compiling the driver as built-in
> > the driver init function will get called before hid and fail registering
> > the driver. late_initcall forces it to be loaded after hid.
> > 
> 
> That seems odd. Why does no other hid driver have that problem ?
> Your argument is pretty much that module_hid_driver() would not work,
> which seems unlikely.
> 
> Thanks,
> Guenter
> 

The order of initcalls is determined by the build order. Hwmon subsystem
gets build before hid subsystem and therefore the initcalls are earlier
than hid. But hid_register_driver needs the hid driver to be there.

Greetings
Marius
Guenter Roeck July 15, 2020, 8:11 p.m. UTC | #5
On 7/15/20 12:55 PM, Marius Zachmann wrote:
> On 15.07.20 at 21:49:59 CEST, Guenter Roeck wrote
>> On 7/15/20 12:12 PM, Marius Zachmann wrote:
>>> On 15.07.20 at 17:35:08 CEST, Guenter Roeck wrote
>>>> On 7/15/20 8:14 AM, Marius Zachmann wrote:
>>>>> This changes corsair-cpro to a hid driver using hid reports.
>>>>>
>>>>> Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
>>>>>
>>>>> ---
>>>>
>>> ...
>>>>> -module_usb_driver(ccp_driver);
>>>>> +static int __init ccp_init(void)
>>>>> +{
>>>>> +	return hid_register_driver(&ccp_driver);
>>>>> +}
>>>>> +
>>>>> +static void __exit ccp_exit(void)
>>>>> +{
>>>>> +	hid_unregister_driver(&ccp_driver);
>>>>> +}
>>>>> +
>>>>> +/* make sure it is loaded after hid */
>>>>> +late_initcall(ccp_init);
>>>>> +module_exit(ccp_exit);
>>>>
>>>> Sorry for not noticing before, but can you use module_hid_driver() ?
>>>> That seems to work for other hid drivers.
>>>>
>>>> Thanks,
>>>> Guenter
>>>>
>>>
>>> When using module_hid_driver() and compiling the driver as built-in
>>> the driver init function will get called before hid and fail registering
>>> the driver. late_initcall forces it to be loaded after hid.
>>>
>>
>> That seems odd. Why does no other hid driver have that problem ?
>> Your argument is pretty much that module_hid_driver() would not work,
>> which seems unlikely.
>>
>> Thanks,
>> Guenter
>>
> 
> The order of initcalls is determined by the build order. Hwmon subsystem
> gets build before hid subsystem and therefore the initcalls are earlier
> than hid. But hid_register_driver needs the hid driver to be there.
> 
Hmm. I am not happy, but I guess we'll have to live with it.

Can you add the detailed explanation to the comment above ?
"make sure it is loaded after hid" doesn't tell the whole story,
and I want to avoid having to deal with "cleanup" patches later.

Alternatively, I wonder if it would make sense to force
this driver to be built as module. Have you thought about that ?

Thanks,
Guenter
Marius Zachmann July 15, 2020, 8:22 p.m. UTC | #6
On 15.07.20 at 22:11:39 CEST, Guenter Roeck wrote
> On 7/15/20 12:55 PM, Marius Zachmann wrote:
> > On 15.07.20 at 21:49:59 CEST, Guenter Roeck wrote
> >> On 7/15/20 12:12 PM, Marius Zachmann wrote:
> >>> On 15.07.20 at 17:35:08 CEST, Guenter Roeck wrote
> >>>> On 7/15/20 8:14 AM, Marius Zachmann wrote:
> >>>>> This changes corsair-cpro to a hid driver using hid reports.
> >>>>>
> >>>>> Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
> >>>>>
> >>>>> ---
> >>>>
> >>> ...
> >>>>> -module_usb_driver(ccp_driver);
> >>>>> +static int __init ccp_init(void)
> >>>>> +{
> >>>>> +	return hid_register_driver(&ccp_driver);
> >>>>> +}
> >>>>> +
> >>>>> +static void __exit ccp_exit(void)
> >>>>> +{
> >>>>> +	hid_unregister_driver(&ccp_driver);
> >>>>> +}
> >>>>> +
> >>>>> +/* make sure it is loaded after hid */
> >>>>> +late_initcall(ccp_init);
> >>>>> +module_exit(ccp_exit);
> >>>>
> >>>> Sorry for not noticing before, but can you use module_hid_driver() ?
> >>>> That seems to work for other hid drivers.
> >>>>
> >>>> Thanks,
> >>>> Guenter
> >>>>
> >>>
> >>> When using module_hid_driver() and compiling the driver as built-in
> >>> the driver init function will get called before hid and fail registering
> >>> the driver. late_initcall forces it to be loaded after hid.
> >>>
> >>
> >> That seems odd. Why does no other hid driver have that problem ?
> >> Your argument is pretty much that module_hid_driver() would not work,
> >> which seems unlikely.
> >>
> >> Thanks,
> >> Guenter
> >>
> > 
> > The order of initcalls is determined by the build order. Hwmon subsystem
> > gets build before hid subsystem and therefore the initcalls are earlier
> > than hid. But hid_register_driver needs the hid driver to be there.
> > 
> Hmm. I am not happy, but I guess we'll have to live with it.
> 
> Can you add the detailed explanation to the comment above ?
> "make sure it is loaded after hid" doesn't tell the whole story,
> and I want to avoid having to deal with "cleanup" patches later.
> 
> Alternatively, I wonder if it would make sense to force
> this driver to be built as module. Have you thought about that ?
> 
> Thanks,
> Guenter
> 

I am also not happy about this and thought about this, but I
think late_initcall is the least ugly solution to this.
I will add a more detailed explanation.

Greetings
Marius
diff mbox series

Patch

diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 7b7bc7737c53..ca8b5c261c7c 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -699,8 +699,6 @@  static const struct hid_device_id hid_ignore_list[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_AXENTIA, USB_DEVICE_ID_AXENTIA_FM_RADIO) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BERKSHIRE, USB_DEVICE_ID_BERKSHIRE_PCWD) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CIDC, 0x0103) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, 0x0c10) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, 0x1d00) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_RADIO_SI470X) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_RADIO_SI4713) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CMEDIA, USB_DEVICE_ID_CM109) },
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 8b046a5dfa40..c603d8c8e3d2 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -441,7 +441,7 @@  config SENSORS_BT1_PVT_ALARMS

 config SENSORS_CORSAIR_CPRO
 	tristate "Corsair Commander Pro controller"
-	depends on USB
+	depends on HID
 	help
 	  If you say yes here you get support for the Corsair Commander Pro
 	  controller.
diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
index fe625190e3a1..ba378574b073 100644
--- a/drivers/hwmon/corsair-cpro.c
+++ b/drivers/hwmon/corsair-cpro.c
@@ -2,16 +2,21 @@ 
 /*
  * corsair-cpro.c - Linux driver for Corsair Commander Pro
  * Copyright (C) 2020 Marius Zachmann <mail@mariuszachmann.de>
+ *
+ * This driver uses hid reports to communicate with the device to allow hidraw userspace drivers
+ * still being used. The device does not use report ids. When using hidraw and this driver
+ * simultaniously, reports could be switched.
  */

 #include <linux/bitops.h>
+#include <linux/completion.h>
+#include <linux/hid.h>
 #include <linux/hwmon.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/types.h>
-#include <linux/usb.h>

 #define USB_VENDOR_ID_CORSAIR			0x1b1c
 #define USB_PRODUCT_ID_CORSAIR_COMMANDERPRO	0x0c10
@@ -20,6 +25,7 @@ 
 #define OUT_BUFFER_SIZE		63
 #define IN_BUFFER_SIZE		16
 #define LABEL_LENGTH		11
+#define REQ_TIMEOUT		300

 #define CTL_GET_TMP_CNCT	0x10	/*
 					 * returns in bytes 1-4 for each temp sensor:
@@ -62,7 +68,9 @@ 
 #define NUM_TEMP_SENSORS	4

 struct ccp_device {
-	struct usb_device *udev;
+	struct hid_device *hdev;
+	struct device *hwmon_dev;
+	struct completion wait_input_report;
 	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
 	u8 *buffer;
 	int pwm[6];
@@ -75,7 +83,7 @@  struct ccp_device {
 /* send command, check for error in response, response in ccp->buffer */
 static int send_usb_cmd(struct ccp_device *ccp, u8 command, u8 byte1, u8 byte2, u8 byte3)
 {
-	int actual_length;
+	unsigned long t;
 	int ret;

 	memset(ccp->buffer, 0x00, OUT_BUFFER_SIZE);
@@ -84,26 +92,39 @@  static int send_usb_cmd(struct ccp_device *ccp, u8 command, u8 byte1, u8 byte2,
 	ccp->buffer[2] = byte2;
 	ccp->buffer[3] = byte3;

-	ret = usb_bulk_msg(ccp->udev, usb_sndintpipe(ccp->udev, 2), ccp->buffer, OUT_BUFFER_SIZE,
-			   &actual_length, 1000);
-	if (ret)
-		return ret;
+	reinit_completion(&ccp->wait_input_report);

-	/* response needs to be received every time */
-	ret = usb_bulk_msg(ccp->udev, usb_rcvintpipe(ccp->udev, 1), ccp->buffer, IN_BUFFER_SIZE,
-			   &actual_length, 1000);
-	if (ret)
+	ret = hid_hw_output_report(ccp->hdev, ccp->buffer, OUT_BUFFER_SIZE);
+	if (ret < 0)
 		return ret;

+	t = wait_for_completion_timeout(&ccp->wait_input_report, msecs_to_jiffies(REQ_TIMEOUT));
+	if (!t)
+		return -ETIMEDOUT;
+
 	/* first byte of response is error code */
 	if (ccp->buffer[0] != 0x00) {
-		dev_dbg(&ccp->udev->dev, "device response error: %d", ccp->buffer[0]);
+		hid_dbg(ccp->hdev, "device response error: %d", ccp->buffer[0]);
 		return -EIO;
 	}

 	return 0;
 }

+static int ccp_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *data, int size)
+{
+	struct ccp_device *ccp = hid_get_drvdata(hdev);
+
+	/* only copy buffer when requested */
+	if (completion_done(&ccp->wait_input_report))
+		return 0;
+
+	memcpy(ccp->buffer, data, min(IN_BUFFER_SIZE, size));
+	complete(&ccp->wait_input_report);
+
+	return 0;
+}
+
 /* requests and returns single data values depending on channel */
 static int get_data(struct ccp_device *ccp, int command, int channel)
 {
@@ -437,57 +458,98 @@  static int get_temp_cnct(struct ccp_device *ccp)
 	return 0;
 }

-static int ccp_probe(struct usb_interface *intf, const struct usb_device_id *id)
+static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
-	struct device *hwmon_dev;
 	struct ccp_device *ccp;
 	int ret;

-	ccp = devm_kzalloc(&intf->dev, sizeof(*ccp), GFP_KERNEL);
+	ccp = devm_kzalloc(&hdev->dev, sizeof(*ccp), GFP_KERNEL);
 	if (!ccp)
 		return -ENOMEM;

-	ccp->buffer = devm_kmalloc(&intf->dev, OUT_BUFFER_SIZE, GFP_KERNEL);
+	ccp->buffer = devm_kmalloc(&hdev->dev, OUT_BUFFER_SIZE, GFP_KERNEL);
 	if (!ccp->buffer)
 		return -ENOMEM;

+	ret = hid_parse(hdev);
+	if (ret)
+		return ret;
+
+	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
+		return ret;
+
+	ret = hid_hw_open(hdev);
+	if (ret)
+		goto out_hw_stop;
+
+	ccp->hdev = hdev;
+	hid_set_drvdata(hdev, ccp);
 	mutex_init(&ccp->mutex);
+	init_completion(&ccp->wait_input_report);

-	ccp->udev = interface_to_usbdev(intf);
+	hid_device_io_start(hdev);

 	/* temp and fan connection status only updates when device is powered on */
 	ret = get_temp_cnct(ccp);
 	if (ret)
-		return ret;
+		goto out_hw_close;

 	ret = get_fan_cnct(ccp);
 	if (ret)
-		return ret;
+		goto out_hw_close;
+	ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsaircpro",
+							 ccp, &ccp_chip_info, 0);
+	if (IS_ERR(ccp->hwmon_dev)) {
+		ret = PTR_ERR(ccp->hwmon_dev);
+		goto out_hw_close;
+	}

-	hwmon_dev = devm_hwmon_device_register_with_info(&intf->dev, "corsaircpro", ccp,
-							 &ccp_chip_info, 0);
+	return 0;

-	return PTR_ERR_OR_ZERO(hwmon_dev);
+out_hw_close:
+	hid_hw_close(hdev);
+out_hw_stop:
+	hid_hw_stop(hdev);
+	return ret;
 }

-static void ccp_disconnect(struct usb_interface *intf)
+static void ccp_remove(struct hid_device *hdev)
 {
+	struct ccp_device *ccp = hid_get_drvdata(hdev);
+
+	hwmon_device_unregister(ccp->hwmon_dev);
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
 }

-static const struct usb_device_id ccp_devices[] = {
-	{ USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_COMMANDERPRO) },
-	{ USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_1000D) },
+static const struct hid_device_id ccp_devices[] = {
+	{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_COMMANDERPRO) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_CORSAIR_1000D) },
 	{ }
 };

-static struct usb_driver ccp_driver = {
+static struct hid_driver ccp_driver = {
 	.name = "corsair-cpro",
+	.id_table = ccp_devices,
 	.probe = ccp_probe,
-	.disconnect = ccp_disconnect,
-	.id_table = ccp_devices
+	.remove = ccp_remove,
+	.raw_event = ccp_raw_event,
 };

-MODULE_DEVICE_TABLE(usb, ccp_devices);
+MODULE_DEVICE_TABLE(hid, ccp_devices);
 MODULE_LICENSE("GPL");

-module_usb_driver(ccp_driver);
+static int __init ccp_init(void)
+{
+	return hid_register_driver(&ccp_driver);
+}
+
+static void __exit ccp_exit(void)
+{
+	hid_unregister_driver(&ccp_driver);
+}
+
+/* make sure it is loaded after hid */
+late_initcall(ccp_init);
+module_exit(ccp_exit);