On Fri, Aug 31, 2018 at 12:41 AM Ajay Gupta <ajayg@xxxxxxxxxx> wrote:
> Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
> over I2C interface.
>
> This UCSI I2C driver uses I2C bus driver interface for communicating
> with Type-C controller.
> Changes from v3 -> v4
> Fixed comments from Andy
Unfortunatelly not all comments was addressed, see below.
> + if (err == ARRAY_SIZE(msgs)) {
> + err = 0;
> + } else if (err >= 0) {
> + dev_err(dev, "i2c_transfer failed, err %d\n", err);
> + return -EIO;
> + }
Shouldn't be simple
if (err < 0) {
...
return err;
}
?
> + if (err == ARRAY_SIZE(msgs)) {
> + err = 0;
> + } else if (err >= 0) {
> + dev_err(dev, "i2c_transfer failed, err %d\n", err);
> + return -EIO;
> + }
Ditto.
> + struct device *dev = uc->dev;
> + u8 data[64];
> + int err;
> + unsigned int count = 10;
Move this line upper a bit.
> + unsigned char buf[3] = {
> + CCGX_I2C_RAB_INTR_REG & 0xff, CCGX_I2C_RAB_INTR_REG >> 8, 0x0};
This should follow the style you applied earlier in the same file.
Also, & 0xff is redundant (better just to use >> 0).
> + struct ucsi_ccg *uc = container_of(ppm,
> + struct ucsi_ccg, ppm);
One line?
> +static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control *ctrl)
> +{
> + struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
> + int err = 0;
Redundant assignment.
> +
> + ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
> + err = ucsi_ccg_send_data(uc);
> +
> + return err;
> +}
> +static int ucsi_ccg_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
One line?
> +static const struct i2c_device_id ucsi_ccg_device_id[] = {
> + {"ccgx-ucsi", 0},
> + {},
Terminator better w/o comma.
> +};
> +MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
--
With Best Regards,
Andy Shevchenko
