Re: [PATCH 1/3] i2c: exynos5: add High Speed I2C controller driver
Hello Felipe, Thomas,
Thanks for your time and valuable comments,
I will post the next version with you comments addressed.
On 27 November 2012 19:04, Thomas Abraham wrote:
>
> On 27 November 2012 18:30, Naveen Krishna Chatradhi
> wrote:
> > Adds support for High Speed I2C driver found in Exynos5 and later
> > SoCs from Samsung. This driver currently supports Auto mode.
> >
> > Driver only supports Device Tree method.
> >
> > Signed-off-by: Taekgyun Ko
> > Signed-off-by: Naveen Krishna Chatradhi
> > ---
> > drivers/i2c/busses/Kconfig |6 +
> > drivers/i2c/busses/Makefile |1 +
> > drivers/i2c/busses/i2c-exynos5.c | 758
> > ++
> > drivers/i2c/busses/i2c-exynos5.h | 80
> > 4 files changed, 845 insertions(+)
> > create mode 100644 drivers/i2c/busses/i2c-exynos5.c
> > create mode 100644 drivers/i2c/busses/i2c-exynos5.h
> >
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 65dd599..88e8833 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -609,6 +609,12 @@ config I2C_S3C2410
> > Say Y here to include support for I2C controller in the
> > Samsung SoCs.
> >
> > +config I2C_EXYNOS5
> > + tristate "Exynos5 HS-I2C Driver"
> > + help
> > + Say Y here to include support for High Speed I2C controller in
> > the
> > + Exynos5 series SoCs from Samsung.
> > +
> > config I2C_S6000
> > tristate "S6000 I2C support"
> > depends on XTENSA_VARIANT_S6000
> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index 2d33d62..426b4fd 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -60,6 +60,7 @@ obj-$(CONFIG_I2C_PUV3)+= i2c-puv3.o
> > obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
> > obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o
> > obj-$(CONFIG_I2C_S3C2410) += i2c-s3c2410.o
> > +obj-$(CONFIG_I2C_EXYNOS5) += i2c-exynos5.o
> > obj-$(CONFIG_I2C_S6000)+= i2c-s6000.o
> > obj-$(CONFIG_I2C_SH7760) += i2c-sh7760.o
> > obj-$(CONFIG_I2C_SH_MOBILE)+= i2c-sh_mobile.o
> > diff --git a/drivers/i2c/busses/i2c-exynos5.c
> > b/drivers/i2c/busses/i2c-exynos5.c
> > new file mode 100644
> > index 000..5983aa9
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-exynos5.c
> > @@ -0,0 +1,758 @@
> > +/* linux/drivers/i2c/busses/i2c-exynos5.c
> > + *
> > + * Copyright (C) 2012 Samsung Electronics Co., Ltd.
> > + *
> > + * Exynos5 series High Speed I2C controller driver
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > +*/
> > +
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include "i2c-exynos5.h"
> > +
> > +#define HSI2C_POLLING 0
> > +#define HSI2C_FAST_SPD 0
> > +#define HSI2C_HIGH_SPD 1
> > +
> > +/* Max time to wait for bus to become idle after a xfer */
> > +#define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(1000))
> > +
> > +struct exynos5_i2c {
> > + unsigned intsuspended:1;
> > +
> > + struct i2c_msg *msg;
> > + struct completion msg_complete;
> > + unsigned intmsg_byte_ptr;
> > +
> > + unsigned intirq;
> > +
> > + void __iomem*regs;
> > + struct clk *clk;
> > + struct device *dev;
> > + struct resource *ioarea;
> > + struct i2c_adapter adap;
> > + unsigned intbus_number;
> > + unsigned intspeed_mode;
> > + unsigned intfast_speed;
> > + unsigned inthigh_speed;
> > + int operation_mode;
> > + int gpios[2];
> > +};
> > +
> > +static struct platform_device_id exynos5_driver_ids[] = {
> > + {
> > + .name = "exynos5-hs-i2c",
> > + .driver_data= 0,
> > + }, { },
> > +};
> > +MODULE_DEVICE_TABLE(platform, exynos5_driver_ids);
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id exynos5_i2c_match[] = {
> > + { .compatible = "samsung,exynos5-hs-i2c", .data = (void *)0 },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, exynos5_i2c_match);
> > +#endif
> > +
> > +static inline void dump_i2c_register(struct exynos5_i2c *i2c)
> > +{
> > + dev_dbg(i2c->dev, "Register dump(%d) :\n %x\n %x\n %x\n %x\n"
> > + " %x\n %x\n %x\n %x\n %x\n"
> > + " %x\n %x\n %x\n %x\n %x\n"
> > + " %x\n %x\n %x\n %x\n %x\n"
> > + " %x\n %x\n %x\n %x\n %x\n"
> > +
Re: [PATCH 1/3] i2c: exynos5: add High Speed I2C controller driver
On 27 November 2012 18:30, Naveen Krishna Chatradhi
wrote:
> Adds support for High Speed I2C driver found in Exynos5 and later
> SoCs from Samsung. This driver currently supports Auto mode.
>
> Driver only supports Device Tree method.
>
> Signed-off-by: Taekgyun Ko
> Signed-off-by: Naveen Krishna Chatradhi
> ---
> drivers/i2c/busses/Kconfig |6 +
> drivers/i2c/busses/Makefile |1 +
> drivers/i2c/busses/i2c-exynos5.c | 758
> ++
> drivers/i2c/busses/i2c-exynos5.h | 80
> 4 files changed, 845 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-exynos5.c
> create mode 100644 drivers/i2c/busses/i2c-exynos5.h
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 65dd599..88e8833 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -609,6 +609,12 @@ config I2C_S3C2410
> Say Y here to include support for I2C controller in the
> Samsung SoCs.
>
> +config I2C_EXYNOS5
> + tristate "Exynos5 HS-I2C Driver"
> + help
> + Say Y here to include support for High Speed I2C controller in the
> + Exynos5 series SoCs from Samsung.
> +
> config I2C_S6000
> tristate "S6000 I2C support"
> depends on XTENSA_VARIANT_S6000
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 2d33d62..426b4fd 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -60,6 +60,7 @@ obj-$(CONFIG_I2C_PUV3)+= i2c-puv3.o
> obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
> obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o
> obj-$(CONFIG_I2C_S3C2410) += i2c-s3c2410.o
> +obj-$(CONFIG_I2C_EXYNOS5) += i2c-exynos5.o
> obj-$(CONFIG_I2C_S6000)+= i2c-s6000.o
> obj-$(CONFIG_I2C_SH7760) += i2c-sh7760.o
> obj-$(CONFIG_I2C_SH_MOBILE)+= i2c-sh_mobile.o
> diff --git a/drivers/i2c/busses/i2c-exynos5.c
> b/drivers/i2c/busses/i2c-exynos5.c
> new file mode 100644
> index 000..5983aa9
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-exynos5.c
> @@ -0,0 +1,758 @@
> +/* linux/drivers/i2c/busses/i2c-exynos5.c
> + *
> + * Copyright (C) 2012 Samsung Electronics Co., Ltd.
> + *
> + * Exynos5 series High Speed I2C controller driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include "i2c-exynos5.h"
> +
> +#define HSI2C_POLLING 0
> +#define HSI2C_FAST_SPD 0
> +#define HSI2C_HIGH_SPD 1
> +
> +/* Max time to wait for bus to become idle after a xfer */
> +#define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(1000))
> +
> +struct exynos5_i2c {
> + unsigned intsuspended:1;
> +
> + struct i2c_msg *msg;
> + struct completion msg_complete;
> + unsigned intmsg_byte_ptr;
> +
> + unsigned intirq;
> +
> + void __iomem*regs;
> + struct clk *clk;
> + struct device *dev;
> + struct resource *ioarea;
> + struct i2c_adapter adap;
> + unsigned intbus_number;
> + unsigned intspeed_mode;
> + unsigned intfast_speed;
> + unsigned inthigh_speed;
> + int operation_mode;
> + int gpios[2];
> +};
> +
> +static struct platform_device_id exynos5_driver_ids[] = {
> + {
> + .name = "exynos5-hs-i2c",
> + .driver_data= 0,
> + }, { },
> +};
> +MODULE_DEVICE_TABLE(platform, exynos5_driver_ids);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id exynos5_i2c_match[] = {
> + { .compatible = "samsung,exynos5-hs-i2c", .data = (void *)0 },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, exynos5_i2c_match);
> +#endif
> +
> +static inline void dump_i2c_register(struct exynos5_i2c *i2c)
> +{
> + dev_dbg(i2c->dev, "Register dump(%d) :\n %x\n %x\n %x\n %x\n"
> + " %x\n %x\n %x\n %x\n %x\n"
> + " %x\n %x\n %x\n %x\n %x\n"
> + " %x\n %x\n %x\n %x\n %x\n"
> + " %x\n %x\n %x\n %x\n %x\n"
> + , i2c->suspended
> + , readl(i2c->regs + HSI2C_CTL)
> + , readl(i2c->regs + HSI2C_FIFO_CTL)
> + , readl(i2c->regs + HSI2C_TRAILIG_CTL)
> + , readl(i2c->regs + HSI2C_CLK_CTL)
> + , readl(i2c->regs + HSI2C_CLK_SLOT)
> + , readl(i2c->regs + HSI2C_INT_ENABLE)
> + , readl(i2c->regs + HSI2C_INT_STATUS)
> + , readl(i2c->regs + HSI2C_ERR_STATUS)
> +
Re: [PATCH 1/3] i2c: exynos5: add High Speed I2C controller driver
Hi,
On Tue, Nov 27, 2012 at 06:30:34PM +0530, Naveen Krishna Chatradhi wrote:
> diff --git a/drivers/i2c/busses/i2c-exynos5.c
> b/drivers/i2c/busses/i2c-exynos5.c
> new file mode 100644
> index 000..5983aa9
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-exynos5.c
> @@ -0,0 +1,758 @@
> +/* linux/drivers/i2c/busses/i2c-exynos5.c
> + *
> + * Copyright (C) 2012 Samsung Electronics Co., Ltd.
> + *
> + * Exynos5 series High Speed I2C controller driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
are you sure you want to include this header ? Just making sure...
> +#include "i2c-exynos5.h"
> +
> +#define HSI2C_POLLING 0
> +#define HSI2C_FAST_SPD 0
> +#define HSI2C_HIGH_SPD 1
> +
> +/* Max time to wait for bus to become idle after a xfer */
> +#define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(1000))
> +
> +struct exynos5_i2c {
> + unsigned intsuspended:1;
> +
> + struct i2c_msg *msg;
> + struct completion msg_complete;
> + unsigned intmsg_byte_ptr;
> +
> + unsigned intirq;
> +
> + void __iomem*regs;
> + struct clk *clk;
> + struct device *dev;
> + struct resource *ioarea;
you don't need to save the struct resource here, see more below.
> + struct i2c_adapter adap;
I would put this as the first memeber of the structure, so that a
container_of() gets optmized into a cast.
> + unsigned intbus_number;
> + unsigned intspeed_mode;
> + unsigned intfast_speed;
> + unsigned inthigh_speed;
> + int operation_mode;
> + int gpios[2];
> +};
> +
> +static struct platform_device_id exynos5_driver_ids[] = {
> + {
> + .name = "exynos5-hs-i2c",
> + .driver_data= 0,
no need to zero-initialize.
> + }, { },
> +};
> +MODULE_DEVICE_TABLE(platform, exynos5_driver_ids);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id exynos5_i2c_match[] = {
> + { .compatible = "samsung,exynos5-hs-i2c", .data = (void *)0 },
remove the (void *)0 initialization. the variable is static and will be
zero-initialized.
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, exynos5_i2c_match);
> +#endif
> +
> +static inline void dump_i2c_register(struct exynos5_i2c *i2c)
> +{
> + dev_dbg(i2c->dev, "Register dump(%d) :\n %x\n %x\n %x\n %x\n"
> + " %x\n %x\n %x\n %x\n %x\n"
> + " %x\n %x\n %x\n %x\n %x\n"
> + " %x\n %x\n %x\n %x\n %x\n"
> + " %x\n %x\n %x\n %x\n %x\n"
> + , i2c->suspended
> + , readl(i2c->regs + HSI2C_CTL)
> + , readl(i2c->regs + HSI2C_FIFO_CTL)
> + , readl(i2c->regs + HSI2C_TRAILIG_CTL)
> + , readl(i2c->regs + HSI2C_CLK_CTL)
> + , readl(i2c->regs + HSI2C_CLK_SLOT)
> + , readl(i2c->regs + HSI2C_INT_ENABLE)
> + , readl(i2c->regs + HSI2C_INT_STATUS)
> + , readl(i2c->regs + HSI2C_ERR_STATUS)
> + , readl(i2c->regs + HSI2C_FIFO_STATUS)
> + , readl(i2c->regs + HSI2C_TX_DATA)
> + , readl(i2c->regs + HSI2C_RX_DATA)
> + , readl(i2c->regs + HSI2C_CONF)
> + , readl(i2c->regs + HSI2C_AUTO_CONFING)
> + , readl(i2c->regs + HSI2C_TIMEOUT)
> + , readl(i2c->regs + HSI2C_MANUAL_CMD)
> + , readl(i2c->regs + HSI2C_TRANS_STATUS)
> + , readl(i2c->regs + HSI2C_TIMING_HS1)
> + , readl(i2c->regs + HSI2C_TIMING_HS2)
> + , readl(i2c->regs + HSI2C_TIMING_HS3)
> + , readl(i2c->regs + HSI2C_TIMING_FS1)
> + , readl(i2c->regs + HSI2C_TIMING_FS2)
> + , readl(i2c->regs + HSI2C_TIMING_FS3)
> + , readl(i2c->regs + HSI2C_TIMING_SLA)
> + , readl(i2c->regs + HSI2C_ADDR));
> +}
I would make this dev_vdbg() since this is really only needed when
something is terribly wrong. In fact, I don't like these big register
dump functions in code. To me that's something that should be done on
debugfs.
Also, your comma character placement is a bit weird. Usually it goes to
end of the line, not to the beginning of the next.
> +static inline void exynos5_i2c_stop(struct exynos5_i2c *i2c)
> +{
> + writel(0, i2c->regs + HSI2C_INT_ENABLE);
> +
> + complete(&i2c->msg_complete);
> +}
> +
> +static inline void exynos5_disable_irq(struct exynos5_i2c *i2c)
> +{
> + unsigned long tmp = readl(i2c->regs + HSI2C_INT_STATUS);
> +
> + writel(tmp, i2c->regs + HSI2C_INT_
[PATCH 1/3] i2c: exynos5: add High Speed I2C controller driver
Adds support for High Speed I2C driver found in Exynos5 and later
SoCs from Samsung. This driver currently supports Auto mode.
Driver only supports Device Tree method.
Signed-off-by: Taekgyun Ko
Signed-off-by: Naveen Krishna Chatradhi
---
drivers/i2c/busses/Kconfig |6 +
drivers/i2c/busses/Makefile |1 +
drivers/i2c/busses/i2c-exynos5.c | 758 ++
drivers/i2c/busses/i2c-exynos5.h | 80
4 files changed, 845 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-exynos5.c
create mode 100644 drivers/i2c/busses/i2c-exynos5.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 65dd599..88e8833 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -609,6 +609,12 @@ config I2C_S3C2410
Say Y here to include support for I2C controller in the
Samsung SoCs.
+config I2C_EXYNOS5
+ tristate "Exynos5 HS-I2C Driver"
+ help
+ Say Y here to include support for High Speed I2C controller in the
+ Exynos5 series SoCs from Samsung.
+
config I2C_S6000
tristate "S6000 I2C support"
depends on XTENSA_VARIANT_S6000
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 2d33d62..426b4fd 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_I2C_PUV3)+= i2c-puv3.o
obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o
obj-$(CONFIG_I2C_S3C2410) += i2c-s3c2410.o
+obj-$(CONFIG_I2C_EXYNOS5) += i2c-exynos5.o
obj-$(CONFIG_I2C_S6000)+= i2c-s6000.o
obj-$(CONFIG_I2C_SH7760) += i2c-sh7760.o
obj-$(CONFIG_I2C_SH_MOBILE)+= i2c-sh_mobile.o
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
new file mode 100644
index 000..5983aa9
--- /dev/null
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -0,0 +1,758 @@
+/* linux/drivers/i2c/busses/i2c-exynos5.c
+ *
+ * Copyright (C) 2012 Samsung Electronics Co., Ltd.
+ *
+ * Exynos5 series High Speed I2C controller driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include "i2c-exynos5.h"
+
+#define HSI2C_POLLING 0
+#define HSI2C_FAST_SPD 0
+#define HSI2C_HIGH_SPD 1
+
+/* Max time to wait for bus to become idle after a xfer */
+#define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(1000))
+
+struct exynos5_i2c {
+ unsigned intsuspended:1;
+
+ struct i2c_msg *msg;
+ struct completion msg_complete;
+ unsigned intmsg_byte_ptr;
+
+ unsigned intirq;
+
+ void __iomem*regs;
+ struct clk *clk;
+ struct device *dev;
+ struct resource *ioarea;
+ struct i2c_adapter adap;
+ unsigned intbus_number;
+ unsigned intspeed_mode;
+ unsigned intfast_speed;
+ unsigned inthigh_speed;
+ int operation_mode;
+ int gpios[2];
+};
+
+static struct platform_device_id exynos5_driver_ids[] = {
+ {
+ .name = "exynos5-hs-i2c",
+ .driver_data= 0,
+ }, { },
+};
+MODULE_DEVICE_TABLE(platform, exynos5_driver_ids);
+
+#ifdef CONFIG_OF
+static const struct of_device_id exynos5_i2c_match[] = {
+ { .compatible = "samsung,exynos5-hs-i2c", .data = (void *)0 },
+ {},
+};
+MODULE_DEVICE_TABLE(of, exynos5_i2c_match);
+#endif
+
+static inline void dump_i2c_register(struct exynos5_i2c *i2c)
+{
+ dev_dbg(i2c->dev, "Register dump(%d) :\n %x\n %x\n %x\n %x\n"
+ " %x\n %x\n %x\n %x\n %x\n"
+ " %x\n %x\n %x\n %x\n %x\n"
+ " %x\n %x\n %x\n %x\n %x\n"
+ " %x\n %x\n %x\n %x\n %x\n"
+ , i2c->suspended
+ , readl(i2c->regs + HSI2C_CTL)
+ , readl(i2c->regs + HSI2C_FIFO_CTL)
+ , readl(i2c->regs + HSI2C_TRAILIG_CTL)
+ , readl(i2c->regs + HSI2C_CLK_CTL)
+ , readl(i2c->regs + HSI2C_CLK_SLOT)
+ , readl(i2c->regs + HSI2C_INT_ENABLE)
+ , readl(i2c->regs + HSI2C_INT_STATUS)
+ , readl(i2c->regs + HSI2C_ERR_STATUS)
+ , readl(i2c->regs + HSI2C_FIFO_STATUS)
+ , readl(i2c->regs + HSI2C_TX_DATA)
+ , readl(i2c->regs + HSI2C_RX_DATA)
+ , readl(i2c->regs + HSI2C_CONF)
+ , readl(i2c->regs + HSI2C_AUTO_CONFING)
+ , readl(i2c->regs + HSI2C_TIMEOUT)
+ , readl(i2c->regs + HSI2
