[PATCH] hwmon: corsair-cpro: Change to hid driver

[PATCH] hwmon: corsair-cpro: Change to hid driver

[Marius Zachmann]

I found a project which uses hidraw to communicate with the device.
Because I do not want to break any existing userspace code, I
changed this to a hid driver, so hidraw can still be used.
Do I need to include the hid maintainers for the undo in hid-quirks?

Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
---
 drivers/hid/hid-quirks.c     |   2 -
 drivers/hwmon/Kconfig        |   4 +-
 drivers/hwmon/corsair-cpro.c | 114 ++++++++++++++++++++++++++---------
 3 files changed, 86 insertions(+), 34 deletions(-)

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.
@@ -1716,7 +1716,7 @@ config SENSORS_ADS7871

 config SENSORS_AMC6821
 	tristate "Texas Instruments AMC6821"
-	depends on I2C
+	depends on I2C
 	help
 	  If you say yes here you get support for the Texas Instruments
 	  AMC6821 hardware monitoring chips.
diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
index fe625190e3a1..4310ee5aca24 100644
--- a/drivers/hwmon/corsair-cpro.c
+++ b/drivers/hwmon/corsair-cpro.c
@@ -5,13 +5,14 @@
  */

 #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
@@ -62,7 +63,8 @@
 #define NUM_TEMP_SENSORS	4

 struct ccp_device {
-	struct usb_device *udev;
+	struct hid_device *hdev;
+	struct completion wait_input_report;
 	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
 	u8 *buffer;
 	int pwm[6];
@@ -75,7 +77,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 +86,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(300));
+	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 +452,96 @@ 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;
-
-	hwmon_dev = devm_hwmon_device_register_with_info(&intf->dev, "corsaircpro", ccp,
+		goto out_hw_close;
+	hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "corsaircpro", ccp,
 							 &ccp_chip_info, 0);
+	if (IS_ERR(hwmon_dev)) {
+		ret = PTR_ERR(hwmon_dev);
+		goto out_hw_close;
+	}
+
+	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)
 {
+	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);
--
2.27.0

Re: [PATCH] hwmon: corsair-cpro: Change to hid driver

[Guenter Roeck]

On Tue, Jul 14, 2020 at 12:52:30PM +0200, Marius Zachmann wrote:
> I found a project which uses hidraw to communicate with the device.
> Because I do not want to break any existing userspace code, I
> changed this to a hid driver, so hidraw can still be used.
> Do I need to include the hid maintainers for the undo in hid-quirks?
> 
That changelog needs some improvements. It should state what change
was made, and why, but not include any questions or personal statements
such as "I found ...".

You never really explained why you had changed the driver from hid to
usb. Maybe you can explain that decision now ?

Thanks,
Guenter

> Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
> ---
>  drivers/hid/hid-quirks.c     |   2 -
>  drivers/hwmon/Kconfig        |   4 +-
>  drivers/hwmon/corsair-cpro.c | 114 ++++++++++++++++++++++++++---------
>  3 files changed, 86 insertions(+), 34 deletions(-)
> 
> 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.
> @@ -1716,7 +1716,7 @@ config SENSORS_ADS7871
> 
>  config SENSORS_AMC6821
>  	tristate "Texas Instruments AMC6821"
> -	depends on I2C
> +	depends on I2C
>  	help
>  	  If you say yes here you get support for the Texas Instruments
>  	  AMC6821 hardware monitoring chips.
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index fe625190e3a1..4310ee5aca24 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -5,13 +5,14 @@
>   */
> 
>  #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
> @@ -62,7 +63,8 @@
>  #define NUM_TEMP_SENSORS	4
> 
>  struct ccp_device {
> -	struct usb_device *udev;
> +	struct hid_device *hdev;
> +	struct completion wait_input_report;
>  	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
>  	u8 *buffer;
>  	int pwm[6];
> @@ -75,7 +77,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 +86,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(300));
> +	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 +452,96 @@ 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;
> -
> -	hwmon_dev = devm_hwmon_device_register_with_info(&intf->dev, "corsaircpro", ccp,
> +		goto out_hw_close;
> +	hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "corsaircpro", ccp,
>  							 &ccp_chip_info, 0);
> +	if (IS_ERR(hwmon_dev)) {
> +		ret = PTR_ERR(hwmon_dev);
> +		goto out_hw_close;
> +	}
> +
> +	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)
>  {
> +	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);
> --
> 2.27.0

