Re: [PATCH 2/2] i2c: designware: Implement atomic transfer suppot

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Aug 20, 2025 at 11:31:25PM +0800, Jisheng Zhang wrote:
> Rework the read and write code paths in the driver to support operation
> in atomic contexts. To achieve this, the driver must not rely on IRQs
> or perform any scheduling, e.g., via a sleep or schedule routine.
> 
> Implement atomic, sleep-free, and IRQ-less operation. This increases
> complexity but is necessary for atomic I2C transfers required by some
> hardware configurations, e.g., to trigger reboots on an external PMIC chip.

...

> -		usleep_range(25, 250);
> +		if (dev->atomic)
> +			udelay(25);
> +		else
> +			usleep_range(25, 250);

Wondering why this delay is not being properly calculated. Why in atomic case
is okay to use the shortest one?

...

> -	if (!dev->acquire_lock)
> +	if (dev->atomic || !dev->acquire_lock)

I think basically we should no allow atomic transfers at all when the lock is
in use. Otherwise it will be interesting case if HW (FW) wants to have an
exclusive access while OS wants to perform an atomic transfer.

...

> +	if (dev->atomic)
> +		return regmap_read_poll_timeout_atomic(dev->map, DW_IC_STATUS, status,
> +						       !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +						       1100, 20000) != 0;
> +	else
> +		return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +					       !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +					       1100, 20000) != 0;

Please, drop ' != 0' parts at the same time, they are redundant.

...

> +static int i2c_dw_wait_transfer_atomic(struct dw_i2c_dev *dev)
> +{
> +	ktime_t timeout = ktime_add_us(ktime_get(), jiffies_to_usecs(dev->adapter.timeout));
> +	unsigned int stat;
> +	int ret;
> +
> +	do {
> +		ret = try_wait_for_completion(&dev->cmd_complete);
> +		if (ret)
> +			break;
> +
> +		stat = i2c_dw_read_clear_intrbits(dev);
> +		if (stat)
> +			i2c_dw_process_transfer(dev, stat);
> +		else
> +			udelay(15);

No explanation about this value.

> +	} while (ktime_compare(ktime_get(), timeout) < 0);

Whe have _before() and _after() APIs, use them.

> +	return ret ? 0 : -ETIMEDOUT;
> +}

...

>  	switch (dev->flags & MODEL_MASK) {
>  	case MODEL_AMD_NAVI_GPU:
> +		if (dev->atomic) {
> +			ret = -EOPNOTSUPP;
> +			goto done_nolock;
> +		}

Why only AMD case?

>  		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
>  		goto done_nolock;
>  	default:
>  		break;
>  	}

-- 
With Best Regards,
Andy Shevchenko