From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 48B32C433E2 for ; Sat, 12 Sep 2020 00:50:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 09EE92054F for ; Sat, 12 Sep 2020 00:50:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725894AbgILAum (ORCPT ); Fri, 11 Sep 2020 20:50:42 -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 > >