Re: [PATCH] hwmon: corsair-cpro: Change to hid driver

[Marius Zachmann]

On 15.07.20 at 04:36:22 CEST, Guenter Roeck wrote
> On Tue, Jul 14, 2020 at 12:52:30PM +0200, Marius Zachmann wrote:
> > I found a project which uses hidraw to communicate with the device.
> > Because I do not want to break any existing userspace code, I
> > changed this to a hid driver, so hidraw can still be used.
> > Do I need to include the hid maintainers for the undo in hid-quirks?
> > 
> That changelog needs some improvements. It should state what change
> was made, and why, but not include any questions or personal statements
> such as "I found ...".
> 
> You never really explained why you had changed the driver from hid to
> usb. Maybe you can explain that decision now ?
> 
> Thanks,
> Guenter
> 

This device does not use numbered hid input reports. As far as I know
it is not possible to determine from which command a response was
requested. This being a usb driver would not allow any other driver
to send data to the device.
In the first (hid) version I did not use hid reports and got
the usb_device directly from the hid_device which is not intended.
In this version, requests are made via hid_hw_output_report, waited
for and received via raw_event.
If one uses hidraw and this driver at the same time, it still could
be possible to get switched responses but there is nothing I can
do about it, as far as I know. This is only about giving the
possibility to unload the driver and using a userspace driver
if one chooses.

Greetings
Marius

> > Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
> > ---
> >  drivers/hid/hid-quirks.c     |   2 -
> >  drivers/hwmon/Kconfig        |   4 +-
> >  drivers/hwmon/corsair-cpro.c | 114 ++++++++++++++++++++++++++---------
> >  3 files changed, 86 insertions(+), 34 deletions(-)
> > 
> > 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.
> > @@ -1716,7 +1716,7 @@ config SENSORS_ADS7871
> > 
> >  config SENSORS_AMC6821
> >  	tristate "Texas Instruments AMC6821"
> > -	depends on I2C
> > +	depends on I2C
> >  	help
> >  	  If you say yes here you get support for the Texas Instruments
> >  	  AMC6821 hardware monitoring chips.
> > diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> > index fe625190e3a1..4310ee5aca24 100644
> > --- a/drivers/hwmon/corsair-cpro.c
> > +++ b/drivers/hwmon/corsair-cpro.c
> > @@ -5,13 +5,14 @@
> >   */
> > 
> >  #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
> > @@ -62,7 +63,8 @@
> >  #define NUM_TEMP_SENSORS	4
> > 
> >  struct ccp_device {
> > -	struct usb_device *udev;
> > +	struct hid_device *hdev;
> > +	struct completion wait_input_report;
> >  	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
> >  	u8 *buffer;
> >  	int pwm[6];
> > @@ -75,7 +77,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 +86,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(300));
> > +	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 +452,96 @@ 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;
> > -
> > -	hwmon_dev = devm_hwmon_device_register_with_info(&intf->dev, "corsaircpro", ccp,
> > +		goto out_hw_close;
> > +	hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "corsaircpro", ccp,
> >  							 &ccp_chip_info, 0);
> > +	if (IS_ERR(hwmon_dev)) {
> > +		ret = PTR_ERR(hwmon_dev);
> > +		goto out_hw_close;
> > +	}
> > +
> > +	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)
> >  {
> > +	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);
> > --
> > 2.27.0
> 

Re: [PATCH] hwmon: corsair-cpro: Change to hid driver

[Guenter Roeck]

