Re: [PATCH v8] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

[Andy Shevchenko]

On Wed, Sep 11, 2024 at 01:37:43AM +0000, Liu Kimriver/刘金河 wrote:
> >From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 
> >Sent: 2024年9月10日 19:59
> >On Tue, Sep 10, 2024 at 11:43:34AM +0000, Liu Kimriver/刘金河 wrote:
> >> >-----Original Message-----
> >> >From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> >> >Sent: 2024年9月10日 18:45
> >> >On Tue, Sep 10, 2024 at 09:38:53AM +0000, Liu Kimriver/刘金河 wrote:
> >> >> >From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> >> >> >Sent: 2024年9月10日 17:03
> >> >> >at 02:13:09PM +0800, Kimriver Liu wrote:

...

> >> > >> +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev)
> >> >> 
> >> >> >Sorry if I made a mistake, but again, looking at the usage you 
> >> >> >have again negation here and there...
> >> > 
> >> >> >	i2c_dw_is_controller_active
> >> >> 
> >> >> > (note new terminology, dunno if it makes sense start using it in 
> >> >> > function names, as we have more of them following old style)
> >> >> 
> >> >>  Last week , You suggested that I used this
> >> >> i2c_dw_is_master_idling(dev)
> >> 
> >> >Yes, sorry about that. I did maybe not clearly get how it is going to look like.
> >> 
> >> >> >> +{
> >> >> >> +	u32 status;
> >> >> >> +
> >> >> >> +	regmap_read(dev->map, DW_IC_STATUS, &status);
> >> >> >> +	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> >> >> >> +		return true;
> >> >> 
> >> >> 		return false;
> >> >> 
> >> >> >> +	return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> >> >> >> +			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> >> >> >> +			1100, 20000);
> >> >> 
> >> >> >...and drop !.
> >> >> 
> >> >>  We reproduce this issue in RTL simulation(About(~1:500) in our soc). 
> >> >> It is necessary  to add waiting DW_IC_STATUS_MASTER_ACTIVITY idling 
> >> >> before disabling I2C when  I2C transfer completed.  as described in 
> >> >> the DesignWare  I2C databook(Flowchart for DW_apb_i2c Controller)
> >> 
> >> >Cool, but here I'm talking purely about inverting the logic (with renaming), nothing more.
> >> 
> >>  as described in the DesignWare I2C databook:
> >>  DW_IC_STATUS[5].MST_ACTIVITY Description as follows:
> >>  Controller FSM Activity Status. When the Controller Finite  State 
> >> Machine (FSM) is not in the IDLE state, this bit is set.
> >>  Note: IC_STATUS[0]-that is, ACTIVITY bit-is the OR of  SLV_ACTIVITY 
> >> and MST_ACTIVITY bits.
> >>  Values:
> >>  ■ 0x1 (ACTIVE): Controller not idle
> >>  ■ 0x0 (IDLE): Controller is idle
> >> 
> >> We need waiting DW_IC_STATUS.MST_ACTIVITY idling, If Controller not 
> >> idle, Wait for a while.
> >> Return value: 
> >>   false(0): Controller is idle
> >>   timeout(-110): Controller activity
> >> 
> >> Ok, change the function name i2c_dw_is_master_idling(dev) to 
> >> i2c_dw_is_controller_active(dev) it seems more reasonable
> 
>  Change above text as a comment:
> 
> /*
>  * This functions waits controller idling before disabling I2C
>  * When the controller is not in the IDLE state, 
>  * MST_ACTIVITY bit (IC_STATUS[5]) is set:
>  * 0x1 (ACTIVE): Controller not idle
>  * 0x0 (IDLE): Controller is idle
>  * The function is called after returning the end of the current transfer
>  * Returns:

>  * Return 0 as controller IDLE,
>  * Return a negative errno as controller ACTIVE

Why does it return a non-boolean value again?

>  */ 
> 
> >> static int i2c_dw_is_controller_active(struct dw_i2c_dev *dev) {
> >> 	u32 status;
> >> 
> >> 	regmap_read(dev->map, DW_IC_STATUS, &status);
> >> 	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> >> 		return 0;
> >> 
> >> 	return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> >> 			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> >> 			1100, 20000);
> >> }
> 
> >Yes, thank you. This is pure readability wise, you may actually leave the
> >above text as a comment on top of that helper. It will add a value of
> >understanding what's behind the scenes.
> 
> > >> >> +}

-- 
With Best Regards,
Andy Shevchenko