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]

 



Hi Andy,
> > 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;
> }
> 
> ?
Ok, will fix
> 
> > +       if (err == ARRAY_SIZE(msgs)) {
> > +               err = 0;
> > +       } else if (err >= 0) {
> > +               dev_err(dev, "i2c_transfer failed, err %d\n", err);
> > +               return -EIO;
> > +       }
> 
> Ditto.
ok
> 
> > +       struct device *dev = uc->dev;
> > +       u8 data[64];
> > +       int err;
> 
> > +       unsigned int count = 10;
> 
> Move this line upper a bit.
ok
> 
> > +       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).
Ok, even ">> 0" will be redundant so will just drop  "& 0xff".

> > +       struct ucsi_ccg *uc = container_of(ppm,
> > +               struct ucsi_ccg, ppm);
> 
> One line?
ok
> 
> > +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.
ok
> 
> > +
> > +       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?
It crosses column width of 80. It takes total 85.

> 
> > +static const struct i2c_device_id ucsi_ccg_device_id[] = {
> > +       {"ccgx-ucsi", 0},
> 
> > +       {},
> 
> Terminator better w/o comma.
Ok.
> 
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
> 
Thanks
Ajay

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