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

[Wolfram Sang <wsa@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3141 bytes --]

Hi,

thank you for your patches!

...

> +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;
> +
> +	w_packet->id = ljca_i2c->i2c_info->id;
> +	w_packet->len = cpu_to_le16(sizeof(*w_packet->data));
> +	w_packet->data[0] = (slave_addr << 1) | 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 || ret < sizeof(*r_packet))
> +		return ret < 0 ? ret : -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);

All dev_err look more like dev_dbg to me. They are not helpful for the
regular user, I'd think.

> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +

...

> +static int ljca_i2c_pure_read(struct ljca_i2c_dev *ljca_i2c, u8 *data, u8 len)
> +{
> +	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;
> +
> +	if (len > LJCA_I2C_MAX_XFER_SIZE)
> +		return -EINVAL;

You can remove this check. You already have a quirk structure, so the
core will do it for you.

...

> +static int ljca_i2c_pure_write(struct ljca_i2c_dev *ljca_i2c, u8 *data, u8 len)
> +{
> +	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 rplen;
> +	int ret;
> +
> +	if (len > LJCA_I2C_MAX_XFER_SIZE)
> +		return -EINVAL;

You can remove this check. You already have a quirk structure, so the
core will do it for you.

...

> +static u32 ljca_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}

Have you successfully tried SMBUS_QUICK (e.g. with scanning a bus with
'i2cdetect')?

...
 
> +static int ljca_i2c_probe(struct auxiliary_device *auxdev,
> +			  const struct auxiliary_device_id *aux_dev_id)
> +{
> +	struct ljca_client *ljca = auxiliary_dev_to_ljca_client(auxdev);
> +	struct ljca_i2c_dev *ljca_i2c;
> +	int ret;
> +
> +	ljca_i2c = devm_kzalloc(&auxdev->dev, sizeof(*ljca_i2c), GFP_KERNEL);
> +	if (!ljca_i2c)
> +		return -ENOMEM;
> +
> +	ljca_i2c->ljca = ljca;
> +	ljca_i2c->i2c_info = dev_get_platdata(&auxdev->dev);
> +
> +	ljca_i2c->adap.owner = THIS_MODULE;
> +	ljca_i2c->adap.class = I2C_CLASS_HWMON;

Just to make sure: you want class based instantiation here because you
have no other way of describing clients? I guess it makes sense for USB,
just wanted to ask.

Other than that, it looks good!

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]