[PATCH 0/6] APM X-Gene platform mailbox and proxy i2c driver

[PATCH 0/6] APM X-Gene platform mailbox and proxy i2c driver

[Feng Kan]

This patch set contain the APM X-Gene platform mailbox driver and 
a proxy i2c driver that uses the mailbox driver to communicate with
the i2c bus through the slimpro coprocessor.

Feng Kan (6):
  mailbox: add support for APM X-Gene platform mailbox driver
  Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts
    documentation
  arm64: dts: mailbox device tree node for APM X-Gene platform.
  i2c: busses: add SLIMpro I2C device driver on APM X-Gene platform
  Documentation: i2c: Add APM X-Gene platform SLIMpro I2C driver
    documentation
  arm64: dts: add proxy I2C device driver on APM X-Gene platform

 .../devicetree/bindings/i2c/i2c-xgene-slimpro.txt  |  20 +
 .../bindings/mailbox/xgene-slimpro-mailbox.txt     |  41 ++
 arch/arm64/boot/dts/apm-storm.dtsi                 |  20 +
 drivers/i2c/busses/Kconfig                         |   9 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-xgene-slimpro.c             | 531 +++++++++++++++++++++
 drivers/mailbox/Kconfig                            |  10 +
 drivers/mailbox/Makefile                           |   1 +
 drivers/mailbox/mailbox-xgene-slimpro.c            | 287 +++++++++++
 9 files changed, 920 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
 create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
 create mode 100644 drivers/i2c/busses/i2c-xgene-slimpro.c
 create mode 100644 drivers/mailbox/mailbox-xgene-slimpro.c

-- 
1.9.1

[PATCH 0/6] APM X-Gene platform mailbox and proxy i2c driver

[Feng Kan]

This patch set contain the APM X-Gene platform mailbox driver and 
a proxy i2c driver that uses the mailbox driver to communicate with
the i2c bus through the slimpro coprocessor.

Feng Kan (6):
  mailbox: add support for APM X-Gene platform mailbox driver
  Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts
    documentation
  arm64: dts: mailbox device tree node for APM X-Gene platform.
  i2c: busses: add SLIMpro I2C device driver on APM X-Gene platform
  Documentation: i2c: Add APM X-Gene platform SLIMpro I2C driver
    documentation
  arm64: dts: add proxy I2C device driver on APM X-Gene platform

 .../devicetree/bindings/i2c/i2c-xgene-slimpro.txt  |  20 +
 .../bindings/mailbox/xgene-slimpro-mailbox.txt     |  41 ++
 arch/arm64/boot/dts/apm-storm.dtsi                 |  20 +
 drivers/i2c/busses/Kconfig                         |   9 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-xgene-slimpro.c             | 531 +++++++++++++++++++++
 drivers/mailbox/Kconfig                            |  10 +
 drivers/mailbox/Makefile                           |   1 +
 drivers/mailbox/mailbox-xgene-slimpro.c            | 287 +++++++++++
 9 files changed, 920 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
 create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
 create mode 100644 drivers/i2c/busses/i2c-xgene-slimpro.c
 create mode 100644 drivers/mailbox/mailbox-xgene-slimpro.c

-- 
1.9.1

[PATCH 1/6] mailbox: add support for APM X-Gene platform mailbox driver

[Feng Kan]

Add support for APM X-Gene platform mailbox driver.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 drivers/mailbox/Kconfig                 |  10 ++
 drivers/mailbox/Makefile                |   1 +
 drivers/mailbox/mailbox-xgene-slimpro.c | 287 ++++++++++++++++++++++++++++++++
 3 files changed, 298 insertions(+)
 create mode 100644 drivers/mailbox/mailbox-xgene-slimpro.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 9fd9c67..12ed5c1 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -33,4 +33,14 @@ config OMAP_MBOX_KFIFO_SIZE
 	  Specify the default size of mailbox's kfifo buffers (bytes).
 	  This can also be changed at runtime (via the mbox_kfifo_size
 	  module parameter).
+
+config XGENE_SLIMPRO_MBOX
+	tristate "APM SoC X-Gene SLIMpro Mailbox Controller"
+	depends on ARCH_XGENE
+	help
+	  An implementation of the APM X-Gene Interprocessor Communication
+	  Mailbox (IPCM) between the ARM 64-bit cores and SLIMpro controller.
+	  It is used to send short messages between ARM64-bit cores and
+	  the SLIMpro Management Engine, primarily for PM. Say Y here if you
+	  want to use the APM X-Gene SLIMpro IPCM support.
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 94ed7ce..33b2709 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -5,3 +5,4 @@ obj-$(CONFIG_MAILBOX)		+= mailbox.o
 obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
 
 obj-$(CONFIG_OMAP2PLUS_MBOX)	+= omap-mailbox.o
+obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
diff --git a/drivers/mailbox/mailbox-xgene-slimpro.c b/drivers/mailbox/mailbox-xgene-slimpro.c
new file mode 100644
index 0000000..ae46cc3
--- /dev/null
+++ b/drivers/mailbox/mailbox-xgene-slimpro.c
@@ -0,0 +1,287 @@
+/*
+ * APM X-Gene SLIMpro MailBox Driver
+ *
+ * Copyright (c) 2014, Applied Micro Circuits Corporation
+ * Author: Feng Kan fkan@apm.com
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/interrupt.h>
+#include <linux/acpi.h>
+#include <linux/spinlock.h>
+#include <linux/delay.h>
+#include <linux/mailbox_controller.h>
+
+#define MBOX_CON_NAME			"slimpro-mbox"
+#define MBOX_REG_SET_OFFSET		0x1000
+#define MBOX_CNT			8
+#define MBOX_STATUS_AVAIL_MASK		0x00010000
+#define MBOX_STATUS_ACK_MASK		0x00000001
+
+/* Configuration and Status Registers */
+struct slimpro_mbox_reg {
+	u32 in;
+	u32 din0;
+	u32 din1;
+	u32 rsvd1;
+	u32 out;
+	u32 dout0;
+	u32 dout1;
+	u32 rsvd2;
+	u32 status;
+	u32 statusmask;
+};
+
+struct slimpro_mbox_chan {
+	struct device *dev;
+	struct mbox_chan *chan;
+	struct slimpro_mbox_reg __iomem *reg;
+	int id;
+	int irq;
+	u32 rx_msg[3];
+};
+
+struct slimpro_mbox {
+	struct mbox_controller mb_ctrl;
+	struct slimpro_mbox_chan mc[MBOX_CNT];
+	struct mbox_chan chans[MBOX_CNT];
+};
+
+static struct slimpro_mbox_chan *to_slimpro_mbox_chan(struct mbox_chan *chan)
+{
+	if (!chan || !chan->con_priv)
+		return NULL;
+
+	return (struct slimpro_mbox_chan *)chan->con_priv;
+}
+
+static void mb_chan_send_msg(struct slimpro_mbox_chan *mb_chan, u32 *msg)
+{
+	writel(msg[1], &mb_chan->reg->dout0);
+	writel(msg[2], &mb_chan->reg->dout1);
+	writel(msg[0], &mb_chan->reg->out);
+}
+
+static void mb_chan_recv_msg(struct slimpro_mbox_chan *mb_chan)
+{
+	mb_chan->rx_msg[1] = readl(&mb_chan->reg->din0);
+	mb_chan->rx_msg[2] = readl(&mb_chan->reg->din1);
+	mb_chan->rx_msg[0] = readl(&mb_chan->reg->in);
+}
+
+static void mb_chan_enable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
+{
+	u32 val = readl(&mb_chan->reg->statusmask);
+
+	val &= ~mask;
+
+	writel(val, &mb_chan->reg->statusmask);
+}
+
+static void mb_chan_disable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
+{
+	u32 val = readl(&mb_chan->reg->statusmask);
+
+	val |= mask;
+
+	writel(val, &mb_chan->reg->statusmask);
+}
+
+static int mb_chan_status_ack(struct slimpro_mbox_chan *mb_chan)
+{
+	u32 val = readl(&mb_chan->reg->status);
+
+	if (val & MBOX_STATUS_ACK_MASK) {
+		writel(MBOX_STATUS_ACK_MASK, &mb_chan->reg->status);
+		return 1;
+	}
+	return 0;
+}
+
+static int mb_chan_status_avail(struct slimpro_mbox_chan *mb_chan)
+{
+	u32 val = readl(&mb_chan->reg->status);
+
+	if (val & MBOX_STATUS_AVAIL_MASK) {
+		mb_chan_recv_msg(mb_chan);
+		writel(MBOX_STATUS_AVAIL_MASK, &mb_chan->reg->status);
+		return 1;
+	}
+	return 0;
+}
+
+static irqreturn_t slimpro_mbox_irq(int irq, void *id)
+{
+	struct slimpro_mbox_chan *mb_chan = id;
+
+	if (mb_chan_status_ack(mb_chan))
+		mbox_chan_txdone(mb_chan->chan, 0);
+
+	if (mb_chan_status_avail(mb_chan)) {
+		mb_chan_recv_msg(mb_chan);
+		mbox_chan_received_data(mb_chan->chan, mb_chan->rx_msg);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int slimpro_mbox_send_data(struct mbox_chan *chan, void *msg)
+{
+	struct slimpro_mbox_chan *mb_chan = to_slimpro_mbox_chan(chan);
+
+	mb_chan_send_msg(mb_chan, msg);
+	return 0;
+}
+
+static int slimpro_mbox_startup(struct mbox_chan *chan)
+{
+	struct slimpro_mbox_chan *mb_chan = to_slimpro_mbox_chan(chan);
+	int rc;
+
+	rc = devm_request_irq(mb_chan->dev, mb_chan->irq, slimpro_mbox_irq, 0,
+			      MBOX_CON_NAME, mb_chan);
+	if (unlikely(rc)) {
+		dev_err(mb_chan->dev, "failed to register mailbox interrupt %d\n",
+			mb_chan->irq);
+		return rc;
+	}
+
+	/* Enable HW interrupt */
+	writel(MBOX_STATUS_ACK_MASK | MBOX_STATUS_AVAIL_MASK,
+		&mb_chan->reg->status);
+	mb_chan_enable_int(mb_chan, MBOX_STATUS_ACK_MASK |
+					MBOX_STATUS_AVAIL_MASK);
+	return 0;
+}
+
+static void slimpro_mbox_shutdown(struct mbox_chan *chan)
+{
+	struct slimpro_mbox_chan *mb_chan = to_slimpro_mbox_chan(chan);
+
+	mb_chan_disable_int(mb_chan, MBOX_STATUS_ACK_MASK |
+				  MBOX_STATUS_AVAIL_MASK);
+	devm_free_irq(mb_chan->dev, mb_chan->irq, mb_chan);
+}
+
+static struct mbox_chan_ops slimpro_mbox_ops = {
+	.send_data = slimpro_mbox_send_data,
+	.startup = slimpro_mbox_startup,
+	.shutdown = slimpro_mbox_shutdown,
+};
+
+static int __init slimpro_mbox_probe(struct platform_device *pdev)
+{
+	struct slimpro_mbox *ctx;
+	struct resource *regs;
+	void __iomem *mb_base;
+	int rc;
+	int i;
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(struct slimpro_mbox), GFP_KERNEL);
+	if (IS_ERR(ctx))
+		return PTR_ERR(ctx);
+	platform_set_drvdata(pdev, ctx);
+
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mb_base = devm_ioremap_resource(&pdev->dev, regs);
+	if (IS_ERR(mb_base))
+		return PTR_ERR(mb_base);
+
+	/* Setup mailbox links */
+	for (i = 0; i < MBOX_CNT; i++) {
+		ctx->mc[i].irq = platform_get_irq(pdev, i);
+		if (ctx->mc[i].irq < 0) {
+			dev_err(&pdev->dev, "no IRQ at index %d\n",
+				ctx->mc[i].irq);
+			return -ENODEV;
+		}
+
+		ctx->mc[i].dev = &pdev->dev;
+		ctx->mc[i].reg = mb_base + i * MBOX_REG_SET_OFFSET;
+		ctx->mc[i].id = i;
+		ctx->mc[i].chan = &ctx->chans[i];
+		ctx->chans[i].con_priv = &ctx->mc[i];
+	}
+
+	/* Setup mailbox controller */
+	ctx->mb_ctrl.dev = &pdev->dev;
+	ctx->mb_ctrl.chans = ctx->chans;
+	ctx->mb_ctrl.txdone_irq = true;
+	ctx->mb_ctrl.ops = &slimpro_mbox_ops;
+	ctx->mb_ctrl.num_chans = MBOX_CNT;
+
+	rc = mbox_controller_register(&ctx->mb_ctrl);
+	if (rc) {
+		dev_err(&pdev->dev,
+			"APM X-Gene SLIMpro MailBox register failed:%d\n", rc);
+		return rc;
+	}
+
+	dev_info(&pdev->dev, "APM X-Gene SLIMpro MailBox registered\n");
+	return 0;
+}
+
+static int slimpro_mbox_remove(struct platform_device *pdev)
+{
+	struct slimpro_mbox *smb = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&smb->mb_ctrl);
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id slimpro_of_match[] = {
+	{.compatible = "apm,xgene-slimpro-mbox" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, slimpro_of_match);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id slimpro_acpi_ids[] = {
+	{"APMC0D01", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, slimpro_acpi_ids);
+#endif
+
+static struct platform_driver slimpro_mbox_driver = {
+	.probe	= slimpro_mbox_probe,
+	.remove = slimpro_mbox_remove,
+	.driver	= {
+		.name = "xgene-slimpro-mbox",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(slimpro_of_match),
+		.acpi_match_table = ACPI_PTR(slimpro_acpi_ids)
+	},
+};
+
+static int __init slimpro_mbox_init(void)
+{
+	return platform_driver_register(&slimpro_mbox_driver);
+}
+
+static void __exit slimpro_mbox_exit(void)
+{
+}
+
+subsys_initcall(slimpro_mbox_init);
+module_exit(slimpro_mbox_exit);
+
+MODULE_DESCRIPTION("APM X-Gene SLIMpro Mailbox Driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH 1/6] mailbox: add support for APM X-Gene platform mailbox driver

[Feng Kan]

Add support for APM X-Gene platform mailbox driver.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 drivers/mailbox/Kconfig                 |  10 ++
 drivers/mailbox/Makefile                |   1 +
 drivers/mailbox/mailbox-xgene-slimpro.c | 287 ++++++++++++++++++++++++++++++++
 3 files changed, 298 insertions(+)
 create mode 100644 drivers/mailbox/mailbox-xgene-slimpro.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 9fd9c67..12ed5c1 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -33,4 +33,14 @@ config OMAP_MBOX_KFIFO_SIZE
 	  Specify the default size of mailbox's kfifo buffers (bytes).
 	  This can also be changed at runtime (via the mbox_kfifo_size
 	  module parameter).
+
+config XGENE_SLIMPRO_MBOX
+	tristate "APM SoC X-Gene SLIMpro Mailbox Controller"
+	depends on ARCH_XGENE
+	help
+	  An implementation of the APM X-Gene Interprocessor Communication
+	  Mailbox (IPCM) between the ARM 64-bit cores and SLIMpro controller.
+	  It is used to send short messages between ARM64-bit cores and
+	  the SLIMpro Management Engine, primarily for PM. Say Y here if you
+	  want to use the APM X-Gene SLIMpro IPCM support.
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 94ed7ce..33b2709 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -5,3 +5,4 @@ obj-$(CONFIG_MAILBOX)		+= mailbox.o
 obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
 
 obj-$(CONFIG_OMAP2PLUS_MBOX)	+= omap-mailbox.o
+obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
diff --git a/drivers/mailbox/mailbox-xgene-slimpro.c b/drivers/mailbox/mailbox-xgene-slimpro.c
new file mode 100644
index 0000000..ae46cc3
--- /dev/null
+++ b/drivers/mailbox/mailbox-xgene-slimpro.c
@@ -0,0 +1,287 @@
+/*
+ * APM X-Gene SLIMpro MailBox Driver
+ *
+ * Copyright (c) 2014, Applied Micro Circuits Corporation
+ * Author: Feng Kan fkan at apm.com
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/interrupt.h>
+#include <linux/acpi.h>
+#include <linux/spinlock.h>
+#include <linux/delay.h>
+#include <linux/mailbox_controller.h>
+
+#define MBOX_CON_NAME			"slimpro-mbox"
+#define MBOX_REG_SET_OFFSET		0x1000
+#define MBOX_CNT			8
+#define MBOX_STATUS_AVAIL_MASK		0x00010000
+#define MBOX_STATUS_ACK_MASK		0x00000001
+
+/* Configuration and Status Registers */
+struct slimpro_mbox_reg {
+	u32 in;
+	u32 din0;
+	u32 din1;
+	u32 rsvd1;
+	u32 out;
+	u32 dout0;
+	u32 dout1;
+	u32 rsvd2;
+	u32 status;
+	u32 statusmask;
+};
+
+struct slimpro_mbox_chan {
+	struct device *dev;
+	struct mbox_chan *chan;
+	struct slimpro_mbox_reg __iomem *reg;
+	int id;
+	int irq;
+	u32 rx_msg[3];
+};
+
+struct slimpro_mbox {
+	struct mbox_controller mb_ctrl;
+	struct slimpro_mbox_chan mc[MBOX_CNT];
+	struct mbox_chan chans[MBOX_CNT];
+};
+
+static struct slimpro_mbox_chan *to_slimpro_mbox_chan(struct mbox_chan *chan)
+{
+	if (!chan || !chan->con_priv)
+		return NULL;
+
+	return (struct slimpro_mbox_chan *)chan->con_priv;
+}
+
+static void mb_chan_send_msg(struct slimpro_mbox_chan *mb_chan, u32 *msg)
+{
+	writel(msg[1], &mb_chan->reg->dout0);
+	writel(msg[2], &mb_chan->reg->dout1);
+	writel(msg[0], &mb_chan->reg->out);
+}
+
+static void mb_chan_recv_msg(struct slimpro_mbox_chan *mb_chan)
+{
+	mb_chan->rx_msg[1] = readl(&mb_chan->reg->din0);
+	mb_chan->rx_msg[2] = readl(&mb_chan->reg->din1);
+	mb_chan->rx_msg[0] = readl(&mb_chan->reg->in);
+}
+
+static void mb_chan_enable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
+{
+	u32 val = readl(&mb_chan->reg->statusmask);
+
+	val &= ~mask;
+
+	writel(val, &mb_chan->reg->statusmask);
+}
+
+static void mb_chan_disable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
+{
+	u32 val = readl(&mb_chan->reg->statusmask);
+
+	val |= mask;
+
+	writel(val, &mb_chan->reg->statusmask);
+}
+
+static int mb_chan_status_ack(struct slimpro_mbox_chan *mb_chan)
+{
+	u32 val = readl(&mb_chan->reg->status);
+
+	if (val & MBOX_STATUS_ACK_MASK) {
+		writel(MBOX_STATUS_ACK_MASK, &mb_chan->reg->status);
+		return 1;
+	}
+	return 0;
+}
+
+static int mb_chan_status_avail(struct slimpro_mbox_chan *mb_chan)
+{
+	u32 val = readl(&mb_chan->reg->status);
+
+	if (val & MBOX_STATUS_AVAIL_MASK) {
+		mb_chan_recv_msg(mb_chan);
+		writel(MBOX_STATUS_AVAIL_MASK, &mb_chan->reg->status);
+		return 1;
+	}
+	return 0;
+}
+
+static irqreturn_t slimpro_mbox_irq(int irq, void *id)
+{
+	struct slimpro_mbox_chan *mb_chan = id;
+
+	if (mb_chan_status_ack(mb_chan))
+		mbox_chan_txdone(mb_chan->chan, 0);
+
+	if (mb_chan_status_avail(mb_chan)) {
+		mb_chan_recv_msg(mb_chan);
+		mbox_chan_received_data(mb_chan->chan, mb_chan->rx_msg);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int slimpro_mbox_send_data(struct mbox_chan *chan, void *msg)
+{
+	struct slimpro_mbox_chan *mb_chan = to_slimpro_mbox_chan(chan);
+
+	mb_chan_send_msg(mb_chan, msg);
+	return 0;
+}
+
+static int slimpro_mbox_startup(struct mbox_chan *chan)
+{
+	struct slimpro_mbox_chan *mb_chan = to_slimpro_mbox_chan(chan);
+	int rc;
+
+	rc = devm_request_irq(mb_chan->dev, mb_chan->irq, slimpro_mbox_irq, 0,
+			      MBOX_CON_NAME, mb_chan);
+	if (unlikely(rc)) {
+		dev_err(mb_chan->dev, "failed to register mailbox interrupt %d\n",
+			mb_chan->irq);
+		return rc;
+	}
+
+	/* Enable HW interrupt */
+	writel(MBOX_STATUS_ACK_MASK | MBOX_STATUS_AVAIL_MASK,
+		&mb_chan->reg->status);
+	mb_chan_enable_int(mb_chan, MBOX_STATUS_ACK_MASK |
+					MBOX_STATUS_AVAIL_MASK);
+	return 0;
+}
+
+static void slimpro_mbox_shutdown(struct mbox_chan *chan)
+{
+	struct slimpro_mbox_chan *mb_chan = to_slimpro_mbox_chan(chan);
+
+	mb_chan_disable_int(mb_chan, MBOX_STATUS_ACK_MASK |
+				  MBOX_STATUS_AVAIL_MASK);
+	devm_free_irq(mb_chan->dev, mb_chan->irq, mb_chan);
+}
+
+static struct mbox_chan_ops slimpro_mbox_ops = {
+	.send_data = slimpro_mbox_send_data,
+	.startup = slimpro_mbox_startup,
+	.shutdown = slimpro_mbox_shutdown,
+};
+
+static int __init slimpro_mbox_probe(struct platform_device *pdev)
+{
+	struct slimpro_mbox *ctx;
+	struct resource *regs;
+	void __iomem *mb_base;
+	int rc;
+	int i;
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(struct slimpro_mbox), GFP_KERNEL);
+	if (IS_ERR(ctx))
+		return PTR_ERR(ctx);
+	platform_set_drvdata(pdev, ctx);
+
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mb_base = devm_ioremap_resource(&pdev->dev, regs);
+	if (IS_ERR(mb_base))
+		return PTR_ERR(mb_base);
+
+	/* Setup mailbox links */
+	for (i = 0; i < MBOX_CNT; i++) {
+		ctx->mc[i].irq = platform_get_irq(pdev, i);
+		if (ctx->mc[i].irq < 0) {
+			dev_err(&pdev->dev, "no IRQ at index %d\n",
+				ctx->mc[i].irq);
+			return -ENODEV;
+		}
+
+		ctx->mc[i].dev = &pdev->dev;
+		ctx->mc[i].reg = mb_base + i * MBOX_REG_SET_OFFSET;
+		ctx->mc[i].id = i;
+		ctx->mc[i].chan = &ctx->chans[i];
+		ctx->chans[i].con_priv = &ctx->mc[i];
+	}
+
+	/* Setup mailbox controller */
+	ctx->mb_ctrl.dev = &pdev->dev;
+	ctx->mb_ctrl.chans = ctx->chans;
+	ctx->mb_ctrl.txdone_irq = true;
+	ctx->mb_ctrl.ops = &slimpro_mbox_ops;
+	ctx->mb_ctrl.num_chans = MBOX_CNT;
+
+	rc = mbox_controller_register(&ctx->mb_ctrl);
+	if (rc) {
+		dev_err(&pdev->dev,
+			"APM X-Gene SLIMpro MailBox register failed:%d\n", rc);
+		return rc;
+	}
+
+	dev_info(&pdev->dev, "APM X-Gene SLIMpro MailBox registered\n");
+	return 0;
+}
+
+static int slimpro_mbox_remove(struct platform_device *pdev)
+{
+	struct slimpro_mbox *smb = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&smb->mb_ctrl);
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id slimpro_of_match[] = {
+	{.compatible = "apm,xgene-slimpro-mbox" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, slimpro_of_match);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id slimpro_acpi_ids[] = {
+	{"APMC0D01", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, slimpro_acpi_ids);
+#endif
+
+static struct platform_driver slimpro_mbox_driver = {
+	.probe	= slimpro_mbox_probe,
+	.remove = slimpro_mbox_remove,
+	.driver	= {
+		.name = "xgene-slimpro-mbox",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(slimpro_of_match),
+		.acpi_match_table = ACPI_PTR(slimpro_acpi_ids)
+	},
+};
+
+static int __init slimpro_mbox_init(void)
+{
+	return platform_driver_register(&slimpro_mbox_driver);
+}
+
+static void __exit slimpro_mbox_exit(void)
+{
+}
+
+subsys_initcall(slimpro_mbox_init);
+module_exit(slimpro_mbox_exit);
+
+MODULE_DESCRIPTION("APM X-Gene SLIMpro Mailbox Driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH 2/6] Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts documentation

[Feng Kan]

This adds the APM X-Gene SLIMpro mailbox device tree node documentation.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 .../bindings/mailbox/xgene-slimpro-mailbox.txt     | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
new file mode 100644
index 0000000..d0b74fb
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
@@ -0,0 +1,41 @@
+The APM X-Gene SLIMpro mailbox driver is used to communicate messages between
+the ARM64 processors and the Cortex M3 (dubbed SLIMpro). It uses a simple
+interrupt based door bell mechanism and can exchange simple messages using the
+internal registers.
+
+There are total of 8 interrupts in this mailbox. Each used for an individual
+door bell (or mailbox channel).
+
+Required properties:
+- compatible:	Should be as "apm, xgene-slimpro-mbox".
+
+- reg:		Contain the mailbox register address range.
+
+- interrupts:	Contain interrupt information for the mailbox device.
+
+- #mbox-cells:	Specify the number of parameters used by the mailbox client.
+		Currently only one to specify the mailbox channel number.
+
+Example:
+
+Mailbox Node:
+		slimpro-mbox: slimpro-mbox@10540000 {
+			compatible = "apm,xgene-slimpro-mbox";
+			reg = <0x0 0x10540000 0x0 0xa000>;
+			#mbox-cells = <1>;
+			interrupts =  	<0x0 0x0 0x4>,
+					<0x0 0x1 0x4>,
+					<0x0 0x2 0x4>,
+					<0x0 0x3 0x4>,
+					<0x0 0x4 0x4>,
+					<0x0 0x5 0x4>,
+					<0x0 0x6 0x4>,
+					<0x0 0x7 0x4>;
+		};
+
+Client Node:
+		i2c-slimpro {
+			compatible = "apm,xgene-slimpro-i2c";
+			mbox = <&slimpro-mbox 0>;
+			mbox-names = "i2c-slimpro";
+		};
-- 
1.9.1

[PATCH 2/6] Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts documentation

[Feng Kan]

This adds the APM X-Gene SLIMpro mailbox device tree node documentation.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 .../bindings/mailbox/xgene-slimpro-mailbox.txt     | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
new file mode 100644
index 0000000..d0b74fb
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
@@ -0,0 +1,41 @@
+The APM X-Gene SLIMpro mailbox driver is used to communicate messages between
+the ARM64 processors and the Cortex M3 (dubbed SLIMpro). It uses a simple
+interrupt based door bell mechanism and can exchange simple messages using the
+internal registers.
+
+There are total of 8 interrupts in this mailbox. Each used for an individual
+door bell (or mailbox channel).
+
+Required properties:
+- compatible:	Should be as "apm, xgene-slimpro-mbox".
+
+- reg:		Contain the mailbox register address range.
+
+- interrupts:	Contain interrupt information for the mailbox device.
+
+- #mbox-cells:	Specify the number of parameters used by the mailbox client.
+		Currently only one to specify the mailbox channel number.
+
+Example:
+
+Mailbox Node:
+		slimpro-mbox: slimpro-mbox at 10540000 {
+			compatible = "apm,xgene-slimpro-mbox";
+			reg = <0x0 0x10540000 0x0 0xa000>;
+			#mbox-cells = <1>;
+			interrupts =  	<0x0 0x0 0x4>,
+					<0x0 0x1 0x4>,
+					<0x0 0x2 0x4>,
+					<0x0 0x3 0x4>,
+					<0x0 0x4 0x4>,
+					<0x0 0x5 0x4>,
+					<0x0 0x6 0x4>,
+					<0x0 0x7 0x4>;
+		};
+
+Client Node:
+		i2c-slimpro {
+			compatible = "apm,xgene-slimpro-i2c";
+			mbox = <&slimpro-mbox 0>;
+			mbox-names = "i2c-slimpro";
+		};
-- 
1.9.1

Re: [PATCH 2/6] Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts documentation

[Mark Rutland]

On Wed, Oct 08, 2014 at 01:06:45AM +0100, Feng Kan wrote:
> This adds the APM X-Gene SLIMpro mailbox device tree node documentation.
> 
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
>  .../bindings/mailbox/xgene-slimpro-mailbox.txt     | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> new file mode 100644
> index 0000000..d0b74fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> @@ -0,0 +1,41 @@
> +The APM X-Gene SLIMpro mailbox driver is used to communicate messages between
> +the ARM64 processors and the Cortex M3 (dubbed SLIMpro). It uses a simple
> +interrupt based door bell mechanism and can exchange simple messages using the
> +internal registers.

The above should be a description of the hardware organisation, not the
Linux driver. Removing the word "driver" in the above should be
sufficient.

> +
> +There are total of 8 interrupts in this mailbox. Each used for an individual
> +door bell (or mailbox channel).

This should be described in the interrupts property.

> +
> +Required properties:
> +- compatible:	Should be as "apm, xgene-slimpro-mbox".

That space shouldn't be there.

> +
> +- reg:		Contain the mailbox register address range.
> +
> +- interrupts:	Contain interrupt information for the mailbox device.

Please describe the specifics of this proeprty. This description is too
general to be of any use. Specific how many interrupts you expect, the
order thereof, etc. If there's not a well-defined ordering, or if you
believe interrupts will be omitted, use interrupt-names, and define
interrutps in terms of it.

> +
> +- #mbox-cells:	Specify the number of parameters used by the mailbox client.
> +		Currently only one to specify the mailbox channel number.

This should be a description of the specific values expected by this
binding rather than the general meaning of this property. How many mbox
cells do you expect (1, it seems?), and what does each encode(the index
of the mailbox?)?

Thanks,
Mark.

> +
> +Example:
> +
> +Mailbox Node:
> +		slimpro-mbox: slimpro-mbox@10540000 {
> +			compatible = "apm,xgene-slimpro-mbox";
> +			reg = <0x0 0x10540000 0x0 0xa000>;
> +			#mbox-cells = <1>;
> +			interrupts =  	<0x0 0x0 0x4>,
> +					<0x0 0x1 0x4>,
> +					<0x0 0x2 0x4>,
> +					<0x0 0x3 0x4>,
> +					<0x0 0x4 0x4>,
> +					<0x0 0x5 0x4>,
> +					<0x0 0x6 0x4>,
> +					<0x0 0x7 0x4>;
> +		};
> +
> +Client Node:
> +		i2c-slimpro {
> +			compatible = "apm,xgene-slimpro-i2c";
> +			mbox = <&slimpro-mbox 0>;
> +			mbox-names = "i2c-slimpro";
> +		};
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 2/6] Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts documentation

[Mark Rutland]

On Wed, Oct 08, 2014 at 01:06:45AM +0100, Feng Kan wrote:
> This adds the APM X-Gene SLIMpro mailbox device tree node documentation.
> 
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
>  .../bindings/mailbox/xgene-slimpro-mailbox.txt     | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> new file mode 100644
> index 0000000..d0b74fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> @@ -0,0 +1,41 @@
> +The APM X-Gene SLIMpro mailbox driver is used to communicate messages between
> +the ARM64 processors and the Cortex M3 (dubbed SLIMpro). It uses a simple
> +interrupt based door bell mechanism and can exchange simple messages using the
> +internal registers.

The above should be a description of the hardware organisation, not the
Linux driver. Removing the word "driver" in the above should be
sufficient.

> +
> +There are total of 8 interrupts in this mailbox. Each used for an individual
> +door bell (or mailbox channel).

This should be described in the interrupts property.

> +
> +Required properties:
> +- compatible:	Should be as "apm, xgene-slimpro-mbox".

That space shouldn't be there.

> +
> +- reg:		Contain the mailbox register address range.
> +
> +- interrupts:	Contain interrupt information for the mailbox device.

Please describe the specifics of this proeprty. This description is too
general to be of any use. Specific how many interrupts you expect, the
order thereof, etc. If there's not a well-defined ordering, or if you
believe interrupts will be omitted, use interrupt-names, and define
interrutps in terms of it.

> +
> +- #mbox-cells:	Specify the number of parameters used by the mailbox client.
> +		Currently only one to specify the mailbox channel number.

This should be a description of the specific values expected by this
binding rather than the general meaning of this property. How many mbox
cells do you expect (1, it seems?), and what does each encode(the index
of the mailbox?)?

Thanks,
Mark.

> +
> +Example:
> +
> +Mailbox Node:
> +		slimpro-mbox: slimpro-mbox at 10540000 {
> +			compatible = "apm,xgene-slimpro-mbox";
> +			reg = <0x0 0x10540000 0x0 0xa000>;
> +			#mbox-cells = <1>;
> +			interrupts =  	<0x0 0x0 0x4>,
> +					<0x0 0x1 0x4>,
> +					<0x0 0x2 0x4>,
> +					<0x0 0x3 0x4>,
> +					<0x0 0x4 0x4>,
> +					<0x0 0x5 0x4>,
> +					<0x0 0x6 0x4>,
> +					<0x0 0x7 0x4>;
> +		};
> +
> +Client Node:
> +		i2c-slimpro {
> +			compatible = "apm,xgene-slimpro-i2c";
> +			mbox = <&slimpro-mbox 0>;
> +			mbox-names = "i2c-slimpro";
> +		};
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 2/6] Documentation: mailbox: Add APM X-Gene SLIMpro mailbox dts documentation

[Mark Rutland]

On Wed, Oct 08, 2014 at 01:06:45AM +0100, Feng Kan wrote:
> This adds the APM X-Gene SLIMpro mailbox device tree node documentation.
> 
> Signed-off-by: Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>
> ---
>  .../bindings/mailbox/xgene-slimpro-mailbox.txt     | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> new file mode 100644
> index 0000000..d0b74fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/xgene-slimpro-mailbox.txt
> @@ -0,0 +1,41 @@
> +The APM X-Gene SLIMpro mailbox driver is used to communicate messages between
> +the ARM64 processors and the Cortex M3 (dubbed SLIMpro). It uses a simple
> +interrupt based door bell mechanism and can exchange simple messages using the
> +internal registers.

The above should be a description of the hardware organisation, not the
Linux driver. Removing the word "driver" in the above should be
sufficient.

> +
> +There are total of 8 interrupts in this mailbox. Each used for an individual
> +door bell (or mailbox channel).

This should be described in the interrupts property.

> +
> +Required properties:
> +- compatible:	Should be as "apm, xgene-slimpro-mbox".

That space shouldn't be there.

> +
> +- reg:		Contain the mailbox register address range.
> +
> +- interrupts:	Contain interrupt information for the mailbox device.

Please describe the specifics of this proeprty. This description is too
general to be of any use. Specific how many interrupts you expect, the
order thereof, etc. If there's not a well-defined ordering, or if you
believe interrupts will be omitted, use interrupt-names, and define
interrutps in terms of it.

> +
> +- #mbox-cells:	Specify the number of parameters used by the mailbox client.
> +		Currently only one to specify the mailbox channel number.

This should be a description of the specific values expected by this
binding rather than the general meaning of this property. How many mbox
cells do you expect (1, it seems?), and what does each encode(the index
of the mailbox?)?

Thanks,
Mark.

> +
> +Example:
> +
> +Mailbox Node:
> +		slimpro-mbox: slimpro-mbox@10540000 {
> +			compatible = "apm,xgene-slimpro-mbox";
> +			reg = <0x0 0x10540000 0x0 0xa000>;
> +			#mbox-cells = <1>;
> +			interrupts =  	<0x0 0x0 0x4>,
> +					<0x0 0x1 0x4>,
> +					<0x0 0x2 0x4>,
> +					<0x0 0x3 0x4>,
> +					<0x0 0x4 0x4>,
> +					<0x0 0x5 0x4>,
> +					<0x0 0x6 0x4>,
> +					<0x0 0x7 0x4>;
> +		};
> +
> +Client Node:
> +		i2c-slimpro {
> +			compatible = "apm,xgene-slimpro-i2c";
> +			mbox = <&slimpro-mbox 0>;
> +			mbox-names = "i2c-slimpro";
> +		};
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 3/6] arm64: dts: mailbox device tree node for APM X-Gene platform.

[Feng Kan]

Mailbox device tree node for APM X-Gene platform.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index 1cd5173..249f025 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -455,6 +455,20 @@
 			clocks = <&pcie4clk 0>;
 		};
 
+		mailbox: slimpro-mbox@10540000 {
+			compatible = "apm,xgene-slimpro-mbox";
+			reg = <0x0 0x10540000 0x0 0xa000>;
+			#mbox-cells = <1>;
+			interrupts =    <0x0 0x0 0x4>,
+					<0x0 0x1 0x4>,
+					<0x0 0x2 0x4>,
+					<0x0 0x3 0x4>,
+					<0x0 0x4 0x4>,
+					<0x0 0x5 0x4>,
+					<0x0 0x6 0x4>,
+					<0x0 0x7 0x4>;
+		};
+
 		serial0: serial@1c020000 {
 			status = "disabled";
 			device_type = "serial";
-- 
1.9.1

[PATCH 3/6] arm64: dts: mailbox device tree node for APM X-Gene platform.

[Feng Kan]

Mailbox device tree node for APM X-Gene platform.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index 1cd5173..249f025 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -455,6 +455,20 @@
 			clocks = <&pcie4clk 0>;
 		};
 
+		mailbox: slimpro-mbox at 10540000 {
+			compatible = "apm,xgene-slimpro-mbox";
+			reg = <0x0 0x10540000 0x0 0xa000>;
+			#mbox-cells = <1>;
+			interrupts =    <0x0 0x0 0x4>,
+					<0x0 0x1 0x4>,
+					<0x0 0x2 0x4>,
+					<0x0 0x3 0x4>,
+					<0x0 0x4 0x4>,
+					<0x0 0x5 0x4>,
+					<0x0 0x6 0x4>,
+					<0x0 0x7 0x4>;
+		};
+
 		serial0: serial at 1c020000 {
 			status = "disabled";
 			device_type = "serial";
-- 
1.9.1

[PATCH 3/6] arm64: dts: mailbox device tree node for APM X-Gene platform.

[Feng Kan]

Mailbox device tree node for APM X-Gene platform.

Signed-off-by: Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>
---
 arch/arm64/boot/dts/apm-storm.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index 1cd5173..249f025 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -455,6 +455,20 @@
 			clocks = <&pcie4clk 0>;
 		};
 
+		mailbox: slimpro-mbox@10540000 {
+			compatible = "apm,xgene-slimpro-mbox";
+			reg = <0x0 0x10540000 0x0 0xa000>;
+			#mbox-cells = <1>;
+			interrupts =    <0x0 0x0 0x4>,
+					<0x0 0x1 0x4>,
+					<0x0 0x2 0x4>,
+					<0x0 0x3 0x4>,
+					<0x0 0x4 0x4>,
+					<0x0 0x5 0x4>,
+					<0x0 0x6 0x4>,
+					<0x0 0x7 0x4>;
+		};
+
 		serial0: serial@1c020000 {
 			status = "disabled";
 			device_type = "serial";
-- 
1.9.1

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

[Feng Kan]

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@apm.com>
Signed-off-by: Hieu Le <hnle@apm.com>
---
 drivers/i2c/busses/Kconfig             |   9 +
 drivers/i2c/busses/Makefile            |   1 +
 drivers/i2c/busses/i2c-xgene-slimpro.c | 531 +++++++++++++++++++++++++++++++++
 3 files changed, 541 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-xgene-slimpro.c

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.
+
 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
 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@apm.com>
+ * Author: Hieu Le <hnle@apm.com>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * This driver provides support for X-Gene SLIMpro I2C device access
+ * using the APM X-Gene SLIMpro mailbox driver.
+ *
+ */
+#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>
+
+#define XGENE_SLIMPRO_I2C		"xgene-slimpro-i2c"
+
+#define SLIMPRO_I2C_WAIT_COUNT		10000
+
+#define SLIMPRO_OP_TO_MS		1000	/* Operation time out in ms */
+#define SLIMPRO_IIC_BUS			1
+
+#define SMBUS_CMD_LEN			1
+#define BYTE_DATA			1
+#define WORD_DATA			2
+#define BLOCK_DATA			3
+
+#define SLIMPRO_IIC_I2C_PROTOCOL	0
+#define SLIMPRO_IIC_SMB_PROTOCOL	1
+
+#define SLIMPRO_IIC_READ		0
+#define SLIMPRO_IIC_WRITE		1
+
+#define IIC_SMB_WITHOUT_DATA_LEN	0
+#define IIC_SMB_WITH_DATA_LEN		1
+
+#define SLIMPRO_DEBUG_MSG		0
+#define SLIMPRO_MSG_TYPE_SHIFT		28
+#define SLIMPRO_DBG_SUBTYPE_I2C1READ	4
+#define SLIMPRO_DBGMSG_TYPE_SHIFT	24
+#define SLIMPRO_DBGMSG_TYPE_MASK	0x0F000000U
+#define SLIMPRO_IIC_DEV_SHIFT		23
+#define SLIMPRO_IIC_DEV_MASK		0x00800000U
+#define SLIMPRO_IIC_DEVID_SHIFT		13
+#define SLIMPRO_IIC_DEVID_MASK		0x007FE000U
+#define SLIMPRO_IIC_RW_SHIFT		12
+#define SLIMPRO_IIC_RW_MASK		0x00001000U
+#define SLIMPRO_IIC_PROTO_SHIFT		11
+#define SLIMPRO_IIC_PROTO_MASK		0x00000800U
+#define SLIMPRO_IIC_ADDRLEN_SHIFT	8
+#define SLIMPRO_IIC_ADDRLEN_MASK	0x00000700U
+#define SLIMPRO_IIC_DATALEN_SHIFT	0
+#define SLIMPRO_IIC_DATALEN_MASK	0x000000FFU
+
+/*
+ * SLIMpro I2C message encode
+ *
+ * dev		- Controller number (0-based)
+ * chip		- I2C chip address
+ * op		- SLIMPRO_IIC_READ or SLIMPRO_IIC_WRITE
+ * proto	- SLIMPRO_IIC_SMB_PROTOCOL or SLIMPRO_IIC_I2C_PROTOCOL
+ * addrlen	- Length of the address field
+ * datalen	- Length of the data field
+ */
+#define SLIMPRO_IIC_ENCODE_MSG(dev, chip, op, proto, addrlen, datalen) \
+	((SLIMPRO_DEBUG_MSG << SLIMPRO_MSG_TYPE_SHIFT) | \
+	((SLIMPRO_DBG_SUBTYPE_I2C1READ << SLIMPRO_DBGMSG_TYPE_SHIFT) & \
+	SLIMPRO_DBGMSG_TYPE_MASK) | \
+	((dev << SLIMPRO_IIC_DEV_SHIFT) & SLIMPRO_IIC_DEV_MASK) | \
+	((chip << SLIMPRO_IIC_DEVID_SHIFT) & SLIMPRO_IIC_DEVID_MASK) | \
+	((op << SLIMPRO_IIC_RW_SHIFT) & SLIMPRO_IIC_RW_MASK) | \
+	((proto << SLIMPRO_IIC_PROTO_SHIFT) & SLIMPRO_IIC_PROTO_MASK) | \
+	((addrlen << SLIMPRO_IIC_ADDRLEN_SHIFT) & SLIMPRO_IIC_ADDRLEN_MASK) | \
+	((datalen << SLIMPRO_IIC_DATALEN_SHIFT) & SLIMPRO_IIC_DATALEN_MASK))
+
+/*
+ * Encode for upper address for block data
+ */
+#define SLIMPRO_IIC_ENCODE_FLAG_BUFADDR			0x80000000
+#define SLIMPRO_IIC_ENCODE_FLAG_WITH_DATA_LEN(a)	((u32) (((a) << 30) \
+								& 0x40000000))
+#define SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(a)		((u32) (((a) >> 12) \
+								& 0x3FF00000))
+#define SLIMPRO_IIC_ENCODE_ADDR(a)			((a) & 0x000FFFFF)
+
+struct slimpro_i2c_dev {
+	struct i2c_adapter adapter;
+	struct device *dev;
+	struct mbox_chan *mbox_chan;
+	struct mbox_client mbox_client;
+	struct completion rd_complete;
+	spinlock_t lock;
+	int i2c_rx_poll;
+	int i2c_tx_poll;
+	u8 dma_buffer[I2C_SMBUS_BLOCK_MAX];
+	u32 *resp_msg;
+};
+
+#define to_slimpro_i2c_dev(cl)	\
+		container_of(cl, struct slimpro_i2c_dev, mbox_client)
+
+static void slimpro_i2c_rx_cb(struct mbox_client *cl, void *mssg)
+{
+	struct slimpro_i2c_dev *ctx = to_slimpro_i2c_dev(cl);
+
+	/*
+	 * Response message format:
+	 * mssg[0] is the return code of the operation
+	 * mssg[1] is the first data word
+	 * mssg[2] is NOT used
+	 */
+	if (ctx->resp_msg)
+		*ctx->resp_msg = ((u32 *) mssg)[1];
+
+	if (ctx->mbox_client.tx_block)
+		complete(&ctx->rd_complete);
+	else
+		ctx->i2c_rx_poll = 0;
+}
+
+static void slimpro_i2c_tx_done(struct mbox_client *cl, void *mssg, int r)
+{
+	struct slimpro_i2c_dev *ctx = to_slimpro_i2c_dev(cl);
+
+	if (r == 0)
+		ctx->i2c_tx_poll = 0;
+}
+
+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;
+	} 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;
+		}
+		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_rd(struct slimpro_i2c_dev *ctx, u32 chip,
+				u32 addr, u32 addrlen, u32 protocol,
+				u32 readlen, u32 *data)
+{
+	u32 msg[3];
+	int rc;
+
+	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_ADDR(addr);
+	msg[2] = 0;
+	ctx->resp_msg = data;
+	if (ctx->mbox_client.tx_block)
+		init_completion(&ctx->rd_complete);
+
+	rc = mbox_send_message(ctx->mbox_chan, &msg);
+	if (rc < 0)
+		goto err;
+
+	rc = start_i2c_msg_xfer(ctx);
+err:
+	ctx->resp_msg = NULL;
+	return rc;
+}
+
+static int slimpro_i2c_wr(struct slimpro_i2c_dev *ctx, u32 chip,
+			  u32 addr, u32 addrlen, u32 protocol, u32 writelen,
+			  u32 data)
+{
+	u32 msg[3];
+	int rc;
+
+	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_ADDR(addr);
+	msg[2] = data;
+	ctx->resp_msg = msg;
+
+	if (ctx->mbox_client.tx_block)
+		init_completion(&ctx->rd_complete);
+
+	rc = mbox_send_message(ctx->mbox_chan, &msg);
+	if (rc < 0)
+		goto err;
+
+	rc = start_i2c_msg_xfer(ctx);
+err:
+	ctx->resp_msg = NULL;
+	return rc;
+}
+
+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;
+		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);
+
+	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;
+		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);
+
+	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 int xgene_slimpro_i2c_xfer(struct i2c_adapter *adap, u16 addr,
+				  unsigned short flags, char read_write,
+				  u8 command, int size,
+				  union i2c_smbus_data *data)
+{
+	struct slimpro_i2c_dev *ctx = i2c_get_adapdata(adap);
+	int ret = -EOPNOTSUPP;
+	u32 val;
+
+	switch (size) {
+	case I2C_SMBUS_BYTE:
+		if (read_write == I2C_SMBUS_READ) {
+			ret = slimpro_i2c_rd(ctx, addr, 0, 0,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     BYTE_DATA, &val);
+			data->byte = val;
+		} else {
+			ret = slimpro_i2c_wr(ctx, addr, command, SMBUS_CMD_LEN,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     0, 0);
+		}
+		break;
+	case I2C_SMBUS_BYTE_DATA:
+		if (read_write == I2C_SMBUS_READ) {
+			ret = slimpro_i2c_rd(ctx, addr, command, SMBUS_CMD_LEN,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     BYTE_DATA, &val);
+			data->byte = val;
+		} else {
+			val = data->byte;
+			ret = slimpro_i2c_wr(ctx, addr, command, SMBUS_CMD_LEN,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     BYTE_DATA, val);
+		}
+		break;
+	case I2C_SMBUS_WORD_DATA:
+		if (read_write == I2C_SMBUS_READ) {
+			ret = slimpro_i2c_rd(ctx, addr, command, SMBUS_CMD_LEN,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     WORD_DATA, &val);
+			data->word = val;
+		} else {
+			val = data->word;
+			ret = slimpro_i2c_wr(ctx, addr, command, SMBUS_CMD_LEN,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     WORD_DATA, val);
+		}
+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		if (read_write == I2C_SMBUS_READ) {
+			ret = slimpro_i2c_blkrd(ctx, addr, command,
+						SMBUS_CMD_LEN,
+						SLIMPRO_IIC_SMB_PROTOCOL,
+						I2C_SMBUS_BLOCK_MAX + 1,
+						IIC_SMB_WITH_DATA_LEN,
+						&data->block[0]);
+
+		} else {
+			ret = slimpro_i2c_blkwr(ctx, addr, command,
+						SMBUS_CMD_LEN,
+						SLIMPRO_IIC_SMB_PROTOCOL,
+						data->block[0] + 1,
+						&data->block[0]);
+		}
+		break;
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		if (read_write == I2C_SMBUS_READ) {
+			ret = slimpro_i2c_blkrd(ctx, addr,
+						command,
+						SMBUS_CMD_LEN,
+						SLIMPRO_IIC_I2C_PROTOCOL,
+						I2C_SMBUS_BLOCK_MAX,
+						IIC_SMB_WITHOUT_DATA_LEN,
+						&data->block[1]);
+		} else {
+			ret = slimpro_i2c_blkwr(ctx, addr, command,
+						SMBUS_CMD_LEN,
+						SLIMPRO_IIC_I2C_PROTOCOL,
+						data->block[0],
+						&data->block[1]);
+		}
+		break;
+	default:
+		break;
+	}
+	return ret;
+}
+
+/*
+* Return list of supported functionality.
+*/
+static u32 xgene_slimpro_i2c_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SMBUS_BYTE_DATA |
+		I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_BLOCK_DATA |
+		I2C_FUNC_SMBUS_I2C_BLOCK;
+}
+
+static struct i2c_algorithm xgene_slimpro_i2c_algorithm = {
+	.smbus_xfer = xgene_slimpro_i2c_xfer,
+	.functionality = xgene_slimpro_i2c_func,
+};
+
+static int __init xgene_slimpro_i2c_probe(struct platform_device *pdev)
+{
+	struct slimpro_i2c_dev *ctx;
+	struct i2c_adapter *adapter;
+	struct mbox_client *cl;
+	int rc;
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->dev = &pdev->dev;
+	platform_set_drvdata(pdev, ctx);
+	cl = &ctx->mbox_client;
+
+	/* Request mailbox channel */
+	cl->dev = &pdev->dev;
+	cl->rx_callback = slimpro_i2c_rx_cb;
+	cl->tx_done = slimpro_i2c_tx_done;
+	cl->tx_block = true;
+	cl->tx_tout = SLIMPRO_OP_TO_MS;
+	cl->knows_txdone = false;
+	cl->chan_name = "i2c-slimpro";
+	ctx->mbox_chan = mbox_request_channel(cl);
+	if (IS_ERR(ctx->mbox_chan)) {
+		dev_err(&pdev->dev, "SLIMpro mailbox channel request failed\n");
+		return PTR_ERR(ctx->mbox_chan);
+	}
+
+	/* Initialiation in case of using poll mode */
+	ctx->i2c_rx_poll = 0;
+	ctx->i2c_tx_poll = 0;
+	spin_lock_init(&ctx->lock);
+
+	/* Setup I2C adapter */
+	adapter = &ctx->adapter;
+	snprintf(adapter->name, sizeof(adapter->name), "X-Gene SLIMpro I2C");
+	adapter->algo = &xgene_slimpro_i2c_algorithm;
+	adapter->class = I2C_CLASS_HWMON;
+	adapter->dev.parent = &pdev->dev;
+	i2c_set_adapdata(adapter, ctx);
+	rc = i2c_add_adapter(adapter);
+	if (rc) {
+		dev_err(&pdev->dev, "Adapter registeration failed\n");
+		return rc;
+	}
+
+	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+	if (rc)
+		dev_warn(&pdev->dev, "Unable to set dma mask\n");
+
+	dev_info(&pdev->dev, "APM X-Gene SLIMpro I2C Adapter registered\n");
+	return 0;
+}
+
+static int xgene_slimpro_i2c_remove(struct platform_device *pdev)
+{
+	struct slimpro_i2c_dev *ctx = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&ctx->adapter);
+
+	mbox_free_channel(ctx->mbox_chan);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id xgene_slimpro_i2c_id[] = {
+	{.compatible = "apm,xgene-slimpro-i2c" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids);
+#endif
+
+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,
+		.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");
-- 
1.9.1

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

[Feng Kan]

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@apm.com>
Signed-off-by: Hieu Le <hnle@apm.com>
---
 drivers/i2c/busses/Kconfig             |   9 +
 drivers/i2c/busses/Makefile            |   1 +
 drivers/i2c/busses/i2c-xgene-slimpro.c | 531 +++++++++++++++++++++++++++++++++
 3 files changed, 541 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-xgene-slimpro.c

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.
+
 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
 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@apm.com>
+ * Author: Hieu Le <hnle@apm.com>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * This driver provides support for X-Gene SLIMpro I2C device access
+ * using the APM X-Gene SLIMpro mailbox driver.
+ *
+ */
+#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>
+
+#define XGENE_SLIMPRO_I2C		"xgene-slimpro-i2c"
+
+#define SLIMPRO_I2C_WAIT_COUNT		10000
+
+#define SLIMPRO_OP_TO_MS		1000	/* Operation time out in ms */
+#define SLIMPRO_IIC_BUS			1
+
+#define SMBUS_CMD_LEN			1
+#define BYTE_DATA			1
+#define WORD_DATA			2
+#define BLOCK_DATA			3
+
+#define SLIMPRO_IIC_I2C_PROTOCOL	0
+#define SLIMPRO_IIC_SMB_PROTOCOL	1
+
+#define SLIMPRO_IIC_READ		0
+#define SLIMPRO_IIC_WRITE		1
+
+#define IIC_SMB_WITHOUT_DATA_LEN	0
+#define IIC_SMB_WITH_DATA_LEN		1
+
+#define SLIMPRO_DEBUG_MSG		0
+#define SLIMPRO_MSG_TYPE_SHIFT		28
+#define SLIMPRO_DBG_SUBTYPE_I2C1READ	4
+#define SLIMPRO_DBGMSG_TYPE_SHIFT	24
+#define SLIMPRO_DBGMSG_TYPE_MASK	0x0F000000U
+#define SLIMPRO_IIC_DEV_SHIFT		23
+#define SLIMPRO_IIC_DEV_MASK		0x00800000U
+#define SLIMPRO_IIC_DEVID_SHIFT		13
+#define SLIMPRO_IIC_DEVID_MASK		0x007FE000U
+#define SLIMPRO_IIC_RW_SHIFT		12
+#define SLIMPRO_IIC_RW_MASK		0x00001000U
+#define SLIMPRO_IIC_PROTO_SHIFT		11
+#define SLIMPRO_IIC_PROTO_MASK		0x00000800U
+#define SLIMPRO_IIC_ADDRLEN_SHIFT	8
+#define SLIMPRO_IIC_ADDRLEN_MASK	0x00000700U
+#define SLIMPRO_IIC_DATALEN_SHIFT	0
+#define SLIMPRO_IIC_DATALEN_MASK	0x000000FFU
+
+/*
+ * SLIMpro I2C message encode
+ *
+ * dev		- Controller number (0-based)
+ * chip		- I2C chip address
+ * op		- SLIMPRO_IIC_READ or SLIMPRO_IIC_WRITE
+ * proto	- SLIMPRO_IIC_SMB_PROTOCOL or SLIMPRO_IIC_I2C_PROTOCOL
+ * addrlen	- Length of the address field
+ * datalen	- Length of the data field
+ */
+#define SLIMPRO_IIC_ENCODE_MSG(dev, chip, op, proto, addrlen, datalen) \
+	((SLIMPRO_DEBUG_MSG << SLIMPRO_MSG_TYPE_SHIFT) | \
+	((SLIMPRO_DBG_SUBTYPE_I2C1READ << SLIMPRO_DBGMSG_TYPE_SHIFT) & \
+	SLIMPRO_DBGMSG_TYPE_MASK) | \
+	((dev << SLIMPRO_IIC_DEV_SHIFT) & SLIMPRO_IIC_DEV_MASK) | \
+	((chip << SLIMPRO_IIC_DEVID_SHIFT) & SLIMPRO_IIC_DEVID_MASK) | \
+	((op << SLIMPRO_IIC_RW_SHIFT) & SLIMPRO_IIC_RW_MASK) | \
+	((proto << SLIMPRO_IIC_PROTO_SHIFT) & SLIMPRO_IIC_PROTO_MASK) | \
+	((addrlen << SLIMPRO_IIC_ADDRLEN_SHIFT) & SLIMPRO_IIC_ADDRLEN_MASK) | \
+	((datalen << SLIMPRO_IIC_DATALEN_SHIFT) & SLIMPRO_IIC_DATALEN_MASK))
+
+/*
+ * Encode for upper address for block data
+ */
+#define SLIMPRO_IIC_ENCODE_FLAG_BUFADDR			0x80000000
+#define SLIMPRO_IIC_ENCODE_FLAG_WITH_DATA_LEN(a)	((u32) (((a) << 30) \
+								& 0x40000000))
+#define SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(a)		((u32) (((a) >> 12) \
+								& 0x3FF00000))
+#define SLIMPRO_IIC_ENCODE_ADDR(a)			((a) & 0x000FFFFF)
+
+struct slimpro_i2c_dev {
+	struct i2c_adapter adapter;
+	struct device *dev;
+	struct mbox_chan *mbox_chan;
+	struct mbox_client mbox_client;
+	struct completion rd_complete;
+	spinlock_t lock;
+	int i2c_rx_poll;
+	int i2c_tx_poll;
+	u8 dma_buffer[I2C_SMBUS_BLOCK_MAX];
+	u32 *resp_msg;
+};
+
+#define to_slimpro_i2c_dev(cl)	\
+		container_of(cl, struct slimpro_i2c_dev, mbox_client)
+
+static void slimpro_i2c_rx_cb(struct mbox_client *cl, void *mssg)
+{
+	struct slimpro_i2c_dev *ctx = to_slimpro_i2c_dev(cl);
+
+	/*
+	 * Response message format:
+	 * mssg[0] is the return code of the operation
+	 * mssg[1] is the first data word
+	 * mssg[2] is NOT used
+	 */
+	if (ctx->resp_msg)
+		*ctx->resp_msg = ((u32 *) mssg)[1];
+
+	if (ctx->mbox_client.tx_block)
+		complete(&ctx->rd_complete);
+	else
+		ctx->i2c_rx_poll = 0;
+}
+
+static void slimpro_i2c_tx_done(struct mbox_client *cl, void *mssg, int r)
+{
+	struct slimpro_i2c_dev *ctx = to_slimpro_i2c_dev(cl);
+
+	if (r == 0)
+		ctx->i2c_tx_poll = 0;
+}
+
+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;
+	} 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;
+		}
+		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_rd(struct slimpro_i2c_dev *ctx, u32 chip,
+				u32 addr, u32 addrlen, u32 protocol,
+				u32 readlen, u32 *data)
+{
+	u32 msg[3];
+	int rc;
+
+	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_ADDR(addr);
+	msg[2] = 0;
+	ctx->resp_msg = data;
+	if (ctx->mbox_client.tx_block)
+		init_completion(&ctx->rd_complete);
+
+	rc = mbox_send_message(ctx->mbox_chan, &msg);
+	if (rc < 0)
+		goto err;
+
+	rc = start_i2c_msg_xfer(ctx);
+err:
+	ctx->resp_msg = NULL;
+	return rc;
+}
+
+static int slimpro_i2c_wr(struct slimpro_i2c_dev *ctx, u32 chip,
+			  u32 addr, u32 addrlen, u32 protocol, u32 writelen,
+			  u32 data)
+{
+	u32 msg[3];
+	int rc;
+
+	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_ADDR(addr);
+	msg[2] = data;
+	ctx->resp_msg = msg;
+
+	if (ctx->mbox_client.tx_block)
+		init_completion(&ctx->rd_complete);
+
+	rc = mbox_send_message(ctx->mbox_chan, &msg);
+	if (rc < 0)
+		goto err;
+
+	rc = start_i2c_msg_xfer(ctx);
+err:
+	ctx->resp_msg = NULL;
+	return rc;
+}
+
+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;
+		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);
+
+	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;
+		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);
+
+	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 int xgene_slimpro_i2c_xfer(struct i2c_adapter *adap, u16 addr,
+				  unsigned short flags, char read_write,
+				  u8 command, int size,
+				  union i2c_smbus_data *data)
+{
+	struct slimpro_i2c_dev *ctx = i2c_get_adapdata(adap);
+	int ret = -EOPNOTSUPP;
+	u32 val;
+
+	switch (size) {
+	case I2C_SMBUS_BYTE:
+		if (read_write == I2C_SMBUS_READ) {
+			ret = slimpro_i2c_rd(ctx, addr, 0, 0,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     BYTE_DATA, &val);
+			data->byte = val;
+		} else {
+			ret = slimpro_i2c_wr(ctx, addr, command, SMBUS_CMD_LEN,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     0, 0);
+		}
+		break;
+	case I2C_SMBUS_BYTE_DATA:
+		if (read_write == I2C_SMBUS_READ) {
+			ret = slimpro_i2c_rd(ctx, addr, command, SMBUS_CMD_LEN,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     BYTE_DATA, &val);
+			data->byte = val;
+		} else {
+			val = data->byte;
+			ret = slimpro_i2c_wr(ctx, addr, command, SMBUS_CMD_LEN,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     BYTE_DATA, val);
+		}
+		break;
+	case I2C_SMBUS_WORD_DATA:
+		if (read_write == I2C_SMBUS_READ) {
+			ret = slimpro_i2c_rd(ctx, addr, command, SMBUS_CMD_LEN,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     WORD_DATA, &val);
+			data->word = val;
+		} else {
+			val = data->word;
+			ret = slimpro_i2c_wr(ctx, addr, command, SMBUS_CMD_LEN,
+					     SLIMPRO_IIC_SMB_PROTOCOL,
+					     WORD_DATA, val);
+		}
+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		if (read_write == I2C_SMBUS_READ) {
+			ret = slimpro_i2c_blkrd(ctx, addr, command,
+						SMBUS_CMD_LEN,
+						SLIMPRO_IIC_SMB_PROTOCOL,
+						I2C_SMBUS_BLOCK_MAX + 1,
+						IIC_SMB_WITH_DATA_LEN,
+						&data->block[0]);
+
+		} else {
+			ret = slimpro_i2c_blkwr(ctx, addr, command,
+						SMBUS_CMD_LEN,
+						SLIMPRO_IIC_SMB_PROTOCOL,
+						data->block[0] + 1,
+						&data->block[0]);
+		}
+		break;
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		if (read_write == I2C_SMBUS_READ) {
+			ret = slimpro_i2c_blkrd(ctx, addr,
+						command,
+						SMBUS_CMD_LEN,
+						SLIMPRO_IIC_I2C_PROTOCOL,
+						I2C_SMBUS_BLOCK_MAX,
+						IIC_SMB_WITHOUT_DATA_LEN,
+						&data->block[1]);
+		} else {
+			ret = slimpro_i2c_blkwr(ctx, addr, command,
+						SMBUS_CMD_LEN,
+						SLIMPRO_IIC_I2C_PROTOCOL,
+						data->block[0],
+						&data->block[1]);
+		}
+		break;
+	default:
+		break;
+	}
+	return ret;
+}
+
+/*
+* Return list of supported functionality.
+*/
+static u32 xgene_slimpro_i2c_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SMBUS_BYTE_DATA |
+		I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_BLOCK_DATA |
+		I2C_FUNC_SMBUS_I2C_BLOCK;
+}
+
+static struct i2c_algorithm xgene_slimpro_i2c_algorithm = {
+	.smbus_xfer = xgene_slimpro_i2c_xfer,
+	.functionality = xgene_slimpro_i2c_func,
+};
+
+static int __init xgene_slimpro_i2c_probe(struct platform_device *pdev)
+{
+	struct slimpro_i2c_dev *ctx;
+	struct i2c_adapter *adapter;
+	struct mbox_client *cl;
+	int rc;
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->dev = &pdev->dev;
+	platform_set_drvdata(pdev, ctx);
+	cl = &ctx->mbox_client;
+
+	/* Request mailbox channel */
+	cl->dev = &pdev->dev;
+	cl->rx_callback = slimpro_i2c_rx_cb;
+	cl->tx_done = slimpro_i2c_tx_done;
+	cl->tx_block = true;
+	cl->tx_tout = SLIMPRO_OP_TO_MS;
+	cl->knows_txdone = false;
+	cl->chan_name = "i2c-slimpro";
+	ctx->mbox_chan = mbox_request_channel(cl);
+	if (IS_ERR(ctx->mbox_chan)) {
+		dev_err(&pdev->dev, "SLIMpro mailbox channel request failed\n");
+		return PTR_ERR(ctx->mbox_chan);
+	}
+
+	/* Initialiation in case of using poll mode */
+	ctx->i2c_rx_poll = 0;
+	ctx->i2c_tx_poll = 0;
+	spin_lock_init(&ctx->lock);
+
+	/* Setup I2C adapter */
+	adapter = &ctx->adapter;
+	snprintf(adapter->name, sizeof(adapter->name), "X-Gene SLIMpro I2C");
+	adapter->algo = &xgene_slimpro_i2c_algorithm;
+	adapter->class = I2C_CLASS_HWMON;
+	adapter->dev.parent = &pdev->dev;
+	i2c_set_adapdata(adapter, ctx);
+	rc = i2c_add_adapter(adapter);
+	if (rc) {
+		dev_err(&pdev->dev, "Adapter registeration failed\n");
+		return rc;
+	}
+
+	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+	if (rc)
+		dev_warn(&pdev->dev, "Unable to set dma mask\n");
+
+	dev_info(&pdev->dev, "APM X-Gene SLIMpro I2C Adapter registered\n");
+	return 0;
+}
+
+static int xgene_slimpro_i2c_remove(struct platform_device *pdev)
+{
+	struct slimpro_i2c_dev *ctx = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&ctx->adapter);
+
+	mbox_free_channel(ctx->mbox_chan);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id xgene_slimpro_i2c_id[] = {
+	{.compatible = "apm,xgene-slimpro-i2c" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids);
+#endif
+
+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,
+		.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");
-- 
1.9.1

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 7664 bytes --]

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@apm.com>
> Signed-off-by: Hieu Le <hnle@apm.com>

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@apm.com>
> + * Author: Hieu Le <hnle@apm.com>
> + *
> + * 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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[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@apm.com>
> Signed-off-by: Hieu Le <hnle@apm.com>

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@apm.com>
> + * Author: Hieu Le <hnle@apm.com>
> + *
> + * 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141111/de6c5f01/attachment.sig>

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

[Feng Kan]

On Tue, Nov 11, 2014 at 12:32 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> 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@apm.com>
>> Signed-off-by: Hieu Le <hnle@apm.com>
>
> 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.
This is a SoC chip but not for embedded platform. Also the fact that this
is not a traditional I2C driver, but one that tunnels to the helper cpu using
mailbox to obtain I2C information. Do you still want me to group it with the
embedded drivers?
>
>>  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@apm.com>
>> + * Author: Hieu Le <hnle@apm.com>
>> + *
>> + * 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
>

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

[Feng Kan]

On Tue, Nov 11, 2014 at 12:32 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> 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@apm.com>
>> Signed-off-by: Hieu Le <hnle@apm.com>
>
> 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.
This is a SoC chip but not for embedded platform. Also the fact that this
is not a traditional I2C driver, but one that tunnels to the helper cpu using
mailbox to obtain I2C information. Do you still want me to group it with the
embedded drivers?
>
>>  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@apm.com>
>> + * Author: Hieu Le <hnle@apm.com>
>> + *
>> + * 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
>

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

[Feng Kan]

On Tue, Nov 11, 2014 at 12:32 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> 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@apm.com>
>> Signed-off-by: Hieu Le <hnle@apm.com>
>
> 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.
This is a SoC chip but not for embedded platform. Also the fact that this
is not a traditional I2C driver, but one that tunnels to the helper cpu using
mailbox to obtain I2C information. Do you still want me to group it with the
embedded drivers?
>
>>  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@apm.com>
>> + * Author: Hieu Le <hnle@apm.com>
>> + *
>> + * 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
>

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

[Feng Kan]

On Tue, Nov 11, 2014 at 12:32 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> 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@apm.com>
>> Signed-off-by: Hieu Le <hnle@apm.com>
>
> 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.
Sorry, having let this dangle for  a while.  I will fix up comments and submit
another version. I can move this to embedded, however Xgene is
suppose to be a server class chip.
>
>>  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@apm.com>
>> + * Author: Hieu Le <hnle@apm.com>
>> + *
>> + * 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.
Will fix the rest of the comments, thanks
>
> Thanks,
>
>    Wolfram
>

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

[Feng Kan]

On Tue, Nov 11, 2014 at 12:32 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> 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@apm.com>
>> Signed-off-by: Hieu Le <hnle@apm.com>
>
> 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.
Sorry, having let this dangle for  a while.  I will fix up comments and submit
another version. I can move this to embedded, however Xgene is
suppose to be a server class chip.
>
>>  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@apm.com>
>> + * Author: Hieu Le <hnle@apm.com>
>> + *
>> + * 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.
Will fix the rest of the comments, thanks
>
> Thanks,
>
>    Wolfram
>

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

[Feng Kan]

On Tue, Nov 11, 2014 at 12:32 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> 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@apm.com>
>> Signed-off-by: Hieu Le <hnle@apm.com>
>
> 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.
Sorry, having let this dangle for  a while.  I will fix up comments and submit
another version. I can move this to embedded, however Xgene is
suppose to be a server class chip.
>
>>  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@apm.com>
>> + * Author: Hieu Le <hnle@apm.com>
>> + *
>> + * 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.
Will fix the rest of the comments, thanks
>
> Thanks,
>
>    Wolfram
>

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 291 bytes --]


