Hi Andi,
On 2023/6/9 20:24, Andi Shyti wrote:
Hi Hans,
+static int zxi2c_byte_xfer(struct zxi2c *i2c, struct i2c_msg *msgs, int num)
+{
+ u16 i, finished;
+ u8 index;
+ int ret = 0;
+ struct i2c_msg *msg;
+ void __iomem *regs = i2c->regs;
+
+ zxi2c_en_byte_mode(regs);
+ zxi2c_enable_irq(regs, ZXI2C_EN_BYTEEND, true);
+
+ for (index = 0; index < num; index++) {
+ int err = 0;
+
+ msg = msgs + index;
+ i2c->addr = msg->addr;
+ i2c->is_read = !!(msg->flags & I2C_M_RD);
+ i2c->byte_left = i2c->len = msg->len;
+
+ zxi2c_set_wr(regs, i2c->is_read);
+ if (i2c->is_read) {
+ zxi2c_prepare_next_read(regs, i2c->byte_left);
+ zxi2c_start(i2c);
+ /* create restart for non-first msg */
+ if (index)
+ zxi2c_continue(i2c);
+
+ for (i = 1; i <= msg->len; i++) {
+ err = zxi2c_wait_event(i2c, ZXI2C_STS_BYTEEND);
+ if (err)
+ break;
+
+ msg->buf[i - 1] = zxi2c_get_byte(regs);
why don't you start the for loop from '0' to avoid "i - 1"?
Introduced during early debugging, will change in v5
+ if (i2c->byte_left == 0)
+ break;
+
+ zxi2c_prepare_next_read(regs, i2c->byte_left);
+ zxi2c_continue(i2c);
+ }
+ } else {
+ zxi2c_set_byte(regs, msg->buf[0]);
+ /* mark whether this is the last msg */
+ i2c->is_last_msg = index == !!(num - 1);
+ zxi2c_start(i2c);
+ /* create restart for non-first msg */
+ if (index)
+ zxi2c_continue(i2c);
+
+ for (i = 1; i <= msg->len; i++) {
+ err = zxi2c_wait_event(i2c, ZXI2C_STS_BYTEEND);
+ if (err)
+ break;
+
+ if (i2c->byte_left == 0)
+ break;
+ zxi2c_set_byte(regs, msg->buf[i]);
+ zxi2c_continue(i2c);
+ }
+ }
+
+ if (err) {
+ /* check if NACK during transmitting */
+ finished = msg->len - i2c->byte_left;
+ if (finished)
+ dev_err(i2c->dev,
+ "%s: %s finished %d bytes: %*ph\n",
+ __func__,
+ i2c->is_read ? "read" : "write",
+ finished, finished, msg->buf);
+ return err;
do you need to disable irq's here?
will change in v5
+ }
+ ret++;
+ }
+
+ zxi2c_enable_irq(regs, ZXI2C_EN_BYTEEND, false);
+ return ret;
+}
+
+static int zxi2c_fifo_xfer(struct zxi2c *i2c, struct i2c_msg *msgs)
+{
+ void __iomem *regs = i2c->regs;
+ struct i2c_msg *msg = msgs;
+ int i;
+ u8 finished;
+ int err;
+
+ i2c->addr = msg->addr;
+ i2c->is_read = !!(msg->flags & I2C_M_RD);
+ i2c->len = msg->len;
+
+ zxi2c_reset_fifo(i2c);
+ zxi2c_en_fifo_mode(regs);
+ zxi2c_enable_irq(regs, ZXI2C_EN_FIFOEND, true);
+
+ zxi2c_set_wr(regs, i2c->is_read);
+ if (i2c->is_read) {
+ zxi2c_set_fifo_rd_len(regs, msg->len - 1);
+ } else {
+ zxi2c_set_fifo_wr_len(regs, msg->len - 1);
+ for (i = 0; i < msg->len; i++)
+ zxi2c_set_fifo_byte(regs, msg->buf[i]);
+ }
+
+ zxi2c_start(i2c);
+ err = zxi2c_wait_event(i2c, ZXI2C_STS_FIFOEND);
+ if (err)
+ return err;
+
+ if (i2c->is_read) {
+ finished = zxi2c_get_fifo_rd_cnt(regs);
+ for (i = 0; i < finished; i++)
+ msg->buf[i] = zxi2c_get_fifo_byte(regs);
+ } else {
+ finished = zxi2c_get_fifo_wr_cnt(regs);
+ }
+
+ /* check if NACK during transmitting */
+ if (finished != msg->len) {
+ if (finished)
+ dev_err(i2c->dev,
+ "%s: %s only finished %d/%d bytes: %*ph\n",
+ __func__, i2c->is_read ? "read" : "write",
+ finished, msg->len, finished, msg->buf);
+ return -EAGAIN;
do you need to disable irq's here?
will change in v5
+ }
+
+ zxi2c_enable_irq(regs, ZXI2C_EN_FIFOEND, false);
+ return 1;
+}
+
+static int zxi2c_master_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ int err;
+ struct zxi2c *i2c = (struct zxi2c *)i2c_get_adapdata(adap);
+
+ if (!zxi2c_is_ready(i2c->regs)) {
one question: what are the cases that the controller is not
ready. What might the controller be doing now?
I am worried that the hardware state machine may make errors under the
influence
of the device. I hope to solve this possible problem through a reset
mechanism.
In fact, I have not come across such a case.
+ dev_warn(i2c->dev, "controller not ready\n");
+ zxi2c_module_reset(i2c);
+ if (!zxi2c_is_ready(i2c->regs)) {
+ dev_err(i2c->dev, "controller can't reset to ready\n");
+ return -EBUSY;
+ }
+ zxi2c_set_bus_speed(i2c);
+ }
+
+ if (num == 1 && msgs->len <= ZXI2C_FIFO_SIZE && msgs->len >= 3)
+ err = zxi2c_fifo_xfer(i2c, msgs);
+ else
+ err = zxi2c_byte_xfer(i2c, msgs, num);
+
+ return err;
+}
+
+static u32 zxi2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm zxi2c_algorithm = {
+ .master_xfer = zxi2c_master_xfer,
+ .functionality = zxi2c_func,
+};
+
+static const struct i2c_adapter_quirks zxi2c_quirks = {
+ .flags = I2C_AQ_NO_ZERO_LEN | I2C_AQ_COMB_WRITE_THEN_READ,
+};
+
+static int zxi2c_parse_resources(struct zxi2c *i2c)
+{
+ struct platform_device *pdev = to_platform_device(i2c->dev);
+ struct resource *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ i2c->regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(i2c->regs))
+ return PTR_ERR(i2c->regs);
+
+ if (res->start & 0x20)
do you mind giving this 0x20 a define?
will change in v5
+ i2c->reset_bitmask = BIT(4);
+ else
+ i2c->reset_bitmask = BIT(5);
I'm not strong on these, but would be nice to have also these two
bits defined, as well.
will change in v5.
+
+ i2c->irq = platform_get_irq(pdev, 0);
+ if (i2c->irq < 0)
+ return i2c->irq;
+
+ i2c->hrv = zxi2c_get_revison(i2c->regs);
+ i2c->dynamic = zxi2c_get_dyn_clk(i2c->regs);
+
+ i2c->fstp = zxi2c_get_fstp_value(i2c->regs);
+ zxi2c_get_bus_speed(i2c);
+ zxi2c_set_bus_speed(i2c);
+
+ return 0;
+}
+
+static int zxi2c_probe(struct platform_device *pdev)
+{
+ int err = 0;
+ struct zxi2c *i2c;
+ struct pci_dev *pci;
+ struct device *dev;
+
+ /* make sure this is zhaoxin platform */
Krzysztof is asking what does this mean? I guess here you are
checking if the device connected is really the zhaixin plaform of
this driver. Makes sense. Just to avoid misunderstandings we
could rephrase this with something like:
/*
* Check if vendor and device ID match the expected
* values for the zhaoxin platform
*/
would it work?
will drop this as Krzysztof suggested. In fact, it is almost impossible
for a non-ZXI2C device to report the HID name IIC1D17 through ACPI.
+ dev = pdev->dev.parent;
+ if (dev && dev_is_pci(dev)) {
+ pci = to_pci_dev(dev);
+ if (pci->vendor != PCI_VENDOR_ID_ZHAOXIN ||
+ pci->device != ZXI2C_PARENT_PCI_DID)
+ return -ENODEV;
+ } else {
+ return -ENODEV;
+ }
+
+ i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+ if (IS_ERR(i2c))
devm_kzalloc doesn't report an error, just check for NULL if (!i2c)
will change in v5.
will use dev_***_ratelimit() replace dev_***() in zxi2c_irq_handle() in v5
Thank you,
Hans
Rest looks good.
Thanks,
Andi
+ return -ENOMEM;
+
+ i2c->pci = pci;
+ i2c->dev = &pdev->dev;
+ err = zxi2c_parse_resources(i2c);
+ if (err)
+ return err;
+
+ platform_set_drvdata(pdev, (void *)i2c);
+
+ err = devm_request_irq(&pdev->dev, i2c->irq, zxi2c_irq_handle,
+ IRQF_SHARED, pdev->name, i2c);
+ if (err)
+ return err;
+
+ init_waitqueue_head(&i2c->waitq);
+
+ i2c->adap.owner = THIS_MODULE;
+ i2c->adap.algo = &zxi2c_algorithm;
+ i2c->adap.retries = 2;
+ i2c->adap.quirks = &zxi2c_quirks;
+ i2c->adap.dev.parent = &pdev->dev;
+ ACPI_COMPANION_SET(&i2c->adap.dev, ACPI_COMPANION(&pdev->dev));
+ snprintf(i2c->adap.name, sizeof(i2c->adap.name), "%s-%s-%s",
+ ZXI2C_NAME, dev_name(&pci->dev), dev_name(i2c->dev));
+ i2c_set_adapdata(&i2c->adap, i2c);
+
+ return devm_i2c_add_adapter(&pdev->dev, &i2c->adap);
+}
+
+static int zxi2c_resume(struct device *dev)
+{
+ struct zxi2c *i2c = dev_get_drvdata(dev);
+
+ zxi2c_module_reset(i2c);
+ zxi2c_set_bus_speed(i2c);
+
+ return 0;
+}
+
+static const struct dev_pm_ops zxi2c_pm = {
+ SET_SYSTEM_SLEEP_PM_OPS(NULL, zxi2c_resume)
+};
+
+static const struct acpi_device_id zxi2c_acpi_match[] = {
+ {"IIC1D17", 0},
+ {},
+};
+MODULE_DEVICE_TABLE(acpi, zxi2c_acpi_match);
+
+static struct platform_driver zxi2c_driver = {
+ .probe = zxi2c_probe,
+ .driver = {
+ .name = ZXI2C_NAME,
+ .acpi_match_table = zxi2c_acpi_match,
+ .pm = &zxi2c_pm,
+ },
+};
+module_platform_driver(zxi2c_driver);
+
+MODULE_AUTHOR("HansHu@xxxxxxxxxxx");
+MODULE_DESCRIPTION("Shanghai Zhaoxin IIC driver");
+MODULE_LICENSE("GPL");
--
2.17.1
