RE: [PATCH v14 01/12] input: cyapa: re-design driver to support multi-trackpad in one driver

[Dudley Du]

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-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/