| Message ID | 1693091643-20867-1-git-send-email-wentong.wu@intel.com |
|---|---|
| Headers | show |
| Series | Add Intel LJCA device driver | expand |
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
Hi Wolfram, Thanks for your review. > From: Wolfram Sang <wsa@kernel.org> > > 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
> 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. Yeah, as I mentioned before, it may make sense for a USB based device. I just wanted to make sure you did it intentionally. Keep it.
> From: Wolfram Sang <wsa@kernel.org> > > > > 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. > > Yeah, as I mentioned before, it may make sense for a USB based device. I just > wanted to make sure you did it intentionally. Keep it. Thanks, please help review v11 patch set as well which is to address your comments. BR, Wentong