From patchwork Mon Jun 19 08:30:02 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jean Delvare X-Patchwork-Id: 777644 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3wrkgN1ygxz9s76 for ; Mon, 19 Jun 2017 18:30:08 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753501AbdFSIaG (ORCPT ); Mon, 19 Jun 2017 04:30:06 -0400 Received: from mx2.suse.de ([195.135.220.15]:45657 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752386AbdFSIaG (ORCPT ); Mon, 19 Jun 2017 04:30:06 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 573CCAB46; Mon, 19 Jun 2017 08:30:04 +0000 (UTC) Date: Mon, 19 Jun 2017 10:30:02 +0200 From: Jean Delvare To: Wolfram Sang Cc: linux-i2c@vger.kernel.org, linux-renesas-soc@vger.kernel.org Subject: Re: [PATCH] i2c: algo-bit: add support for I2C_M_STOP Message-ID: <20170619103002.5d502c92@endymion> In-Reply-To: <20170617171238.19638-1-wsa+renesas@sang-engineering.com> References: <20170617171238.19638-1-wsa+renesas@sang-engineering.com> Organization: SUSE Linux X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.31; x86_64-suse-linux-gnu) MIME-Version: 1.0 Sender: linux-i2c-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-i2c@vger.kernel.org Hi Wolfram, On Sat, 17 Jun 2017 19:12:38 +0200, Wolfram Sang wrote: > Support for enforced STOPs will allow us to use SCCB compatible devices. > > Signed-off-by: Wolfram Sang > --- > > Notes: I don't actually have any SCCB compatible sensor but verified with a > logic analyzer that repeated starts got replaced with a stop + start sequence. > However, we have members in our team who might need this feature soon. I'll > likely wait for their Tested-by unless something unforseen happens. > > drivers/i2c/algos/i2c-algo-bit.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c > index a8e89df665b904..4758058352959d 100644 > --- a/drivers/i2c/algos/i2c-algo-bit.c > +++ b/drivers/i2c/algos/i2c-algo-bit.c > @@ -549,12 +549,17 @@ static int bit_xfer(struct i2c_adapter *i2c_adap, > bit_dbg(3, &i2c_adap->dev, "emitting start condition\n"); > i2c_start(adap); > for (i = 0; i < num; i++) { > + bool did_stop = false; I'm pretty certain you want to declare and initialize this variable outside the loop. > + > pmsg = &msgs[i]; > nak_ok = pmsg->flags & I2C_M_IGNORE_NAK; > if (!(pmsg->flags & I2C_M_NOSTART)) { > - if (i) { > - bit_dbg(3, &i2c_adap->dev, "emitting " > - "repeated start condition\n"); > + if (did_stop) { > + bit_dbg(3, &i2c_adap->dev, "emitting start condition\n"); > + i2c_start(adap); > + did_stop = false; > + } else if (i) { > + bit_dbg(3, &i2c_adap->dev, "emitting repeated start condition\n"); > i2c_repstart(adap); > } > ret = bit_doAddress(i2c_adap, pmsg); > @@ -588,6 +593,12 @@ static int bit_xfer(struct i2c_adapter *i2c_adap, > goto bailout; > } > } > + > + if (pmsg->flags & I2C_M_STOP && i != num - 1) { I recommend adding parentheses around the bit matching when combined with &&. I know it is not strictly needed and the compiler doesn't care, but I find it easier to read, and there seems to be a consensus (90 %) on that in the kernel tree. > + bit_dbg(3, &i2c_adap->dev, "emitting enforced stop condition\n"); > + i2c_stop(adap); > + did_stop = true; > + } > } > ret = i; > I have 2 questions: 1* Repeated start happens between messages of a same transaction, and you handle that case above. However in the case of 10-bit address clients, there is also a repeated start happening during the address phase of the transaction, if the first message is a read. Did you check what the SCCB protocol expects in that case? 2* I'm not sure why you add the enforced stop at the end of one iteration and the start at the beginning of the next iteration. It would be more simple and efficient to do both at the beginning of the next iteration. The only case where it would make a difference is if both I2C_M_NOSTART and I2C_M_STOP are specified. In this case you currently emit a stop condition but no start, which I don't think can work at all. What about something like that instead? Or I am missing something? Tested-by: Wolfram Sang --- linux-4.11.orig/drivers/i2c/algos/i2c-algo-bit.c 2017-06-19 09:57:17.949074198 +0200 +++ linux-4.11/drivers/i2c/algos/i2c-algo-bit.c 2017-06-19 10:23:26.711088536 +0200 @@ -553,8 +553,17 @@ static int bit_xfer(struct i2c_adapter * nak_ok = pmsg->flags & I2C_M_IGNORE_NAK; if (!(pmsg->flags & I2C_M_NOSTART)) { if (i) { - bit_dbg(3, &i2c_adap->dev, "emitting " - "repeated start condition\n"); + if (msgs[i - 1].flags & I2C_M_STOP) { + bit_dbg(3, &i2c_adap->dev, + "emitting enforced stop condition\n"); + i2c_stop(adap); + bit_dbg(3, &i2c_adap->dev, + "emitting start condition\n"); + i2c_start(adap); + } + + bit_dbg(3, &i2c_adap->dev, + "emitting repeated start condition\n"); i2c_repstart(adap); } ret = bit_doAddress(i2c_adap, pmsg);