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
