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

[Andy Shevchenko]

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
> 
> 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 false;
> 
> 	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.

> >> >> +}

...

> I will be off work, If there are still emails that I have not been replied
> to, I will reply to your email immediately after going to work tomorrow.

No problem. Just keep your time, proof-read and test the v9 before sending and
I believe it will be the last iteration. Thank you for your patience and energy
to push this change forward!

...

> Thanks you for your suggestion!

You are welcome!

-- 
With Best Regards,
Andy Shevchenko