Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <0664b935-d201-419a-3f1d-3df4226a8db1@roeck-us.net>
Date:   Sat, 11 Feb 2023 10:08:27 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Content-Language: en-US
To:     Leonard Anderweit <leonard.anderweit@gmail.com>,
        linux-hwmon@vger.kernel.org
Cc:     Aleksa Savic <savicaleksa83@gmail.com>,
        Jack Doan <me@jackdoan.com>, Jean Delvare <jdelvare@suse.com>,
        Jonathan Corbet <corbet@lwn.net>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230211165923.17807-1-leonard.anderweit@gmail.com>
 <20230211165923.17807-4-leonard.anderweit@gmail.com>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH 3/5] hwmon: (aquacomputer_d5next) Add temperature offset
 control for Aquaero
In-Reply-To: <20230211165923.17807-4-leonard.anderweit@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/11/23 08:59, Leonard Anderweit wrote:
> Support sending control reports to Aquacomputer Aquaero. This commit only
> enables control of the temperature offset as this works in the same manner as
> with already supported devices.
> Also, mention temperature offset control for Aquaero in docs.
> 
> Signed-off-by: Leonard Anderweit <leonard.anderweit@gmail.com>
> ---
>   Documentation/hwmon/aquacomputer_d5next.rst |  4 +-
>   drivers/hwmon/aquacomputer_d5next.c         | 62 ++++++++++++++++-----
>   2 files changed, 50 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/hwmon/aquacomputer_d5next.rst b/Documentation/hwmon/aquacomputer_d5next.rst
> index 7d0d015b1a52..de43374940b8 100644
> --- a/Documentation/hwmon/aquacomputer_d5next.rst
> +++ b/Documentation/hwmon/aquacomputer_d5next.rst
> @@ -25,7 +25,7 @@ communicate through proprietary USB HID protocols.
>   
>   The Aquaero devices expose eight physical, eight virtual and four calculated
>   virtual temperature sensors, as well as two flow sensors. The fans expose their
> -speed (in RPM), power, voltage and current.
> +speed (in RPM), power, voltage and current. The temperature offset can be controlled.
>   
>   For the D5 Next pump, available sensors are pump and fan speed, power, voltage
>   and current, as well as coolant temperature and eight virtual temp sensors. Also
> @@ -75,7 +75,7 @@ Sysfs entries
>   
>   ================ ==============================================================
>   temp[1-20]_input Physical/virtual temperature sensors (in millidegrees Celsius)
> -temp[1-4]_offset Temperature sensor correction offset (in millidegrees Celsius)
> +temp[1-8]_offset Temperature sensor correction offset (in millidegrees Celsius)
>   fan[1-8]_input   Pump/fan speed (in RPM) / Flow speed (in dL/h)
>   fan5_pulses      Quadro flow sensor pulses
>   power[1-8]_input Pump/fan power (in micro Watts)
> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> index 03ef9e0258d2..95461e2907e1 100644
> --- a/drivers/hwmon/aquacomputer_d5next.c
> +++ b/drivers/hwmon/aquacomputer_d5next.c
> @@ -56,6 +56,7 @@ static const char *const aqc_device_names[] = {
>   #define SERIAL_PART_OFFSET		2
>   
>   #define CTRL_REPORT_ID			0x03
> +#define AQUAERO_CTRL_REPORT_ID		0x0b
>   
>   /* The HID report that the official software always sends
>    * after writing values, currently same for all devices
> @@ -67,6 +68,14 @@ static u8 secondary_ctrl_report[] = {
>   	0x02, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x34, 0xC6
>   };
>   
> +/* Secondary HID report values for Aquaero */
> +#define AQUAERO_SECONDARY_CTRL_REPORT_ID	0x06
> +#define AQUAERO_SECONDARY_CTRL_REPORT_SIZE	0x07
> +
> +static u8 aquaero_secondary_ctrl_report[] = {
> +	0x06, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00
> +};
> +
>   /* Report IDs for legacy devices */
>   #define POWERADJUST3_STATUS_REPORT_ID	0x03
>   
> @@ -94,6 +103,7 @@ static u8 secondary_ctrl_report[] = {
>   #define AQUAERO_NUM_VIRTUAL_SENSORS		8
>   #define AQUAERO_NUM_CALC_VIRTUAL_SENSORS	4
>   #define AQUAERO_NUM_FLOW_SENSORS		2
> +#define AQUAERO_CTRL_REPORT_SIZE		0xa93
>   
>   /* Sensor report offsets for Aquaero fan controllers */
>   #define AQUAERO_SENSOR_START			0x65
> @@ -106,6 +116,9 @@ static u8 secondary_ctrl_report[] = {
>   #define AQUAERO_FAN_SPEED_OFFSET		0x00
>   static u16 aquaero_sensor_fan_offsets[] = { 0x167, 0x173, 0x17f, 0x18B };
>   
> +/* Control report offsets for the Aquaero fan controllers */
> +#define AQUAERO_TEMP_CTRL_OFFSET	0xdb
> +
>   /* Specs of the D5 Next pump */
>   #define D5NEXT_NUM_FANS			2
>   #define D5NEXT_NUM_SENSORS		1
> @@ -441,6 +454,10 @@ struct aqc_data {
>   	const char *name;
>   
>   	int status_report_id;	/* Used for legacy devices, report is stored in buffer */
> +	int ctrl_report_id;
> +	int secondary_ctrl_report_id;
> +	int secondary_ctrl_report_size;
> +	u8 *secondary_ctrl_report;
>   
>   	int buffer_size;
>   	u8 *buffer;
> @@ -513,7 +530,7 @@ static int aqc_get_ctrl_data(struct aqc_data *priv)
>   	int ret;
>   
>   	memset(priv->buffer, 0x00, priv->buffer_size);
> -	ret = hid_hw_raw_request(priv->hdev, CTRL_REPORT_ID, priv->buffer, priv->buffer_size,
> +	ret = hid_hw_raw_request(priv->hdev, priv->ctrl_report_id, priv->buffer, priv->buffer_size,
>   				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>   	if (ret < 0)
>   		ret = -ENODATA;
> @@ -527,23 +544,27 @@ static int aqc_send_ctrl_data(struct aqc_data *priv)
>   	int ret;
>   	u16 checksum;
>   
> -	/* Init and xorout value for CRC-16/USB is 0xffff */
> -	checksum = crc16(0xffff, priv->buffer + priv->checksum_start, priv->checksum_length);
> -	checksum ^= 0xffff;
> +	/* Checksum is not needed for Aquaero */
> +	if (priv->kind != aquaero) {
> +		/* Init and xorout value for CRC-16/USB is 0xffff */
> +		checksum = crc16(0xffff, priv->buffer + priv->checksum_start,
> +				 priv->checksum_length);
> +		checksum ^= 0xffff;

aquaero is already supported, and the checksum is so far generated
and sent. Is it ignored ? Also, is it guaranteed that _all_ aquero devices
don't need it ?

If it is not needed and ignored, does it really add value to selectively drop it ?

Either case, this change is not mentioned in the commit log, and it
violates the "one logical change per patch" rule. Please split it into
a separate patch and explain why the change is needed.

Another change to separate is the introduction of ctrl_report_id
and the secondary_ctrl_report variables, which is also done silently
and not explained. That should also be a separate patch to simplify
review.

Thanks,
Guenter

>   
> -	/* Place the new checksum at the end of the report */
> -	put_unaligned_be16(checksum, priv->buffer + priv->checksum_offset);
> +		/* Place the new checksum at the end of the report */
> +		put_unaligned_be16(checksum, priv->buffer + priv->checksum_offset);
> +	}
>   
>   	/* Send the patched up report back to the device */
> -	ret = hid_hw_raw_request(priv->hdev, CTRL_REPORT_ID, priv->buffer, priv->buffer_size,
> +	ret = hid_hw_raw_request(priv->hdev, priv->ctrl_report_id, priv->buffer, priv->buffer_size,
>   				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
>   	if (ret < 0)
>   		return ret;
>   
>   	/* The official software sends this report after every change, so do it here as well */
> -	ret = hid_hw_raw_request(priv->hdev, SECONDARY_CTRL_REPORT_ID, secondary_ctrl_report,
> -				 SECONDARY_CTRL_REPORT_SIZE, HID_FEATURE_REPORT,
> -				 HID_REQ_SET_REPORT);
> +	ret = hid_hw_raw_request(priv->hdev, priv->secondary_ctrl_report_id,
> +				 priv->secondary_ctrl_report, priv->secondary_ctrl_report_size,
> +				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
>   	return ret;
>   }
>   
> @@ -969,10 +990,10 @@ static const struct hwmon_channel_info *aqc_info[] = {
>   			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
>   			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
>   			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> -			   HWMON_T_INPUT | HWMON_T_LABEL,
> -			   HWMON_T_INPUT | HWMON_T_LABEL,
> -			   HWMON_T_INPUT | HWMON_T_LABEL,
> -			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
>   			   HWMON_T_INPUT | HWMON_T_LABEL,
>   			   HWMON_T_INPUT | HWMON_T_LABEL,
>   			   HWMON_T_INPUT | HWMON_T_LABEL,
> @@ -1275,6 +1296,9 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   		priv->num_flow_sensors = AQUAERO_NUM_FLOW_SENSORS;
>   		priv->flow_sensors_start_offset = AQUAERO_FLOW_SENSORS_START;
>   
> +		priv->buffer_size = AQUAERO_CTRL_REPORT_SIZE;
> +		priv->temp_ctrl_offset = AQUAERO_TEMP_CTRL_OFFSET;
> +
>   		priv->temp_label = label_temp_sensors;
>   		priv->virtual_temp_label = label_virtual_temp_sensors;
>   		priv->calc_virt_temp_label = label_aquaero_calc_temp_sensors;
> @@ -1438,6 +1462,11 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   		priv->firmware_version_offset = AQUAERO_FIRMWARE_VERSION;
>   
>   		priv->fan_structure = &aqc_aquaero_fan_structure;
> +
> +		priv->ctrl_report_id = AQUAERO_CTRL_REPORT_ID;
> +		priv->secondary_ctrl_report_id = AQUAERO_SECONDARY_CTRL_REPORT_ID;
> +		priv->secondary_ctrl_report_size = AQUAERO_SECONDARY_CTRL_REPORT_SIZE;
> +		priv->secondary_ctrl_report = aquaero_secondary_ctrl_report;
>   		break;
>   	case poweradjust3:
>   		priv->status_report_id = POWERADJUST3_STATUS_REPORT_ID;
> @@ -1446,6 +1475,11 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   		priv->serial_number_start_offset = AQC_SERIAL_START;
>   		priv->firmware_version_offset = AQC_FIRMWARE_VERSION;
>   
> +		priv->ctrl_report_id = CTRL_REPORT_ID;
> +		priv->secondary_ctrl_report_id = SECONDARY_CTRL_REPORT_ID;
> +		priv->secondary_ctrl_report_size = SECONDARY_CTRL_REPORT_SIZE;
> +		priv->secondary_ctrl_report = secondary_ctrl_report;
> +
>   		if (priv->kind == aquastreamult)
>   			priv->fan_structure = &aqc_aquastreamult_fan_structure;
>   		else