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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2A77FC47088 for ; Fri, 2 Dec 2022 08:31:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232817AbiLBIbl (ORCPT ); Fri, 2 Dec 2022 03:31:41 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48462 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232738AbiLBI37 (ORCPT ); Fri, 2 Dec 2022 03:29:59 -0500 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9697860B66; Fri, 2 Dec 2022 00:29:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1669969786; x=1701505786; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=4tZ8P1rjFZqQdEDu1Entw+Faj29DY+fm4wogYriUb4s=; b=Jdt8OOBFCU2iBd2rV6fjNUU1WhsCSQfugO5vi918dZQOBVDmUURee7kF wy4DNQmulxM9x2mekJdHFLN6ZLJc91iOJz7qwS3qFjUgjUujIKaGnQboA 0b1nADg7T/1N2kz+QR31RJem6yAOD9cxhO7pHMPmOM1+adtqmoLK55QvU Tq4jJ0nBdNViYr7fKzjgIPgqe3LRl95LOeif7HKCBeZd9ZOCClCQdeZru xiC3dKvU0NIdH8g9k13TV7JVkXB7WZ9Xiu8hwQ9UmCMZine8eaPBRayq/ cbqZA2psQ5Lrurx4B3yQwAHqcrtyA8sLt9uCX2Xl4/IASLIsn7Ry5L5PL Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10548"; a="317764272" X-IronPort-AV: E=Sophos;i="5.96,210,1665471600"; d="scan'208";a="317764272" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Dec 2022 00:29:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10548"; a="973832094" X-IronPort-AV: E=Sophos;i="5.96,210,1665471600"; d="scan'208";a="973832094" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga005.fm.intel.com with ESMTP; 02 Dec 2022 00:29:24 -0800 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1p11QA-003HNL-05; Fri, 02 Dec 2022 10:29:22 +0200 Date: Fri, 2 Dec 2022 10:29:21 +0200 From: Andy Shevchenko To: Binbin Zhou Cc: Wolfram Sang , Wolfram Sang , Mika Westerberg , linux-i2c@vger.kernel.org, loongarch@lists.linux.dev, devicetree@vger.kernel.org, Huacai Chen , WANG Xuerui , Arnd Bergmann , Rob Herring , Krzysztof Kozlowski , Jianmin Lv Subject: Re: [PATCH V4 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Fri, Dec 02, 2022 at 11:22:19AM +0800, Binbin Zhou wrote: > 在 2022/12/1 04:41, Andy Shevchenko 写道: > > On Wed, Nov 30, 2022 at 01:56:20PM +0800, Binbin Zhou wrote: ... > > > + for (retry = 0; retry < adap->retries; retry++) { > > > + ret = ls2x_i2c_doxfer(adap, msgs, num); > > > + if (ret != -EAGAIN) > > > + return ret; > > > + > > > + dev_dbg(priv->dev, "Retrying transmission (%d)\n", retry); > > > + udelay(100); > > Why atomic? This long (esp. atomic) delay must be explained. > > The modification records for this part of the source code are no longer > traceable. > > Communicating with colleagues offline, I learned that this part of the code > first appeared on Linux 2.6.36, which was done to circumvent the problem of > probable failure to scan the device for i2c devices on some boards. > > How about I add a comment here to explain the reason for this? Yes, that's what we want, and not what you said above. I.o.w. the comment like "reason is unknown" is not accepted. Can you be more specific about the boards and why do you still need this delay? And also why is it atomic? > > > + } ... > > > + r = devm_request_irq(dev, irq, ls2x_i2c_irq_handler, > > > + IRQF_SHARED, "ls2x-i2c", priv); > > Indentation. > > Do you mean  "IRQF_SHARE"  should be aligned to "dev"  ? Yes. ... > > > +static const struct dev_pm_ops ls2x_i2c_pm_ops = { > > > + SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend, ls2x_i2c_resume) > > > +}; > > Use corresponding DEFINE_ macro. > > ok. > > I will use > > "static DEFINE_SIMPLE_DEV_PM_OPS(ls2x_i2c_pm_ops, ls2x_i2c_suspend, > ls2x_i2c_resume);"  corresponding to  ".pm     = pm_ptr(&ls2x_i2c_pm_ops)," Shouldn't be pm_sleep_ptr()? -- With Best Regards, Andy Shevchenko