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

[Andi Shyti]

Hi Stefan,

On Wed, Sep 04, 2024 at 01:27:55PM GMT, Stefan Eichenberger wrote:
> On Tuesday 3 September 2024, Andi Shyti <andi.shyti@xxxxxxxxxx> wrote:
> 
>     Hi Stefan,
> 
>     One final ask...
> 
>     On Mon, Sep 02, 2024 at 09:42:04AM GMT, Stefan Eichenberger wrote:
>     > From: Stefan Eichenberger <stefan.eichenberger@xxxxxxxxxxx>
>     >
>     > 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 <stefan.eichenberger@xxxxxxxxxxx>
>     > Acked-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> 
>     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 a lot for the feedback. I only have limited access to my computer
> the next two-three weeks but will add comments as soon as I'm back. 

Other than the comment, there are also some checkpatch errors.
Please, do fix those errors, add the code and then I will take
your patches.

Sorry about that,
Andi