On 10.06.20 at 06:01:45 CEST, Guenter Roeck wrote:
> On Wed, Jun 10, 2020 at 12:26:40AM +0200, Marius Zachmann wrote:
> > This is a driver for the Corsair Commander Pro.
> > Since this is my first addition to the kernel I would be happy, if you 
> > would take a look at it.
> > I am using this driver on my computer at home and I had a few people test 
> > it.
> > 
> > The device:
> > 
> > The Corsair Commander Pro is a USB device, which is usually connected on 
> > the internal USB headers on the mainboard.
> > It has 6 fan connectors, 4 thermal sensor connectors and 2 RGB connectors.
> > It also reads the voltages on the SATA connector, which it needs for power.
> > 
> > The driver:
> > 
> > I hope hwmon is the right place.
> > The driver is a HID driver, but it uses usb_bulk_msg for communication.
> > The device registers as a HID device and there are userspace tools, which 
> > use hidraw to access it. I think, it would be good to maintain 
> > compatibility with these tools, but the driver can easily be rewritten to 
> > be a pure USB driver.
> > For now it can read temp sensors, fan speeds, voltage values and set pwm 
> > values.
> > It also reads the connection status on the fan headers.
> > 
> > There are a few more things, which I would like to add in the near future:
> > * Fan curves (not yet sure about the nicest way to provide sysfs access)
> > * Force 3pin or 4pin mode. (Sometimes the device doesn't detect the fans 
> > correctly)
> > * Setting fixed RPM
> > 
> > I do not work for Corsair and I intend to keep the driver maintainted as 
> > long as I use the device privately.
> > 
> The above should be limited to the driver description. Discussion should be
> below the '---'. Either case, make sure to split your lines.
> 
> > Signed-off-by: Marius Zachmann <m...@mariuszachmann.de>
> > ---
> >  Documentation/hwmon/corsair-cpro.rst |  42 +++
> 
> Needs to be added to Documentation/hwmon/index.rst.
> 
> >  MAINTAINERS                          |   6 +
> >  drivers/hwmon/Kconfig                |  10 +
> >  drivers/hwmon/Makefile               |   1 +
> >  drivers/hwmon/corsair-cpro.c         | 481 +++++++++++++++++++++++++++
> >  5 files changed, 540 insertions(+)
> >  create mode 100644 Documentation/hwmon/corsair-cpro.rst
> >  create mode 100644 drivers/hwmon/corsair-cpro.c
> > 
> > diff --git a/Documentation/hwmon/corsair-cpro.rst 
> > b/Documentation/hwmon/corsair-cpro.rst
> > new file mode 100644
> > index 000000000000..d4ea1b6b9336
> > --- /dev/null
> > +++ b/Documentation/hwmon/corsair-cpro.rst
> > @@ -0,0 +1,42 @@
> > +Kernel driver corsair-cpro
> > +==========================
> > +
> > +Supported devices:
> > +
> > +  * Corsair Commander Pro
> > +  * Corsair Commander Pro (1000D)
> > +
> > +Author: Marius Zachmann
> > +
> > +Description
> > +-----------
> > +
> > +This driver implements the sysfs interface for the Corsair Commander Pro.
> > +The Corsair Commander Pro is a USB device with 6 fan connectors,
> > +4 temperature sensor connectors and 2 Corsair LED connectors.
> > +It can read the voltage levels on the SATA power connector.
> > +
> > +Usage Notes
> > +-----------
> > +
> > +Since it is a USB device, hotswapping is possible. The device is 
> > autodetected.
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +in0_input          Voltage on SATA 12v
> > +in1_input          Voltage on SATA 5v
> > +in2_input          Voltage on SATA 3.3v
> > +
> > +temp[0-3]_input            Connected temperature sensors
> > +
> Index starts with 1 for everything except inX.
> 
> > +fan[0-5]_input             Connected fan rpm.
> > +fan[0-5]_label             Shows connection status of the fan as detected 
> > by the
> > +                   device.
> > +                   "fanX nc"   no connection
> > +                   "fanX 3pin" 3-pin fan detected
> > +                   "fanX 4pin" 4-pin fan detected
> > +fan[0-5]_enable            the driver only reports fan speeds when 1
> > +pwm[0-5]           Sets the fan speed. Values from 0-255.
> > +                   When reading, it reports the last value, which
> > +                   was set by the driver.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f08f290df174..169530c7eede 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4386,6 +4386,12 @@ S:   Maintained
> >  F: Documentation/hwmon/coretemp.rst
> >  F: drivers/hwmon/coretemp.c
> >  
> > +CORSAIR-CPRO HARDWARE MONITOR DRIVER
> > +M: Marius Zachmann <m...@mariuszachmann.de>
> > +L: linux-hw...@vger.kernel.org
> > +S: Maintained
> > +F: drivers/hwmon/corsair-cpro.c
> > +
> >  COSA/SRP SYNC SERIAL DRIVER
> >  M: Jan "Yenya" Kasprzak <k...@fi.muni.cz>
> >  S: Maintained
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 288ae9f63588..9f5a808768ca 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -439,6 +439,16 @@ config SENSORS_BT1_PVT_ALARMS
> >       the data conversion will be periodically performed and the data will 
> > be
> >       saved in the internal driver cache.
> >  
> > +config SENSORS_CORSAIR_CPRO
> > +   tristate "Corsair Commander Pro controller"
> > +   depends on USB_HID
> > +   help
> > +     If you say yes here you get support for the Corsair Commander Pro
> > +     controller.
> > +
> > +     This driver can also be built as a module. If so, the module
> > +     will be called corsair-cpro.
> > +
> >  config SENSORS_DRIVETEMP
> >     tristate "Hard disk drives with temperature sensors"
> >     depends on SCSI && ATA
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 3e32c21f5efe..18e1ef74ade7 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -56,6 +56,7 @@ obj-$(CONFIG_SENSORS_ATXP1)       += atxp1.o
> >  obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
> >  obj-$(CONFIG_SENSORS_BT1_PVT)      += bt1-pvt.o
> >  obj-$(CONFIG_SENSORS_CORETEMP)     += coretemp.o
> > +obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
> >  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> >  obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> >  obj-$(CONFIG_SENSORS_DELL_SMM)     += dell-smm-hwmon.o
> > diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> > new file mode 100644
> > index 000000000000..8ba7fb061184
> > --- /dev/null
> > +++ b/drivers/hwmon/corsair-cpro.c
> > @@ -0,0 +1,481 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> 
> This conflicts with MODULE_LICENSE which explicitly states GPL v2.
> 
> > +/*
> > + * corsair-cpro.c - Linux driver for Corsair Commander Pro
> > + * Copyright (C) 2020 Marius Zachmann <m...@mariuszachmann.de>
> > + *
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/hid.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/mutex.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb.h>
> > +
> > +#define    hid_to_usb_dev(hid_dev) \
> > +   to_usb_device(hid_dev->dev.parent->parent)
> 
> Unnecessary line break.
> 
> > +
> > +#define USB_VENDOR_ID_CORSAIR               0x1b1c
> > +#define USB_PRODUCT_ID_CORSAIR_COMMANDERPRO 0x0c10
> > +#define USB_PRODUCT_ID_CORSAIR_1000D           0x1d00
> > +
> > +#define OUT_BUFFER_SIZE 63
> > +#define IN_BUFFER_SIZE 16
> > +#define LABEL_LENGTH 10
> > +
> > +#define CTL_GET_TMP         0x11  /* byte 1 is channel, rest zero          
> >     */
> > +                          /* returns temp for channel in bytes 1 and 2 */
> > +#define CTL_GET_VOLT        0x12  /* byte 1 = rail number 12, 5, 3.3 */
> > +                          /* returns volt in bytes 1,2       */
> > +#define CTL_GET_FAN_CNCT 0x20  /* returns in bytes 1-6   */
> > +                          /* 0 for no connect       */
> > +                          /* 1 for 3pin, 2 for 4pin */
> > +#define CTL_GET_FAN_RPM     0x21  /* works like CTL_GET_TMP */
> > +#define CTL_SET_FAN_FPWM 0x23  /* byte 1 is fan number              */
> > +                          /* byte 2 is percentage from 0 - 100 */
> > +
> 
> Please use tabs consistently after the defined name, eg
> 
> #define OUT_BUFFER_SIZE       63
> #define IN_BUFFER_SIZE        16
> #define LABEL_LENGTH  10
> 
> > +struct ccp_device {
> > +   struct hid_device *hdev;
> > +   struct device *hwmondev;
> 
> Not used anywhere.
> 
> > +   struct mutex mutex;
> > +   int pwm[6];
> > +   int fan_enable[6];
> > +   char fan_label[6][LABEL_LENGTH];
> > +
> 
> Unnecessary empty line.
> 
> > +};
> > +
> > +/* send 63 byte buffer and receive response in same buffer */
> > +static int send_usb_cmd(struct ccp_device *ccp, u8 *buffer)
> > +{
> > +   int ret;
> > +   struct usb_device *udev = hid_to_usb_dev(ccp->hdev);
> > +   int actual_length;
> > +
> > +
> Unnecessary empty line
> 
> > +   mutex_lock(&ccp->mutex);
> > +
> > +   ret = usb_bulk_msg(udev,
> > +                   usb_sndintpipe(udev, 2),
> > +                   buffer,
> > +                   OUT_BUFFER_SIZE,
> > +                   &actual_length,
> > +                   1000);
> 
> Too many line breaks. All over the place. Please only split lines where
> necessary (line length limit is now 100).
> 
> > +   if (ret < 0) {
> > +           hid_err(ccp->hdev,
> > +                   "usb_bulk_msg send failed: %d", ret);
> > +           goto exit;
> > +   }
> > +
> > +   ret = usb_bulk_msg(udev,
> > +                   usb_rcvintpipe(udev, 1),
> > +                   buffer,
> > +                   IN_BUFFER_SIZE,
> > +                   &actual_length,
> > +                   1000);
> > +   if (ret) {
> > +           hid_err(ccp->hdev,
> > +                   "usb_bulk_msg receive failed: %d", ret);
> > +           goto exit;
> > +   }
> > +
> > +exit:
> > +   mutex_unlock(&ccp->mutex);
> > +   return ret;
> > +}
> > +
> > +/* for commands, which return just a number depending on a channel: */
> > +/* get_temp, get_volt, get_fan_rpm */
> > +static int get_data(struct ccp_device *ccp, int command, int channel, long 
> > *val)
> > +{
> > +   int ret = 0;
> 
> Unnecessary initialization (almost everywhere).
> 
> > +   u8 *buffer;
> > +
> > +   buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
> > +   if (buffer == 0)
> > +           return -ENOMEM;
> 
> AFAICS all those commands end up locking anyway. Might as well
> move the buffer into struct ccp_device and use it from there
> (ie lock while using it).
> 
> > +
> > +   buffer[0] = command;
> > +   buffer[1] = channel;
> > +   ret = send_usb_cmd(ccp, buffer);
> > +   if (ret)
> > +           return -EIO;
> 
> Please do not replace error codes - not just here, everywhere.
> 
> > +
> > +   *val = (buffer[1] << 8) + buffer[2];
> > +
> > +   kfree(buffer);
> > +   return ret;
> 
> Just return 0;
> 
> > +}
> > +
> > +static int set_pwm(struct ccp_device *ccp, int channel, long val)
> > +{
> > +   int ret = 0;
> > +   u8 *buffer;
> > +
> > +   if (val > 255)
> > +           return -EINVAL;
> 
> How about val < 0 ?
> 
> > +
> > +   ccp->pwm[channel] = val;
> > +
> > +   /* The Corsair Commander Pro uses values from 0-100 */
> > +   val = val << 8;
> > +   val = val / 255;
> > +   val = val * 100;
> > +   val = val >> 8;
> 
>       val = DIV_ROUND_CLOSEST(val * 100, 255);
> 
> would be much easier to read.
> 
> > +
> > +   buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
> > +   if (buffer == 0)
> 
>       if (!buffer)
> > +           return -ENOMEM;
> > +
> > +   buffer[0] = CTL_SET_FAN_FPWM;
> > +   buffer[1] = channel;
> > +   buffer[2] = val;
> > +   ret = send_usb_cmd(ccp, buffer);
> > +
> > +   kfree(buffer);
> > +   return ret == 0 ? 0 : -EIO;
> > +}
> > +
> > +static int get_fan_mode_label(struct ccp_device *ccp, int channel)
> > +{
> > +   int ret = 0;
> > +   int mode;
> > +   u8 *buffer;
> > +
> > +   buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
> > +   if (buffer == 0)
> > +           return -ENOMEM;
> > +
> > +   buffer[0] = CTL_GET_FAN_CNCT;
> > +   ret = send_usb_cmd(ccp, buffer);
> > +   if (ret)
> > +           goto exit;
> > +
> > +   mode = buffer[channel+1];
> 
> Space around operators (use checkpatch --strict to find all locations).
> 
> > +
> > +   switch (mode) {
> > +   case 0:
> > +           scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
> > +                     "fan%d nc", channel+1);
> > +           break;
> > +   case 1:
> > +           scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
> > +                     "fan%d 3pin", channel+1);
> > +           break;
> > +   case 2:
> > +           scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
> > +                     "fan%d 4pin", channel+1);
> > +           break;
> > +   default:
> > +           dev_err(&ccp->hdev->dev,
> > +                   "Mode Description %d not implemented", mode);
> 
> If this is an error that needs to be logged you have to return an error.
> I would suggest to name the label something like "fanX other" instead
> in this situation.
> 
> > +           break;
> > +   }
> > +
> > +exit:
> > +   kfree(buffer);
> > +   return ret == 0 ? 0 : -EIO;
> 
> While it is unacceptable, I am curious why you override error codes 
> everywhere.
> 
> > +}
> > +
> > +static int get_voltages(struct ccp_device *ccp, int channel, long *val)
> > +{
> > +   int ret = 0;
> > +
> > +   ret = get_data(ccp, CTL_GET_VOLT, channel, val);
> > +
> > +   return ret == 0 ? 0 : -EIO;
> > +}
> > +
> > +static int get_temp(struct ccp_device *ccp, int channel, long *val)
> > +{
> > +   int ret = 0;
> > +
> > +   ret = get_data(ccp, CTL_GET_TMP, channel, val);
> > +   *val = *val * 10;
> > +
> > +   return ret == 0 ? 0 : -EIO;
> > +}
> > +
> > +static int get_rpm(struct ccp_device *ccp, int channel, long *val)
> > +{
> > +   int ret = 0;
> > +
> > +   if (ccp->fan_enable[channel] != 1)
> 
> Valid values are 0 and 1, so != 1 is unnecessary. Use
>       if (!ccp->fan_enable[channel])
> instead.
> 
> > +           return -ENODATA;
> > +
> > +   ret = get_data(ccp, CTL_GET_FAN_RPM, channel, val);
> > +
> > +   return ret == 0 ? 0 : -EIO;
> > +}
> > +
> > +static int ccp_read_string(struct device *dev, enum hwmon_sensor_types 
> > type,
> > +                      u32 attr, int channel, const char **str)
> > +{
> > +   int ret = 0;
> > +   struct ccp_device *ccp = dev_get_drvdata(dev);
> > +
> > +   switch (type) {
> > +   case hwmon_fan:
> > +           switch (attr) {
> > +           case hwmon_fan_label:
> > +                   ret = get_fan_mode_label(ccp, channel);
> > +                   *str = ccp->fan_label[channel];
> > +                   break;
> > +           default:
> > +                   ret = -EINVAL;
> > +                   break;
> > +           }
> > +           break;
> > +   default:
> > +           ret = -EINVAL;
> > +           break;
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> > +static int ccp_read(struct device *dev, enum hwmon_sensor_types type,
> > +               u32 attr, int channel, long *val)
> > +{
> > +   int ret = 0;
> > +   struct ccp_device *ccp = dev_get_drvdata(dev);
> > +
> > +   switch (type) {
> > +   case hwmon_temp:
> > +           switch (attr) {
> > +           case hwmon_temp_input:
> > +                   ret = get_temp(ccp, channel, val);
> > +                   break;
> > +           default:
> > +                   ret = -EINVAL;
> > +                   break;
> > +           }
> > +           break;
> > +   case hwmon_fan:
> > +           switch (attr) {
> > +           case hwmon_fan_input:
> > +                   ret = get_rpm(ccp, channel, val);
> > +                   break;
> > +           case hwmon_fan_enable:
> > +                   *val = ccp->fan_enable[channel];
> > +                   break;
> > +           default:
> > +                   ret = -EINVAL;
> > +                   break;
> > +           }
> > +           break;
> > +   case hwmon_pwm:
> > +           switch (attr) {
> > +           case hwmon_pwm_input:
> > +                   *val = ccp->pwm[channel];
> 
> This returns 0 if pwm wasn't set. Is this indeed not readable from the
> device ?

I could not find any possibility, because the official Corsair software does 
not use it.
I am not sure, whether it would be better to return an error, if it was not set.

> 
> > +                   break;
> > +           default:
> > +                   ret = -EINVAL;
> > +                   break;
> > +           }
> > +           break;
> > +   case hwmon_in:
> > +           switch (attr) {
> > +           case hwmon_in_input:
> > +                   ret = get_voltages(ccp, channel, val);
> > +                   break;
> > +           default:
> > +                   ret = -EINVAL;
> > +                   break;
> > +           }
> > +           break;
> > +   default:
> > +           ret = -EINVAL;
> > +   }
> > +   return ret;
> > +};
> > +
> > +static int ccp_write(struct device *dev, enum hwmon_sensor_types type,
> > +                u32 attr, int channel, long val)
> > +{
> > +   int ret = 0;
> > +   struct ccp_device *ccp = dev_get_drvdata(dev);
> > +
> > +   switch (type) {
> > +   case hwmon_fan:
> > +           switch (attr) {
> > +           case hwmon_fan_enable:
> > +                   ccp->fan_enable[channel] = val;
> 
> Needs to be validated. Accepting random values does not make sense here.
> 
> > +                   break;
> > +           default:
> > +                   ret = -EINVAL;
> 
> -EOPNOTSUPP for those cases.
> 
> > +                   break;
> > +           }
> > +           break;
> > +   case hwmon_pwm:
> > +           switch (attr) {
> > +           case hwmon_pwm_input:
> > +                   set_pwm(ccp, channel, val);
> 
> Do not ignore errors.
> 
> > +                   break;
> > +           default:
> > +                   ret = -EINVAL;
> > +                   break;
> > +           }
> > +           break;
> > +   default:
> > +           ret = -EINVAL;
> > +           break;
> > +   }
> > +   return ret;
> > +};
> > +
> > +static umode_t ccp_is_visible(const void *data, enum hwmon_sensor_types 
> > type,
> > +                         u32 attr, int channel)
> > +{
> > +   switch (type) {
> > +   case hwmon_chip:
> > +           switch (attr) {
> > +           case hwmon_chip_update_interval:
> > +                   return 0644;
> 
>               default:
> 
> missing everywhere here.
> 
> > +           }
> > +           break;
> > +   case hwmon_temp:
> > +           switch (attr) {
> > +           case hwmon_temp_input:
> > +                   return 0444;
> > +           }
> > +           break;
> > +   case hwmon_fan:
> > +           switch (attr) {
> > +           case hwmon_fan_input:
> > +                   return 0444;
> > +           case hwmon_fan_label:
> > +                   return 0444;
> > +           case hwmon_fan_enable:
> > +                   return 0644;
> > +           }
> > +           break;
> > +   case hwmon_pwm:
> > +           switch (attr) {
> > +           case hwmon_pwm_input:
> > +                   return 0644;
> > +           }
> > +           break;
> > +   case hwmon_in:
> > +           switch (attr) {
> > +           case hwmon_in_input:
> > +                   return 0444;
> > +           }
> > +           break;
> > +   default:
> > +           break;
> > +   }
> > +   return 0;
> > +};
> > +
> > +static const struct hwmon_ops ccp_hwmon_ops = {
> > +   .is_visible = ccp_is_visible,
> > +   .read = ccp_read,
> > +   .write = ccp_write,
> > +   .read_string = ccp_read_string,
> > +};
> > +
> > +static const struct hwmon_channel_info *ccp_info[] = {
> > +   HWMON_CHANNEL_INFO(chip,
> > +                      HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL),
> > +   HWMON_CHANNEL_INFO(temp,
> > +                      HWMON_T_INPUT | HWMON_T_MAX,
> > +                      HWMON_T_INPUT | HWMON_T_MAX,
> > +                      HWMON_T_INPUT | HWMON_T_MAX,
> > +                      HWMON_T_INPUT | HWMON_T_MAX
> > +                      ),
> > +   HWMON_CHANNEL_INFO(fan,
> > +                      HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> > +                      HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> > +                      HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> > +                      HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> > +                      HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> > +                      HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL
> > +                      ),
> > +   HWMON_CHANNEL_INFO(pwm,
> > +                      HWMON_PWM_INPUT,
> > +                      HWMON_PWM_INPUT,
> > +                      HWMON_PWM_INPUT,
> > +                      HWMON_PWM_INPUT,
> > +                      HWMON_PWM_INPUT,
> > +                      HWMON_PWM_INPUT
> > +                      ),
> > +   HWMON_CHANNEL_INFO(in,
> > +                      HWMON_I_INPUT,
> > +                      HWMON_I_INPUT,
> > +                      HWMON_I_INPUT
> > +                      ),
> > +   NULL
> > +};
> > +
> > +static const struct hwmon_chip_info ccp_chip_info = {
> > +   .ops = &ccp_hwmon_ops,
> > +   .info = ccp_info,
> > +
> 
> 
> Another unnecessary empty line.
> 
> > +};
> > +
> > +static int ccp_probe(struct hid_device *hdev, const struct hid_device_id 
> > *id)
> > +{
> > +   struct ccp_device *ccp;
> > +   int ret = 0;
> > +
> > +   ret = hid_parse(hdev);
> > +   if (ret) {
> > +           hid_err(hdev, "hid_parse failed\n");
> > +           goto exit;
> 
>               return ret;
> 
> > +   }
> > +
> > +   ccp = devm_kzalloc(&hdev->dev, sizeof(struct ccp_device), GFP_KERNEL);
> > +   if (ccp == NULL)
> > +           goto exit;
> 
>               return -ENOMEM;
> 
> > +
> > +   mutex_init(&ccp->mutex);
> > +
> > +   ccp->fan_enable[0] = 1;
> > +   ccp->fan_enable[1] = 1;
> > +   ccp->fan_enable[2] = 1;
> > +   ccp->fan_enable[3] = 1;
> > +   ccp->fan_enable[4] = 1;
> > +   ccp->fan_enable[5] = 1;
> > +
> > +   hid_set_drvdata(hdev, ccp);
> > +
> > +   ccp->hdev = hdev;
> > +   ccp->hwmondev = devm_hwmon_device_register_with_info(&hdev->dev,
> > +                           "corsaircpro",
> > +                           ccp,
> > +                           &ccp_chip_info,
> > +                           0);
> 
> This can return an ERR_PTR.
> 
> > +
> > +exit:
> > +   return ret;
> > +}
> > +
> > +static void ccp_remove(struct hid_device *hdev)
> > +{
> > +   struct ccp_device *ccp;
> > +
> > +   ccp = hid_get_drvdata(hdev);
> > +   mutex_destroy(&ccp->mutex);
> 
> The memory is about to be released, so this doesn't really add value.
> Just drop the remove function and forget about mutex_destroy().
> 
> > +}
> > +
> > +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 hid_driver ccp_driver = {
> > +   .name = "corsair-cpro",
> > +   .id_table = ccp_devices,
> > +   .probe = ccp_probe,
> > +   .remove = ccp_remove
> > +};
> > +
> > +MODULE_DEVICE_TABLE(hid, ccp_devices);
> > +MODULE_LICENSE("GPL v2");
> > +
> > +module_hid_driver(ccp_driver);
> 

Thank you for your time!


Reply via email to