RE: [PATCH] drivers:i2c:add controller reset when the timeout occurred

[Tommy Huang]

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