> > I guess this is a driver for embedded? If so, please sort it properly.
> Sorry, having let this dangle for  a while.  I will fix up comments and submit
> another version. I can move this to embedded, however Xgene is
> suppose to be a server class chip.

Then simply leave it as is :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Wolfram Sang]

> > I guess this is a driver for embedded? If so, please sort it properly.
> Sorry, having let this dangle for  a while.  I will fix up comments and submit
> another version. I can move this to embedded, however Xgene is
> suppose to be a server class chip.

Then simply leave it as is :)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150109/c44a27a2/attachment.sig>

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 291 bytes --]


> > I guess this is a driver for embedded? If so, please sort it properly.
> Sorry, having let this dangle for  a while.  I will fix up comments and submit
> another version. I can move this to embedded, however Xgene is
> suppose to be a server class chip.

Then simply leave it as is :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Arnd Bergmann]

On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
> 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

Why this dependency on XGENE_SLIMPRO_MBOX?

Better replace it with a dependency on MAILBOX.

> +	} 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);
> +		}

No, you can't block the CPU for an extended amount of time with
interrupts disabled. Please kill this code.

> +	ctx->resp_msg = data;
> +	if (ctx->mbox_client.tx_block)
> +		init_completion(&ctx->rd_complete);

reinit_completion()?

