i2c_master: Add support for reading/writing to 16-bit registers

[fauxpark]

Description

I2C itself has no concept of registers, but for devices that do specify the first two bytes of a given transaction as the "register address", this should provide an easier to use interface than splitting the address up manually into the regaddr parameter and the first actual data byte.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[drashna]

I definitely don't like this.

But part of that is because I think it has already been implemented using i2c already, elsewhere.

Eg, for the cirque trackpad that I've been working on:

/*  ERA (Extended Register Access) Functions  */
// Reads <count> bytes from an extended register at <address> (16-bit address),
// stores values in <*data>
void ERA_ReadBytes(uint16_t address, uint8_t* data, uint16_t count) {
    uint8_t ERAControlValue = 0xFF;

    Pinnacle_EnableFeed(false);  // Disable feed

    RAP_Write(ERA_HIGH_BYTE, (uint8_t)(address >> 8));     // Send upper byte of ERA address
    RAP_Write(ERA_LOW_BYTE, (uint8_t)(address & 0x00FF));  // Send lower byte of ERA address

    for (uint16_t i = 0; i < count; i++) {
        RAP_Write(ERA_CONTROL, 0x05);  // Signal ERA-read (auto-increment) to Pinnacle

        // Wait for status register 0x1E to clear
        do {
            RAP_ReadBytes(ERA_CONTROL, &ERAControlValue, 1);
        } while (ERAControlValue != 0x00);

        RAP_ReadBytes(ERA_VALUE, data + i, 1);

        Pinnacle_ClearFlags();
    }
}

// Writes a byte, <data>, to an extended register at <address> (16-bit address)
void ERA_WriteByte(uint16_t address, uint8_t data) {
    uint8_t ERAControlValue = 0xFF;

    Pinnacle_EnableFeed(false);  // Disable feed

    RAP_Write(ERA_VALUE, data);  // Send data byte to be written

    RAP_Write(ERA_HIGH_BYTE, (uint8_t)(address >> 8));     // Upper byte of ERA address
    RAP_Write(ERA_LOW_BYTE, (uint8_t)(address & 0x00FF));  // Lower byte of ERA address

    RAP_Write(ERA_CONTROL, 0x02);  // Signal an ERA-write to Pinnacle

    // Wait for status register 0x1E to clear
    do {
        RAP_ReadBytes(ERA_CONTROL, &ERAControlValue, 1);
    } while (ERAControlValue != 0x00);

    Pinnacle_ClearFlags();
}

/*  RAP Functions */
// Reads <count> Pinnacle registers starting at <address>
void RAP_ReadBytes(uint8_t address, uint8_t* data, uint8_t count) {
    uint8_t cmdByte = READ_MASK | address;  // Form the READ command byte
    // uint8_t i       = 0;
    if (touchpad_init[0]) {
        i2c_writeReg(SLAVE_ADDR << 1, cmdByte, NULL, 0, I2C_TIMEOUT);
        if (i2c_readReg(SLAVE_ADDR << 1, cmdByte, data, count, I2C_TIMEOUT) != I2C_STATUS_SUCCESS) {
            dprintf("error right touchpad\n");
            touchpad_init[0] = false;
        }
        i2c_stop();
    }

    if (touchpad_init[1]) {
        i2c2_writeReg(SLAVE_ADDR << 1, cmdByte, NULL, 0, I2C_TIMEOUT);
        if (i2c2_readReg(SLAVE_ADDR << 1, cmdByte, data, count, I2C_TIMEOUT) != I2C_STATUS_SUCCESS) {
            dprintf("error left touchpad\n");
            touchpad_init[1] = false;
        }
        i2c2_stop();
    }
}

// Writes single-byte <data> to <address>
void RAP_Write(uint8_t address, uint8_t data) {
    uint8_t cmdByte = WRITE_MASK | address;  // Form the WRITE command byte

    if (touchpad_init[0]) {
        if (i2c_writeReg(SLAVE_ADDR << 1, cmdByte, &data, sizeof(data), I2C_TIMEOUT) != I2C_STATUS_SUCCESS) {
            dprintf("error right touchpad\n");
            touchpad_init[0] = false;
        }
        i2c_stop();
    }
    if (address == FEEDCONFIG_1 && data == FEEDCONFIG_1_VALUE) {
        data = 0xC3;
    }
    if (touchpad_init[1]) {
        if (i2c2_writeReg(SLAVE_ADDR << 1, cmdByte, &data, sizeof(data), I2C_TIMEOUT) != I2C_STATUS_SUCCESS) {
            dprintf("error left touchpad\n");
            touchpad_init[1] = false;
        }
        i2c2_stop();
    }
}

[fauxpark]

I'm not sure what your point is here. Yes, you could build on the existing API to "emulate" 16-bit register access (which this PR is doing too), but then you need to do that for every such device driver. Why not just have it in core? It won't be compiled in if it's not used.

[sigprof]

More importantly, it seems that the cirque trackpad just has some features so that it could work even when the I2C master cannot do 16-bit addressing; however, the trackpad that inspired this PR apparently does not have any such features.

Maybe it could even make sense to expose the underlying API in a more general form, allowing arbitrary sizes for both phases:

i2c_status_t i2c_transaction(uint8_t devaddr, const uint8_t *txdata, uint16_t txsize, uint8_t *rxdata, uint16_t rxsize, uint16_t timeout);

[purdeaandrei]

Looks good to me

[fauxpark]

@purdeaandrei did you have anything else to add on this?

[purdeaandrei]

@purdeaandrei did you have anything else to add on this?

Nope, looks good to me

[sigprof]

Looks good, but I don't have any hardware to verify this.