Re: [PATCH v4 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux