lkml.org 
[lkml]   [2020]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC v2] hwmon: add Corsair Commander Pro driver
From
Date
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

\
 
 \ /
  Last update: 2020-06-15 20:06    [W:0.060 / U:0.708 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site