Hi Wolfram,
Thanks for your review.
> From: Wolfram Sang <wsa@xxxxxxxxxx>
>
> 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.
Ack, thanks
>
> > + 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.
Ack, thanks
>
> ...
>
> > +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.
Ack, thanks
>
> ...
>
> > +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')?
Thanks, we just suppose it support SMBUS function after reviewed the
core code, but never try the SMBUS functionality, so I decide to remove
the I2C_FUNC_SMBUS_EMUL support currently and after we verify it working
correctly, we will add it in another patch.
Thanks
>
> ...
>
> > +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?
Actually it's about the method 3 of instantiate I2C devices described in
https://www.kernel.org/doc/Documentation/i2c/instantiating-devices
and it's not preferred wherever possible. And below are the definitions
about i2c adapter class, we only have I2C_CLASS_HWMON class tested,
so just put it here.
/* i2c adapter classes (bitmask) */
#define I2C_CLASS_HWMON (1<<0) /* lm_sensors, ... */
#define I2C_CLASS_DDC (1<<3) /* DDC bus on graphics adapters */
#define I2C_CLASS_SPD (1<<7) /* Memory modules */
/* Warn users that the adapter doesn't support classes anymore */
#define I2C_CLASS_DEPRECATED (1<<8)
BR,
Wentong
> I guess it makes sense for USB, just wanted to
> ask.
>
> Other than that, it looks good!
>
> All the best,
>
> Wolfram
