From: Martin Sperl <kernel@martin.sperl.org> To: "Noralf Trønnes" <noralf@tronnes.org>, wsa@the-dreams.de, swarren@wwwdotorg.org, eric@anholt.net Cc: linux-rpi-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org Subject: Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer Date: Tue, 20 Sep 2016 09:19:13 +0200 [thread overview] Message-ID: <c47a0a10-4e0b-9025-8d38-af299d5f55da@martin.sperl.org> (raw) In-Reply-To: <1474298777-5858-2-git-send-email-noralf@tronnes.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 <noralf@tronnes.org> > --- > 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] <http://lxr.free-electrons.com/ident?i=msg>->flags <http://lxr.free-electrons.com/ident?i=flags> &I2C_M_STOP <http://lxr.free-electrons.com/ident?i=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
WARNING: multiple messages have this Message-ID (diff)
From: kernel@martin.sperl.org (Martin Sperl) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer Date: Tue, 20 Sep 2016 09:19:13 +0200 [thread overview] Message-ID: <c47a0a10-4e0b-9025-8d38-af299d5f55da@martin.sperl.org> (raw) In-Reply-To: <1474298777-5858-2-git-send-email-noralf@tronnes.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 <noralf@tronnes.org> > --- > 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] <http://lxr.free-electrons.com/ident?i=msg>->flags <http://lxr.free-electrons.com/ident?i=flags> &I2C_M_STOP <http://lxr.free-electrons.com/ident?i=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
next prev parent reply other threads:[~2016-09-20 7:18 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-09-19 15:26 [PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes Noralf Trønnes 2016-09-19 15:26 ` Noralf Trønnes 2016-09-19 15:26 ` [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer Noralf Trønnes 2016-09-19 15:26 ` Noralf Trønnes 2016-09-20 7:19 ` Martin Sperl [this message] 2016-09-20 7:19 ` Martin Sperl 2016-09-20 8:41 ` Noralf Trønnes 2016-09-20 8:41 ` Noralf Trønnes 2016-09-20 8:41 ` Noralf Trønnes 2016-09-20 10:15 ` Martin Sperl 2016-09-20 10:15 ` Martin Sperl 2016-09-20 10:56 ` Noralf Trønnes 2016-09-20 10:56 ` Noralf Trønnes 2016-09-20 11:29 ` kernel 2016-09-20 11:29 ` kernel at martin.sperl.org 2016-09-21 13:45 ` Noralf Trønnes 2016-09-21 13:45 ` Noralf Trønnes 2016-09-19 15:26 ` [PATCH 3/3] i2c: bcm2835: Use ratelimited logging on transfer errors Noralf Trønnes 2016-09-19 15:26 ` Noralf Trønnes 2016-09-19 16:51 ` [PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes Eric Anholt 2016-09-19 16:51 ` Eric Anholt 2016-09-19 16:51 ` Eric Anholt 2016-09-19 17:36 ` Noralf Trønnes 2016-09-19 17:36 ` Noralf Trønnes 2016-09-20 6:46 ` Martin Sperl 2016-09-20 6:46 ` Martin Sperl 2016-09-20 6:46 ` Martin Sperl
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c47a0a10-4e0b-9025-8d38-af299d5f55da@martin.sperl.org \ --to=kernel@martin.sperl.org \ --cc=eric@anholt.net \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rpi-kernel@lists.infradead.org \ --cc=noralf@tronnes.org \ --cc=swarren@wwwdotorg.org \ --cc=wsa@the-dreams.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.