> +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);

ctx->dev is probably the wrong device here. The i2c controller is not
DMA capable itself, you need to have a pointer to the device that actually
performs the DMA here.


> +	/* Request mailbox channel */
> +	cl->dev = &pdev->dev;
> +	cl->rx_callback = slimpro_i2c_rx_cb;
> +	cl->tx_done = slimpro_i2c_tx_done;
> +	cl->tx_block = true;
> +	cl->tx_tout = SLIMPRO_OP_TO_MS;
> +	cl->knows_txdone = false;
> +	cl->chan_name = "i2c-slimpro";
> +	ctx->mbox_chan = mbox_request_channel(cl);

This is not the correct interface.

> +	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> +	if (rc)
> +		dev_warn(&pdev->dev, "Unable to set dma mask\n");

Are you sure that this is the correct device to perform the DMA?

Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd
function passes in only the lower 32 bit of the address, which would
be DMA_BIT_MASK(32).

> +#ifdef CONFIG_OF
> +static struct of_device_id xgene_slimpro_i2c_id[] = {
> +	{.compatible = "apm,xgene-slimpro-i2c" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids);
> +#endif
> +
> +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,
> +		.of_match_table = of_match_ptr(xgene_slimpro_i2c_id)
> +	},
> +};

The driver only supports DT, so just drop the #ifdef and the of_match_ptr().

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

