Re: [PATCH v2] i2c: add support for Zhaoxin I2C controller

[HansHu-oc <HansHu-oc@zhaoxin.com>]

Hi Andi,

Thank you very much for reviewing this. I'll take most of your
proposed changes and double check patch with checkpatch.pl.

For the following two proposed changes, please see if my
consideration is reasonable.

On 2023/5/31 19:00, Andi Shyti wrote:
> [...]
>
>> +static irqreturn_t zxi2c_irq_handle(int irq, void *dev_id)
>> +{
>> +	struct zxi2c *i2c = (struct zxi2c *)dev_id;
>> +	void __iomem *regs = i2c->regs;
>> +	u8 status = get_irq_status(regs);
>> +
>> +	if ((status & IRQ_STS_MASK) == 0)
>> +		return IRQ_NONE;
>
> unlikely?

This irq is shared, so it is possible.

>> +	if (status & IRQ_SCL_TIMEOUT)
>> +		dev_warn(i2c->dev, "timeout(HW), ID: 0x%X\n", i2c->addr);
>> +
>> +	if (status & IRQ_STS_ADDRNACK) {
>> +		dev_dbg(i2c->dev, "addr NACK, ID: 0x%X\n", i2c->addr);
>> +	} else if (status & IRQ_STS_BYTEEND) {
>> +		i2c->byte_left--;
>> +		if (!i2c->is_read) {
>> +			if (is_nack(regs)) {
>> +				status = IRQ_STS_BYTENACK;
>> +				i2c->byte_left++;
>> +				dev_err(i2c->dev, "data NACK, ID: 0x%X\n",
>> +					i2c->addr);
>> +			} else if (i2c->byte_left == 0 && i2c->is_last_msg) {
>> +				stop_write_byte(regs);
>> +			}
>> +		}
>> +	}
>> +
>> +	i2c->event = status;
>> +	clear_irq_status(regs);
>> +	wake_up(&i2c->waitq);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int zxi2c_wait_event(struct zxi2c *i2c, u8 event)
>> +{
>> +	int timeout;
>> +
>> +	timeout = wait_event_interruptible_timeout(i2c->waitq,
>> +			i2c->event != 0,
>> +			msecs_to_jiffies(ZXI2C_TIMEOUT));
>> +
>> +	if (i2c->event & event)
>> +		return 0;
>
> is this valid even when "timeout == 0"?

Let's see the description of the value returned by wait_event_interruptible_timeout():
https://elixir.bootlin.com/linux/v6.3.5/source/include/linux/wait.h#L525
 * Returns:
 * 0 if the @condition evaluated to %false after the @timeout elapsed,
 * 1 if the @condition evaluated to %true after the @timeout elapsed,
 * the remaining jiffies (at least 1) if the @condition evaluated
 * to %true before the @timeout elapsed, or -%ERESTARTSYS if it was
 * interrupted by a signal.
So, "timeout == 0" and @condition evaluated to %true unlikely to happen at the same time.

Or is it OK to change it to like below?
+    timeout = wait_event_interruptible_timeout(i2c->waitq,
+            i2c->event != 0,
+            msecs_to_jiffies(ZXI2C_TIMEOUT));
+
+    if (i2c->event & event) {
+        if (timeout == 1)
+            dev_warn(i2c->dev, "thread may be blocked\n");
+        return 0;
+    }
+
+    if (timeout == 0) {

Hans