Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i2cget doesn't support implicit "next" address (BUS CHIP vs BUS CHIP ADDR) #414

Closed
nfstoney opened this issue Mar 14, 2023 · 27 comments
Closed

Comments

@nfstoney
Copy link

The functions i2cget and i2cset do not allow 16bit addresses.

But they should be available. There are devices that want to be addressed 16bit.

According the source-code I can't see an option for a workaround.

i2cget BUS CHIP ADDR
i2cset BUS CHIP ADDR VALUE... MODE

In both cases I speak about the parameter ADDR.

@enh-google
Copy link
Collaborator

any idea how larger addresses work in linux? the command field used for the address is only 8 bits:

struct i2c_smbus_ioctl_data {
	__u8 read_write;
	__u8 command;
	__u32 size;
	union i2c_smbus_data __user *data;
};

@enh-google enh-google changed the title i2ctools.c can't handle 16 bit addresses i2ctools.c can't handle 10-bit addresses Mar 15, 2023
@enh-google
Copy link
Collaborator

okay, so it looks like 16-bit addresses aren't a thing, but 10-bit addresses are. they sound pretty unsupported on linux though, judging by https://docs.kernel.org/i2c/ten-bit-addresses.html --- which implies that it's not just the toybox versions of these tools that don't support 10-bit addresses?

@enh-google
Copy link
Collaborator

https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/i2c-dev.h#L25 doesn't look good either:

/* NOTE: Slave address is 7 or 10 bits, but 10-bit addresses
 * are NOT supported! (due to code brokenness)
 */

@nfstoney
Copy link
Author

In Debian distributions we can read and write with i2cset and i2cget to our EEPROM successfully. With the same command line in the toybox tools it doesn't work.
I dont't talk about the slave address. I talk about the register address in the slave device.
If you want to address 16000 bytes in the EEPROM you must have at least 14 bits available. These are then contained in the two bytes (16 bit) address which are needed.

https://kernel.googlesource.com/pub/scm/utils/i2c-tools/i2c-tools/+/v3.0.1/tools/i2cset.c

@enh-google
Copy link
Collaborator

okay, but even for the data address, the code you linked to seems to have the same limit as toybox. toybox says:

  int bus = atolx_range(toys.optargs[0], 0, INT_MAX);
  int chip = atolx_range(toys.optargs[1], 0, 0x7f);
  int addr = atolx_range(toys.optargs[2], 0, 0xff);

whereas the i2c-tools code has limits of 0x3f, 0x7f, and 0xff respectively? (so the only obvious difference is that -- if i can find a reference confirm that there is a lower limit on bus number -- we should reduce INT_MAX to 0x3f.)

can you give a specific command that you claim works with i2c-tools but not with toybox?

@nfstoney
Copy link
Author

Examples:

i2c-tools


Prepare to read on bus 1 from device 0x57 at address 0x00 (addres, high byte) 0x40 (address, low byte)
i2cset -y 1 0x57 0x00 0x40 --> return nothing, because it works
Read byte on bus 1 from device 0x57 at set address
i2cget -y 1 0x57 --> return 0x79
Read byte on bus 1 from device 0x57 at set address + 1
i2cget -y 1 0x57 --> return 0x34

toybox


Prepare to read on bus 1 from device 0x57 at address 0x00 (addres, high byte) 0x40 (address, low byte)
i2cset -y 1 0x57 0x00 0x40 i --> return nothing, because it works
Read byte on bus 1 from device 0x57 at set address
i2cget -y 1 0x57 0x00 0x40 --> return i2cget: Max 3 arguments

For the first 255 bytes it would work, but what if I wan't to read from address 256 or higher

i2c-tools


Usage: i2cget [-f] [-y] [-a] I2CBUS CHIP-ADDRESS [DATA-ADDRESS [MODE]]
  I2CBUS is an integer or an I2C bus name
  ADDRESS is an integer (0x03 - 0x77, or 0x00 - 0x7f if -a is given)
  MODE is one of:
    b (read byte data, default)
    w (read word data)
    c (write byte/read byte)
    Append p for SMBus PEC

