Re: [PATCH v3 4/4] i2c: imx: prevent rescheduling in non dma mode

Hi Stefan,

One final ask...

On Mon, Sep 02, 2024 at 09:42:04AM GMT, Stefan Eichenberger wrote:

From: Stefan Eichenberger <redacted>

We are experiencing a problem with the i.MX I2C controller when
communicating with SMBus devices. We are seeing devices time-out because
the time between sending/receiving two bytes is too long, and the SMBus
device returns to the idle state. This happens because the i.MX I2C
controller sends and receives byte by byte. When a byte is sent or
received, we get an interrupt and can send or receive the next byte.

The current implementation sends a byte and then waits for an event
generated by the interrupt subroutine. After the event is received, the
next byte is sent and we wait again. This waiting allows the scheduler
to reschedule other tasks, with the disadvantage that we may not send
the next byte for a long time because the send task is not immediately
scheduled. For example, if the rescheduling takes more than 25ms, this
can cause SMBus devices to timeout and communication to fail.

This patch changes the behavior so that we do not reschedule the
send/receive task, but instead send or receive the next byte in the
interrupt subroutine. This prevents rescheduling and drastically reduces
the time between sending/receiving bytes. The cost in the interrupt
subroutine is relatively small, we check what state we are in and then
send/receive the next byte. Before we had to call wake_up, which is even
less expensive. However, we also had to do some scheduling, which
increased the overall cost compared to the new solution. The wake_up
function to wake up the send/receive task is now only called when an
error occurs or when the transfer is complete.

Signed-off-by: Stefan Eichenberger <redacted>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

The code is fine and looks good to me. The commit log is also
very descriptive. However, there isn’t a single line of comment
in the code.

If someone else encounters this code without understanding your
specific context, they might find it too complex and attempt to
simplify it or revert to the previous implementation.

Please add comments to describe the state machine you
implemented, the reasoning behind it (as you explained in the
commit log), and make it understandable for those who haven’t
reviewed your patches.

Thanks,
Andi