RE: [PATCH v9 2/4] i2c: Add support for Intel LJCA USB I2C driver

["Wu, Wentong" <wentong.wu@xxxxxxxxx>]

Hi Andi,

Thanks for your review. 

> From: Andi Shyti <andi.shyti@xxxxxxxxxx>
> 
> Hi Weontong,
> 
> patch looks good, just few minor comments.

Thanks, and below comments will be fixed in next version patch set.

BR,
Wentong

> 
> [...]
> 
> > +static u8 ljca_i2c_format_slave_addr(u8 slave_addr, u8 type) {
> > +	return (slave_addr << 1) | type;
> > +}
> 
> this is used only once, you could eventually ger rid of it and use explicitely in the
> start function.

Ack, thanks.

> 
> [...]
> 
> > +static int ljca_i2c_start(struct ljca_i2c_dev *ljca_i2c, u8 slave_addr,
> > +			  enum ljca_xfer_type type)
> > +{
> > +	struct ljca_i2c_rw_packet *w_packet =
> > +			(struct ljca_i2c_rw_packet *)ljca_i2c->obuf;
> > +	struct ljca_i2c_rw_packet *r_packet =
> > +			(struct ljca_i2c_rw_packet *)ljca_i2c->ibuf;
> > +	s16 rp_len;
> > +	int ret;
> > +
> > +	memset(w_packet, 0, sizeof(*w_packet));
> 
> do you really need to set this to '0' as you are initializing the header

Ack, thanks

> 
> > +	w_packet->id = ljca_i2c->i2c_info->id;
> > +	w_packet->len = cpu_to_le16(sizeof(*w_packet->data));
> > +	w_packet->data[0] = ljca_i2c_format_slave_addr(slave_addr, type);
> > +
> > +	ret = ljca_transfer(ljca_i2c->ljca, LJCA_I2C_START, w_packet,
> > +			    struct_size(w_packet, data, 1), r_packet,
> > +			    LJCA_I2C_BUF_SIZE);
> > +	if (ret < 0)
> > +		return ret;
> > +	else if (ret < sizeof(*r_packet))
> > +		return -EIO;
> > +
> > +	rp_len = le16_to_cpu(r_packet->len);
> > +	if (rp_len < 0 || r_packet->id != w_packet->id) {
> > +		dev_err(&ljca_i2c->adap.dev,
> > +			"i2c start failed len: %d id: %d %d\n",
> > +			rp_len, r_packet->id, w_packet->id);
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ljca_i2c_stop(struct ljca_i2c_dev *ljca_i2c, u8
> > +slave_addr)
> 
> the return value here is not used anywhere, what about making this function
> void and printing a warning in case of failure?

Ack, thanks

> 
> [...]
> 
> > +static int ljca_i2c_probe(struct auxiliary_device *auxdev,
> > +			  const struct auxiliary_device_id *aux_dev_id) {
> 
> [...]
> 
> > +	ret = ljca_i2c_init(ljca_i2c, ljca_i2c->i2c_info->id);
> > +	if (ret) {
> > +		dev_err(&auxdev->dev, "i2c init failed id: %d\n",
> > +			ljca_i2c->i2c_info->id);
> > +		return -EIO;
> 
> please use dev_err_probe();

Ack, thanks

> 
> Andi