toybox


usage: i2cset [-fy] BUS CHIP ADDR VALUE... MODE
    Write an i2c register. MODE is b for byte, w for 16-bit word, i for I2C block.
    -f	Force access to busy devices
    -y	Answer "yes" to confirmation prompts (for script use)

@enh-google
Copy link
Collaborator

ah, okay, so the actual bug here is that toybox is

    usage: i2cget [-fy] BUS CHIP ADDR

but should be

    usage: i2cget [-fy] BUS CHIP [ADDR]

and you need the "implicit next address" functionality.

seems easy enough (i think we just pass -1 to the kernel as the address in that case?), but i probably won't have time to prepare a patch before the weekend. (and i don't have an appropriate i2c device to test with, so you'll have to tell us whether or not it works!)

@enh-google enh-google changed the title i2ctools.c can't handle 10-bit addresses i2cget doesn't support implicit "next" address (BUS CHIP vs BUS CHIP ADDR) Mar 15, 2023
@landley
Copy link
Owner

landley commented Mar 15, 2023

I took a wild stab at it (commit e8f2f55) but don't have a test environment set up here? (Can't even tell if I broke it, if somebody could give it a try...)

@enh-google
Copy link
Collaborator

heh, that looks exactly like the patch i'd have written, so we're in "fools seldom differ" territory at the very least :-)

@nfstoney
Copy link
Author

I have a hardware on my table, I tested it.
The message after i2cget -y 1 0x57 0x00 0x40 --> return i2cget: Max 3 arguments ist weg.

It doesn't work correct.

I give an example what I measure on the I2C-bus with the oscilloscope.

i2c-tools


i2cset -y 1 0x57 0x00 0x40
on the bus: 0x57wa 0x00a 0x40a
i2cget -y 1 0x57
on the bus: 0x57ra 0x37a
0x37 is the value at the address 0x0040
w: write, r: read, a: acknowledge

toybox


i2cset -y 1 0x57 0x00 0x40 i
on the bus: 0x57wa 0x00a 0x40a
i2cget -y 1 0x57
on the bus: 0x57wa 0xFFa 0x57ra 0xFFa
It seems that the first incorrectly writes down a new read address and then reads out the cell at the wrong location.
w: write, r: read, a: acknowledge

@landley
Copy link
Owner

landley commented Mar 16, 2023

We just changed i2cget so if you provide just the bus and chip, it would read the next (first?) available address, which is the ability to provide one LESS argument, not one more.

According to https://manpages.debian.org/unstable/i2c-tools/i2cget.8.en.html the usage line there is i2cget [-f] [-y] [-a] i2cbus chip-address [data-address [mode [length]]] which is up to FIVE arguments. And you're providing chip address 1, data address 0x57, mode 0, length 64. Hmmm...

I'll see what I can figure out? Still haven't got a test setup for this...

@landley
Copy link
Owner

landley commented Mar 16, 2023

I'm missing some domain expertise here.

I read the "mode parameter" paragraph of the man page 3 times and don't understand the different cases. In linux/i2c.h there's I2C_FUNC_SMBUS_READ_BYTE and also I2C_FUNC_SMBUS_READ_BYTE_DATA and I have no idea the difference between them. See also I2C_FUNC_SMBUS_READ_BLOCK_DATA and I2C_FUNC_SMBUS_READ_I2C_BLOCK with the helpful comment on the second "I2C-like block xfer".

I'm gonna try to go find some tutorials. I last used i2c seriously in 2010 and even that was just "bytes at addresses" using the existing command line tools. And if I do change anything: no test environment. (sudo i2cdetect -l on my laptop shows nothing).

@landley
Copy link
Owner

landley commented Mar 16, 2023

