On Tue, Oct 07, 2014 at 05:06:47PM -0700, Feng Kan wrote:
> Add SLIMpro I2C device driver on APM X-Gene platform. This I2C
> device driver use the SLIMpro Mailbox driver to tunnel message to
> the SLIMpro coprocessor to do the work of accessing I2C components.
>
> Signed-off-by: Feng Kan <fkan@xxxxxxx>
> Signed-off-by: Hieu Le <hnle@xxxxxxx>
Thank you for this submission!
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2e45ae3..a03042c 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL
> connected there. This will work whatever the interface used to
> talk to the EC (SPI, I2C or LPC).
>
> +config I2C_XGENE_SLIMPRO
> + tristate "APM X-Gene SoC I2C SLIMpro devices support"
> + depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX
> + help
> + Enable I2C bus access using the APM X-Gene SoC SLIMpro
> + co-processor. The I2C device access the I2C bus via the X-Gene
> + to SLIMpro (On chip coprocessor) mailbox mechanism.
> + If unsure, say N.
> +
I guess this is a driver for embedded? If so, please sort it properly.
> config SCx200_ACB
> tristate "Geode ACCESS.bus support"
> depends on X86_32 && PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 49bf07e..22b5f0c 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_I2C_CROS_EC_TUNNEL) += i2c-cros-ec-tunnel.o
> obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o
> obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o
> obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o
> +obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
Ditto.
> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
>
> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
> new file mode 100644
> index 0000000..2cf6c33
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
> @@ -0,0 +1,531 @@
> +/*
> + * X-Gene SLIMpro I2C Driver
> + *
> + * Copyright (c) 2014, Applied Micro Circuits Corporation
> + * Author: Feng Kan <fkan@xxxxxxx>
> + * Author: Hieu Le <hnle@xxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
The license preamble and MODULE_LICENSE do not match!
> +#include <linux/module.h>
> +#include <linux/version.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/i2c.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/delay.h>
> +#include <linux/spinlock.h>
> +#include <linux/interrupt.h>
> +#include <linux/dma-mapping.h>
Please sort the includes.
> +
> +#define XGENE_SLIMPRO_I2C "xgene-slimpro-i2c"
Only used once, not really needed
> +
> +#define SLIMPRO_I2C_WAIT_COUNT 10000
> +
> +#define SLIMPRO_OP_TO_MS 1000 /* Operation time out in ms */
*_TOUT_* or *_TIMEOUT_*. TO sounds like a conversion, from op to ms :)
> +#define SLIMPRO_IIC_BUS 1
What does this '1' mean?
> +static int start_i2c_msg_xfer(struct slimpro_i2c_dev *ctx)
> +{
> + int count;
> + unsigned long flags;
> +
> + if (ctx->mbox_client.tx_block) {
> + if (!wait_for_completion_timeout(&ctx->rd_complete,
> + msecs_to_jiffies
> + (SLIMPRO_OP_TO_MS)))
> + return -EIO;
That should be -ETIMEDOUT, no?
> + } else {
> + spin_lock_irqsave(&ctx->lock, flags);
> + ctx->i2c_rx_poll = 1;
> + for (count = SLIMPRO_I2C_WAIT_COUNT; count > 0; count--) {
> + if (ctx->i2c_rx_poll == 0)
> + break;
> + udelay(100);
> + }
> +
> + if (count == 0) {
> + ctx->i2c_rx_poll = 0;
> + ctx->mbox_client.tx_block = true;
> + spin_unlock_irqrestore(&ctx->lock, flags);
> + return -EIO;
ditto.
> + }
> + ctx->mbox_client.tx_block = true;
> + spin_unlock_irqrestore(&ctx->lock, flags);
> + }
> +
> + /* Check of invalid data or no device */
> + if (*ctx->resp_msg == 0xffffffff)
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr,
> + u32 addrlen, u32 protocol, u32 readlen,
> + u32 with_data_len, void *data)
> +{
> + dma_addr_t paddr;
> + u32 msg[3];
> + int rc;
> +
> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen,
> + DMA_FROM_DEVICE);
> + if (dma_mapping_error(ctx->dev, paddr)) {
> + dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n",
> + ctx->dma_buffer);
> + rc = -EIO;
Return the err value from dma_mapping_error?
> + goto err;
> + }
> +
> + if (irqs_disabled())
> + ctx->mbox_client.tx_block = false;
> +
> + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip,
> + SLIMPRO_IIC_READ, protocol, addrlen,
> + readlen);
> + msg[1] = SLIMPRO_IIC_ENCODE_FLAG_BUFADDR |
> + SLIMPRO_IIC_ENCODE_FLAG_WITH_DATA_LEN(with_data_len) |
> + SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(paddr) |
> + SLIMPRO_IIC_ENCODE_ADDR(addr);
> + msg[2] = (u32) paddr;
> + ctx->resp_msg = msg;
> +
> + if (ctx->mbox_client.tx_block)
> + init_completion(&ctx->rd_complete);
reinit_completion? and init_completion in probe?
> +
> + rc = mbox_send_message(ctx->mbox_chan, &msg);
> + if (rc < 0)
> + goto err_unmap;
> +
> + rc = start_i2c_msg_xfer(ctx);
> +
> + /* Copy to destination */
> + memcpy(data, ctx->dma_buffer, readlen);
> +
> +err_unmap:
> + dma_unmap_single(ctx->dev, paddr, readlen, DMA_FROM_DEVICE);
> +err:
> + ctx->resp_msg = NULL;
> + return rc;
> +}
> +
> +static int slimpro_i2c_blkwr(struct slimpro_i2c_dev *ctx, u32 chip,
> + u32 addr, u32 addrlen, u32 protocol, u32 writelen,
> + void *data)
> +{
> + dma_addr_t paddr;
> + u32 msg[3];
> + int rc;
> +
> + memcpy(ctx->dma_buffer, data, writelen);
> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, writelen,
> + DMA_TO_DEVICE);
> + if (dma_mapping_error(ctx->dev, paddr)) {
> + dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n",
> + ctx->dma_buffer);
> + rc = -EIO;
same as above
> + goto err;
> + }
> +
> + if (irqs_disabled())
> + ctx->mbox_client.tx_block = false;
> +
> + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip,
> + SLIMPRO_IIC_WRITE, protocol, addrlen,
> + writelen);
> + msg[1] = SLIMPRO_IIC_ENCODE_FLAG_BUFADDR |
> + SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(paddr) |
> + SLIMPRO_IIC_ENCODE_ADDR(addr);
> + msg[2] = (u32) paddr;
> + ctx->resp_msg = msg;
> +
> + if (ctx->mbox_client.tx_block)
> + init_completion(&ctx->rd_complete);
ditto
> +
> + rc = mbox_send_message(ctx->mbox_chan, &msg);
> + if (rc < 0)
> + goto err_unmap;
> +
> + rc = start_i2c_msg_xfer(ctx);
> +
> +err_unmap:
> + dma_unmap_single(ctx->dev, paddr, writelen, DMA_TO_DEVICE);
> +err:
> + ctx->resp_msg = NULL;
> + return rc;
> +}
> +
> +static struct platform_driver xgene_slimpro_i2c_driver = {
> + .probe = xgene_slimpro_i2c_probe,
> + .remove = xgene_slimpro_i2c_remove,
> + .driver = {
> + .name = XGENE_SLIMPRO_I2C,
> + .owner = THIS_MODULE,
Not needed.
> + .of_match_table = of_match_ptr(xgene_slimpro_i2c_id)
> + },
> +};
> +
> +module_platform_driver(xgene_slimpro_i2c_driver);
> +
> +MODULE_DESCRIPTION("APM X-Gene SLIMpro I2C driver");
> +MODULE_LICENSE("GPL v2");
So, only minor stuff. Looking good already to me.
Thanks,
Wolfram
Attachment:
signature.asc
Description: Digital signature
