Re: [PATCH v14 01/12] input: cyapa: re-design driver to support multi-trackpad in one driver
flat view
From: Jeremiah Mahler <hidden>
Date: 2014-12-13 11:16:22
Also in:
lkml
Dudley, A few suggestions and questions below... On Fri, Dec 12, 2014 at 10:27:31AM +0800, Dudley Du wrote:
In order to support multiple different chipsets and communication protocols trackpad devices in one cyapa driver, the new cyapa driver is re-designed with one cyapa driver core and multiple device specific functions component. The cyapa driver core is contained in this patch, it supplies basic functions that working with kernel and input subsystem, and also supplies the interfaces that the specific devices' component can connect and work together with as one driver. TEST=test on Chromebooks. Signed-off-by: Dudley Du <redacted> --- drivers/input/mouse/Makefile | 3 +- drivers/input/mouse/cyapa.c | 1050 ++++++++++++++------------------------ drivers/input/mouse/cyapa.h | 316 ++++++++++++
[...]
quoted hunk ↗ jump to hunk
- { REG_OFFSET_QUERY_BASE, QUERY_DATA_SIZE }, - { BL_HEAD_OFFSET, 3 }, - { BL_HEAD_OFFSET, 16 }, - { BL_HEAD_OFFSET, 16 }, - { BL_DATA_OFFSET, 16 }, - { BL_HEAD_OFFSET, 32 }, - { REG_OFFSET_QUERY_BASE, PRODUCT_ID_SIZE }, - { REG_OFFSET_DATA_BASE, 32 } -}; +const char unique_str[] = "CYTRA";
Since 'unique_str' is used to check the product id why not call it 'product_id'? [...]
quoted hunk ↗ jump to hunk
+/** + * cyapa_i2c_write - Execute i2c block data write operation + * @cyapa: Handle to this driver + * @ret: Offset of the data to written in the register map + * @len: number of bytes to write + * @values: Data to be written * - * Note: - * In trackpad device, the memory block allocated for I2C register map - * is 256 bytes, so the max read block for I2C bus is 256 bytes. + * Return negative errno code on error; return zero when success. */ -static ssize_t cyapa_smbus_read_block(struct cyapa *cyapa, u8 cmd, size_t len, - u8 *values) +ssize_t cyapa_i2c_write(struct cyapa *cyapa, u8 reg, + size_t len, const void *values) { - ssize_t ret; - u8 index; - u8 smbus_cmd; - u8 *buf; + int ret; struct i2c_client *client = cyapa->client; + char data[32], *buf; - if (!(SMBUS_BYTE_BLOCK_CMD_MASK & cmd)) - return -EINVAL; - - if (SMBUS_GROUP_BLOCK_CMD_MASK & cmd) { - /* read specific block registers command. */ - smbus_cmd = SMBUS_ENCODE_RW(cmd, SMBUS_READ); - ret = i2c_smbus_read_block_data(client, smbus_cmd, values); - goto out; - } - - ret = 0; - for (index = 0; index * I2C_SMBUS_BLOCK_MAX < len; index++) { - smbus_cmd = SMBUS_ENCODE_IDX(cmd, index); - smbus_cmd = SMBUS_ENCODE_RW(smbus_cmd, SMBUS_READ); - buf = values + I2C_SMBUS_BLOCK_MAX * index; - ret = i2c_smbus_read_block_data(client, smbus_cmd, buf); - if (ret < 0) - goto out; - } - -out: - return ret > 0 ? len : ret; -} - -static s32 cyapa_read_byte(struct cyapa *cyapa, u8 cmd_idx) -{ - u8 cmd; - - if (cyapa->smbus) { - cmd = cyapa_smbus_cmds[cmd_idx].cmd; - cmd = SMBUS_ENCODE_RW(cmd, SMBUS_READ); + if (len > 31) { + buf = kzalloc(len + 1, GFP_KERNEL); + if (!buf) + return -ENOMEM; } else { - cmd = cyapa_i2c_cmds[cmd_idx].cmd; + buf = data; } - return i2c_smbus_read_byte_data(cyapa->client, cmd); -} -static s32 cyapa_write_byte(struct cyapa *cyapa, u8 cmd_idx, u8 value) -{ - u8 cmd; + buf[0] = reg; + memcpy(&buf[1], values, len); + ret = i2c_master_send(client, buf, len + 1); - if (cyapa->smbus) { - cmd = cyapa_smbus_cmds[cmd_idx].cmd; - cmd = SMBUS_ENCODE_RW(cmd, SMBUS_WRITE); - } else { - cmd = cyapa_i2c_cmds[cmd_idx].cmd; - } - return i2c_smbus_write_byte_data(cyapa->client, cmd, value); + if (buf != data) + kfree(buf); + return (ret == (len + 1)) ? 0 : ((ret < 0) ? ret : -EIO); }
Ugh. This is hard to read since diff replaced three functions with one,
cyapa_i2c_write(). Nothing can be done about this, just a note for the
other reviewers.
The final cyapa_i2c_write() is shown below.
/**
* cyapa_i2c_write - Execute i2c block data write operation
* @cyapa: Handle to this driver
* @ret: Offset of the data to written in the register map
* @len: number of bytes to write
* @values: Data to be written
*
* Return negative errno code on error; return zero when success.
*/
ssize_t cyapa_i2c_write(struct cyapa *cyapa, u8 reg,
size_t len, const void *values)
{
int ret;
struct i2c_client *client = cyapa->client;
char data[32], *buf;
if (len > 31) {
buf = kzalloc(len + 1, GFP_KERNEL);
if (!buf)
return -ENOMEM;
} else {
buf = data;
}
buf[0] = reg;
memcpy(&buf[1], values, len);
ret = i2c_master_send(client, buf, len + 1);
if (buf != data)
kfree(buf);
return (ret == (len + 1)) ? 0 : ((ret < 0) ? ret : -EIO);
}
What is interesting about this code is that it switches between buffers
depending on length. If it is less than or equal to 31 bytes a static
buffer is used. If it is larger, memory is allocated.
Is this complexity justified or is this premature optimization?
I could only find one place where cyapa_i2c_write() was used and it only
involved 2 bytes so 32 is plenty.
Why not simplify it and only use the static buffer and just reject
anything that is too large?
ssize_t cyapa_i2c_write(struct cyapa *cyapa, u8 reg,
size_t len, const void *values)
{
int ret;
struct i2c_client *client = cyapa->client;
char buf[32];
if (len > 31)
return -ENOMEM;
buf[0] = reg;
memcpy(&buf[1], values, len);
ret = i2c_master_send(client, buf, len + 1);
return (ret == (len + 1)) ? 0 : ((ret < 0) ? ret : -EIO);
}
[...]
--
- Jeremiah Mahler