On Tue, Jul 14, 2020 at 12:52:30PM +0200, Marius Zachmann wrote:
> I found a project which uses hidraw to communicate with the device.
> Because I do not want to break any existing userspace code, I
> changed this to a hid driver, so hidraw can still be used.
> Do I need to include the hid maintainers for the undo in hid-quirks?

I think I'll squash this patch with the others.

> 
> Signed-off-by: Marius Zachmann <mail@mariuszachmann.de>
> ---
>  drivers/hid/hid-quirks.c     |   2 -
>  drivers/hwmon/Kconfig        |   4 +-
>  drivers/hwmon/corsair-cpro.c | 114 ++++++++++++++++++++++++++---------
>  3 files changed, 86 insertions(+), 34 deletions(-)
> 
> 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.
> @@ -1716,7 +1716,7 @@ config SENSORS_ADS7871
> 
>  config SENSORS_AMC6821
>  	tristate "Texas Instruments AMC6821"
> -	depends on I2C
> +	depends on I2C

Unrelated change; please drop.

>  	help
>  	  If you say yes here you get support for the Texas Instruments
>  	  AMC6821 hardware monitoring chips.
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index fe625190e3a1..4310ee5aca24 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -5,13 +5,14 @@
>   */
> 
>  #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
> @@ -62,7 +63,8 @@
>  #define NUM_TEMP_SENSORS	4
> 
>  struct ccp_device {
> -	struct usb_device *udev;
> +	struct hid_device *hdev;
> +	struct completion wait_input_report;
>  	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
>  	u8 *buffer;
>  	int pwm[6];
> @@ -75,7 +77,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 +86,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(300));

Please use a define for the 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 +452,96 @@ 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;
> -
> -	hwmon_dev = devm_hwmon_device_register_with_info(&intf->dev, "corsaircpro", ccp,
> +		goto out_hw_close;
> +	hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "corsaircpro", ccp,
>  							 &ccp_chip_info, 0);

You'll have to use hwmon_device_register_with_info() and call the remove
function explicitly in ccp_remove() to avoid race conditions.
Alternatively, you could use devm_add_action_or_reset() to handle
hid_hw_close() and hid_hw_stop(), but that would probably not be worth
the added complexity.

> +	if (IS_ERR(hwmon_dev)) {
> +		ret = PTR_ERR(hwmon_dev);
> +		goto out_hw_close;
> +	}
> +
> +	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)
>  {
> +	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);
> --
> 2.27.0

Re: [PATCH] hwmon: corsair-cpro: Change to hid driver

[Guenter Roeck]

On Wed, Jul 15, 2020 at 08:59:52AM +0200, Marius Zachmann wrote:
> On 15.07.20 at 04:36:22 CEST, Guenter Roeck wrote
> > On Tue, Jul 14, 2020 at 12:52:30PM +0200, Marius Zachmann wrote:
> > > I found a project which uses hidraw to communicate with the device.
> > > Because I do not want to break any existing userspace code, I
> > > changed this to a hid driver, so hidraw can still be used.
> > > Do I need to include the hid maintainers for the undo in hid-quirks?
> > > 
> > That changelog needs some improvements. It should state what change
> > was made, and why, but not include any questions or personal statements
> > such as "I found ...".
> > 
> > You never really explained why you had changed the driver from hid to
> > usb. Maybe you can explain that decision now ?
> > 
> > Thanks,
> > Guenter
> > 
> 
> This device does not use numbered hid input reports. As far as I know
> it is not possible to determine from which command a response was
> requested. This being a usb driver would not allow any other driver
> to send data to the device.
> In the first (hid) version I did not use hid reports and got
> the usb_device directly from the hid_device which is not intended.
> In this version, requests are made via hid_hw_output_report, waited
> for and received via raw_event.
> If one uses hidraw and this driver at the same time, it still could
> be possible to get switched responses but there is nothing I can
> do about it, as far as I know. This is only about giving the
> possibility to unload the driver and using a userspace driver
> if one chooses.
> 
What a mess. Can you add some of this as comment to the top of the driver
to explain the reasoning ?

Thanks,
Guenter