Hi Andi,
Thanks for your review.
I will fix the patch with your comment and resend it.
BR,
by Tommy
> -----Original Message-----
> From: Andi Shyti <andi.shyti@xxxxxxxxxx>
> Sent: Sunday, September 3, 2023 10:15 PM
> To: Tommy Huang <tommy_huang@xxxxxxxxxxxxxx>
> Cc: brendan.higgins@xxxxxxxxx; p.zabel@xxxxxxxxxxxxxx;
> linux-i2c@xxxxxxxxxxxxxxx; openbmc@xxxxxxxxxxxxxxxx;
> benh@xxxxxxxxxxxxxxxxxxx; joel@xxxxxxxxx; andrew@xxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-aspeed@xxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; BMC-SW <BMC-SW@xxxxxxxxxxxxxx>; Jae
> Hyun Yoo <jae.hyun.yoo@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH] drivers:i2c:add controller reset when the timeout
> occurred
>
> Hi Tommy,
>
> Please fix the title of the patch from:
>
> drivers:i2c:add controller reset when the timeout occurred
>
> to something like:
>
> i2c: aspeed: Reset the controller when timeout occurs
>
> Note:
>
> - leave a space after the ':'
> - start with a capital letter after the last ':'
>
> On Mon, Aug 14, 2023 at 07:15:34PM +0800, Tommy Huang wrote:
> > 1.Call i2c controller reset when the i2c transfer timeout occurred.
> > The rest of interrupts and device should be reset avoid unperdicted
> > controller behavior occurred.
>
> Please remove the '1.' and please rewrite this sentence in order to be
> grammatically correct, something like:
>
> "Call the i2c controller reset when an i2c transfer timeout occurs. The
> remaining interrupts and the device should be reset to avoid unpredictable
> controller behavior."
>
> > Signed-off-by: Tommy Huang <tommy_huang@xxxxxxxxxxxxxx>
>
> Is this a fix? If so please add:
>
> Fixes: 2e57b7cebb98 ("i2c: aspeed: Add multi-master use case support")
> Cc: Jae Hyun Yoo <jae.hyun.yoo@xxxxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v5.1+
>
> Jae Hyun is the author of the line you are changing therefore he needs to be
> Cc'ed
>
> [...]
>
> > /*
> > * If timed out and bus is still busy in a multi master
> > - * environment, attempt recovery at here.
> > + * environment, attempt recovery at here. Even the bus is
> > + * idle, we still need reset i2c controller avoid rest of
> > + * interrupts.
>
> Please fix the grammar here, as well
>
> In a multi-master setup, if a timeout occurs, attempt
> recovery. But if the bus is idle, we still need to reset the
> i2c controller to clear the remaining interrupts.
>
> We take this chance to improve the previous comment, as well.
>
> > */
> > if (bus->multi_master &&
> > (readl(bus->base + ASPEED_I2C_CMD_REG) &
> > ASPEED_I2CD_BUS_BUSY_STS))
> > aspeed_i2c_recover_bus(bus);
> > + else
> > + aspeed_i2c_reset(bus);
>
> I'd like also someone from Jae Hyun, Brendan, Benjamin or Joel to take a look
> here, as well. Thanks!
>
> Andi
