Re: [PATCH v2] usb: typec: ucsi_ccg: Add retry mechanism in ccg_read

[Jon Hunter <jonathanh@nvidia.com>]

On 16/09/2026 07:18, Wayne Chang wrote:
> CCG4 firmware occasionally fails to respond to I2C read requests,
> especially when reading cci and intr_reg from the ISR path. In these
> cases, i2c_transfer() returns -EREMOTEIO.
> 
> The CCG4 is EOL, and Infineon no longer supports firmware updates, so
> a software workaround is necessary.
> 
> Attempt the read up to CCG_READ_MAX_RETRIES (4) times in total when
> -EREMOTEIO is returned, sleeping ~1 ms via fsleep() between attempts.
> This allows recovery from transient I2C failures without affecting
> other error paths.
> 
> The sleep is safe because ccg_read() is called both from process
> context and from ccg_irq_handler(), which runs in threaded IRQ
> context (registered via request_threaded_irq() with a NULL hard-IRQ
> handler), so sleeping is allowed in both cases.
> 
> Fixes: 247c554a14aa ("usb: typec: ucsi: add support for Cypress CCGx")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wayne Chang <waynec@nvidia.com>
> ---
> v1 -> v2: Per Heikki's review:
> - Fix off-by-one: CCG_READ_MAX_RETRIES was 3 but the loop ran 4 times
>    (retry_count <= CCG_READ_MAX_RETRIES). Changed the macro to 4 (the
>    actual total attempt count) and the loop condition to
>    retry_count < CCG_READ_MAX_RETRIES, so the name and the loop bound
>    now agree. Adjusted the "skip the sleep on the last attempt" check
>    to retry_count == CCG_READ_MAX_RETRIES - 1 to match.
> - Use fsleep(1000) instead of usleep_range(1000, 2000).
> - Reworded the commit message to say "up to CCG_READ_MAX_RETRIES (4)
>    times in total" instead of the ambiguous "up to three times (four
>    attempts total)" that prompted the off-by-one question.
> 
>   drivers/usb/typec/ucsi/ucsi_ccg.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 91c2958a708c..4b6c53d2a998 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -135,6 +135,8 @@ struct version_format {
>   #define NVIDIA_FTB_DP_OFFSET	(2)
>   #define NVIDIA_FTB_DBG_OFFSET	(3)
>   
> +#define CCG_READ_MAX_RETRIES	4
> +
>   struct version_info {
>   	struct version_format base;
>   	struct version_format app;
> @@ -255,6 +257,7 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
>   	};
>   	u32 rlen, rem_len = len, max_read_len = len;
>   	int status;
> +	int retry_count;
>   
>   	/* check any max_read_len limitation on i2c adapter */
>   	if (quirks && quirks->max_read_len)
> @@ -266,7 +269,17 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
>   		rlen = min_t(u16, rem_len, max_read_len);
>   		msgs[1].len = rlen;
>   		put_unaligned_le16(rab, buf);
> -		status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +
> +		for (retry_count = 0; retry_count < CCG_READ_MAX_RETRIES; retry_count++) {
> +			status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +
> +			/* Only retry on -EREMOTEIO, and not after the last attempt */
> +			if (status != -EREMOTEIO || retry_count == CCG_READ_MAX_RETRIES - 1)
> +				break;
> +
> +			fsleep(1000);
> +		}
> +
>   		if (status < 0) {
>   			dev_err(uc->dev, "i2c_transfer failed %d\n", status);
>   			pm_runtime_put_sync(uc->dev);

I was wondering if we could avoid the extra check inside the loop for
'CCG_READ_MAX_RETRIES - 1' but this does avoid an addition sleep. So ...

Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks!
Jon

-- 
nvpublic