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 <[email protected]> > > --- > > 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 >
