From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1486632753-17363-1-git-send-email-baoyou.xie@linaro.org> <1486632753-17363-3-git-send-email-baoyou.xie@linaro.org> <8322ae7c-a74d-c6a3-9cdb-4cd5584f37d4@linaro.org> From: Baoyou Xie Date: Thu, 16 Feb 2017 08:52:47 +0800 Message-ID: Subject: Re: [PATCH v6 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family Content-Type: multipart/alternative; boundary=001a1144ca224a31ed05489b37fe To: Andy Shevchenko Cc: Jun Nie , Shawn Guo , Wolfram Sang , Rob Herring , Mark Rutland , linux-arm Mailing List , linux-i2c@vger.kernel.org, devicetree , "linux-kernel@vger.kernel.org" , "xie.baoyou" , chen.chaokai@zte.com.cn, wang.qiang01@zte.com.cn List-ID: --001a1144ca224a31ed05489b37fe Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi, Andy: On 14 February 2017 at 17:29, Andy Shevchenko wrote: > On Tue, Feb 14, 2017 at 6:05 AM, Jun Nie wrote: > > On 2017=E5=B9=B402=E6=9C=8809=E6=97=A5 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 =3D (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 =3D 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 > --001a1144ca224a31ed05489b37fe Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi, Andy:

On 14 February 2017 at 17:29, Andy Shevchenko <andy.s= hevchenko@gmail.com> wrote:
On Tue, Feb 14, 2017 at 6:05 AM, Jun Nie <jun.nie@linaro.org> wrote:
> On 2017=E5=B9=B402=E6=9C=8809=E6=97=A5 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)
>> +{
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0u32 status;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct zx2967_i2c_info *zx_i2c =3D (st= ruct zx2967_i2c_info *)dev_id;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long flags;
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0spin_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<= br> 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 d= river is a bit special, Inward I use the lock to prevent irq runs in severa= l cpus in parallel.
In fact, it's redundant here. So I wi= ll resend a patch to remove it.

=C2=A0
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0status =3D zx2967_i2c_readl(zx_i2c, RE= G_STAT) & I2C_INT_MASK;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0zx2967_i2c_isr_clr(zx_i2c);
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (status & I2C_ERROR_MASK) {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0spin_unloc= k_irqrestore(&zx_i2c->lock, flags);
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return IRQ= _HANDLED;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (status & I2C_TRANS_DONE)
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0complete(&= amp;zx_i2c->complete);
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0spin_unlock_irqrestore(&zx_i2= c->lock, flags);
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0return IRQ_HANDLED;
>> +}

--
With Best Regards,
Andy Shevchenko

--001a1144ca224a31ed05489b37fe--