From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757271AbcEFDUc (ORCPT ); Thu, 5 May 2016 23:20:32 -0400 Received: from lucky1.263xmail.com ([211.157.147.132]:42003 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755416AbcEFDUa (ORCPT ); Thu, 5 May 2016 23:20:30 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-KSVirus-check: 0 X-RL-SENDER: david.wu@rock-chips.com X-FST-TO: linux-arm-kernel@lists.infradead.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: david.wu@rock-chips.com X-UNIQUE-TAG: <64a412a670cb02dde063524da51c8ff3> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP To: Doug Anderson References: <1462371194-5809-1-git-send-email-david.wu@rock-chips.com> <1462371194-5809-5-git-send-email-david.wu@rock-chips.com> Cc: =?UTF-8?Q?Heiko_St=c3=bcbner?= , Wolfram Sang , Mark Rutland , Tao Huang , Jianqun Xu , Lin Huang , Pawel Moll , Ian Campbell , "devicetree@vger.kernel.org" , "open list:ARM/Rockchip SoC..." , "linux-kernel@vger.kernel.org" , Eddie Cai , Andy Shevchenko , Rob Herring , "linux-i2c@vger.kernel.org" , Kumar Gala , Chris , Brian Norris , David Riley , "linux-arm-kernel@lists.infradead.org" From: "David.Wu" Message-ID: <572C0D89.9050708@rock-chips.com> Date: Fri, 6 May 2016 11:20:41 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Doug, 在 2016/5/6 6:56, Doug Anderson 写道: > David, > > On Wed, May 4, 2016 at 7:13 AM, David Wu wrote: >> Signed-off-by: David Wu >> --- >> Change in v7: >> - none >> >> drivers/i2c/busses/i2c-rk3x.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) > > Probably this change could be dropped now. Switching the location of > setting START_START was much more important when you were supporting > HIGH SPEED mode. I don't think we need it anymore, right? > Yes, I would drop it next version，STATE_SETUP didn't make sense, it was not much more important, because there was a error printk for rk3x_i2c_setup() called failed. > ...if we want to keep this change, I'd say: > > 1. Add a description, like maybe: > > To help with debugging add a STATE_SETUP between STATE_IDLE and > STATE_START to make it more obvious that we're not actually idle but we > also haven't initiated the start bit. This change is not expected to > have any impact but it does delay the changing of state to STATE_START. > If previously we were getting an erroneous interrupt before we actually > sent the start bit we'll now be treating that differently. The new > behavior (catching the erroneous interrupt) should be better. > > 2. Change "i2c->state = STATE_SETUP" to the _start_ of > rk3x_i2c_setup(). That would have a better chance of catching a > spurious interrupt. > > 3. Add an error check at the start of rk3x_i2c_irq() similar to the > check for STATE_IDLE (or use the same check and modify the printk). > Specifically the justification for adding STATE_SETUP is to help with > debugging (catch interrupts that were unexpected and print more info > about our state), so we should make it useful for this. > > > -Doug > > >