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 |
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
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
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
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
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
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 --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);
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