On Tue, Jul 21, 2020 at 10:54:47AM +0200, Marius Zachmann wrote:
> This adds the possibility for reading pwm values.
> These can not be read if the device is controlled via
> fan_target or a fan curve and will return an error in
> this case. Since an error is expected, this adds some
> rudimentary error handling.
>
> Changes:
> - add CTL_GET_FAN_PWM and use it via get_data
> - pwm returns -ENODATA if the device returns error 0x12
> - fan_target now returns -ENODATA when the driver is
> started or a pwm value is set.
> - add ccp_get_errno to determine errno from device error.
> - get_data now has a parameter to determine whether
> to read one or two bytes of data.
> - update documentation
> - fix missing surname in MAINTAINERS
>
> Signed-off-by: Marius Zachmann <mail@xxxxxxxxxxxxxxxxx>
Applied.
Thanks,
Guenter
> ---
> Documentation/hwmon/corsair-cpro.rst | 7 ++-
> MAINTAINERS | 2 +-
> drivers/hwmon/corsair-cpro.c | 64 +++++++++++++++++++---------
> 3 files changed, 48 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/hwmon/corsair-cpro.rst b/Documentation/hwmon/corsair-cpro.rst
> index 78820156f07d..751f95476b57 100644
> --- a/Documentation/hwmon/corsair-cpro.rst
> +++ b/Documentation/hwmon/corsair-cpro.rst
> @@ -35,8 +35,7 @@ fan[1-6]_input Connected fan rpm.
> fan[1-6]_label Shows fan type as detected by the device.
> fan[1-6]_target Sets fan speed target rpm.
> When reading, it reports the last value if it was set by the driver.
> - Otherwise returns 0.
> -pwm[1-6] Sets the fan speed. Values from 0-255.
> - When reading, it reports the last value if it was set by the driver.
> - Otherwise returns 0.
> + Otherwise returns an error.
> +pwm[1-6] Sets the fan speed. Values from 0-255. Can only be read if pwm
> + was set directly.
> ======================= =====================================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 06607125b793..a93aefab91f1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4402,7 +4402,7 @@ F: Documentation/hwmon/coretemp.rst
> F: drivers/hwmon/coretemp.c
>
> CORSAIR-CPRO HARDWARE MONITOR DRIVER
> -M: Marius <mail@xxxxxxxxxxxxxxxxx>
> +M: Marius Zachmann <mail@xxxxxxxxxxxxxxxxx>
> L: linux-hwmon@xxxxxxxxxxxxxxx
> S: Maintained
> F: drivers/hwmon/corsair-cpro.c
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index e8504267d0e8..591929ec217a 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -36,11 +36,12 @@
> * send: byte 1 is channel, rest zero
> * rcv: returns temp for channel in centi-degree celsius
> * in bytes 1 and 2
> - * returns 17 in byte 0 if no sensor is connected
> + * returns 0x11 in byte 0 if no sensor is connected
> */
> #define CTL_GET_VOLT 0x12 /*
> * send: byte 1 is rail number: 0 = 12v, 1 = 5v, 2 = 3.3v
> * rcv: returns millivolt in bytes 1,2
> + * returns error 0x10 if request is invalid
> */
> #define CTL_GET_FAN_CNCT 0x20 /*
> * returns in bytes 1-6 for each fan:
> @@ -52,6 +53,12 @@
> * send: byte 1 is channel, rest zero
> * rcv: returns rpm in bytes 1,2
> */
> +#define CTL_GET_FAN_PWM 0x22 /*
> + * send: byte 1 is channel, rest zero
> + * rcv: returns pwm in byte 1 if it was set
> + * returns error 0x12 if fan is controlled via
> + * fan_target or fan curve
> + */
> #define CTL_SET_FAN_FPWM 0x23 /*
> * set fixed pwm
> * send: byte 1 is fan number
> @@ -73,13 +80,31 @@ struct ccp_device {
> struct completion wait_input_report;
> struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
> u8 *buffer;
> - int pwm[6];
> int target[6];
> DECLARE_BITMAP(temp_cnct, NUM_TEMP_SENSORS);
> DECLARE_BITMAP(fan_cnct, NUM_FANS);
> char fan_label[6][LABEL_LENGTH];
> };
>
> +/* converts response error in buffer to errno */
> +static int ccp_get_errno(struct ccp_device *ccp)
> +{
> + switch (ccp->buffer[0]) {
> + case 0x00: /* success */
> + return 0;
> + case 0x01: /* called invalid command */
> + return -EOPNOTSUPP;
> + case 0x10: /* called GET_VOLT / GET_TMP with invalid arguments */
> + return -EINVAL;
> + case 0x11: /* requested temps of disconnected sensors */
> + case 0x12: /* requested pwm of not pwm controlled channels */
> + return -ENODATA;
> + default:
> + hid_dbg(ccp->hdev, "unknown device response error: %d", ccp->buffer[0]);
> + return -EIO;
> + }
> +}
> +
> /* 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)
> {
> @@ -102,13 +127,7 @@ static int send_usb_cmd(struct ccp_device *ccp, u8 command, u8 byte1, u8 byte2,
> if (!t)
> return -ETIMEDOUT;
>
> - /* first byte of response is error code */
> - if (ccp->buffer[0] != 0x00) {
> - hid_dbg(ccp->hdev, "device response error: %d", ccp->buffer[0]);
> - return -EIO;
> - }
> -
> - return 0;
> + return ccp_get_errno(ccp);
> }
>
> static int ccp_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *data, int size)
> @@ -126,7 +145,7 @@ static int ccp_raw_event(struct hid_device *hdev, struct hid_report *report, u8
> }
>
> /* requests and returns single data values depending on channel */
> -static int get_data(struct ccp_device *ccp, int command, int channel)
> +static int get_data(struct ccp_device *ccp, int command, int channel, bool two_byte_data)
> {
> int ret;
>
> @@ -136,7 +155,9 @@ static int get_data(struct ccp_device *ccp, int command, int channel)
> if (ret)
> goto out_unlock;
>
> - ret = (ccp->buffer[1] << 8) + ccp->buffer[2];
> + ret = ccp->buffer[1];
> + if (two_byte_data)
> + ret = (ret << 8) + ccp->buffer[2];
>
> out_unlock:
> mutex_unlock(&ccp->mutex);
> @@ -150,14 +171,14 @@ static int set_pwm(struct ccp_device *ccp, int channel, long val)
> if (val < 0 || val > 255)
> return -EINVAL;
>
> - ccp->pwm[channel] = val;
> -
> /* The Corsair Commander Pro uses values from 0-100 */
> val = DIV_ROUND_CLOSEST(val * 100, 255);
>
> mutex_lock(&ccp->mutex);
>
> ret = send_usb_cmd(ccp, CTL_SET_FAN_FPWM, channel, val, 0);
> + if (!ret)
> + ccp->target[channel] = -ENODATA;
>
> mutex_unlock(&ccp->mutex);
> return ret;
> @@ -171,7 +192,6 @@ static int set_target(struct ccp_device *ccp, int channel, long val)
> ccp->target[channel] = val;
>
> mutex_lock(&ccp->mutex);
> -
> ret = send_usb_cmd(ccp, CTL_SET_FAN_TARGET, channel, val >> 8, val);
>
> mutex_unlock(&ccp->mutex);
> @@ -210,7 +230,7 @@ static int ccp_read(struct device *dev, enum hwmon_sensor_types type,
> case hwmon_temp:
> switch (attr) {
> case hwmon_temp_input:
> - ret = get_data(ccp, CTL_GET_TMP, channel);
> + ret = get_data(ccp, CTL_GET_TMP, channel, true);
> if (ret < 0)
> return ret;
> *val = ret * 10;
> @@ -222,7 +242,7 @@ static int ccp_read(struct device *dev, enum hwmon_sensor_types type,
> case hwmon_fan:
> switch (attr) {
> case hwmon_fan_input:
> - ret = get_data(ccp, CTL_GET_FAN_RPM, channel);
> + ret = get_data(ccp, CTL_GET_FAN_RPM, channel, true);
> if (ret < 0)
> return ret;
> *val = ret;
> @@ -230,6 +250,8 @@ static int ccp_read(struct device *dev, enum hwmon_sensor_types type,
> case hwmon_fan_target:
> /* how to read target values from the device is unknown */
> /* driver returns last set value or 0 */
> + if (ccp->target[channel] < 0)
> + return -ENODATA;
> *val = ccp->target[channel];
> return 0;
> default:
> @@ -239,9 +261,10 @@ static int ccp_read(struct device *dev, enum hwmon_sensor_types type,
> case hwmon_pwm:
> switch (attr) {
> case hwmon_pwm_input:
> - /* how to read pwm values from the device is currently unknown */
> - /* driver returns last set value or 0 */
> - *val = ccp->pwm[channel];
> + ret = get_data(ccp, CTL_GET_FAN_PWM, channel, false);
> + if (ret < 0)
> + return ret;
> + *val = DIV_ROUND_CLOSEST(ret * 255, 100);
> return 0;
> default:
> break;
> @@ -250,7 +273,7 @@ static int ccp_read(struct device *dev, enum hwmon_sensor_types type,
> case hwmon_in:
> switch (attr) {
> case hwmon_in_input:
> - ret = get_data(ccp, CTL_GET_VOLT, channel);
> + ret = get_data(ccp, CTL_GET_VOLT, channel, true);
> if (ret < 0)
> return ret;
> *val = ret;
> @@ -416,6 +439,7 @@ static int get_fan_cnct(struct ccp_device *ccp)
> continue;
>
> set_bit(channel, ccp->fan_cnct);
> + ccp->target[channel] = -ENODATA;
>
> switch (mode) {
> case 1:
> --
> 2.27.0