[Arnd Bergmann]

On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
> 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

Why this dependency on XGENE_SLIMPRO_MBOX?

Better replace it with a dependency on MAILBOX.

> +	} 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);
> +		}

No, you can't block the CPU for an extended amount of time with
interrupts disabled. Please kill this code.

> +	ctx->resp_msg = data;
> +	if (ctx->mbox_client.tx_block)
> +		init_completion(&ctx->rd_complete);

reinit_completion()?

> +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);

ctx->dev is probably the wrong device here. The i2c controller is not
DMA capable itself, you need to have a pointer to the device that actually
performs the DMA here.


> +	/* Request mailbox channel */
> +	cl->dev = &pdev->dev;
> +	cl->rx_callback = slimpro_i2c_rx_cb;
> +	cl->tx_done = slimpro_i2c_tx_done;
> +	cl->tx_block = true;
> +	cl->tx_tout = SLIMPRO_OP_TO_MS;
> +	cl->knows_txdone = false;
> +	cl->chan_name = "i2c-slimpro";
> +	ctx->mbox_chan = mbox_request_channel(cl);

This is not the correct interface.

> +	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> +	if (rc)
> +		dev_warn(&pdev->dev, "Unable to set dma mask\n");

Are you sure that this is the correct device to perform the DMA?

Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd
function passes in only the lower 32 bit of the address, which would
be DMA_BIT_MASK(32).

> +#ifdef CONFIG_OF
> +static struct of_device_id xgene_slimpro_i2c_id[] = {
> +	{.compatible = "apm,xgene-slimpro-i2c" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids);
> +#endif
> +
> +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,
> +		.of_match_table = of_match_ptr(xgene_slimpro_i2c_id)
> +	},
> +};

The driver only supports DT, so just drop the #ifdef and the of_match_ptr().

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

[Arnd Bergmann]

On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
> 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

Why this dependency on XGENE_SLIMPRO_MBOX?

Better replace it with a dependency on MAILBOX.

> +	} 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);
> +		}

No, you can't block the CPU for an extended amount of time with
interrupts disabled. Please kill this code.

> +	ctx->resp_msg = data;
> +	if (ctx->mbox_client.tx_block)
> +		init_completion(&ctx->rd_complete);

reinit_completion()?

> +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);

ctx->dev is probably the wrong device here. The i2c controller is not
DMA capable itself, you need to have a pointer to the device that actually
performs the DMA here.


> +	/* Request mailbox channel */
> +	cl->dev = &pdev->dev;
> +	cl->rx_callback = slimpro_i2c_rx_cb;
> +	cl->tx_done = slimpro_i2c_tx_done;
> +	cl->tx_block = true;
> +	cl->tx_tout = SLIMPRO_OP_TO_MS;
> +	cl->knows_txdone = false;
> +	cl->chan_name = "i2c-slimpro";
> +	ctx->mbox_chan = mbox_request_channel(cl);

This is not the correct interface.

> +	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> +	if (rc)
> +		dev_warn(&pdev->dev, "Unable to set dma mask\n");

Are you sure that this is the correct device to perform the DMA?

Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd
function passes in only the lower 32 bit of the address, which would
be DMA_BIT_MASK(32).

> +#ifdef CONFIG_OF
> +static struct of_device_id xgene_slimpro_i2c_id[] = {
> +	{.compatible = "apm,xgene-slimpro-i2c" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids);
> +#endif
> +
> +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,
> +		.of_match_table = of_match_ptr(xgene_slimpro_i2c_id)
> +	},
> +};

The driver only supports DT, so just drop the #ifdef and the of_match_ptr().

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

[Feng Kan]

On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
>> 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
>
> Why this dependency on XGENE_SLIMPRO_MBOX?
>
> Better replace it with a dependency on MAILBOX.
Arnd, just a question. Is this because this possibly help with future
compatibility by choosing a more broad dependency?
>
>> +     } 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);
>> +             }
>
> No, you can't block the CPU for an extended amount of time with
> interrupts disabled. Please kill this code.
>
>> +     ctx->resp_msg = data;
>> +     if (ctx->mbox_client.tx_block)
>> +             init_completion(&ctx->rd_complete);
>
> reinit_completion()?
>
>> +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);
>
> ctx->dev is probably the wrong device here. The i2c controller is not
> DMA capable itself, you need to have a pointer to the device that actually
> performs the DMA here.
>
>
>> +     /* Request mailbox channel */
>> +     cl->dev = &pdev->dev;
>> +     cl->rx_callback = slimpro_i2c_rx_cb;
>> +     cl->tx_done = slimpro_i2c_tx_done;
>> +     cl->tx_block = true;
>> +     cl->tx_tout = SLIMPRO_OP_TO_MS;
>> +     cl->knows_txdone = false;
>> +     cl->chan_name = "i2c-slimpro";
>> +     ctx->mbox_chan = mbox_request_channel(cl);
>
> This is not the correct interface.
>
>> +     rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +     if (rc)
>> +             dev_warn(&pdev->dev, "Unable to set dma mask\n");
>
> Are you sure that this is the correct device to perform the DMA?
>
> Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd
> function passes in only the lower 32 bit of the address, which would
> be DMA_BIT_MASK(32).
>
>> +#ifdef CONFIG_OF
>> +static struct of_device_id xgene_slimpro_i2c_id[] = {
>> +     {.compatible = "apm,xgene-slimpro-i2c" },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids);
>> +#endif
>> +
>> +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,
>> +             .of_match_table = of_match_ptr(xgene_slimpro_i2c_id)
>> +     },
>> +};
>
> The driver only supports DT, so just drop the #ifdef and the of_match_ptr().
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Feng Kan]

On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
>> 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
>
> Why this dependency on XGENE_SLIMPRO_MBOX?
>
> Better replace it with a dependency on MAILBOX.
Arnd, just a question. Is this because this possibly help with future
compatibility by choosing a more broad dependency?
>
>> +     } 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);
>> +             }
>
> No, you can't block the CPU for an extended amount of time with
> interrupts disabled. Please kill this code.
>
>> +     ctx->resp_msg = data;
>> +     if (ctx->mbox_client.tx_block)
>> +             init_completion(&ctx->rd_complete);
>
> reinit_completion()?
>
>> +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);
>
> ctx->dev is probably the wrong device here. The i2c controller is not
> DMA capable itself, you need to have a pointer to the device that actually
> performs the DMA here.
>
>
>> +     /* Request mailbox channel */
>> +     cl->dev = &pdev->dev;
>> +     cl->rx_callback = slimpro_i2c_rx_cb;
>> +     cl->tx_done = slimpro_i2c_tx_done;
>> +     cl->tx_block = true;
>> +     cl->tx_tout = SLIMPRO_OP_TO_MS;
>> +     cl->knows_txdone = false;
>> +     cl->chan_name = "i2c-slimpro";
>> +     ctx->mbox_chan = mbox_request_channel(cl);
>
> This is not the correct interface.
>
>> +     rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +     if (rc)
>> +             dev_warn(&pdev->dev, "Unable to set dma mask\n");
>
> Are you sure that this is the correct device to perform the DMA?
>
> Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd
> function passes in only the lower 32 bit of the address, which would
> be DMA_BIT_MASK(32).
>
>> +#ifdef CONFIG_OF
>> +static struct of_device_id xgene_slimpro_i2c_id[] = {
>> +     {.compatible = "apm,xgene-slimpro-i2c" },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids);
>> +#endif
>> +
>> +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,
>> +             .of_match_table = of_match_ptr(xgene_slimpro_i2c_id)
>> +     },
>> +};
>
> The driver only supports DT, so just drop the #ifdef and the of_match_ptr().
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Feng Kan]

On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
>> 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
>
> Why this dependency on XGENE_SLIMPRO_MBOX?
>
> Better replace it with a dependency on MAILBOX.
Arnd, just a question. Is this because this possibly help with future
compatibility by choosing a more broad dependency?
>
>> +     } 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);
>> +             }
>
> No, you can't block the CPU for an extended amount of time with
> interrupts disabled. Please kill this code.
>
>> +     ctx->resp_msg = data;
>> +     if (ctx->mbox_client.tx_block)
>> +             init_completion(&ctx->rd_complete);
>
> reinit_completion()?
>
>> +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);
>
> ctx->dev is probably the wrong device here. The i2c controller is not
> DMA capable itself, you need to have a pointer to the device that actually
> performs the DMA here.
>
>
>> +     /* Request mailbox channel */
>> +     cl->dev = &pdev->dev;
>> +     cl->rx_callback = slimpro_i2c_rx_cb;
>> +     cl->tx_done = slimpro_i2c_tx_done;
>> +     cl->tx_block = true;
>> +     cl->tx_tout = SLIMPRO_OP_TO_MS;
>> +     cl->knows_txdone = false;
>> +     cl->chan_name = "i2c-slimpro";
>> +     ctx->mbox_chan = mbox_request_channel(cl);
>
> This is not the correct interface.
>
>> +     rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +     if (rc)
>> +             dev_warn(&pdev->dev, "Unable to set dma mask\n");
>
> Are you sure that this is the correct device to perform the DMA?
>
> Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd
> function passes in only the lower 32 bit of the address, which would
> be DMA_BIT_MASK(32).
>
>> +#ifdef CONFIG_OF
>> +static struct of_device_id xgene_slimpro_i2c_id[] = {
>> +     {.compatible = "apm,xgene-slimpro-i2c" },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids);
>> +#endif
>> +
>> +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,
>> +             .of_match_table = of_match_ptr(xgene_slimpro_i2c_id)
>> +     },
>> +};
>
> The driver only supports DT, so just drop the #ifdef and the of_match_ptr().
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

On Friday 09 January 2015 10:56:51 Feng Kan wrote:
> On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
> >> 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
> >
> > Why this dependency on XGENE_SLIMPRO_MBOX?
> >
> > Better replace it with a dependency on MAILBOX.
> Arnd, just a question. Is this because this possibly help with future
> compatibility by choosing a more broad dependency?

The dependency should ideally describe build-time dependencies,
to make it possible to build on other architectures for static
code analysis purposes. If the driver makes no sense on other
platforms you can also use

	depends on ARCH_XGENE || COMPILE_TEST
	depends on MAILBOX

to cover both the build-time and run-time dependencies. But the
dependency on XGENE_SLIMPRO_MBOX just shouldn't be there, the driver
will work with any mailbox implementation if someone puts the same
hardware into a different SoC.

	Arnd

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

[Arnd Bergmann]

On Friday 09 January 2015 10:56:51 Feng Kan wrote:
> On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
> >> 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
> >
> > Why this dependency on XGENE_SLIMPRO_MBOX?
> >
> > Better replace it with a dependency on MAILBOX.
> Arnd, just a question. Is this because this possibly help with future
> compatibility by choosing a more broad dependency?

The dependency should ideally describe build-time dependencies,
to make it possible to build on other architectures for static
code analysis purposes. If the driver makes no sense on other
platforms you can also use

	depends on ARCH_XGENE || COMPILE_TEST
	depends on MAILBOX

to cover both the build-time and run-time dependencies. But the
dependency on XGENE_SLIMPRO_MBOX just shouldn't be there, the driver
will work with any mailbox implementation if someone puts the same
hardware into a different SoC.

	Arnd

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

[Arnd Bergmann]

On Friday 09 January 2015 10:56:51 Feng Kan wrote:
> On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
> >> 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
> >
> > Why this dependency on XGENE_SLIMPRO_MBOX?
> >
> > Better replace it with a dependency on MAILBOX.
> Arnd, just a question. Is this because this possibly help with future
> compatibility by choosing a more broad dependency?

The dependency should ideally describe build-time dependencies,
to make it possible to build on other architectures for static
code analysis purposes. If the driver makes no sense on other
platforms you can also use

	depends on ARCH_XGENE || COMPILE_TEST
	depends on MAILBOX

to cover both the build-time and run-time dependencies. But the
dependency on XGENE_SLIMPRO_MBOX just shouldn't be there, the driver
will work with any mailbox implementation if someone puts the same
hardware into a different SoC.

	Arnd

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

[Feng Kan]

On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
>> 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
>
> Why this dependency on XGENE_SLIMPRO_MBOX?

>> +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);
>
> ctx->dev is probably the wrong device here. The i2c controller is not
> DMA capable itself, you need to have a pointer to the device that actually
> performs the DMA here.

Arnd, I do agree this may not be the best identification. There is no
representation for this on the Linux side. I could put it as NULL. However,
there are other i2c bus drivers that seem to do the same thing. Please
let me know what you think.

>
>
>> +     /* Request mailbox channel */
>> +     cl->dev = &pdev->dev;
>> +     cl->rx_callback = slimpro_i2c_rx_cb;
>> +     cl->tx_done = slimpro_i2c_tx_done;
>> +     cl->tx_block = true;
>> +     cl->tx_tout = SLIMPRO_OP_TO_MS;
>> +     cl->knows_txdone = false;
>> +     cl->chan_name = "i2c-slimpro";
>> +     ctx->mbox_chan = mbox_request_channel(cl);
>
> This is not the correct interface.
>
>> +     rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +     if (rc)
>> +             dev_warn(&pdev->dev, "Unable to set dma mask\n");
>
> Are you sure that this is the correct device to perform the DMA?
>
> Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd
> function passes in only the lower 32 bit of the address, which would
> be DMA_BIT_MASK(32).
The upper part of the address is passed in as part of the msg[1]

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

[Feng Kan]

On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
>> 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
>
> Why this dependency on XGENE_SLIMPRO_MBOX?

>> +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);
>
> ctx->dev is probably the wrong device here. The i2c controller is not
> DMA capable itself, you need to have a pointer to the device that actually
> performs the DMA here.

Arnd, I do agree this may not be the best identification. There is no
representation for this on the Linux side. I could put it as NULL. However,
there are other i2c bus drivers that seem to do the same thing. Please
let me know what you think.

>
>
>> +     /* Request mailbox channel */
>> +     cl->dev = &pdev->dev;
>> +     cl->rx_callback = slimpro_i2c_rx_cb;
>> +     cl->tx_done = slimpro_i2c_tx_done;
>> +     cl->tx_block = true;
>> +     cl->tx_tout = SLIMPRO_OP_TO_MS;
>> +     cl->knows_txdone = false;
>> +     cl->chan_name = "i2c-slimpro";
>> +     ctx->mbox_chan = mbox_request_channel(cl);
>
> This is not the correct interface.
>
>> +     rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +     if (rc)
>> +             dev_warn(&pdev->dev, "Unable to set dma mask\n");
>
> Are you sure that this is the correct device to perform the DMA?
>
> Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd
> function passes in only the lower 32 bit of the address, which would
> be DMA_BIT_MASK(32).
The upper part of the address is passed in as part of the msg[1]

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

[Feng Kan]

On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
>> 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
>
> Why this dependency on XGENE_SLIMPRO_MBOX?

>> +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);
>
> ctx->dev is probably the wrong device here. The i2c controller is not
> DMA capable itself, you need to have a pointer to the device that actually
> performs the DMA here.

Arnd, I do agree this may not be the best identification. There is no
representation for this on the Linux side. I could put it as NULL. However,
there are other i2c bus drivers that seem to do the same thing. Please
let me know what you think.

>
>
>> +     /* Request mailbox channel */
>> +     cl->dev = &pdev->dev;
>> +     cl->rx_callback = slimpro_i2c_rx_cb;
>> +     cl->tx_done = slimpro_i2c_tx_done;
>> +     cl->tx_block = true;
>> +     cl->tx_tout = SLIMPRO_OP_TO_MS;
>> +     cl->knows_txdone = false;
>> +     cl->chan_name = "i2c-slimpro";
>> +     ctx->mbox_chan = mbox_request_channel(cl);
>
> This is not the correct interface.
>
>> +     rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +     if (rc)
>> +             dev_warn(&pdev->dev, "Unable to set dma mask\n");
>
> Are you sure that this is the correct device to perform the DMA?
>
> Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd
> function passes in only the lower 32 bit of the address, which would
> be DMA_BIT_MASK(32).
The upper part of the address is passed in as part of the msg[1]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]


> > ctx->dev is probably the wrong device here. The i2c controller is not
> > DMA capable itself, you need to have a pointer to the device that actually
> > performs the DMA here.
> 
> Arnd, I do agree this may not be the best identification. There is no
> representation for this on the Linux side. I could put it as NULL. However,
> there are other i2c bus drivers that seem to do the same thing. Please
> let me know what you think.

For completeness: Those drivers need fixing!

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Wolfram Sang]

> > ctx->dev is probably the wrong device here. The i2c controller is not
> > DMA capable itself, you need to have a pointer to the device that actually
> > performs the DMA here.
> 
> Arnd, I do agree this may not be the best identification. There is no
> representation for this on the Linux side. I could put it as NULL. However,
> there are other i2c bus drivers that seem to do the same thing. Please
> let me know what you think.

For completeness: Those drivers need fixing!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150130/41b913cb/attachment.sig>

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]


> > ctx->dev is probably the wrong device here. The i2c controller is not
> > DMA capable itself, you need to have a pointer to the device that actually
> > performs the DMA here.
> 
> Arnd, I do agree this may not be the best identification. There is no
> representation for this on the Linux side. I could put it as NULL. However,
> there are other i2c bus drivers that seem to do the same thing. Please
> let me know what you think.

For completeness: Those drivers need fixing!

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Feng Kan]

On Thu, Jan 29, 2015 at 10:11 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > ctx->dev is probably the wrong device here. The i2c controller is not
>> > DMA capable itself, you need to have a pointer to the device that actually
>> > performs the DMA here.
>>
>> Arnd, I do agree this may not be the best identification. There is no
>> representation for this on the Linux side. I could put it as NULL. However,
>> there are other i2c bus drivers that seem to do the same thing. Please
>> let me know what you think.
>
> For completeness: Those drivers need fixing!
I have some doubts. I could hack up a dts node for the helper
processor that is doing the dma. However, the dma master could be
anything for that matter. I am not sure how I can write this in a
generic manner.

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

[Feng Kan]

On Thu, Jan 29, 2015 at 10:11 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > ctx->dev is probably the wrong device here. The i2c controller is not
>> > DMA capable itself, you need to have a pointer to the device that actually
>> > performs the DMA here.
>>
>> Arnd, I do agree this may not be the best identification. There is no
>> representation for this on the Linux side. I could put it as NULL. However,
>> there are other i2c bus drivers that seem to do the same thing. Please
>> let me know what you think.
>
> For completeness: Those drivers need fixing!
I have some doubts. I could hack up a dts node for the helper
processor that is doing the dma. However, the dma master could be
anything for that matter. I am not sure how I can write this in a
generic manner.

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

[Feng Kan]

On Thu, Jan 29, 2015 at 10:11 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
>> > ctx->dev is probably the wrong device here. The i2c controller is not
>> > DMA capable itself, you need to have a pointer to the device that actually
>> > performs the DMA here.
>>
>> Arnd, I do agree this may not be the best identification. There is no
>> representation for this on the Linux side. I could put it as NULL. However,
>> there are other i2c bus drivers that seem to do the same thing. Please
>> let me know what you think.
>
> For completeness: Those drivers need fixing!
I have some doubts. I could hack up a dts node for the helper
processor that is doing the dma. However, the dma master could be
anything for that matter. I am not sure how I can write this in a
generic manner.

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

On Mon, Feb 02, 2015 at 02:15:44PM -0800, Feng Kan wrote:
> On Thu, Jan 29, 2015 at 10:11 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> >
> >> > ctx->dev is probably the wrong device here. The i2c controller is not
> >> > DMA capable itself, you need to have a pointer to the device that actually
> >> > performs the DMA here.
> >>
> >> Arnd, I do agree this may not be the best identification. There is no
> >> representation for this on the Linux side. I could put it as NULL. However,
> >> there are other i2c bus drivers that seem to do the same thing. Please
> >> let me know what you think.
> >
> > For completeness: Those drivers need fixing!
> I have some doubts. I could hack up a dts node for the helper
> processor that is doing the dma. However, the dma master could be
> anything for that matter. I am not sure how I can write this in a
> generic manner.

All I wanted to say is that, to the best of my knowledge, currently all
DMA capable i2c bus drivers which do not pass the DMA capable device
when mapping, need a fix like 8cfcae9f0595. So, the argument "other
drivers do it, too" is invalid IMO. You might have other, better, more
technical arguments for your case. Like above, that could be fine, I'd
say. But I leave this to the DMA experts...


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Wolfram Sang]

On Mon, Feb 02, 2015 at 02:15:44PM -0800, Feng Kan wrote:
> On Thu, Jan 29, 2015 at 10:11 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> >
> >> > ctx->dev is probably the wrong device here. The i2c controller is not
> >> > DMA capable itself, you need to have a pointer to the device that actually
> >> > performs the DMA here.
> >>
> >> Arnd, I do agree this may not be the best identification. There is no
> >> representation for this on the Linux side. I could put it as NULL. However,
> >> there are other i2c bus drivers that seem to do the same thing. Please
> >> let me know what you think.
> >
> > For completeness: Those drivers need fixing!
> I have some doubts. I could hack up a dts node for the helper
> processor that is doing the dma. However, the dma master could be
> anything for that matter. I am not sure how I can write this in a
> generic manner.

All I wanted to say is that, to the best of my knowledge, currently all
DMA capable i2c bus drivers which do not pass the DMA capable device
when mapping, need a fix like 8cfcae9f0595. So, the argument "other
drivers do it, too" is invalid IMO. You might have other, better, more
technical arguments for your case. Like above, that could be fine, I'd
say. But I leave this to the DMA experts...

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150203/3aff6ca8/attachment.sig>

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

On Mon, Feb 02, 2015 at 02:15:44PM -0800, Feng Kan wrote:
> On Thu, Jan 29, 2015 at 10:11 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> >
> >> > ctx->dev is probably the wrong device here. The i2c controller is not
> >> > DMA capable itself, you need to have a pointer to the device that actually
> >> > performs the DMA here.
> >>
> >> Arnd, I do agree this may not be the best identification. There is no
> >> representation for this on the Linux side. I could put it as NULL. However,
> >> there are other i2c bus drivers that seem to do the same thing. Please
> >> let me know what you think.
> >
> > For completeness: Those drivers need fixing!
> I have some doubts. I could hack up a dts node for the helper
> processor that is doing the dma. However, the dma master could be
> anything for that matter. I am not sure how I can write this in a
> generic manner.

All I wanted to say is that, to the best of my knowledge, currently all
DMA capable i2c bus drivers which do not pass the DMA capable device
when mapping, need a fix like 8cfcae9f0595. So, the argument "other
drivers do it, too" is invalid IMO. You might have other, better, more
technical arguments for your case. Like above, that could be fine, I'd
say. But I leave this to the DMA experts...


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 5/6] Documentation: i2c: Add APM X-Gene platform SLIMpro I2C driver documentation

[Feng Kan]

Add APM X-Gene platform SLIMpro I2C driver documentation.

Signed-off-by: Feng Kan <fkan@apm.com>
Signed-off-by: Hieu Le <hnle@apm.com>
---
 .../devicetree/bindings/i2c/i2c-xgene-slimpro.txt    | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
new file mode 100644
index 0000000..1a79d53
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
@@ -0,0 +1,20 @@
+APM X-Gene SLIMpro Mailbox I2C Driver
+
+This is a I2C driver that access the I2C bus through the mailbox mechanism.
+There is documentation of the mailbox driver in the
+Documentation/devicetree/binding/mailbox/xgene-slimpro-mbox.txt
+
+Required properties :
+
+ - compatible : should be "apm,xgene-slimpro-i2c"
+ - mbox : ptr to the mailbox dts node, use the name of the mailbox as the
+	  first parameter. The second parameter is the channel number.
+	  The APM X-Gene SLIMpro mailbox has 8 channels.
+ - mbox-names : the name of the mailbox channel.
+
+Example :
+	i2cslimpro {
+		compatible = "apm,xgene-slimpro-i2c";
+		mbox = <&mailbox 0>;
+		mbox-names = "i2c-slimpro";
+	};
-- 
1.9.1

[PATCH 5/6] Documentation: i2c: Add APM X-Gene platform SLIMpro I2C driver documentation

[Feng Kan]

Add APM X-Gene platform SLIMpro I2C driver documentation.

Signed-off-by: Feng Kan <fkan@apm.com>
Signed-off-by: Hieu Le <hnle@apm.com>
---
 .../devicetree/bindings/i2c/i2c-xgene-slimpro.txt    | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
new file mode 100644
index 0000000..1a79d53
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
@@ -0,0 +1,20 @@
+APM X-Gene SLIMpro Mailbox I2C Driver
+
+This is a I2C driver that access the I2C bus through the mailbox mechanism.
+There is documentation of the mailbox driver in the
+Documentation/devicetree/binding/mailbox/xgene-slimpro-mbox.txt
+
+Required properties :
+
+ - compatible : should be "apm,xgene-slimpro-i2c"
+ - mbox : ptr to the mailbox dts node, use the name of the mailbox as the
+	  first parameter. The second parameter is the channel number.
+	  The APM X-Gene SLIMpro mailbox has 8 channels.
+ - mbox-names : the name of the mailbox channel.
+
+Example :
+	i2cslimpro {
+		compatible = "apm,xgene-slimpro-i2c";
+		mbox = <&mailbox 0>;
+		mbox-names = "i2c-slimpro";
+	};
-- 
1.9.1

Re: [PATCH 5/6] Documentation: i2c: Add APM X-Gene platform SLIMpro I2C driver documentation

[Mark Rutland]

On Wed, Oct 08, 2014 at 01:06:48AM +0100, Feng Kan wrote:
> Add APM X-Gene platform SLIMpro I2C driver documentation.
> 
> Signed-off-by: Feng Kan <fkan@apm.com>
> Signed-off-by: Hieu Le <hnle@apm.com>
> ---
>  .../devicetree/bindings/i2c/i2c-xgene-slimpro.txt    | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> new file mode 100644
> index 0000000..1a79d53
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> @@ -0,0 +1,20 @@
> +APM X-Gene SLIMpro Mailbox I2C Driver
> +
> +This is a I2C driver that access the I2C bus through the mailbox mechanism.
> +There is documentation of the mailbox driver in the
> +Documentation/devicetree/binding/mailbox/xgene-slimpro-mbox.txt

This should not describe the driver. It should describe the hardware (an
I2C controller accessed over the "slimpro" mailbox).

> +
> +Required properties :
> +
> + - compatible : should be "apm,xgene-slimpro-i2c"
> + - mbox : ptr to the mailbox dts node, use the name of the mailbox as the
> +	  first parameter. The second parameter is the channel number.

This is a phandle + specificer pair, the "name" you mention is a label
reference (which generates a phandle).

> +	  The APM X-Gene SLIMpro mailbox has 8 channels.

Does it really make sense to model the slimpro in this way if all the
clients are going to rely on the particulars of this mailbox?

Either this is somewhat generic and the mailbox details are
unimportant, or it is not and the entire device would be better modelled
as an MFD.

> + - mbox-names : the name of the mailbox channel.

For any *-names property, you must define the _exact_ names you expect,
or you shouldn't use the property. The entire point of the property is
to disambiguate entries, and this is _more_ ambiguous than without the
property.

Mark.

> +
> +Example :
> +	i2cslimpro {
> +		compatible = "apm,xgene-slimpro-i2c";
> +		mbox = <&mailbox 0>;
> +		mbox-names = "i2c-slimpro";
> +	};
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 5/6] Documentation: i2c: Add APM X-Gene platform SLIMpro I2C driver documentation

[Mark Rutland]

On Wed, Oct 08, 2014 at 01:06:48AM +0100, Feng Kan wrote:
> Add APM X-Gene platform SLIMpro I2C driver documentation.
> 
> Signed-off-by: Feng Kan <fkan@apm.com>
> Signed-off-by: Hieu Le <hnle@apm.com>
> ---
>  .../devicetree/bindings/i2c/i2c-xgene-slimpro.txt    | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> new file mode 100644
> index 0000000..1a79d53
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> @@ -0,0 +1,20 @@
> +APM X-Gene SLIMpro Mailbox I2C Driver
> +
> +This is a I2C driver that access the I2C bus through the mailbox mechanism.
> +There is documentation of the mailbox driver in the
> +Documentation/devicetree/binding/mailbox/xgene-slimpro-mbox.txt

This should not describe the driver. It should describe the hardware (an
I2C controller accessed over the "slimpro" mailbox).

> +
> +Required properties :
> +
> + - compatible : should be "apm,xgene-slimpro-i2c"
> + - mbox : ptr to the mailbox dts node, use the name of the mailbox as the
> +	  first parameter. The second parameter is the channel number.

This is a phandle + specificer pair, the "name" you mention is a label
reference (which generates a phandle).

> +	  The APM X-Gene SLIMpro mailbox has 8 channels.

Does it really make sense to model the slimpro in this way if all the
clients are going to rely on the particulars of this mailbox?

Either this is somewhat generic and the mailbox details are
unimportant, or it is not and the entire device would be better modelled
as an MFD.

> + - mbox-names : the name of the mailbox channel.

For any *-names property, you must define the _exact_ names you expect,
or you shouldn't use the property. The entire point of the property is
to disambiguate entries, and this is _more_ ambiguous than without the
property.

Mark.

> +
> +Example :
> +	i2cslimpro {
> +		compatible = "apm,xgene-slimpro-i2c";
> +		mbox = <&mailbox 0>;
> +		mbox-names = "i2c-slimpro";
> +	};
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 5/6] Documentation: i2c: Add APM X-Gene platform SLIMpro I2C driver documentation

[Mark Rutland]

On Wed, Oct 08, 2014 at 01:06:48AM +0100, Feng Kan wrote:
> Add APM X-Gene platform SLIMpro I2C driver documentation.
> 
> Signed-off-by: Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>
> Signed-off-by: Hieu Le <hnle-qTEPVZfXA3Y@public.gmane.org>
> ---
>  .../devicetree/bindings/i2c/i2c-xgene-slimpro.txt    | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> new file mode 100644
> index 0000000..1a79d53
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> @@ -0,0 +1,20 @@
> +APM X-Gene SLIMpro Mailbox I2C Driver
> +
> +This is a I2C driver that access the I2C bus through the mailbox mechanism.
> +There is documentation of the mailbox driver in the
> +Documentation/devicetree/binding/mailbox/xgene-slimpro-mbox.txt

This should not describe the driver. It should describe the hardware (an
I2C controller accessed over the "slimpro" mailbox).

> +
> +Required properties :
> +
> + - compatible : should be "apm,xgene-slimpro-i2c"
> + - mbox : ptr to the mailbox dts node, use the name of the mailbox as the
> +	  first parameter. The second parameter is the channel number.

This is a phandle + specificer pair, the "name" you mention is a label
reference (which generates a phandle).

> +	  The APM X-Gene SLIMpro mailbox has 8 channels.

Does it really make sense to model the slimpro in this way if all the
clients are going to rely on the particulars of this mailbox?

Either this is somewhat generic and the mailbox details are
unimportant, or it is not and the entire device would be better modelled
as an MFD.

> + - mbox-names : the name of the mailbox channel.

For any *-names property, you must define the _exact_ names you expect,
or you shouldn't use the property. The entire point of the property is
to disambiguate entries, and this is _more_ ambiguous than without the
property.

Mark.

> +
> +Example :
> +	i2cslimpro {
> +		compatible = "apm,xgene-slimpro-i2c";
> +		mbox = <&mailbox 0>;
> +		mbox-names = "i2c-slimpro";
> +	};
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 5/6] Documentation: i2c: Add APM X-Gene platform SLIMpro I2C driver documentation

[Arnd Bergmann]

On Tuesday 07 October 2014 17:06:48 Feng Kan wrote:
> Add APM X-Gene platform SLIMpro I2C driver documentation.
> 

Don't just repeat the subject line, explain what this is for.

> Signed-off-by: Feng Kan <fkan@apm.com>
> Signed-off-by: Hieu Le <hnle@apm.com>
> ---
>  .../devicetree/bindings/i2c/i2c-xgene-slimpro.txt    | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> new file mode 100644
> index 0000000..1a79d53
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> @@ -0,0 +1,20 @@
> +APM X-Gene SLIMpro Mailbox I2C Driver
> +
> +This is a I2C driver that access the I2C bus through the mailbox mechanism.
> +There is documentation of the mailbox driver in the
> +Documentation/devicetree/binding/mailbox/xgene-slimpro-mbox.txt
> +
> +Required properties :
> +
> + - compatible : should be "apm,xgene-slimpro-i2c"
> + - mbox : ptr to the mailbox dts node, use the name of the mailbox as the
> +	  first parameter. The second parameter is the channel number.
> +	  The APM X-Gene SLIMpro mailbox has 8 channels.
> + - mbox-names : the name of the mailbox channel.

The current form of the mailbox interface no longer uses mbox-names, so
just drop that. In the old form, the binding would have been incomplete
because you don't list the required name.

	Arnd

[PATCH 5/6] Documentation: i2c: Add APM X-Gene platform SLIMpro I2C driver documentation

[Arnd Bergmann]

On Tuesday 07 October 2014 17:06:48 Feng Kan wrote:
> Add APM X-Gene platform SLIMpro I2C driver documentation.
> 

Don't just repeat the subject line, explain what this is for.

> Signed-off-by: Feng Kan <fkan@apm.com>
> Signed-off-by: Hieu Le <hnle@apm.com>
> ---
>  .../devicetree/bindings/i2c/i2c-xgene-slimpro.txt    | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> new file mode 100644
> index 0000000..1a79d53
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> @@ -0,0 +1,20 @@
> +APM X-Gene SLIMpro Mailbox I2C Driver
> +
> +This is a I2C driver that access the I2C bus through the mailbox mechanism.
> +There is documentation of the mailbox driver in the
> +Documentation/devicetree/binding/mailbox/xgene-slimpro-mbox.txt
> +
> +Required properties :
> +
> + - compatible : should be "apm,xgene-slimpro-i2c"
> + - mbox : ptr to the mailbox dts node, use the name of the mailbox as the
> +	  first parameter. The second parameter is the channel number.
> +	  The APM X-Gene SLIMpro mailbox has 8 channels.
> + - mbox-names : the name of the mailbox channel.

The current form of the mailbox interface no longer uses mbox-names, so
just drop that. In the old form, the binding would have been incomplete
because you don't list the required name.

	Arnd

Re: [PATCH 5/6] Documentation: i2c: Add APM X-Gene platform SLIMpro I2C driver documentation

[Arnd Bergmann]

On Tuesday 07 October 2014 17:06:48 Feng Kan wrote:
> Add APM X-Gene platform SLIMpro I2C driver documentation.
> 

Don't just repeat the subject line, explain what this is for.

> Signed-off-by: Feng Kan <fkan-qTEPVZfXA3Y@public.gmane.org>
> Signed-off-by: Hieu Le <hnle-qTEPVZfXA3Y@public.gmane.org>
> ---
>  .../devicetree/bindings/i2c/i2c-xgene-slimpro.txt    | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> new file mode 100644
> index 0000000..1a79d53
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-xgene-slimpro.txt
> @@ -0,0 +1,20 @@
> +APM X-Gene SLIMpro Mailbox I2C Driver
> +
> +This is a I2C driver that access the I2C bus through the mailbox mechanism.
> +There is documentation of the mailbox driver in the
> +Documentation/devicetree/binding/mailbox/xgene-slimpro-mbox.txt
> +
> +Required properties :
> +
> + - compatible : should be "apm,xgene-slimpro-i2c"
> + - mbox : ptr to the mailbox dts node, use the name of the mailbox as the
> +	  first parameter. The second parameter is the channel number.
> +	  The APM X-Gene SLIMpro mailbox has 8 channels.
> + - mbox-names : the name of the mailbox channel.

The current form of the mailbox interface no longer uses mbox-names, so
just drop that. In the old form, the binding would have been incomplete
because you don't list the required name.

	Arnd

[PATCH 6/6] arm64: dts: add proxy I2C device driver on APM X-Gene platform

[Feng Kan]

Add proxy I2C device driver on APM X-Gene platform.

Signed-off-by: Feng Kan <fkan@apm.com>
Signed-off-by: Hieu Le <hnle@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index 249f025..09daf4c 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -469,6 +469,12 @@
 					<0x0 0x7 0x4>;
 		};
 
+		i2cslimpro {
+			compatible = "apm,xgene-slimpro-i2c";
+			mbox = <&mailbox 0>;
+			mbox-names = "i2c-slimpro";
+		};
+
 		serial0: serial@1c020000 {
 			status = "disabled";
 			device_type = "serial";
-- 
1.9.1

[PATCH 6/6] arm64: dts: add proxy I2C device driver on APM X-Gene platform

[Feng Kan]

Add proxy I2C device driver on APM X-Gene platform.

Signed-off-by: Feng Kan <fkan@apm.com>
Signed-off-by: Hieu Le <hnle@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index 249f025..09daf4c 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -469,6 +469,12 @@
 					<0x0 0x7 0x4>;
 		};
 
+		i2cslimpro {
+			compatible = "apm,xgene-slimpro-i2c";
+			mbox = <&mailbox 0>;
+			mbox-names = "i2c-slimpro";
+		};
+
 		serial0: serial at 1c020000 {
 			status = "disabled";
 			device_type = "serial";
-- 
1.9.1