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"?
> + 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?
> + }
> + 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?
> + }
> +
> + 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?
> + 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?
> + 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.
> +
> + 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?
> + 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)
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
>
