Re: [PATCH 1/2] i2c: add i2c-lpc2k driver
Hi Wolfram,
On 15 August 2015 at 22:24, Wolfram Sang wrote:
> On Mon, Jul 13, 2015 at 12:47:21AM +0200, Joachim Eastwood wrote:
>> Add support for the I2C controller found on several NXP devices
>> including LPC2xxx, LPC178x/7x and LPC18xx/43xx. The controller
>> is implemented as a state machine and the driver act upon the
>> state changes when the bus is accessed.
>>
>> The I2C controller supports master/slave operation, bus
>> arbitration, programmable clock rate, and speeds up to 1 Mbit/s.
>>
>> Signed-off-by: Joachim Eastwood
>
> Thanks for the submission! Looking mostly good already.
>
>> +static void i2c_lpc2k_pump_msg(struct lpc2k_i2c *i2c)
>> +{
>> + unsigned char data;
>> + u32 status;
>> +
>> + /*
>> + * I2C in the LPC2xxx series is basically a state machine.
>> + * Just run through the steps based on the current status.
>> + */
>> + status = readl(i2c->reg_base + LPC24XX_I2STAT);
>> +
>> + switch (status) {
>> + case M_START:
>> + case M_REPSTART:
>> + /* Start bit was just sent out, send out addr and dir */
>> + data = (i2c->msg->addr << 1);
>> + if (i2c->msg->flags & I2C_M_RD)
>> + data |= 1;
>> +
>> + writel(data, i2c->reg_base + LPC24XX_I2DAT);
>> + writel(LPC24XX_STA, i2c->reg_base + LPC24XX_I2CONCLR);
>> +
>> + dev_dbg(&i2c->adap.dev, "Start sent with addr 0x%02x\n", data);
>
> I assume the driver is basically working. So, in my book, most of this
> debug output is useful when the driver was developed, not so much
> afterwards. Also, it is printed in interrupt context possibly causing
> latency. However, the information is useful for understanding the
> driver, so I'd suggest to convert them into comments?
I'll go through all the dev_dbg stuff with what you wrote in mind.
Think I'll end up with removing most of it.
Think most of the debugging stuff was added by Emcraft while they were
bring it up on lpc178x and lpc18xx.
>> + i2c->adap.nr = pdev->id;
>> +
>> + ret = i2c_add_numbered_adapter(&i2c->adap);
>
> Intended? Most DT enabled drivers simply user i2c_add_adapter(). The
> core then takes care of aliases defined in DT.
No, it's not intended. It's an old driver and I just didn't know about
i2c_add_adapter(). I'll change it in the next version.
> And please have a look at Documentation/i2c/fault-codes. Arbitration
> Lost should be -EAGAIN, NACK should be -ENXIO, and so forth...
Ok, will do.
Thanks for looking, Wolfram. I'll address your comments and try to
send out a new version tomorrow.
regards,
Joachim Eastwood
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] i2c: add i2c-lpc2k driver
On Mon, Jul 13, 2015 at 12:47:21AM +0200, Joachim Eastwood wrote:
> Add support for the I2C controller found on several NXP devices
> including LPC2xxx, LPC178x/7x and LPC18xx/43xx. The controller
> is implemented as a state machine and the driver act upon the
> state changes when the bus is accessed.
>
> The I2C controller supports master/slave operation, bus
> arbitration, programmable clock rate, and speeds up to 1 Mbit/s.
>
> Signed-off-by: Joachim Eastwood
Thanks for the submission! Looking mostly good already.
> +static void i2c_lpc2k_pump_msg(struct lpc2k_i2c *i2c)
> +{
> + unsigned char data;
> + u32 status;
> +
> + /*
> + * I2C in the LPC2xxx series is basically a state machine.
> + * Just run through the steps based on the current status.
> + */
> + status = readl(i2c->reg_base + LPC24XX_I2STAT);
> +
> + switch (status) {
> + case M_START:
> + case M_REPSTART:
> + /* Start bit was just sent out, send out addr and dir */
> + data = (i2c->msg->addr << 1);
> + if (i2c->msg->flags & I2C_M_RD)
> + data |= 1;
> +
> + writel(data, i2c->reg_base + LPC24XX_I2DAT);
> + writel(LPC24XX_STA, i2c->reg_base + LPC24XX_I2CONCLR);
> +
> + dev_dbg(&i2c->adap.dev, "Start sent with addr 0x%02x\n", data);
I assume the driver is basically working. So, in my book, most of this
debug output is useful when the driver was developed, not so much
afterwards. Also, it is printed in interrupt context possibly causing
latency. However, the information is useful for understanding the
driver, so I'd suggest to convert them into comments?
> + i2c->adap.nr = pdev->id;
> +
> + ret = i2c_add_numbered_adapter(&i2c->adap);
Intended? Most DT enabled drivers simply user i2c_add_adapter(). The
core then takes care of aliases defined in DT.
And please have a look at Documentation/i2c/fault-codes. Arbitration
Lost should be -EAGAIN, NACK should be -ENXIO, and so forth...
Thanks,
Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] i2c: add i2c-lpc2k driver
Add support for the I2C controller found on several NXP devices
including LPC2xxx, LPC178x/7x and LPC18xx/43xx. The controller
is implemented as a state machine and the driver act upon the
state changes when the bus is accessed.
The I2C controller supports master/slave operation, bus
arbitration, programmable clock rate, and speeds up to 1 Mbit/s.
Signed-off-by: Joachim Eastwood
---
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile| 1 +
drivers/i2c/busses/i2c-lpc2k.c | 551 +
3 files changed, 562 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-lpc2k.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 35ac23768ce9..a880c82bed3d 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -612,6 +612,16 @@ config I2C_KEMPLD
This driver can also be built as a module. If so, the module
will be called i2c-kempld.
+config I2C_LPC2K
+ tristate "I2C bus support for NXP LPC2K/LPC178x/18xx/43xx"
+ depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
+ help
+ This driver supports the I2C interface found several NXP
+ devices including LPC2xxx, LPC178x/7x and LPC18xx/43xx.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-lpc2k.
+
config I2C_MESON
tristate "Amlogic Meson I2C controller"
depends on ARCH_MESON
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e5f537c80da0..65b84097e665 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_I2C_IMX) += i2c-imx.o
obj-$(CONFIG_I2C_IOP3XX) += i2c-iop3xx.o
obj-$(CONFIG_I2C_JZ4780) += i2c-jz4780.o
obj-$(CONFIG_I2C_KEMPLD) += i2c-kempld.o
+obj-$(CONFIG_I2C_LPC2K)+= i2c-lpc2k.o
obj-$(CONFIG_I2C_MESON)+= i2c-meson.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
obj-$(CONFIG_I2C_MT65XX) += i2c-mt65xx.o
diff --git a/drivers/i2c/busses/i2c-lpc2k.c b/drivers/i2c/busses/i2c-lpc2k.c
new file mode 100644
index ..7bcbecc8dabe
--- /dev/null
+++ b/drivers/i2c/busses/i2c-lpc2k.c
@@ -0,0 +1,551 @@
+/*
+ * Copyright (C) 2011 NXP Semiconductors
+ *
+ * Code portions referenced from the i2x-pxa and i2c-pnx drivers
+ *
+ * Make SMBus byte and word transactions work on LPC178x/7x
+ * Copyright (c) 2012
+ * Alexander Potashev, Emcraft Systems, aspotas...@emcraft.com
+ * Anton Protopopov, Emcraft Systems, ant...@emcraft.com
+ *
+ * Copyright (C) 2015 Joachim Eastwood
+ *
+ * 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.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* LPC24xx register offsets and bits */
+#define LPC24XX_I2CONSET 0x00
+#define LPC24XX_I2STAT 0x04
+#define LPC24XX_I2DAT 0x08
+#define LPC24XX_I2ADDR 0x0c
+#define LPC24XX_I2SCLH 0x10
+#define LPC24XX_I2SCLL 0x14
+#define LPC24XX_I2CONCLR 0x18
+
+#define LPC24XX_AA BIT(2)
+#define LPC24XX_SI BIT(3)
+#define LPC24XX_STOBIT(4)
+#define LPC24XX_STABIT(5)
+#define LPC24XX_I2EN BIT(6)
+
+#define LPC24XX_CLEAR_ALL (LPC24XX_AA | LPC24XX_SI | LPC24XX_STO | \
+LPC24XX_STA | LPC24XX_I2EN)
+
+/* I2C SCL clock has different duty cycle depending on mode */
+#define I2C_STD_MODE_DUTY 46
+#define I2C_FAST_MODE_DUTY 32
+#define I2C_FAST_MODE_PLUS_DUTY34
+
+/*
+ * 26 possible I2C status codes, but codes applicable only
+ * to master are listed here and used in this driver
+ */
+enum {
+ M_BUS_ERROR = 0x00,
+ M_START = 0x08,
+ M_REPSTART = 0x10,
+ MX_ADDR_W_ACK = 0x18,
+ MX_ADDR_W_NACK = 0x20,
+ MX_DATA_W_ACK = 0x28,
+ MX_DATA_W_NACK = 0x30,
+ M_DATA_ARB_LOST = 0x38,
+ MR_ADDR_R_ACK = 0x40,
+ MR_ADDR_R_NACK = 0x48,
+ MR_DATA_R_ACK = 0x50,
+ MR_DATA_R_NACK = 0x58,
+ M_I2C_IDLE = 0xf8,
+};
+
+struct lpc2k_i2c {
+ void __iomem*reg_base;
+ struct clk *clk;
+ int irq;
+ wait_queue_head_t wait;
+ struct i2c_adapter adap;
+ struct i2c_msg *msg;
+ int msg_idx;
+ int msg_status;
+ int is_last;
+};
+
+static void i2c_lpc2k_reset(struct lpc2k_i2c *i2c)
+{
+ /* Will force clear all statuses */
