On 12/30/2024 6:44 PM, Pengyu Luo wrote:
> On Mon, Dec 30, 2024 at 5:04 PM Aiqun(Maria) Yu <quic_aiquny@xxxxxxxxxxx> wrote:
>> On 12/28/2024 1:13 AM, Pengyu Luo wrote:
[...]
>>> + i2c_transfer(client->adapter, msgs, 2);
>>
>> ARRAY_SIZE(msgs) is suggested instead of pure 2.
>>
>
> Agree
>
>>> + usleep_range(2000, 2500);
>>
>> Why is a sleep needed here? Is this information specified in any datasheet?
>>
>
> Have a break between 2 transaction. This sleep happens in acpi code, also
> inside a critical region. I rearranged it.
>
> Local7 = Acquire (\_SB.IC16.MUEC, 0x03E8)
> ...
> write ops
> ...
> Sleep (0x02)
> ...
> read ops
> ...
> Release (\_SB.IC16.MUEC)
Could you please share the exact code snippet that is being referenced?
I'm a bit confused because it doesn't seem to align with the current
logic, which doesn't have read operations within the same mutex lock. I
also want to understand the background and necessity of the sleep function.
>
>>> +
>>> + mutex_unlock(&ec->lock);
>>> +
>>> + return *resp;
>>> +}
>>> +
>>> +/* -------------------------------------------------------------------------- */
>>> +/* Common API */
[...]
>>> + int i, ret;
>>> + u8 _resp[RESP_HDR_SIZE + 1];
>>> + u8 req[REQ_HDR_SIZE + 1] = {0x02, EC_READ, 1, };
>>
>> Could it be made more readable by specifying the macro names for 0x02
>> and 1? This would help in understanding the meaning of these numbers.
>>
>
> I really don't know the meaning of master command 0x02, 1 is the size for
> the data_seq behind of it. There are many possible sizes. It is not a good
> idea to define a macro name for everyone.
>
Perhaps you didn't get the "arg..." magic here. A single definition is
sufficient for all sizes.
>> Also, please ensure the actual size of the request buffer is handled
>> properly. In gaokun_ec_request(), the req is passed down directly, and
>> the i2c_msg.len is used dynamically with req[INPUT_SIZE_OFFSET] +
>> REQ_HDR_SIZE. This requires the caller to carefully manage the contents
>> to avoid memory over-read, making the code difficult to read.
>>
>> Creating a defined macro can help you avoid manually defining the size.
>> For example:
>> #define REQ(size, data_0, data_1, args...) \
>> u8 req[REQ_HDR_SIZE + size] = {data_0, data_1, size, args};
>>
>
> I think wrapping like this is not recommended, see '5)' in [1]
>
> Best wishes,
> Pengyu
>
> [1] https://www.kernel.org/doc/html/v4.10/process/coding-style.html#macros-enums-and-rtl
I believe that the consideration of namespace collisions is a valid concern.
Some examples can be like have a naming pattern as well:
/*To have a name pattern to reflect the size like reg0/reg1/reg2*/
#define REQ(variable_name, size, data_0, data_1, args...) \
u8 ##variable_name[REQ_HDR_SIZE + size] = {data_0, data_1, size, args};
/*u8 req1[REQ_HDR_SIZE + 1] = {0x02, EC_READ, 1, };*/
REQ(req, 1, 0x02, EC_READ);
/*u8 req2[REQ_HDR_SIZE + 2] = {0x02, 0x68, 2, 3, 0x5a}; */
REQ(req, 2, 0x02, 0x68, 3, 0x5a);
Please note that this is just an example and a suggestion to avoid the
current manual variable pattern setting. The final decision still
requires the current maintainers' agreement.
--
Thx and BRs,
Aiqun(Maria) Yu
