Re: [RFC v2] hwmon: add Corsair Commander Pro driver

[Guenter Roeck <linux@roeck-us.net>]

On 6/15/20 7:54 AM, Marius Zachmann wrote:
[ ... ]
>>> +	memset(ccp->buffer, 0x00, OUT_BUFFER_SIZE);
>>> +	ccp->buffer[0] = CTL_SET_FAN_FPWM;
>>> +	ccp->buffer[1] = channel;
>>> +	ccp->buffer[2] = val;
>>
>> As it turns out (unless I am missing something), all commands have
>> one, two, or three parameters. Also, the full buffer is always
>> written. With that, it would be easier to add command, channel,
>> and value as parameters to send_usb_cmd() and handle both
>> memset() and assignments there.
>>
> 
> There is at least one command for setting a fan curve (0x25),
> which I intend to implement at a later point. It needs 27 bytes.
> There seem to be more long commands related to the RGB output.
> Also there is CTL_GET_FAN_CNCT which needs read 6 bytes of the
> response.
> 
In that case it might make sense to add a helper function to handle
_most_ of the commands. CTL_GET_FAN_CNCT can still be handled
with that helper function since the response is 6 bytes, not the
command itself.

>>> +	ret = send_usb_cmd(ccp);
>>> +
>>
>> This makes me wonder if reading a response is always necessary.
>> If not, it might make sense to add a flag to the function indicating
>> that no response is needed, and skip the read part there. If it is
>> needed, it might make sense to add a comment into the send function,
>> explaining that a read command must be executed even if no data is
>> returned.
>>
> 
> Actually yes. After a bit of further testing it seems, there is an
> error code in the first byte of the response, when sending invalid
> commands. I will check for it, return -EIO and log it, since I
> do not know, what all the error codes are.

Makes sense.

Thanks,
Guenter