From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754518AbcITHSu (ORCPT ); Tue, 20 Sep 2016 03:18:50 -0400 Received: from 212-186-180-163.static.upcbusiness.at ([212.186.180.163]:60354 "EHLO cgate.sperl.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751211AbcITHSr (ORCPT ); Tue, 20 Sep 2016 03:18:47 -0400 X-Greylist: delayed 1935 seconds by postgrey-1.27 at vger.kernel.org; Tue, 20 Sep 2016 03:18:47 EDT Subject: Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer To: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= , wsa@the-dreams.de, swarren@wwwdotorg.org, eric@anholt.net References: <1474298777-5858-1-git-send-email-noralf@tronnes.org> <1474298777-5858-2-git-send-email-noralf@tronnes.org> Cc: linux-rpi-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org From: Martin Sperl Message-ID: Date: Tue, 20 Sep 2016 09:19:13 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <1474298777-5858-2-git-send-email-noralf@tronnes.org> 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 Noralf! On 19.09.2016 17:26, Noralf Trønnes wrote: > Some SMBus protocols use Repeated Start Condition to switch from write > mode to read mode. Devices like MMA8451 won't work without it. > > When downstream implemented support for this in i2c-bcm2708, it broke > support for some devices, so a module parameter was added and combined > transfer was disabled by default. > See https://github.com/raspberrypi/linux/issues/599 > It doesn't seem to have been any investigation into what the problem > really was. Later there was added a timeout on the polling loop. > > One of the devices mentioned to partially stop working was DS1307. > > I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel) > and AT24C32 (eeprom) in parallel without problems. > > Signed-off-by: Noralf Trønnes > --- > drivers/i2c/busses/i2c-bcm2835.c | 107 +++++++++++++++++++++++++++++++++++---- > 1 file changed, 98 insertions(+), 9 deletions(-) ... > @@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], > int i; > int ret = 0; > > + /* Combined write-read to the same address (smbus) */ > + if (num == 2 && (msgs[0].addr == msgs[1].addr) && > + !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) && > + (msgs[0].len <= 16)) { > + ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]); > + > + return ret ? ret : 2; > + } > + > for (i = 0; i < num; i++) { > - ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]); > + ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], NULL); > if (ret) > break; > } This does not seem to implement the i2c_msg api correctly. As per comments in include/uapi/linux/i2c.h on line 58 only the last message in a group should - by default - send a STOP. As far as I understand you would need to implement the I2C_M_STOP flag (by exposing I2C_FUNC_PROTOCOL_MANGLING in bcm2835_i2c_func) to make this work correctly: for (i = 0; i < num; i++) { + bool send_stop = (i == num - 1) ||msgs[i] ->flags &I2C_M_STOP ; - ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]); + ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], send_stop); if (ret) break; } The corresponding device driver (or userspace) will need to set the flag correctly. Martin