Re: [PATCH 4/6] i2c: busses: add SLIMpro I2C device driver on APM X-Gene platform

[Wolfram Sang]

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