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

[Pengyu Luo <mitltlatltl@gmail.com>]

On Fri, Jan 3, 2025 at 1:38 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Tue, Dec 31, 2024 at 03:44:36PM +0800, Pengyu Luo wrote:
> > 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.
> > >
> >
> > I mentioned I rearranged it to optimize it. In a EC transaction,
> > write sleep read => write read sleep, in this way, we sleep once a
> > transaction.
>
> Sleeping between write and read is logical: it provides EC some time to
> respond. Sleeping after read is complete doesn't seem to have any
> reason.
>

OK, if you are interested, I explain this in details

First, EC transaction in acpi on this device is doing like
======== this transaction =========
lock
...
write
...
sleep
...
read
...
release
======== this transaction =========

When there are intensive transactions, another sleep is added in

======== this transaction =========
...
======== this transaction =========
...
sleep
...
======== next transaction =========
...
======== next transaction =========

Can we eliminate this? I am not sure, I have not tested it.

Generally, the code in acpi is terrible, it can just do the jobs, so I did
some changes and tested.

The process(reading after writing) and data structure(cmd, count, data...)
are very similar to I2C_FUNC_SMBUS_BLOCK_PROC_CALL(see [1]), see also ACPI
Specification 13.3.7. (It like this in acpi, BUFF = VREG = BUFF)

So I tried to send two messages in one shot without a break. Why not using
a smbus API? Qualcomm I2C driver in kernel does not support it
(Fall back to i2c_smbus_xfer_emulated).

Why not using a I2C Block Read/Write API?
One transaction with this api would send 3 messages, and return the wrong
status in return buffer.

Write:
i2c_smbus_write_i2c_block_data(mcmd, ilen+2, {scmd, ilen, ibuf})

i2c_msg = {
	.len = ilen + 3,
	.buf = {mcmd, scmd, ilen, ibuf}
}

Read:
i2c_smbus_read_i2c_block_data(mcmd, olen)

i2c_msg[0] = {
	.len = 1,
	.buf = {mcmd},
};
i2c_msg[1] = {
	.flags = I2C_M_RD,
	.len = olen,
	.buf = {}, /* the first byte return is wrong */
};

Best wishes,
Pengyu