P.S. I still don't think you've given it a ten bit address, which the kernel supports: when the driver returns I2C_M_TEN you can set I2C_M_TEN in the i2c_msg flags.

The userspace tool hasn't got an option to say to do so, and the bus definition is insane enough that the 8 bit and 10 bit addresses are completely separate rather than 8 bit naturally being the first quarter of the 10 bit range which is why it can't just autodetect this because always using 10 bit when available means you won't see anything wired to 8 bit, but always using 8 bit for addresses that fit means you won't see those addresses in 10 bit...)

I could add a flag for that to toybox, but despite the initial title that doesn't seem to be the problem you're actually having...

@nfstoney
Copy link
Author

P.S. I still don't think you've given it a ten bit address, which the kernel supports: when the driver returns I2C_M_TEN you can set I2C_M_TEN in the i2c_msg flags.

The userspace tool hasn't got an option to say to do so, and the bus definition is insane enough that the 8 bit and 10 bit addresses are completely separate rather than 8 bit naturally being the first quarter of the 10 bit range which is why it can't just autodetect this because always using 10 bit when available means you won't see anything wired to 8 bit, but always using 8 bit for addresses that fit means you won't see those addresses in 10 bit...)

I could add a flag for that to toybox, but despite the initial title that doesn't seem to be the problem you're actually having...

Please forget the 10bit topic. This concerns the device address and has nothing to do with the current topic of 16bit addressing of a cell in the device.

For toybox it would be of advantage, if it would support this. i2c-tools can't do this either, i don't think there is a cry for it.

By the way, why are there two different tools for linux (toybox and i2c-tools) which support the same functions? Does it make sense to invent the wheel twice?

@nfstoney
Copy link
Author

I'm missing some domain expertise here.

I read the "mode parameter" paragraph of the man page 3 times and don't understand the different cases. In linux/i2c.h there's I2C_FUNC_SMBUS_READ_BYTE and also I2C_FUNC_SMBUS_READ_BYTE_DATA and I have no idea the difference between them. See also I2C_FUNC_SMBUS_READ_BLOCK_DATA and I2C_FUNC_SMBUS_READ_I2C_BLOCK with the helpful comment on the second "I2C-like block xfer".

I'm gonna try to go find some tutorials. I last used i2c seriously in 2010 and even that was just "bytes at addresses" using the existing command line tools. And if I do change anything: no test environment. (sudo i2cdetect -l on my laptop shows nothing).

With i2cset -y 1 0x57 0x00 0x40
0x00 isn't mode and 0x40 isn't length.
I set the start address (0x0040) on the device 0x57.

And with i2cget -y 1 0x57
I start read byte per byte from the set address (0x0040).

@nfstoney
Copy link
Author

https://manpages.debian.org/unstable/i2c-tools/i2cget.8.en.html
Sectio examples:
Set the internal pointer register of a 24C32 EEPROM at 7-bit address 0x53 on bus 9 (i2c-9) to 0x0000, then read the first 2 bytes from that EEPROM:
i2cset -y 9 0x53 0x00 0x00 ; i2cget -y 9 0x53 ; i2cget -y 9 0x53
This again assumes that the device automatically increments its internal pointer register on every read, and supports read byte transactions. While the previous example was for a small EEPROM using 8-bit internal addressing, this example is for a larger EEPROM using 16-bit internal addressing. Beware that running this command on a small EEPROM using 8-bit internal addressing would actually write 0x00 to the first byte of that EEPROM. The safety concerns raised above still stand, however in this case there is no SMBus equivalent, so this is the only way to read data from a large EEPROM if your master isn't fully I2C capable. With a fully I2C capable master, you would use i2ctransfer to achieve the same in a safe and faster way.

The main problem is @ i2cget -y 1 0x57
are the two first bytes written on the bus (written following in italic)
0x57wa 0xFFa 0x57ra 0xFFa

If the function would only writes an 0x57ra on the bus then it would work
on the bus: 0x57ra 0x37a
w: write, r: read, a: acknowledge

