Thanks for your remove and comments. Dudley > -----Original Message----- > From: linux-input-owner@xxxxxxxxxxxxxxx > [mailto:linux-input-owner@xxxxxxxxxxxxxxx] On Behalf Of Jeremiah Mahler > Sent: 2014?12?13? 19:16 > To: Dudley Du > Cc: dmitry.torokhov@xxxxxxxxx; rydberg@xxxxxxxxxxx; bleung@xxxxxxxxxx; > linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v14 01/12] input: cyapa: re-design driver to support > multi-trackpad in one driver > > 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'? Thanks, I will rename it to 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); > } > > [...] Thanks, I will modify it based on your suggestion. > > -- > - Jeremiah Mahler > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
