Re: [PATCH v4 2/2] usb: typec: ucsi: Add ITE IT8851/IT8853 Type-C PD controller driver

[Heikki Krogerus]

Hi Amber,

This looks okay otherwise, but I have to ask about the i2c reads and
writes..

> +static int itepd_read_reg(struct itepd *itepd, u8 reg, void *data, u32 len)
> +{
> +	struct i2c_client *client = itepd->client;
> +	struct i2c_msg msg[2];
> +	u8 *tx_buf, *rx_buf;
> +	int ret;
> +
> +	tx_buf = kzalloc(1, GFP_KERNEL);
> +	if (!tx_buf)
> +		return -ENOMEM;

That looks completely unnecessary. You really don't need to allocate
local buffer for one byte.

> +	rx_buf = kzalloc(len, GFP_KERNEL);
> +	if (!rx_buf) {
> +		kfree(tx_buf);
> +		return -ENOMEM;
> +	}

I'm not sure about this either.

> +	tx_buf[0] = reg;
> +
> +	msg[0].addr	= client->addr;
> +	msg[0].flags	= 0;
> +	msg[0].len	= 1;
> +	msg[0].buf	= tx_buf;
> +
> +	msg[1].addr	= client->addr;
> +	msg[1].flags	= I2C_M_RD;
> +	msg[1].len	= len;
> +	msg[1].buf	= rx_buf;
> +
> +	mutex_lock(&itepd->i2c_lock);
> +	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> +	mutex_unlock(&itepd->i2c_lock);
> +	if (ret < 0) {
> +		dev_err_ratelimited(&client->dev, "reg 0x%02x read failed: %d\n",
> +				    reg, ret);
> +		goto out_free;
> +	}
> +	if (ret != ARRAY_SIZE(msg)) {
> +		ret = -EIO;
> +		goto out_free;
> +	}
> +
> +	memcpy(data, rx_buf, len);

This whole thing looks almost like SMBus Block Read protocol. Can't
you just use
i2c_smbus_read_i2c_block_data(client, reg, len, data)?

> +	ret = 0;
> +
> +out_free:
> +	kfree(rx_buf);
> +	kfree(tx_buf);
> +	return ret;
> +}
> +
> +static int itepd_write_reg(struct itepd *itepd, u8 reg, const void *data, u32 len)
> +{
> +	struct i2c_client *client = itepd->client;
> +	struct i2c_msg msg[1];
> +	u8 *tx_buf;
> +	int ret;
> +
> +	tx_buf = kzalloc(len + 1, GFP_KERNEL);
> +	if (!tx_buf)
> +		return -ENOMEM;
> +
> +	tx_buf[0] = reg;
> +	memcpy(tx_buf + 1, data, len);
> +
> +	msg[0].addr	= client->addr;
> +	msg[0].flags	= 0;
> +	msg[0].len	= len + 1;
> +	msg[0].buf	= tx_buf;
> +
> +	mutex_lock(&itepd->i2c_lock);
> +	ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> +	mutex_unlock(&itepd->i2c_lock);

i2c_smbus_write_i2c_block_data(client, reg, len, data)?

> +	if (ret < 0) {
> +		dev_err_ratelimited(&client->dev, "reg 0x%02x write failed: %d\n",
> +				    reg, ret);
> +		goto out_free;
> +	}
> +	if (ret != ARRAY_SIZE(msg)) {
> +		ret = -EIO;
> +		goto out_free;
> +	}
> +
> +	ret = 0;
> +
> +out_free:
> +	kfree(tx_buf);
> +	return ret;
> +}

Thanks,

-- 
heikki