@nfstoney
Copy link
Author

Do you have access to a raspberry pi4 hardware?
So you could plug this together with an external EEPROM.

@landley
Copy link
Owner

landley commented Mar 17, 2023

"Does it make sense to invent the wheel twice." If it didn't Linux wouldn't exist because BSD already existed.

You complained: "The message after i2cget -y 1 0x57 0x00 0x40" was "Max 3 arguments", and now you're saying that's an i2cset command line. (i2cset already takes 4 arguments.)

I'm confused.

@jmakip
Copy link
Contributor

jmakip commented Mar 17, 2023

I think user here wants to use i2cget without giving command byte. So the device will spit out whatever is in its shift register. This can be done by simply reading opened i2c file instead of sending "command" with ioctl.

I send patch to mailing list

http://lists.landley.net/pipermail/toybox-landley.net/2023-March/029502.html

I tested this briefly by reading EDID info out my monitor. Seems to behave more like i2cget i have installed on my distro now.

@nfstoney
Copy link
Author

I recommend you to get a harware (e.g. EEPROM 24FC128 from Microchip) and test it yourself.
You will be able to read and write the first 255 bytes successfully with toybox.
From byte 256 you will have the same problems as I described.

By the way with reading EDID toybox will always work. They only have 128 bytes to read.

@landley
Copy link
Owner

landley commented Mar 21, 2023

Did commit ba5b7c2 help?

@nfstoney
Copy link
Author

Looks like a step forward.

Measured on the bus with the oscilloscope now looks correct.

as answer I get in each case
i2cset -y 1 0x57 0x00 0x40 i
return: nothing
i2cget -y 1 0x57
return: 0xb6d89e37
i2cget -y 1 0x57
return: 0xb6d0ee34
i2cget -y 1 0x57
return: 0xb6d0de3a
i2cget -y 1 0x57
return: 0xb6d80e32

The last byte of the return it is the value in the cell. I don't know what the other 3 bytes before are.
On the bus there is always the value un the cell. The other three bytes must be add from the code. If I repeat this sequence, the 3 bytes before aren't the same. Also not on the same address. The value in the cell is always correct.

My cells are:
address / value
0x0040: 37
0x0041: 34
0x0042: 3A
0x0043: 32

I testes also addresse over 256. Works as well.

Good job.

I could already live with this solution. However, it would be trustworthy to know what the three bytes before the cell content are and if they are really necessary.

@enh-google
Copy link
Collaborator

i suspect it's just uninitialized memory from:

  int fd, byte;
...
  } else if (read(fd, &byte, 1) != 1) perror_exit("i2c_read");

  printf("0x%02x\n", byte);

@jmakip
Copy link
Contributor

jmakip commented Mar 21, 2023

Yes integer sized byte variable is most likely containing some extra garbage in some cases.

There seems to be also some argument parsing issues if

Artifact build with this
make i2cget

./i2cget -y 3 0x50
Read register 0xffffffff from chip 0x50 on bus 3? (Y/n):Y
0x20

And this
make defconfig
make
ln -s ./toybox i2cget

./i2cget -y 3 0x50
0x5620

behave differently

@landley
Copy link
Owner

landley commented Mar 21, 2023

Sigh. The reason I haven't given this one more thorough review/cleanup is I'm afraid to introduce regressions without a proper test environment.

I'll try to fix this up after lunch. (The argument parsing is missing FORCE_FLAGS: confirm() lives in i2cdetect's flag context but when you only build i2cget standalone then i2cdetext=n which means all its FLAG_y macros are constant 0, which lets the dead code eliminator drop out code that's not used in the current build. FORCE_FLAGS diables that...)

@landley
Copy link
Owner

landley commented Mar 21, 2023

Give commit 1819be9 a try?

@nfstoney
Copy link
Author

Give commit 1819be9 a try?

Works as I expect.

@landley landley closed this as completed Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants