Hi, Andy:

On 14 February 2017 at 17:29, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

On Tue, Feb 14, 2017 at 6:05 AM, Jun Nie <jun.nie@linaro.org> wrote:
> On 2017年02月09日 17:32, Baoyou Xie wrote:
>>

First of all, please, Jun, remove tons of lines of over quoting when
answering to emails with one line question. Respect others time.

See below.

>> +static irqreturn_t zx2967_i2c_isr(int irq, void *dev_id)
>> +{
>> + u32 status;
>> + struct zx2967_i2c_info *zx_i2c = (struct zx2967_i2c_info *)dev_id;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&zx_i2c->lock, flags);
>
>
> Is this spin lock really necessary?

If you protect IO and one CPU is in interrupt while another in some
other function you need to have a spin lock.

I reviewed the code, we often need spin-lock to protect IO and the CPU in interrupt, but this driver is a bit special, Inward I use the lock to prevent irq runs in several cpus in parallel.

In fact, it's redundant here. So I will resend a patch to remove it.

>> +
>> + status = zx2967_i2c_readl(zx_i2c, REG_STAT) & I2C_INT_MASK;
>> + zx2967_i2c_isr_clr(zx_i2c);
>> +
>> + if (status & I2C_ERROR_MASK) {
>> + spin_unlock_irqrestore(&zx_i2c->lock, flags);
>> + return IRQ_HANDLED;
>> + }
>> +
>> + if (status & I2C_TRANS_DONE)
>> + complete(&zx_i2c->complete);
>> +
>> + spin_unlock_irqrestore(&zx_i2c->lock, flags);
>> +
>> + return IRQ_HANDLED;
>> +}

--
With Best Regards,
Andy Shevchenko