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

[Andi Shyti]

Hi Kimriver,

On Mon, Sep 09, 2024 at 09:56:46AM GMT, Kimriver Liu wrote:
> It was observed issuing ABORT bit(IC_ENABLE[1]) will not work when
> IC_ENABLE is already disabled.
> 
> Check if ENABLE bit(IC_ENABLE[0]) is disabled when the master is
> holding SCL low. If ENABLE bit is disabled, the software need
> enable it before trying to issue ABORT bit. otherwise,
> the controller ignores any write to ABORT bit.
> 
> Signed-off-by: Kimriver Liu <kimriver.liu@xxxxxxxxxxxx>

You forgot:

Reviewed-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>

> ---
> V6->V7:
> 	1. add Subject versioning [PATCH v7]
> 	2. change fsleep(25) to usleep_range(25, 250)
> 	3. Add macro definition DW_iC_ENABLE_ENABLE to fix compile errors
> 	4. base: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=master

Thanks a lot for following up! :-)

> V5->V6: restore i2c_dw_is_master_idling() function checking
> V4->V5: delete master idling checking
> V3->V4:
> 	1. update commit messages and add patch version and changelog
> 	2. move print the error message in i2c_dw_xfer
> V2->V3: change (!enable) to (!(enable & DW_IC_ENABLE_ENABLE))
> V1->V2: used standard words in function names and addressed review comments
> 
> link to V1:
> https://lore.kernel.org/lkml/20240904064224.2394-1-kimriver.liu@xxxxxxxxxxxx/
> ---

...

> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -453,6 +453,18 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
>  
>  	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>  	if (abort_needed) {
> +		if (!(enable & DW_IC_ENABLE_ENABLE)) {
> +			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
> +			enable |= DW_IC_ENABLE_ENABLE;
> +			/*
> +			 * Need two ic_clk delay when enabling the I2C to ensure ENABLE bit
> +			 * is already set. Wait 10 times the signaling period of the highest
> +			 * I2C transfer supported by the driver(for 400KHz this is 25us)
> +			 * as described in the DesignWare I2C databook.
> +			 */
> +			usleep_range(25, 250);

I think there is a misunderstanding here. Andy asked you to use
flseep and improve the calculation: "Please, calculate this delay
based on the actual speed in use (or about to be in use)."[*]

Andy can you please clarify with Kimriver here?

Thanks,
Andi

[*] Message-ID: <6392ecd3f9934e9d8641b5f608ee6d60@xxxxxxxxxxxx>