Re: [RFC] drivers/hwmon: Corsair Commander Pro driver
On 6/10/20 5:03 AM, Marius Zachmann wrote:
[ ... ]
>>> +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]_inputConnected 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]_enablethe 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.
Change to:
When reading, it reports the last value if it was set
by the driver.
Otherwise returns 0.
[ ... ]
>>> + 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.
>
Bummer. Then change the documentation as suggested above, and add
a comment here explaining that it is unknown how to read pwm values
from the device.
Thanks,
Guenter
Re: [RFC] drivers/hwmon: Corsair Commander Pro driver
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 > > --- > > 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 ..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]_inputConnected 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]_enablethe 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 > > +L: linux-hw...@vger.kernel.org > > +S: Maintained > > +F: drivers/hwmon/corsair-cpro.c > > + > > COSA/SRP SYNC SERIAL DRIVER > > M: Jan "Yenya" Kasprzak > > 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
Re: [RFC] drivers/hwmon: Corsair Commander Pro driver
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 > --- > 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 ..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_inputVoltage on SATA 12v > +in1_inputVoltage on SATA 5v > +in2_inputVoltage 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 > +L: linux-hw...@vger.kernel.org > +S: Maintained > +F: drivers/hwmon/corsair-cpro.c > + > COSA/SRP SYNC SERIAL DRIVER > M: Jan "Yenya" Kasprzak > 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,
