RE: [PATCH v5 2/2] usb: typec: ucsi: add support for Cypress CCGx

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

 



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



[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