Received: by 2002:a05:6a10:22f:0:0:0:0 with SMTP id 15csp655553pxk; Fri, 11 Sep 2020 17:51:37 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwHKHIrk5gZBdjdFoF0ROECoLwibPIV2upADEvIDSkPBfnb6jrl7zgjkBpveBNMhEnAcD2D X-Received: by 2002:a17:906:b10:: with SMTP id u16mr4291727ejg.179.1599871897455; Fri, 11 Sep 2020 17:51:37 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1599871897; cv=none; d=google.com; s=arc-20160816; b=W7VC7xUHf28Hnf58wCVl8zHy/AJcw71SoOBJjULW7xAJGABG0QU+D042NMXLSyzU79 uLtLQGJjM8xQtiSJwC2jJym94dj2JS23qtJjC2S05KvujOLwqOZVSgyIEtEwcpN1hKcb dPBJmvlk2O3p7+Z0BJwSlyVZSQhwXJmY5HkWABUm+soHc2QnBAyMvAb7EHynjcoWxZFV pe5AX1EYe8JjrmYqTLwgI1XL9QBadsZF4Hm3wsMg7Go4pmyk63wSRJ1L1rAt2sMzmv2K d0nL+OCuz9+ZNBacNpFDJnucaiK1DmZs6w9gFEjEjDjEkGkslIIxQh8fL99qUuvKlt4U IOtg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:ironport-sdr:ironport-sdr; bh=F0bNfpNwIII/uKQLas9SzDPE84MqeM65kHscmH6knIQ=; b=T8bi+Rny6vdjq9zbDq9RGiDWYPx8zwtosDjISgC/Qc4i2yOzqcTQnDDUFWJErm23B9 u/e4cbMoUVdGSfDZUme7gz06ADdVk+yMm0xRC9MekIaCLeGx81WrzTUvz9zdOKcZyW8/ XEauX5A4FennVNXxcZAd5S5QBivXHZgbktBaoG8RvaHuMtWh2a8LB/e8FurZItTrjRy8 iuiC5QlFaeDdR0x03aHje7vhB3t+3HtN4oKZ0hgJSKjS430Kz+5k5QZ6mgdWxAEwQxgf /KikzjJ1il2vBiLj6OuNNnRT0bZ1DfUIzDta76XHAwlcBPaJbJHrYXW8L1xdnS6OWxMu p77w== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id yw17si2369630ejb.394.2020.09.11.17.51.15; Fri, 11 Sep 2020 17:51:37 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725979AbgILAuk (ORCPT + 99 others); Fri, 11 Sep 2020 20:50:40 -0400 Received: from esa1.mentor.iphmx.com ([68.232.129.153]:52418 "EHLO esa1.mentor.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725857AbgILAuh (ORCPT ); Fri, 11 Sep 2020 20:50:37 -0400 IronPort-SDR: S7HKLIEgVQCnVBwByHnjbWJSIAn4vkbuUc8GmSB/lCpxPOpJaOdY0h7UZetQNlwdyH0BqNhSJf gRBHCWXeyhe3R76LAbWIIutPaRoUctShu+lJ1RWDRbgjpSLKkdZqxEf00/FvKX7wnqXREu4gCg +5a83F09TJw72HJweJatqdho7MKLoce5gIZGBBlg+LMdeCHyrvy1L9nmcIORczC5BoPJ//loA5 ojexcn7KFtPNA08RmfvfAVd9VKk+gq72eruUm25zSMFsEYnXXAPnyLgoFWuGhk4fY7tCX6W1mI lS0= X-IronPort-AV: E=Sophos;i="5.76,418,1592899200"; d="scan'208";a="55030244" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa1.mentor.iphmx.com with ESMTP; 11 Sep 2020 16:50:36 -0800 IronPort-SDR: KLfswWze6mM3NcT/VAwlPzjKpTgBvcxDKrC3RMH+3ebPQz3g3B0AlcamgH8b/6BerHVi/xzSjk TmAzUIJVC3PbKKMaoKZpxoCsuKRgjZOXp30IWFiDNmRzQFvvRqQST5rmUEpSy5j4kXNBXrLZwv YSdapYxkMPFN4WYbsh+WiiNDpgIrCvjyCSrTlRpPxwc892DdHuacZsC7vOlvjcrl6+BzrUZp4S yzhq745oT69N7nSOGpSuhm/aMHm0w8y01mJjqLkP5VSR/2w/9VDFUL7CBsV4RkQPxFEZOOQmgS PsU= Subject: Re: [PATCH v2 1/1] Input: atmel_mxt_ts - implement I2C retries To: Andy Shevchenko CC: "nick@shmanahar.org" , "dmitry.torokhov@gmail.com" , "linux-input@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "Andrew_Gabbasov@mentor.com" , "erosca@de.adit-jv.com" , "digetx@gmail.com" References: <20200903155904.17454-1-jiada_wang@mentor.com> From: "Wang, Jiada" Message-ID: <7c57ae66-41ad-aba3-a466-6c6f79652638@mentor.com> Date: Sat, 12 Sep 2020 09:50:22 +0900 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-ClientProxiedBy: svr-orw-mbx-02.mgc.mentorg.com (147.34.90.202) To svr-orw-mbx-01.mgc.mentorg.com (147.34.90.201) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Andy Thanks for your comment On 2020/09/06 3:02, Andy Shevchenko wrote: > > > On Thursday, September 3, 2020, Jiada Wang > wrote: > > From: Nick Dyer > > > Some maXTouch chips (eg mXT1386) will not respond on the first I2C > request > when they are in a sleep state. It must be retried after a delay for the > chip to wake up. > > Signed-off-by: Nick Dyer > > Acked-by: Yufeng Shen > > > > (cherry picked from ndyer/linux/for-upstream commit > 63fd7a2cd03c3a572a5db39c52f4856819e1835d) > > > It’s a noise for upstream. > sure, I will remove it > [gdavis: Forward port and fix conflicts.] > Signed-off-by: George G. Davis > > [jiada: return exact errno when i2c_transfer & i2c_master_send fails] > Signed-off-by: Jiada Wang > > --- >  drivers/input/touchscreen/atmel_mxt_ts.c | 45 ++++++++++++++++-------- >  1 file changed, 30 insertions(+), 15 deletions(-) > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c > b/drivers/input/touchscreen/atmel_mxt_ts.c > index a2189739e30f..5d4cb15d21dc 100644 > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > @@ -196,6 +196,7 @@ enum t100_type { >  #define MXT_CRC_TIMEOUT                1000    /* msec */ >  #define MXT_FW_RESET_TIME      3000    /* msec */ >  #define MXT_FW_CHG_TIMEOUT     300     /* msec */ > +#define MXT_WAKEUP_TIME                25      /* msec */ > > > Can we simple add _MS unit suffix to the definition? As Dmitry commented, I'd like to keep it as-is, probably a separate patch to update all these together. > > >  /* Command to unlock bootloader */ >  #define MXT_UNLOCK_CMD_MSB     0xaa > @@ -626,6 +627,7 @@ static int __mxt_read_reg(struct i2c_client *client, >         struct i2c_msg xfer[2]; >         u8 buf[2]; >         int ret; > +       bool retry = false; > > > Keep this ordered by length. I will move "retry" upper. > > >         buf[0] = reg & 0xff; >         buf[1] = (reg >> 8) & 0xff; > @@ -642,17 +644,22 @@ static int __mxt_read_reg(struct i2c_client > *client, >         xfer[1].len = len; >         xfer[1].buf = val; > > -       ret = i2c_transfer(client->adapter, xfer, 2); > -       if (ret == 2) { > -               ret = 0; > -       } else { > -               if (ret >= 0) > -                       ret = -EIO; > -               dev_err(&client->dev, "%s: i2c transfer failed (%d)\n", > -                       __func__, ret); > +retry_read: > +       ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer)); > +       if (ret != ARRAY_SIZE(xfer)) { > +               if (!retry) { > > > Why not positive conditional? to me, it's not much different either positive or negative conditional, can you elaborate more about this? > > +                       dev_dbg(&client->dev, "%s: i2c retry\n", > __func__); > > > __func__ is redundant for dev_dbg(). > > +                       msleep(MXT_WAKEUP_TIME); > +                       retry = true; > +                       goto retry_read; > > +               } else { > > > Redundant in either case of conditional. Allows to drop indentation level. I will remove the redundant conditional Thanks, Jiada > > +                       dev_err(&client->dev, "%s: i2c transfer > failed (%d)\n", > +                               __func__, ret); > +                       return ret < 0 ? ret : -EIO; > +               } >         } > > -       return ret; > +       return 0; >  } > > > Same comments about below. > >  static int __mxt_write_reg(struct i2c_client *client, u16 reg, u16 > len, > @@ -661,6 +668,7 @@ static int __mxt_write_reg(struct i2c_client > *client, u16 reg, u16 len, >         u8 *buf; >         size_t count; >         int ret; > +       bool retry = false; > >         count = len + 2; >         buf = kmalloc(count, GFP_KERNEL); > @@ -671,14 +679,21 @@ static int __mxt_write_reg(struct i2c_client > *client, u16 reg, u16 len, >         buf[1] = (reg >> 8) & 0xff; >         memcpy(&buf[2], val, len); > > +retry_write: >         ret = i2c_master_send(client, buf, count); > -       if (ret == count) { > -               ret = 0; > +       if (ret != count) { > +               if (!retry) { > +                       dev_dbg(&client->dev, "%s: i2c retry\n", > __func__); > +                       msleep(MXT_WAKEUP_TIME); > +                       retry = true; > +                       goto retry_write; > +               } else { > +                       dev_err(&client->dev, "%s: i2c send failed > (%d)\n", > +                               __func__, ret); > +                       ret = ret < 0 ? ret : -EIO; > +               } >         } else { > -               if (ret >= 0) > -                       ret = -EIO; > -               dev_err(&client->dev, "%s: i2c send failed (%d)\n", > -                       __func__, ret); > +               ret = 0; >         } > >         kfree(buf); > -- > 2.17.1 > > > > -- > With Best Regards, > Andy Shevchenko > >