On Tue, Dec 31, 2024 at 1:00 PM Aiqun(Maria) Yu <quic_aiquny@xxxxxxxxxxx> wrote:
> 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.
>
I am gonna do this, Aiqun(Maria), Bryan, any suggestions?
/*
* for tx, command sequences are arranged as
* {master_cmd, slave_cmd, data_len, data_seq}
*/
#define REQ_HDR_SIZE 3
#define INPUT_SIZE_OFFSET 2
/*
* for rx, data sequences are arranged as
* {status, data_len(unreliable), data_seq}
*/
#define RESP_HDR_SIZE 2
#define MKREQ(REG0, REG1, SIZE, ...) \
{ \
/* ## will remove comma when no __VA_ARGS__ */ \
REG0, REG1, SIZE, ## __VA_ARGS__, \
/* make sure len(pkt[3:]) >= SIZE */ \
[3 + SIZE] = 0, \
}
#define MKRESP(SIZE) \
{ \
[RESP_HDR_SIZE + SIZE - 1] = 0, \
}
static inline void refill_req(u8 *dest, const u8 *src, size_t size)
{
int i;
for (i = 0; i < size; ++i)
dest[REQ_HDR_SIZE + i] = src[i];
}
static inline void extr_resp(u8 *dest, const u8 *src, size_t size)
{
int i;
for (i = 0; i < size; ++i)
dest[i] = src[RESP_HDR_SIZE + i];
}
[...]
example:
int gaokun_ec_psy_multi_read(struct gaokun_ec *ec, u8 reg,
size_t resp_len, u8 *resp)
{
int i, ret;
u8 _resp[] = MKRESP(1);
u8 req[] = MKREQ(0x02, EC_READ, 1);
for (i = 0; i < resp_len; ++i, ++reg) {
refill_req(req, ®, 1);
ret = gaokun_ec_read(ec, req, sizeof(_resp), _resp);
if (ret)
return ret;
extr_resp(&resp[i], _resp, 1);
}
return 0;
}
Best wishes,
Pengyu
