Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver

[Pengyu Luo <mitltlatltl@gmail.com>]

On Tue, Dec 31, 2024 at 1:00 PM Aiqun(Maria) Yu <quic_aiquny@quicinc.com> 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@quicinc.com> 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, &reg, 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