Re: [PATCH v14 01/12] input: cyapa: re-design driver to support multi-trackpad in one driver
From: Jeremiah Mahler
Date: Sat Dec 13 2014 - 06:16:25 EST
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 <dudley.dulixin@xxxxxxxxx>
> ---
> drivers/input/mouse/Makefile | 3 +-
> drivers/input/mouse/cyapa.c | 1050 ++++++++++++++------------------------
> drivers/input/mouse/cyapa.h | 316 ++++++++++++
[...]
> - { 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'?
[...]
> +/**
> + * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/