Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Sun, 12 Mar 2023 10:44:41 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Leonard Anderweit <leonard.anderweit@gmail.com>
Cc:     linux-hwmon@vger.kernel.org,
        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
Subject: Re: [PATCH v2 4/6] hwmon: (aquacomputer_d5next) Add infrastructure
 for Aquaero control reports
Message-ID: <210eddaf-cbd1-473e-920d-1c76c2923ee3@roeck-us.net>
References: <20230214220221.15003-1-leonard.anderweit@gmail.com>
 <20230214220221.15003-5-leonard.anderweit@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230214220221.15003-5-leonard.anderweit@gmail.com>
Precedence: bulk

On Tue, Feb 14, 2023 at 11:02:19PM +0100, Leonard Anderweit wrote:
> Add information on the Aquacomputer Aquaero control report and disable the
> control report checksum for Aquaero. The Aquaero does not use the checksum so
> it must be disabled to avoid overwriting the last two bytes of the control
> report.
> 
> Signed-off-by: Leonard Anderweit <leonard.anderweit@gmail.com>

Applied. And again:

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#85:
control report checksum for Aquaero. The Aquaero does not use the checksum so

Please keep in mind that you are causing extra work for me.

Guenter

> ---
>  drivers/hwmon/aquacomputer_d5next.c | 31 ++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> index 535d2fc0e55c..eb185318098a 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
> @@ -531,12 +541,16 @@ 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;
>  
> -	/* 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, priv->ctrl_report_id, priv->buffer, priv->buffer_size,
> @@ -1280,6 +1294,8 @@ 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_label = label_temp_sensors;
>  		priv->virtual_temp_label = label_virtual_temp_sensors;
>  		priv->calc_virt_temp_label = label_aquaero_calc_temp_sensors;
> @@ -1443,6 +1459,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;