Re: [PATCH i2c-tools-3.1] i2c-tools: add new tool 'i2ctransfer'
On Wed, 19 Apr 2017 20:12:57 +0200, Wolfram Sang wrote: > On Fri, Apr 07, 2017 at 02:44:03PM +0200, Wolfram Sang wrote: > > This tool allows to construct and concat multiple I2C messages into one > > single transfer. Its aim is to test I2C master controllers, and so there > > is no SMBus fallback. > > > > I've been missing such a tool a number of times now, so I finally got > > around to writing it myself. As with all I2C tools, it can be dangerous, > > but it can also be very useful when developing. > > > > It has been tested with various Renesas I2C IP cores as well as Tegra, > > i.MX and AT91. > > > > Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com> > > --- > > Pushed to i2c-tools-3.1. As usual I'm late to the party... I don't think this should have gone to branch 3.1 which is a bug-fix only branch. If people want the newest shiny stuff they should use the master branch. And if it bothers them that there is no released version of it... well they may be right, and it suggests it's about time to push a first version of i2c-tools 4 out the door. -- Jean Delvare SUSE L3 Support
Re: [PATCH 1/2] i2c: stub: move module_init/exit annotations to the proper place
Hi Wolfram,
On Tue, 16 May 2017 13:21:04 +0200, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
> ---
> drivers/i2c/i2c-stub.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/i2c-stub.c b/drivers/i2c/i2c-stub.c
> index 06af583d510150..0aa4d646f8fb26 100644
> --- a/drivers/i2c/i2c-stub.c
> +++ b/drivers/i2c/i2c-stub.c
> @@ -406,16 +406,15 @@ static int __init i2c_stub_init(void)
> i2c_stub_free();
> return ret;
> }
> +module_init(i2c_stub_init);
>
> static void __exit i2c_stub_exit(void)
> {
> i2c_del_adapter(_adapter);
> i2c_stub_free();
> }
> +module_exit(i2c_stub_exit);
>
> MODULE_AUTHOR("Mark M. Hoffman <mhoff...@lightlink.com>");
> MODULE_DESCRIPTION("I2C stub driver");
> MODULE_LICENSE("GPL");
> -
> -module_init(i2c_stub_init);
> -module_exit(i2c_stub_exit);
I'm not sure on what you base your claim that this is the "proper
place". checkpatch doesn't complain about either, and if anything, the
original style (module_init/exit) at end of file seems a lot more
popular through the kernel tree.
--
Jean Delvare
SUSE L3 Support
Re: [RFC] i2c-stub: make it usable with DT when booting
Hi Wolfram,
On Tue, 16 May 2017 13:27:58 +0200, Wolfram Sang wrote:
> This patch makes the stub driver parse the device tree when booting and
> create a virtual bus with the desired devices attached. Here is an
> example DTS snipplet making use of it. It is for simulating a more
> complex camera device which has dependencies which needs to be sorted
> out at boot time:
>
> i2c@42 {
> compatible = "i2c-stub";
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> des@4c {
> compatible = "maxim,max9268";
> reg = <0x4c>;
> };
>
> eeprom@57 {
> compatible = "atmel,24c02";
> reg = <0x57>;
> };
> };
I get the idea and I understand the need to emulate the devices early
in the boot process. However, how do you deal with the I2C devices
pre-initialization (setting initial register values)? This is typically
done in user-space with the i2c-stub-from-dump script, or manual calls
to i2cset, but that would be too late in your case, wouldn't it?
>
> FIXME: can fw_* calls be used instead of of_*?
>
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
> ---
>
> It was really useful when developing. When using fw_* calls, we probably could
> make use of it with ACPI as well. Still, calling for opinions if we want this
> upstream or if we want to stay module-only? Also, the binding should really
> be marked as "development only". Or is it better to keep it as a separate
> patch to keep the binding un-official?
Code looks sane, but I can't comment on the bindings side of things as
this isn't my area.
--
Jean Delvare
SUSE L3 Support
Re: [PATCH 2/2] i2c: stub: use pr_fmt
On Tue, 16 May 2017 13:21:05 +0200, Wolfram Sang wrote:
> Instead of hard coding "i2c-stub:", let's use the pr_fmt mechanism to
> achieve the same more easily. This makes it easier to stay consistent
> when adding new messages. Also, remove an unneeded OOM message while we
> are here.
>
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
> ---
> drivers/i2c/i2c-stub.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/i2c-stub.c b/drivers/i2c/i2c-stub.c
> index 0aa4d646f8fb26..5627b1df391c7e 100644
> --- a/drivers/i2c/i2c-stub.c
> +++ b/drivers/i2c/i2c-stub.c
> @@ -15,7 +15,8 @@
> GNU General Public License for more details.
> */
>
> -#define DEBUG 1
> +#define DEBUG
> +#define pr_fmt(fmt) "i2c-stub: " fmt
>
> #include
> #include
> @@ -342,7 +343,7 @@ static int __init i2c_stub_allocate_banks(int i)
> if (!chip->bank_words)
> return -ENOMEM;
>
> - pr_debug("i2c-stub: Allocated %u banks of %u words each (registers
> 0x%02x to 0x%02x)\n",
> + pr_debug("Allocated %u banks of %u words each (registers 0x%02x to
> 0x%02x)\n",
>chip->bank_mask, chip->bank_size, chip->bank_start,
>chip->bank_end);
>
> @@ -363,28 +364,27 @@ static int __init i2c_stub_init(void)
> int i, ret;
>
> if (!chip_addr[0]) {
> - pr_err("i2c-stub: Please specify a chip address\n");
> + pr_err("Please specify a chip address\n");
> return -ENODEV;
> }
>
> for (i = 0; i < MAX_CHIPS && chip_addr[i]; i++) {
> if (chip_addr[i] < 0x03 || chip_addr[i] > 0x77) {
> - pr_err("i2c-stub: Invalid chip address 0x%02x\n",
> + pr_err("Invalid chip address 0x%02x\n",
> chip_addr[i]);
> return -EINVAL;
> }
>
> - pr_info("i2c-stub: Virtual chip at 0x%02x\n", chip_addr[i]);
> + pr_info("Virtual chip at 0x%02x\n", chip_addr[i]);
> }
>
> /* Allocate memory for all chips at once */
> stub_chips_nr = i;
> stub_chips = kcalloc(stub_chips_nr, sizeof(struct stub_chip),
> GFP_KERNEL);
> - if (!stub_chips) {
> - pr_err("i2c-stub: Out of memory\n");
> + if (!stub_chips)
> return -ENOMEM;
> - }
> +
> for (i = 0; i < stub_chips_nr; i++) {
> INIT_LIST_HEAD(_chips[i].smbus_blocks);
>
Looks good to me.
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
[PATCH v2] i2c: algo-bit: add support for I2C_M_STOP
Support for enforced STOPs will allow us to use SCCB compatible devices.
Based on a preliminary patch by Wolfram Sang.
Signed-off-by: Jean Delvare <jdelv...@suse.de>
Tested-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
---
Changes since v1:
* Simplify logic
drivers/i2c/algos/i2c-algo-bit.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
--- linux-4.11.orig/drivers/i2c/algos/i2c-algo-bit.c2017-06-19
09:57:17.949074198 +0200
+++ linux-4.11/drivers/i2c/algos/i2c-algo-bit.c 2017-06-21 09:07:07.910049960
+0200
@@ -553,9 +553,18 @@ 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, _adap->dev, "emitting "
- "repeated start condition\n");
- i2c_repstart(adap);
+ if (msgs[i - 1].flags & I2C_M_STOP) {
+ bit_dbg(3, _adap->dev,
+ "emitting enforced stop
condition\n");
+ i2c_stop(adap);
+ bit_dbg(3, _adap->dev,
+ "emitting start condition\n");
+ i2c_start(adap);
+ } else {
+ bit_dbg(3, _adap->dev,
+ "emitting repeated start
condition\n");
+ i2c_repstart(adap);
+ }
}
ret = bit_doAddress(i2c_adap, pmsg);
if ((ret != 0) && !nak_ok) {
--
Jean Delvare
SUSE L3 Support
Re: [PATCH] i2c: algo-bit: add support for I2C_M_STOP
Hi Wolfram,
On Tue, 20 Jun 2017 19:28:10 +0200, Wolfram Sang wrote:
> Hi Jean,
>
> > > + bool did_stop = false;
> >
> > I'm pretty certain you want to declare and initialize this variable
> > outside the loop.
>
> Why? Well, it doesn't matter anymore but what is wrong with limiting the
> variable like this?
There's no problem with the scope. The problem is with the
initialization. The way you did, did_stop gets reset to false with
every iteration, which isn't what you want.
> > (...)
> > 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?
>
> SCCB defines addresses to be 7 bit.
I looked at how 10-bit addressing works again and in fact it is simply
impossible to not use repeated start when reading from a 10-bit address
slave. Only the first 2 bits of the 10-bit address are repeated in the
read part of the transaction. If there was a stop between the two parts
then there would be no way to know which 10-bit slave should send the
data.
> > (...)
> > --- linux-4.11.orig/drivers/i2c/algos/i2c-algo-bit.c2017-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, _adap->dev, "emitting "
> > - "repeated start condition\n");
> > + if (msgs[i - 1].flags & I2C_M_STOP) {
> > + bit_dbg(3, _adap->dev,
> > + "emitting enforced stop
> > condition\n");
> > + i2c_stop(adap);
> > + bit_dbg(3, _adap->dev,
> > + "emitting start condition\n");
> > + i2c_start(adap);
> > + }
>
> else
Oops. Right, fixed, thanks.
>
> > +
> > + bit_dbg(3, _adap->dev,
> > + "emitting repeated start condition\n");
> > i2c_repstart(adap);
> > }
> > ret = bit_doAddress(i2c_adap, pmsg);
>
> A lot better! I like it very much. And also
>
> Tested-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
>
> Do you want to cook up a patch or shall I? I'd just need a SoB then.
I'll send a proper patch shortly.
> Thanks for the improvement!
You're welcome.
--
Jean Delvare
SUSE L3 Support
Re: [PATCH v2] i2c: algo-bit: add support for I2C_M_STOP
On Wed, 21 Jun 2017 16:54:26 +0200, Wolfram Sang wrote:
> > + if (msgs[i - 1].flags & I2C_M_STOP) {
> > + bit_dbg(3, _adap->dev,
> > + "emitting enforced stop
> > condition\n");
> > + i2c_stop(adap);
> > + bit_dbg(3, _adap->dev,
> > + "emitting start condition\n");
>
> Do you mind if I combine the two debug outputs into one?
>
> > + bit_dbg(3, _adap->dev,
> > + "emitting enforced stop/start
> > condition\n");
>
Go ahead, fine with me.
--
Jean Delvare
SUSE L3 Support
Re: [PATCH] i2c: algo-bit: add support for I2C_M_STOP
On Wed, 21 Jun 2017 16:30:18 +0200, Wolfram Sang wrote: > Which reminds me: Have you ever seen a 10-bit client device in the wild? > I wanted to buy one for testing reasons but was not able to locate one. > I only know a Renesas IP core which has 10-bit slave capability (but no > driver support for that yet). I remember wishing I could drop support, asking around, and a few people replying to me that 10-bit slaves actually exist. But of course I can't find the discussion thread again. I can see one driver for a 10-bit address I2C device: drivers/media/i2c/tw2804.c (device instantiated from drivers/media/usb/go7007/go7007-usb.c.) However it seems that in many cases I2C_M_TEN is used directly (instead of the more correct I2C_CLIENT_TEN.) See for example drivers/input/touchscreen/atmel_mxt_ts.c, drivers/input/touchscreen/elants_i2c.c, drivers/input/touchscreen/mms114.c, drivers/media/pci/cx23885/cx23885-i2c.c, drivers/media/pci/cx25821/cx25821-i2c.c, which are clearly talking to 10-bit I2C devices, but without instantiating an i2c_client for them. I see also commits explicitly adding or fixing 10-bit address support in various I2C bus drivers, I don't think developers would be doing that if they didn't need it. -- Jean Delvare SUSE L3 Support
Re: [PATCH] i2c: algo-bit: add support for I2C_M_STOP
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 <wsa+rene...@sang-engineering.com>
> ---
>
> 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, _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 = [i];
> nak_ok = pmsg->flags & I2C_M_IGNORE_NAK;
> if (!(pmsg->flags & I2C_M_NOSTART)) {
> - if (i) {
> - bit_dbg(3, _adap->dev, "emitting "
> - "repeated start condition\n");
> + if (did_stop) {
> + bit_dbg(3, _adap->dev, "emitting start
> condition\n");
> + i2c_start(adap);
> + did_stop = false;
> + } else if (i) {
> + bit_dbg(3, _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, _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?
--- linux-4.11.orig/drivers/i2c/algos/i2c-algo-bit.c2017-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, _adap->dev, "emitting "
- "repeated start condition\n");
+ if (msgs[i - 1].flags & I2C_M_STOP) {
+ bit_dbg(3, _adap->dev,
+ "emitting enforced stop
condition\n");
+ i2c_stop(adap);
+ bit_dbg(3, _adap->dev,
+ "emitting start condition\n");
+ i2c_start(adap);
+ }
+
+ bit_dbg(3, _adap->dev,
+ "emitting repeated start condition\n");
i2c_repstart(adap);
}
ret = bit_doAddress(i2c_adap, pmsg);
--
Jean Delvare
SUSE L3 Support
