Hi Greg,
> > +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> > +{
> > + struct device *dev = uc->dev;
> > + struct i2c_client *client = uc->client;
> > + unsigned char buf[2];
> > + struct i2c_msg msgs[] = {
> > + {
> > + .addr = client->addr,
> > + .flags = 0x0,
> > + .len = 0x2,
> > + .buf = buf,
> > + },
> > + {
> > + .addr = client->addr,
> > + .flags = I2C_M_RD,
> > + .buf = data,
> > + },
> > + };
>
> Are you sure you are allowed to do i2c messages off of the stack like this?
> Will that work on all platforms?
Most of the drivers in kernel today seem to use stack memory for
i2c_transfer() but I will fix it in next version.
> > + u32 rlen, rem_len = len;
> > + int err;
> > +
> > + while (rem_len > 0) {
> > + msgs[1].buf = &data[len - rem_len];
> > + rlen = min_t(u16, rem_len, 4);
> > + msgs[1].len = rlen;
> > + put_unaligned_le16(rab, buf);
> > + err = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > + if (err < 0) {
> > + dev_err(dev, "i2c_transfer failed, err %d\n", err);
>
> You are printing out an error here, no need to print out another error every
> time you call this function and it fails, right? Only do it in one place, otherwise
> it is extra noisy when things fail.
I will fix and add error print in only one location. I think better to keep it here
only and remove error print line from all the caller of this function.
> > + return err;
> > + }
> > + rab += rlen;
> > + rem_len -= rlen;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> > +{
> > + struct device *dev = uc->dev;
> > + struct i2c_client *client = uc->client;
> > + unsigned char buf[2];
> > + struct i2c_msg msgs[] = {
> > + {
> > + .addr = client->addr,
> > + .flags = 0x0,
> > + .len = 0x2,
> > + .buf = buf,
> > + },
> > + {
> > + .addr = client->addr,
> > + .flags = 0x0,
> > + .buf = data,
> > + .len = len,
> > + },
> > + {
> > + .addr = client->addr,
> > + .flags = I2C_M_STOP,
> > + },
> > + };
> > + int err;
>
> Same question here, i2c message off of the stack?
>
> > +
> > + put_unaligned_le16(rab, buf);
> > + err = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > + if (err < 0) {
> > + dev_err(dev, "i2c_transfer failed, err %d\n", err);
> > + return err;
>
> And again, only print an error in one place please.
Ok.
> This can be simplified to just:
> return i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); right?
Only issue is that i2c_transfer() return number of messages on success but ucsi
APIs sync() and cmd() at drivers/usb/typec/ucsi/ucsi.h expects return
value of '0' on success.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ucsi_ccg_init(struct ucsi_ccg *uc) {
> > + struct device *dev = uc->dev;
> > + unsigned int count = 10;
> > + u8 data[64];
> > + int err;
> > +
> > + /*
> > + * Selectively issue device reset
> > + * - if RESPONSE register is RESET_COMPLETE, do not issue device
> reset
> > + * (will cause usb device disconnect / reconnect)
> > + * - if RESPONSE register is not RESET_COMPLETE, issue device reset
> > + * (causes PPC to resync device connect state by re-issuing
> > + * set mux command)
> > + */
> > + data[0] = 0x00;
> > + data[1] = 0x00;
>
> Again, it's ok to do messages off of the stack?
Will fix.
Thanks
Ajay
--
nvpublic
--
>
> thanks,
>
> greg k-h
