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
