From: Felipe Balbi <balbi@ti.com>
To: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Cc: <linux-arm-kernel@lists.infradead.org>,
<linux-samsung-soc@vger.kernel.org>,
<devicetree-discuss@lists.ozlabs.org>,
<linux-i2c@vger.kernel.org>, <naveenkrishna.ch@gmail.com>,
<kgene.kim@samsung.com>, <grant.likely@secretlab.ca>,
<w.sang@pengutronix.de>, <linux-kernel@vger.kernel.org>,
<taeggyun.ko@samsung.com>
Subject: Re: [PATCH 1/3] i2c: exynos5: add High Speed I2C controller driver
Date: Tue, 27 Nov 2012 15:23:36 +0200 [thread overview]
Message-ID: <20121127132336.GB22556@arwen.pp.htv.fi> (raw)
In-Reply-To: <1354021236-28596-2-git-send-email-ch.naveen@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 25309 bytes --]
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 0000000..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 <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/time.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/of_i2c.h>
> +#include <linux/of_gpio.h>
> +
> +#include <asm/irq.h>
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 int suspended:1;
> +
> + struct i2c_msg *msg;
> + struct completion msg_complete;
> + unsigned int msg_byte_ptr;
> +
> + unsigned int irq;
> +
> + 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 int bus_number;
> + unsigned int speed_mode;
> + unsigned int fast_speed;
> + unsigned int high_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_STATUS);
> +}
> +
> +static void exynos5_i2c_en_timeout(struct exynos5_i2c *i2c)
> +{
> + unsigned long i2c_timeout = readl(i2c->regs + HSI2C_TIMEOUT);
> +
> + /* Clear to enable Timeout */
> + i2c_timeout &= ~HSI2C_TIMEOUT_EN;
> + writel(i2c_timeout, i2c->regs + HSI2C_TIMEOUT);
> +}
> +
> +static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
> +{
> + struct exynos5_i2c *i2c = dev_id;
> + unsigned char byte;
> +
> + if (i2c->msg->flags & I2C_M_RD) {
> + while ((readl(i2c->regs + HSI2C_FIFO_STATUS) &
> + 0x1000000) == 0) {
> + byte = (unsigned char)readl(i2c->regs + HSI2C_RX_DATA);
> + i2c->msg->buf[i2c->msg_byte_ptr++] = byte;
> + }
> +
> + if (i2c->msg_byte_ptr >= i2c->msg->len)
> + exynos5_i2c_stop(i2c);
> + } else {
> + byte = i2c->msg->buf[i2c->msg_byte_ptr++];
> + writel(byte, i2c->regs + HSI2C_TX_DATA);
> +
> + if (i2c->msg_byte_ptr >= i2c->msg->len)
> + exynos5_i2c_stop(i2c);
> + }
you never check which event generated the IRQ. Why don't you use
register address 0x24 ??
> + exynos5_disable_irq(i2c);
why are you masking IRQs here ?
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int exynos5_i2c_init(struct exynos5_i2c *i2c);
> +
> +static int exynos5_i2c_reset(struct exynos5_i2c *i2c)
> +{
> + unsigned long usi_ctl;
> +
> + usi_ctl = readl(i2c->regs + HSI2C_CTL);
> + usi_ctl |= (1u << 31);
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> + usi_ctl = readl(i2c->regs + HSI2C_CTL);
> + usi_ctl &= ~(1u << 31);
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> + exynos5_i2c_init(i2c);
> +
> + return 0;
> +}
> +
> +static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
> + struct i2c_msg *msgs, int num, int stop)
this "num" argument is useless.
> +{
> + unsigned long timeout;
> + unsigned long trans_status;
> + unsigned long usi_fifo_stat;
> + unsigned long usi_ctl;
> + unsigned long i2c_auto_conf;
> + unsigned long i2c_addr;
> + unsigned long usi_int_en;
> + unsigned long usi_fifo_ctl;
> + unsigned char byte;
> + int ret = 0;
> + int operation_mode = i2c->operation_mode;
> +
> + i2c->msg = msgs;
> + i2c->msg_byte_ptr = 0;
> +
> + init_completion(&i2c->msg_complete);
actually, init_completion() should be done on your probe(). Here you
should be calling INIT_COMPLETION() instead.
> + usi_ctl = readl(i2c->regs + HSI2C_CTL);
> + i2c_auto_conf = readl(i2c->regs + HSI2C_AUTO_CONFING);
do you really need to read these two registers ? You're programming a
new transfer, meaning that you're likely to change the entire
register...
> + exynos5_i2c_en_timeout(i2c);
> +
> + /* Set default trigger level for TXFIFO and RXFIFO */
> + usi_fifo_ctl = HSI2C_TXFIFO_TRIGGER_LEVEL |
> + HSI2C_RXFIFO_TRIGGER_LEVEL;
> + /* Enable RXFIFO and TXFIFO */
> + usi_fifo_ctl = HSI2C_RXFIFO_EN | HSI2C_TXFIFO_EN;
> +
> + writel(usi_fifo_ctl, i2c->regs + HSI2C_FIFO_CTL);
> +
> + usi_int_en = 0;
> + if (msgs->flags & I2C_M_RD) {
> + usi_ctl &= ~HSI2C_TXCHON;
> + usi_ctl |= HSI2C_RXCHON;
> +
> + i2c_auto_conf |= HSI2C_READ_WRITE;
> +
> + usi_int_en |= (HSI2C_INT_RX_ALMOSTFULL_EN |
> + HSI2C_INT_TRAILING_EN);
> + } else {
> + usi_ctl &= ~HSI2C_RXCHON;
> + usi_ctl |= HSI2C_TXCHON;
> +
> + i2c_auto_conf &= ~HSI2C_READ_WRITE;
> +
> + usi_int_en |= HSI2C_INT_TX_ALMOSTEMPTY_EN;
> + }
> +
> + if (stop == 1)
> + i2c_auto_conf |= HSI2C_STOP_AFTER_TRANS;
> + else
> + i2c_auto_conf &= ~HSI2C_STOP_AFTER_TRANS;
How about:
i2c_auto_conf |= stop ? HSI2C_STOP_AFTER_TRANS : 0; ??
> +
> +
one blank line only.
> + i2c_addr = readl(i2c->regs + HSI2C_ADDR);
why do you need to read it ? Is there more information in this register
other than the address ???
> + /* Clear Slave Address for I2C Master (Auto mode only) */
> + i2c_addr &= ~(0x3ff << 10);
> + /* Clear Slave Address for I2C Slave */
> + i2c_addr &= ~(0x3ff << 0);
> + /* Clear Master ID for I2C Master (Auto and HS mode only) */
> + if (i2c->speed_mode == HSI2C_HIGH_SPD)
> + i2c_addr &= ~(0xff << 24);
> +
> + i2c_addr |= ((msgs->addr & 0x7f) << 10);
> + writel(i2c_addr, i2c->regs + HSI2C_ADDR);
> +
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> +
> + /* Clear and set TRANS_LEN */
> + i2c_auto_conf &= ~(0xffff);
> + i2c_auto_conf |= i2c->msg->len;
> + writel(i2c_auto_conf, i2c->regs + HSI2C_AUTO_CONFING);
> +
> + /* Start data transfer in Master mode */
> + i2c_auto_conf = readl(i2c->regs + HSI2C_AUTO_CONFING);
> + i2c_auto_conf |= HSI2C_MASTER_RUN;
> + writel(i2c_auto_conf, i2c->regs + HSI2C_AUTO_CONFING);
> +
> + /* Enable appropriate interrupts */
> + if (operation_mode != HSI2C_POLLING)
> + writel(usi_int_en, i2c->regs + HSI2C_INT_ENABLE);
> +
> + ret = -EAGAIN;
> + if (msgs->flags & I2C_M_RD) {
> + if (operation_mode == HSI2C_POLLING) {
why do you want to support polling ? Do you really have situations where
IRQ wouldn't cut it ?
> + timeout = jiffies + EXYNOS5_I2C_TIMEOUT;
> + while (time_before(jiffies, timeout)) {
> + if ((readl(i2c->regs + HSI2C_FIFO_STATUS) &
> + 0x1000000) == 0) {
no magic constants, please.
> + byte = (unsigned char)readl
> + (i2c->regs + HSI2C_RX_DATA);
> + i2c->msg->buf[i2c->msg_byte_ptr++]
> + = byte;
> + }
> +
> + if (i2c->msg_byte_ptr >= i2c->msg->len) {
> + ret = 0;
> + break;
> + }
> + }
> +
> + if (ret == -EAGAIN) {
you can avoid this (and the above initialization of ret to -EAGAIN) if
you reorder this code a little bit.
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "rx timeout\n");
> + return ret;
> + }
> + } else {
> + timeout = wait_for_completion_interruptible_timeout
> + (&i2c->msg_complete, EXYNOS5_I2C_TIMEOUT);
> +
> + if (timeout == 0) {
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "rx timeout\n");
> + return ret;
> + }
> +
> + ret = 0;
> + }
if you really *must* support polling and IRQ modes, I suggest you
re-factor all of this into separate functions so that your code looks
like:
if (operation_mode == HSI2C_POLLING)
ret = exynos5_i2c_xfer_msg_polling(i2c, msgs, stop,
msg->flags & I2C_M_RD);
else
ret = exynos5_i2c_xfer_msg_irq(i2c, msgs, stop,
msg->flags & I2C_M_RD);
> + } else {
> + if (operation_mode == HSI2C_POLLING) {
> + timeout = jiffies + EXYNOS5_I2C_TIMEOUT;
> + while (time_before(jiffies, timeout) &&
> + (i2c->msg_byte_ptr < i2c->msg->len)) {
> + if ((readl(i2c->regs + HSI2C_FIFO_STATUS)
> + & 0x7f) < 64) {
> + byte = i2c->msg->buf
> + [i2c->msg_byte_ptr++];
> + writel(byte,
> + i2c->regs + HSI2C_TX_DATA);
> + }
> + }
> + } else {
> + timeout = wait_for_completion_interruptible_timeout
> + (&i2c->msg_complete, EXYNOS5_I2C_TIMEOUT);
> +
> + if (timeout == 0) {
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "tx timeout\n");
> + return ret;
> + }
> +
> + timeout = jiffies + timeout;
> + }
> + while (time_before(jiffies, timeout)) {
> + usi_fifo_stat = readl(i2c->regs + HSI2C_FIFO_STATUS);
> + trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
> + if ((usi_fifo_stat == HSI2C_FIFO_EMPTY) &&
> + ((trans_status == 0) ||
> + ((stop == 0) &&
> + (trans_status == 0x20000)))) {
> + ret = 0;
> + break;
> + }
> + }
> + if (ret == -EAGAIN) {
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "tx timeout\n");
> + return ret;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int exynos5_i2c_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msgs, int num)
> +{
> + struct exynos5_i2c *i2c = (struct exynos5_i2c *)adap->algo_data;
> + int retry, i;
> + int ret;
> + int stop = 0;
> + struct i2c_msg *msgs_ptr = msgs;
> +
> + if (i2c->suspended) {
> + dev_err(i2c->dev, "HS-I2C is not initialzed.\n");
> + return -EIO;
> + }
> +
> + clk_prepare_enable(i2c->clk);
> +
> + for (retry = 0; retry < adap->retries; retry++) {
> + for (i = 0; i < num; i++) {
> + if (i == num - 1)
> + stop = 1;
> + ret = exynos5_i2c_xfer_msg(i2c, msgs_ptr, 1, stop);
> + msgs_ptr++;
> +
> + if (ret == -EAGAIN) {
> + msgs_ptr = msgs;
> + stop = 0;
> + break;
> + }
> + }
> + if (i == num) {
> + clk_disable_unprepare(i2c->clk);
why don't you use pm_runtime for that ? With the OMAP driver, we figured
that asynchronous PM improved transfer processing time quite a lot
because we would only cut controller's clocks when it was idle for at
least 500ms.
> + return num;
> + }
> +
> + dev_dbg(i2c->dev, "retrying transfer (%d)\n", retry);
> +
> + udelay(100);
> + }
> +
> + clk_disable_unprepare(i2c->clk);
> +
> + return -EREMOTEIO;
> +}
> +
> +static u32 exynos5_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C;
do you not support SMBUS ? Should you request for SW emulation, then ?
> +}
> +
> +static const struct i2c_algorithm exynos5_i2c_algorithm = {
> + .master_xfer = exynos5_i2c_xfer,
> + .functionality = exynos5_i2c_func,
> +};
> +
> +static int exynos5_i2c_set_timing(struct exynos5_i2c *i2c)
> +{
> + unsigned long i2c_timing_s1;
> + unsigned long i2c_timing_s2;
> + unsigned long i2c_timing_s3;
> + unsigned long i2c_timing_sla;
> + unsigned int op_clk;
> + unsigned int clkin = clk_get_rate(i2c->clk);
> + unsigned int n_clkdiv;
> + unsigned int t_start_su, t_start_hd;
> + unsigned int t_stop_su;
> + unsigned int t_data_su, t_data_hd;
> + unsigned int t_scl_l, t_scl_h;
> + unsigned int t_sr_release;
> + unsigned int t_ftl_cycle;
> + unsigned int i = 0, utemp0 = 0, utemp1 = 0, utemp2 = 0;
> +
> + if (i2c->speed_mode == HSI2C_HIGH_SPD)
> + op_clk = i2c->high_speed;
> + else
> + op_clk = i2c->fast_speed;
> +
> + /* FPCLK / FI2C =
> + * (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) + 8 + 2 * FLT_CYCLE
> + * uTemp0 = (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2)
> + * uTemp1 = (TSCLK_L + TSCLK_H + 2)
> + * uTemp2 = TSCLK_L + TSCLK_H
> + */
> + t_ftl_cycle = (readl(i2c->regs + HSI2C_CONF) >> 16) & 0x7;
> + utemp0 = (clkin / op_clk) - 8 - 2 * t_ftl_cycle;
> +
> + /* CLK_DIV max is 256 */
> + for (i = 0; i < 256; i++) {
> + utemp1 = utemp0 / (i + 1);
> + /* SCLK_L/H max is 256 / 2 */
> + if (utemp1 < 128) {
> + utemp2 = utemp1 - 2;
> + break;
> + }
> + }
> +
> + n_clkdiv = i;
> + t_scl_l = utemp2 / 2;
> + t_scl_h = utemp2 / 2;
> + t_start_su = t_scl_l;
> + t_start_hd = t_scl_l;
> + t_stop_su = t_scl_l;
> + t_data_su = t_scl_l / 2;
> + t_data_hd = t_scl_l / 2;
> + t_sr_release = utemp2;
> +
> + i2c_timing_s1 = t_start_su << 24 | t_start_hd << 16 | t_stop_su << 8;
> + i2c_timing_s2 = t_data_su << 24 | t_scl_l << 8 | t_scl_h << 0;
> + i2c_timing_s3 = n_clkdiv << 16 | t_sr_release << 0;
> + i2c_timing_sla = t_data_hd << 0;
> +
> + dev_dbg(i2c->dev, "tSTART_SU: %X, tSTART_HD: %X, tSTOP_SU: %X\n",
> + t_start_su, t_start_hd, t_stop_su);
> + dev_dbg(i2c->dev, "tDATA_SU: %X, tSCL_L: %X, tSCL_H: %X\n",
> + t_data_su, t_scl_l, t_scl_h);
> + dev_dbg(i2c->dev, "nClkDiv: %X, tSR_RELEASE: %X\n",
> + n_clkdiv, t_sr_release);
> + dev_dbg(i2c->dev, "tDATA_HD: %X\n", t_data_hd);
> +
> + if (i2c->speed_mode == HSI2C_HIGH_SPD) {
> + writel(i2c_timing_s1, i2c->regs + HSI2C_TIMING_HS1);
> + writel(i2c_timing_s2, i2c->regs + HSI2C_TIMING_HS2);
> + writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_HS3);
> + } else {
> + writel(i2c_timing_s1, i2c->regs + HSI2C_TIMING_FS1);
> + writel(i2c_timing_s2, i2c->regs + HSI2C_TIMING_FS2);
> + writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_FS3);
> + }
> + writel(i2c_timing_sla, i2c->regs + HSI2C_TIMING_SLA);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static int exynos5_i2c_parse_dt_gpio(struct exynos5_i2c *i2c)
> +{
> + int idx, gpio, ret;
> +
> + for (idx = 0; idx < 2; idx++) {
> + gpio = of_get_gpio(i2c->dev->of_node, idx);
> + if (!gpio_is_valid(gpio)) {
> + dev_err(i2c->dev, "invalid gpio[%d]: %d\n", idx, gpio);
> + goto free_gpio;
> + }
> + i2c->gpios[idx] = gpio;
> +
> + ret = gpio_request(gpio, "hsi2c-bus");
> + if (ret) {
> + dev_err(i2c->dev, "gpio [%d] request failed\n", gpio);
> + goto free_gpio;
> + }
> + }
> + return 0;
> +
> +free_gpio:
> + while (--idx >= 0)
> + gpio_free(i2c->gpios[idx]);
> + return -EINVAL;
> +}
> +
> +static void exynos5_i2c_dt_gpio_free(struct exynos5_i2c *i2c)
> +{
> + unsigned int idx;
> +
> + for (idx = 0; idx < 2; idx++)
> + gpio_free(i2c->gpios[idx]);
> +}
> +#else
> +static int exynos5_i2c_parse_dt_gpio(struct exynos5_i2c *i2c)
> +{
> + return 0;
> +}
> +
> +static void exynos5_i2c_dt_gpio_free(struct exynos5_i2c *i2c)
> +{
> +}
> +#endif
> +
> +static int exynos5_i2c_init(struct exynos5_i2c *i2c)
> +{
> + unsigned long usi_ctl = HSI2C_FUNC_MODE_I2C | HSI2C_MASTER;
> + unsigned long usi_trailing_ctl = HSI2C_TRAILING_COUNT;
> + unsigned long i2c_conf = readl(i2c->regs + HSI2C_CONF);
> +
> + if (exynos5_i2c_parse_dt_gpio(i2c))
> + return -EINVAL;
> +
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> +
> + writel(usi_trailing_ctl, i2c->regs + HSI2C_TRAILIG_CTL);
> +
> + exynos5_i2c_set_timing(i2c);
> +
> + if (i2c->speed_mode == HSI2C_HIGH_SPD) {
> + i2c_conf |= HSI2C_HS_MODE;
> + writel(i2c_conf, i2c->regs + HSI2C_CONF);
> + }
> +
> + return 0;
> +}
> +
> +static int exynos5_i2c_probe(struct platform_device *pdev)
> +{
> + struct exynos5_i2c *i2c;
> + struct resource *res;
> + int ret;
> +
> + i2c = devm_kzalloc(&pdev->dev, sizeof(struct exynos5_i2c), GFP_KERNEL);
> + if (!i2c) {
> + dev_err(&pdev->dev, "no memory for state\n");
> + return -ENOMEM;
> + }
> +
> + if (pdev->dev.of_node) {
> + /* i2c bus number is dynamically assigned */
> + i2c->bus_number = -1;
> +
> + if (of_property_read_u32(pdev->dev.of_node,
> + "samsung,hsi2c-speed-mode", &i2c->speed_mode))
> + i2c->speed_mode = 1;
> + if (!of_property_read_u32(pdev->dev.of_node,
> + "samsung,hsi2c-hs-clk", &i2c->high_speed))
> + i2c->high_speed = 2500000;
> + if (!of_property_read_u32(pdev->dev.of_node,
> + "samsung,hsi2c-fs-clk", &i2c->fast_speed))
> + i2c->fast_speed = 400000;
> + } else {
> + dev_err(&pdev->dev, "no device node\n");
> + return -ENOENT;
> + }
> +
> + strlcpy(i2c->adap.name, "exynos5250-i2c", sizeof(i2c->adap.name));
> + i2c->adap.owner = THIS_MODULE;
> + i2c->adap.algo = &exynos5_i2c_algorithm;
> + i2c->adap.retries = 2;
> + i2c->adap.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> +
> + i2c->dev = &pdev->dev;
> + i2c->clk = clk_get(&pdev->dev, "hsi2c");
how about devm_clk_get() ?
> + if (IS_ERR(i2c->clk)) {
> + dev_err(&pdev->dev, "cannot get clock\n");
> + ret = -ENOENT;
> + goto err_noclk;
> + }
> +
> + clk_prepare_enable(i2c->clk);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "cannot find HS-I2C IO resource\n");
> + ret = -ENOENT;
> + goto err_clk;
> + }
> +
> + i2c->ioarea = request_mem_region(res->start, resource_size(res),
> + pdev->name);
please use devm_request_and_ioremap() instead.
> +
> + if (i2c->ioarea == NULL) {
> + dev_err(&pdev->dev, "cannot request HS-I2C IO\n");
> + ret = -ENXIO;
> + goto err_clk;
> + }
> +
> + i2c->regs = ioremap(res->start, resource_size(res));
> +
> + if (i2c->regs == NULL) {
> + dev_err(&pdev->dev, "cannot map HS-I2C IO\n");
> + ret = -ENXIO;
> + goto err_ioarea;
> + }
> +
> + dev_dbg(&pdev->dev, "registers %p (%p, %p)\n",
> + i2c->regs, i2c->ioarea, res);
> +
> + i2c->adap.algo_data = i2c;
> + i2c->adap.dev.parent = &pdev->dev;
> +
> + ret = exynos5_i2c_init(i2c);
> + if (ret != 0)
> + goto err_iomap;
> +
> + i2c->irq = ret = platform_get_irq(pdev, 0);
> + if (ret <= 0) {
> + dev_err(&pdev->dev, "cannot find HS-I2C IRQ\n");
> + goto err_iomap;
> + }
> +
> + ret = request_irq(i2c->irq, exynos5_i2c_irq, IRQF_DISABLED,
> + dev_name(&pdev->dev), i2c);
devm_request_irq().
> +
> + if (ret != 0) {
> + dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq);
> + goto err_iomap;
> + }
> +
> + i2c->adap.nr = i2c->bus_number;
> + i2c->adap.dev.of_node = pdev->dev.of_node;
> +
> + ret = i2c_add_numbered_adapter(&i2c->adap);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to add bus to i2c core\n");
> + goto err_irq;
> + }
> +
> + of_i2c_register_devices(&i2c->adap);
> + platform_set_drvdata(pdev, i2c);
> +
> + dev_info(&pdev->dev, "%s: Exynos5 HS-I2C adapter\n",
> + dev_name(&i2c->adap.dev));
please drop this dev_info() as it brings no useful information. The core
I2C subsystem already prints enough information when you register an
adapter.
> + clk_disable_unprepare(i2c->clk);
> + return 0;
> +
> + err_irq:
> + free_irq(i2c->irq, i2c);
> +
> + err_iomap:
> + iounmap(i2c->regs);
> +
> + err_ioarea:
> + release_resource(i2c->ioarea);
> + kfree(i2c->ioarea);
> +
> + err_clk:
> + clk_disable_unprepare(i2c->clk);
> + clk_put(i2c->clk);
> +
> + err_noclk:
> + return ret;
> +}
> +
> +static int exynos5_i2c_remove(struct platform_device *pdev)
> +{
> + struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&i2c->adap);
> + free_irq(i2c->irq, i2c);
> +
> + clk_disable_unprepare(i2c->clk);
> + clk_put(i2c->clk);
> +
> + iounmap(i2c->regs);
> +
> + release_resource(i2c->ioarea);
> + exynos5_i2c_dt_gpio_free(i2c);
> + kfree(i2c->ioarea);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int exynos5_i2c_suspend_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_lock_adapter(&i2c->adap);
> + i2c->suspended = 1;
> + i2c_unlock_adapter(&i2c->adap);
> +
> + return 0;
> +}
> +
> +static int exynos5_i2c_resume(struct device *dev)
maybe you could add _noirq suffix here too.
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_lock_adapter(&i2c->adap);
> + clk_prepare_enable(i2c->clk);
> + exynos5_i2c_init(i2c);
> + clk_disable_unprepare(i2c->clk);
> + i2c->suspended = 0;
> + i2c_unlock_adapter(&i2c->adap);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops exynos5_i2c_dev_pm_ops = {
> + .suspend_noirq = exynos5_i2c_suspend_noirq,
> + .resume_noirq = exynos5_i2c_resume,
you need to define poweroff, thaw, freeze, restore.
> +};
> +
> +#define EXYNOS5_DEV_PM_OPS (&exynos5_i2c_dev_pm_ops)
> +#else
> +#define EXYNOS5_DEV_PM_OPS NULL
> +#endif
> +
> +static struct platform_driver exynos5_i2c_driver = {
> + .probe = exynos5_i2c_probe,
> + .remove = exynos5_i2c_remove,
> + .id_table = exynos5_driver_ids,
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "exynos5-i2c",
> + .pm = EXYNOS5_DEV_PM_OPS,
> + .of_match_table = of_match_ptr(exynos5_i2c_match),
> + },
> +};
> +
> +static int __init i2c_adap_exynos5_init(void)
> +{
> + return platform_driver_register(&exynos5_i2c_driver);
> +}
> +subsys_initcall(i2c_adap_exynos5_init);
> +
> +static void __exit i2c_adap_exynos5_exit(void)
> +{
> + platform_driver_unregister(&exynos5_i2c_driver);
> +}
> +module_exit(i2c_adap_exynos5_exit);
> +
> +MODULE_DESCRIPTION("Exynos5 HS-I2C Bus driver");
> +MODULE_AUTHOR("Taekgyun Ko, <taeggyun.ko@samsung.com>");
> +MODULE_LICENSE("GPL");
looks like this should be GPL v2, judging by the comment in the
beginning of this file.
> diff --git a/drivers/i2c/busses/i2c-exynos5.h b/drivers/i2c/busses/i2c-exynos5.h
> new file mode 100644
> index 0000000..063051e
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-exynos5.h
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright (C) 2012 Samsung Electronics Co., Ltd.
> + *
> + * Exynos5 series HS-I2C Controller
> + *
> + * 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.
> +*/
> +
> +#ifndef __ASM_ARCH_REGS_HS_IIC_H
> +#define __ASM_ARCH_REGS_HS_IIC_H __FILE__
no need for the __FILE__ at the end...
> +
> +/*
> + * Register Map
> + */
this is a single line comment. Should be /* Register Map */ instead.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Felipe Balbi <balbi@ti.com>
To: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Cc: linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-i2c@vger.kernel.org,
naveenkrishna.ch@gmail.com, kgene.kim@samsung.com,
grant.likely@secretlab.ca, w.sang@pengutronix.de,
linux-kernel@vger.kernel.org, taeggyun.ko@samsung.com
Subject: Re: [PATCH 1/3] i2c: exynos5: add High Speed I2C controller driver
Date: Tue, 27 Nov 2012 15:23:36 +0200 [thread overview]
Message-ID: <20121127132336.GB22556@arwen.pp.htv.fi> (raw)
In-Reply-To: <1354021236-28596-2-git-send-email-ch.naveen@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 25309 bytes --]
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 0000000..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 <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/time.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/of_i2c.h>
> +#include <linux/of_gpio.h>
> +
> +#include <asm/irq.h>
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 int suspended:1;
> +
> + struct i2c_msg *msg;
> + struct completion msg_complete;
> + unsigned int msg_byte_ptr;
> +
> + unsigned int irq;
> +
> + 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 int bus_number;
> + unsigned int speed_mode;
> + unsigned int fast_speed;
> + unsigned int high_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_STATUS);
> +}
> +
> +static void exynos5_i2c_en_timeout(struct exynos5_i2c *i2c)
> +{
> + unsigned long i2c_timeout = readl(i2c->regs + HSI2C_TIMEOUT);
> +
> + /* Clear to enable Timeout */
> + i2c_timeout &= ~HSI2C_TIMEOUT_EN;
> + writel(i2c_timeout, i2c->regs + HSI2C_TIMEOUT);
> +}
> +
> +static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
> +{
> + struct exynos5_i2c *i2c = dev_id;
> + unsigned char byte;
> +
> + if (i2c->msg->flags & I2C_M_RD) {
> + while ((readl(i2c->regs + HSI2C_FIFO_STATUS) &
> + 0x1000000) == 0) {
> + byte = (unsigned char)readl(i2c->regs + HSI2C_RX_DATA);
> + i2c->msg->buf[i2c->msg_byte_ptr++] = byte;
> + }
> +
> + if (i2c->msg_byte_ptr >= i2c->msg->len)
> + exynos5_i2c_stop(i2c);
> + } else {
> + byte = i2c->msg->buf[i2c->msg_byte_ptr++];
> + writel(byte, i2c->regs + HSI2C_TX_DATA);
> +
> + if (i2c->msg_byte_ptr >= i2c->msg->len)
> + exynos5_i2c_stop(i2c);
> + }
you never check which event generated the IRQ. Why don't you use
register address 0x24 ??
> + exynos5_disable_irq(i2c);
why are you masking IRQs here ?
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int exynos5_i2c_init(struct exynos5_i2c *i2c);
> +
> +static int exynos5_i2c_reset(struct exynos5_i2c *i2c)
> +{
> + unsigned long usi_ctl;
> +
> + usi_ctl = readl(i2c->regs + HSI2C_CTL);
> + usi_ctl |= (1u << 31);
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> + usi_ctl = readl(i2c->regs + HSI2C_CTL);
> + usi_ctl &= ~(1u << 31);
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> + exynos5_i2c_init(i2c);
> +
> + return 0;
> +}
> +
> +static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
> + struct i2c_msg *msgs, int num, int stop)
this "num" argument is useless.
> +{
> + unsigned long timeout;
> + unsigned long trans_status;
> + unsigned long usi_fifo_stat;
> + unsigned long usi_ctl;
> + unsigned long i2c_auto_conf;
> + unsigned long i2c_addr;
> + unsigned long usi_int_en;
> + unsigned long usi_fifo_ctl;
> + unsigned char byte;
> + int ret = 0;
> + int operation_mode = i2c->operation_mode;
> +
> + i2c->msg = msgs;
> + i2c->msg_byte_ptr = 0;
> +
> + init_completion(&i2c->msg_complete);
actually, init_completion() should be done on your probe(). Here you
should be calling INIT_COMPLETION() instead.
> + usi_ctl = readl(i2c->regs + HSI2C_CTL);
> + i2c_auto_conf = readl(i2c->regs + HSI2C_AUTO_CONFING);
do you really need to read these two registers ? You're programming a
new transfer, meaning that you're likely to change the entire
register...
> + exynos5_i2c_en_timeout(i2c);
> +
> + /* Set default trigger level for TXFIFO and RXFIFO */
> + usi_fifo_ctl = HSI2C_TXFIFO_TRIGGER_LEVEL |
> + HSI2C_RXFIFO_TRIGGER_LEVEL;
> + /* Enable RXFIFO and TXFIFO */
> + usi_fifo_ctl = HSI2C_RXFIFO_EN | HSI2C_TXFIFO_EN;
> +
> + writel(usi_fifo_ctl, i2c->regs + HSI2C_FIFO_CTL);
> +
> + usi_int_en = 0;
> + if (msgs->flags & I2C_M_RD) {
> + usi_ctl &= ~HSI2C_TXCHON;
> + usi_ctl |= HSI2C_RXCHON;
> +
> + i2c_auto_conf |= HSI2C_READ_WRITE;
> +
> + usi_int_en |= (HSI2C_INT_RX_ALMOSTFULL_EN |
> + HSI2C_INT_TRAILING_EN);
> + } else {
> + usi_ctl &= ~HSI2C_RXCHON;
> + usi_ctl |= HSI2C_TXCHON;
> +
> + i2c_auto_conf &= ~HSI2C_READ_WRITE;
> +
> + usi_int_en |= HSI2C_INT_TX_ALMOSTEMPTY_EN;
> + }
> +
> + if (stop == 1)
> + i2c_auto_conf |= HSI2C_STOP_AFTER_TRANS;
> + else
> + i2c_auto_conf &= ~HSI2C_STOP_AFTER_TRANS;
How about:
i2c_auto_conf |= stop ? HSI2C_STOP_AFTER_TRANS : 0; ??
> +
> +
one blank line only.
> + i2c_addr = readl(i2c->regs + HSI2C_ADDR);
why do you need to read it ? Is there more information in this register
other than the address ???
> + /* Clear Slave Address for I2C Master (Auto mode only) */
> + i2c_addr &= ~(0x3ff << 10);
> + /* Clear Slave Address for I2C Slave */
> + i2c_addr &= ~(0x3ff << 0);
> + /* Clear Master ID for I2C Master (Auto and HS mode only) */
> + if (i2c->speed_mode == HSI2C_HIGH_SPD)
> + i2c_addr &= ~(0xff << 24);
> +
> + i2c_addr |= ((msgs->addr & 0x7f) << 10);
> + writel(i2c_addr, i2c->regs + HSI2C_ADDR);
> +
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> +
> + /* Clear and set TRANS_LEN */
> + i2c_auto_conf &= ~(0xffff);
> + i2c_auto_conf |= i2c->msg->len;
> + writel(i2c_auto_conf, i2c->regs + HSI2C_AUTO_CONFING);
> +
> + /* Start data transfer in Master mode */
> + i2c_auto_conf = readl(i2c->regs + HSI2C_AUTO_CONFING);
> + i2c_auto_conf |= HSI2C_MASTER_RUN;
> + writel(i2c_auto_conf, i2c->regs + HSI2C_AUTO_CONFING);
> +
> + /* Enable appropriate interrupts */
> + if (operation_mode != HSI2C_POLLING)
> + writel(usi_int_en, i2c->regs + HSI2C_INT_ENABLE);
> +
> + ret = -EAGAIN;
> + if (msgs->flags & I2C_M_RD) {
> + if (operation_mode == HSI2C_POLLING) {
why do you want to support polling ? Do you really have situations where
IRQ wouldn't cut it ?
> + timeout = jiffies + EXYNOS5_I2C_TIMEOUT;
> + while (time_before(jiffies, timeout)) {
> + if ((readl(i2c->regs + HSI2C_FIFO_STATUS) &
> + 0x1000000) == 0) {
no magic constants, please.
> + byte = (unsigned char)readl
> + (i2c->regs + HSI2C_RX_DATA);
> + i2c->msg->buf[i2c->msg_byte_ptr++]
> + = byte;
> + }
> +
> + if (i2c->msg_byte_ptr >= i2c->msg->len) {
> + ret = 0;
> + break;
> + }
> + }
> +
> + if (ret == -EAGAIN) {
you can avoid this (and the above initialization of ret to -EAGAIN) if
you reorder this code a little bit.
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "rx timeout\n");
> + return ret;
> + }
> + } else {
> + timeout = wait_for_completion_interruptible_timeout
> + (&i2c->msg_complete, EXYNOS5_I2C_TIMEOUT);
> +
> + if (timeout == 0) {
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "rx timeout\n");
> + return ret;
> + }
> +
> + ret = 0;
> + }
if you really *must* support polling and IRQ modes, I suggest you
re-factor all of this into separate functions so that your code looks
like:
if (operation_mode == HSI2C_POLLING)
ret = exynos5_i2c_xfer_msg_polling(i2c, msgs, stop,
msg->flags & I2C_M_RD);
else
ret = exynos5_i2c_xfer_msg_irq(i2c, msgs, stop,
msg->flags & I2C_M_RD);
> + } else {
> + if (operation_mode == HSI2C_POLLING) {
> + timeout = jiffies + EXYNOS5_I2C_TIMEOUT;
> + while (time_before(jiffies, timeout) &&
> + (i2c->msg_byte_ptr < i2c->msg->len)) {
> + if ((readl(i2c->regs + HSI2C_FIFO_STATUS)
> + & 0x7f) < 64) {
> + byte = i2c->msg->buf
> + [i2c->msg_byte_ptr++];
> + writel(byte,
> + i2c->regs + HSI2C_TX_DATA);
> + }
> + }
> + } else {
> + timeout = wait_for_completion_interruptible_timeout
> + (&i2c->msg_complete, EXYNOS5_I2C_TIMEOUT);
> +
> + if (timeout == 0) {
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "tx timeout\n");
> + return ret;
> + }
> +
> + timeout = jiffies + timeout;
> + }
> + while (time_before(jiffies, timeout)) {
> + usi_fifo_stat = readl(i2c->regs + HSI2C_FIFO_STATUS);
> + trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
> + if ((usi_fifo_stat == HSI2C_FIFO_EMPTY) &&
> + ((trans_status == 0) ||
> + ((stop == 0) &&
> + (trans_status == 0x20000)))) {
> + ret = 0;
> + break;
> + }
> + }
> + if (ret == -EAGAIN) {
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "tx timeout\n");
> + return ret;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int exynos5_i2c_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msgs, int num)
> +{
> + struct exynos5_i2c *i2c = (struct exynos5_i2c *)adap->algo_data;
> + int retry, i;
> + int ret;
> + int stop = 0;
> + struct i2c_msg *msgs_ptr = msgs;
> +
> + if (i2c->suspended) {
> + dev_err(i2c->dev, "HS-I2C is not initialzed.\n");
> + return -EIO;
> + }
> +
> + clk_prepare_enable(i2c->clk);
> +
> + for (retry = 0; retry < adap->retries; retry++) {
> + for (i = 0; i < num; i++) {
> + if (i == num - 1)
> + stop = 1;
> + ret = exynos5_i2c_xfer_msg(i2c, msgs_ptr, 1, stop);
> + msgs_ptr++;
> +
> + if (ret == -EAGAIN) {
> + msgs_ptr = msgs;
> + stop = 0;
> + break;
> + }
> + }
> + if (i == num) {
> + clk_disable_unprepare(i2c->clk);
why don't you use pm_runtime for that ? With the OMAP driver, we figured
that asynchronous PM improved transfer processing time quite a lot
because we would only cut controller's clocks when it was idle for at
least 500ms.
> + return num;
> + }
> +
> + dev_dbg(i2c->dev, "retrying transfer (%d)\n", retry);
> +
> + udelay(100);
> + }
> +
> + clk_disable_unprepare(i2c->clk);
> +
> + return -EREMOTEIO;
> +}
> +
> +static u32 exynos5_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C;
do you not support SMBUS ? Should you request for SW emulation, then ?
> +}
> +
> +static const struct i2c_algorithm exynos5_i2c_algorithm = {
> + .master_xfer = exynos5_i2c_xfer,
> + .functionality = exynos5_i2c_func,
> +};
> +
> +static int exynos5_i2c_set_timing(struct exynos5_i2c *i2c)
> +{
> + unsigned long i2c_timing_s1;
> + unsigned long i2c_timing_s2;
> + unsigned long i2c_timing_s3;
> + unsigned long i2c_timing_sla;
> + unsigned int op_clk;
> + unsigned int clkin = clk_get_rate(i2c->clk);
> + unsigned int n_clkdiv;
> + unsigned int t_start_su, t_start_hd;
> + unsigned int t_stop_su;
> + unsigned int t_data_su, t_data_hd;
> + unsigned int t_scl_l, t_scl_h;
> + unsigned int t_sr_release;
> + unsigned int t_ftl_cycle;
> + unsigned int i = 0, utemp0 = 0, utemp1 = 0, utemp2 = 0;
> +
> + if (i2c->speed_mode == HSI2C_HIGH_SPD)
> + op_clk = i2c->high_speed;
> + else
> + op_clk = i2c->fast_speed;
> +
> + /* FPCLK / FI2C =
> + * (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) + 8 + 2 * FLT_CYCLE
> + * uTemp0 = (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2)
> + * uTemp1 = (TSCLK_L + TSCLK_H + 2)
> + * uTemp2 = TSCLK_L + TSCLK_H
> + */
> + t_ftl_cycle = (readl(i2c->regs + HSI2C_CONF) >> 16) & 0x7;
> + utemp0 = (clkin / op_clk) - 8 - 2 * t_ftl_cycle;
> +
> + /* CLK_DIV max is 256 */
> + for (i = 0; i < 256; i++) {
> + utemp1 = utemp0 / (i + 1);
> + /* SCLK_L/H max is 256 / 2 */
> + if (utemp1 < 128) {
> + utemp2 = utemp1 - 2;
> + break;
> + }
> + }
> +
> + n_clkdiv = i;
> + t_scl_l = utemp2 / 2;
> + t_scl_h = utemp2 / 2;
> + t_start_su = t_scl_l;
> + t_start_hd = t_scl_l;
> + t_stop_su = t_scl_l;
> + t_data_su = t_scl_l / 2;
> + t_data_hd = t_scl_l / 2;
> + t_sr_release = utemp2;
> +
> + i2c_timing_s1 = t_start_su << 24 | t_start_hd << 16 | t_stop_su << 8;
> + i2c_timing_s2 = t_data_su << 24 | t_scl_l << 8 | t_scl_h << 0;
> + i2c_timing_s3 = n_clkdiv << 16 | t_sr_release << 0;
> + i2c_timing_sla = t_data_hd << 0;
> +
> + dev_dbg(i2c->dev, "tSTART_SU: %X, tSTART_HD: %X, tSTOP_SU: %X\n",
> + t_start_su, t_start_hd, t_stop_su);
> + dev_dbg(i2c->dev, "tDATA_SU: %X, tSCL_L: %X, tSCL_H: %X\n",
> + t_data_su, t_scl_l, t_scl_h);
> + dev_dbg(i2c->dev, "nClkDiv: %X, tSR_RELEASE: %X\n",
> + n_clkdiv, t_sr_release);
> + dev_dbg(i2c->dev, "tDATA_HD: %X\n", t_data_hd);
> +
> + if (i2c->speed_mode == HSI2C_HIGH_SPD) {
> + writel(i2c_timing_s1, i2c->regs + HSI2C_TIMING_HS1);
> + writel(i2c_timing_s2, i2c->regs + HSI2C_TIMING_HS2);
> + writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_HS3);
> + } else {
> + writel(i2c_timing_s1, i2c->regs + HSI2C_TIMING_FS1);
> + writel(i2c_timing_s2, i2c->regs + HSI2C_TIMING_FS2);
> + writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_FS3);
> + }
> + writel(i2c_timing_sla, i2c->regs + HSI2C_TIMING_SLA);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static int exynos5_i2c_parse_dt_gpio(struct exynos5_i2c *i2c)
> +{
> + int idx, gpio, ret;
> +
> + for (idx = 0; idx < 2; idx++) {
> + gpio = of_get_gpio(i2c->dev->of_node, idx);
> + if (!gpio_is_valid(gpio)) {
> + dev_err(i2c->dev, "invalid gpio[%d]: %d\n", idx, gpio);
> + goto free_gpio;
> + }
> + i2c->gpios[idx] = gpio;
> +
> + ret = gpio_request(gpio, "hsi2c-bus");
> + if (ret) {
> + dev_err(i2c->dev, "gpio [%d] request failed\n", gpio);
> + goto free_gpio;
> + }
> + }
> + return 0;
> +
> +free_gpio:
> + while (--idx >= 0)
> + gpio_free(i2c->gpios[idx]);
> + return -EINVAL;
> +}
> +
> +static void exynos5_i2c_dt_gpio_free(struct exynos5_i2c *i2c)
> +{
> + unsigned int idx;
> +
> + for (idx = 0; idx < 2; idx++)
> + gpio_free(i2c->gpios[idx]);
> +}
> +#else
> +static int exynos5_i2c_parse_dt_gpio(struct exynos5_i2c *i2c)
> +{
> + return 0;
> +}
> +
> +static void exynos5_i2c_dt_gpio_free(struct exynos5_i2c *i2c)
> +{
> +}
> +#endif
> +
> +static int exynos5_i2c_init(struct exynos5_i2c *i2c)
> +{
> + unsigned long usi_ctl = HSI2C_FUNC_MODE_I2C | HSI2C_MASTER;
> + unsigned long usi_trailing_ctl = HSI2C_TRAILING_COUNT;
> + unsigned long i2c_conf = readl(i2c->regs + HSI2C_CONF);
> +
> + if (exynos5_i2c_parse_dt_gpio(i2c))
> + return -EINVAL;
> +
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> +
> + writel(usi_trailing_ctl, i2c->regs + HSI2C_TRAILIG_CTL);
> +
> + exynos5_i2c_set_timing(i2c);
> +
> + if (i2c->speed_mode == HSI2C_HIGH_SPD) {
> + i2c_conf |= HSI2C_HS_MODE;
> + writel(i2c_conf, i2c->regs + HSI2C_CONF);
> + }
> +
> + return 0;
> +}
> +
> +static int exynos5_i2c_probe(struct platform_device *pdev)
> +{
> + struct exynos5_i2c *i2c;
> + struct resource *res;
> + int ret;
> +
> + i2c = devm_kzalloc(&pdev->dev, sizeof(struct exynos5_i2c), GFP_KERNEL);
> + if (!i2c) {
> + dev_err(&pdev->dev, "no memory for state\n");
> + return -ENOMEM;
> + }
> +
> + if (pdev->dev.of_node) {
> + /* i2c bus number is dynamically assigned */
> + i2c->bus_number = -1;
> +
> + if (of_property_read_u32(pdev->dev.of_node,
> + "samsung,hsi2c-speed-mode", &i2c->speed_mode))
> + i2c->speed_mode = 1;
> + if (!of_property_read_u32(pdev->dev.of_node,
> + "samsung,hsi2c-hs-clk", &i2c->high_speed))
> + i2c->high_speed = 2500000;
> + if (!of_property_read_u32(pdev->dev.of_node,
> + "samsung,hsi2c-fs-clk", &i2c->fast_speed))
> + i2c->fast_speed = 400000;
> + } else {
> + dev_err(&pdev->dev, "no device node\n");
> + return -ENOENT;
> + }
> +
> + strlcpy(i2c->adap.name, "exynos5250-i2c", sizeof(i2c->adap.name));
> + i2c->adap.owner = THIS_MODULE;
> + i2c->adap.algo = &exynos5_i2c_algorithm;
> + i2c->adap.retries = 2;
> + i2c->adap.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> +
> + i2c->dev = &pdev->dev;
> + i2c->clk = clk_get(&pdev->dev, "hsi2c");
how about devm_clk_get() ?
> + if (IS_ERR(i2c->clk)) {
> + dev_err(&pdev->dev, "cannot get clock\n");
> + ret = -ENOENT;
> + goto err_noclk;
> + }
> +
> + clk_prepare_enable(i2c->clk);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "cannot find HS-I2C IO resource\n");
> + ret = -ENOENT;
> + goto err_clk;
> + }
> +
> + i2c->ioarea = request_mem_region(res->start, resource_size(res),
> + pdev->name);
please use devm_request_and_ioremap() instead.
> +
> + if (i2c->ioarea == NULL) {
> + dev_err(&pdev->dev, "cannot request HS-I2C IO\n");
> + ret = -ENXIO;
> + goto err_clk;
> + }
> +
> + i2c->regs = ioremap(res->start, resource_size(res));
> +
> + if (i2c->regs == NULL) {
> + dev_err(&pdev->dev, "cannot map HS-I2C IO\n");
> + ret = -ENXIO;
> + goto err_ioarea;
> + }
> +
> + dev_dbg(&pdev->dev, "registers %p (%p, %p)\n",
> + i2c->regs, i2c->ioarea, res);
> +
> + i2c->adap.algo_data = i2c;
> + i2c->adap.dev.parent = &pdev->dev;
> +
> + ret = exynos5_i2c_init(i2c);
> + if (ret != 0)
> + goto err_iomap;
> +
> + i2c->irq = ret = platform_get_irq(pdev, 0);
> + if (ret <= 0) {
> + dev_err(&pdev->dev, "cannot find HS-I2C IRQ\n");
> + goto err_iomap;
> + }
> +
> + ret = request_irq(i2c->irq, exynos5_i2c_irq, IRQF_DISABLED,
> + dev_name(&pdev->dev), i2c);
devm_request_irq().
> +
> + if (ret != 0) {
> + dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq);
> + goto err_iomap;
> + }
> +
> + i2c->adap.nr = i2c->bus_number;
> + i2c->adap.dev.of_node = pdev->dev.of_node;
> +
> + ret = i2c_add_numbered_adapter(&i2c->adap);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to add bus to i2c core\n");
> + goto err_irq;
> + }
> +
> + of_i2c_register_devices(&i2c->adap);
> + platform_set_drvdata(pdev, i2c);
> +
> + dev_info(&pdev->dev, "%s: Exynos5 HS-I2C adapter\n",
> + dev_name(&i2c->adap.dev));
please drop this dev_info() as it brings no useful information. The core
I2C subsystem already prints enough information when you register an
adapter.
> + clk_disable_unprepare(i2c->clk);
> + return 0;
> +
> + err_irq:
> + free_irq(i2c->irq, i2c);
> +
> + err_iomap:
> + iounmap(i2c->regs);
> +
> + err_ioarea:
> + release_resource(i2c->ioarea);
> + kfree(i2c->ioarea);
> +
> + err_clk:
> + clk_disable_unprepare(i2c->clk);
> + clk_put(i2c->clk);
> +
> + err_noclk:
> + return ret;
> +}
> +
> +static int exynos5_i2c_remove(struct platform_device *pdev)
> +{
> + struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&i2c->adap);
> + free_irq(i2c->irq, i2c);
> +
> + clk_disable_unprepare(i2c->clk);
> + clk_put(i2c->clk);
> +
> + iounmap(i2c->regs);
> +
> + release_resource(i2c->ioarea);
> + exynos5_i2c_dt_gpio_free(i2c);
> + kfree(i2c->ioarea);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int exynos5_i2c_suspend_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_lock_adapter(&i2c->adap);
> + i2c->suspended = 1;
> + i2c_unlock_adapter(&i2c->adap);
> +
> + return 0;
> +}
> +
> +static int exynos5_i2c_resume(struct device *dev)
maybe you could add _noirq suffix here too.
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_lock_adapter(&i2c->adap);
> + clk_prepare_enable(i2c->clk);
> + exynos5_i2c_init(i2c);
> + clk_disable_unprepare(i2c->clk);
> + i2c->suspended = 0;
> + i2c_unlock_adapter(&i2c->adap);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops exynos5_i2c_dev_pm_ops = {
> + .suspend_noirq = exynos5_i2c_suspend_noirq,
> + .resume_noirq = exynos5_i2c_resume,
you need to define poweroff, thaw, freeze, restore.
> +};
> +
> +#define EXYNOS5_DEV_PM_OPS (&exynos5_i2c_dev_pm_ops)
> +#else
> +#define EXYNOS5_DEV_PM_OPS NULL
> +#endif
> +
> +static struct platform_driver exynos5_i2c_driver = {
> + .probe = exynos5_i2c_probe,
> + .remove = exynos5_i2c_remove,
> + .id_table = exynos5_driver_ids,
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "exynos5-i2c",
> + .pm = EXYNOS5_DEV_PM_OPS,
> + .of_match_table = of_match_ptr(exynos5_i2c_match),
> + },
> +};
> +
> +static int __init i2c_adap_exynos5_init(void)
> +{
> + return platform_driver_register(&exynos5_i2c_driver);
> +}
> +subsys_initcall(i2c_adap_exynos5_init);
> +
> +static void __exit i2c_adap_exynos5_exit(void)
> +{
> + platform_driver_unregister(&exynos5_i2c_driver);
> +}
> +module_exit(i2c_adap_exynos5_exit);
> +
> +MODULE_DESCRIPTION("Exynos5 HS-I2C Bus driver");
> +MODULE_AUTHOR("Taekgyun Ko, <taeggyun.ko@samsung.com>");
> +MODULE_LICENSE("GPL");
looks like this should be GPL v2, judging by the comment in the
beginning of this file.
> diff --git a/drivers/i2c/busses/i2c-exynos5.h b/drivers/i2c/busses/i2c-exynos5.h
> new file mode 100644
> index 0000000..063051e
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-exynos5.h
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright (C) 2012 Samsung Electronics Co., Ltd.
> + *
> + * Exynos5 series HS-I2C Controller
> + *
> + * 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.
> +*/
> +
> +#ifndef __ASM_ARCH_REGS_HS_IIC_H
> +#define __ASM_ARCH_REGS_HS_IIC_H __FILE__
no need for the __FILE__ at the end...
> +
> +/*
> + * Register Map
> + */
this is a single line comment. Should be /* Register Map */ instead.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: balbi@ti.com (Felipe Balbi)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/3] i2c: exynos5: add High Speed I2C controller driver
Date: Tue, 27 Nov 2012 15:23:36 +0200 [thread overview]
Message-ID: <20121127132336.GB22556@arwen.pp.htv.fi> (raw)
In-Reply-To: <1354021236-28596-2-git-send-email-ch.naveen@samsung.com>
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 0000000..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 <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/time.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/of_i2c.h>
> +#include <linux/of_gpio.h>
> +
> +#include <asm/irq.h>
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 int suspended:1;
> +
> + struct i2c_msg *msg;
> + struct completion msg_complete;
> + unsigned int msg_byte_ptr;
> +
> + unsigned int irq;
> +
> + 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 int bus_number;
> + unsigned int speed_mode;
> + unsigned int fast_speed;
> + unsigned int high_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_STATUS);
> +}
> +
> +static void exynos5_i2c_en_timeout(struct exynos5_i2c *i2c)
> +{
> + unsigned long i2c_timeout = readl(i2c->regs + HSI2C_TIMEOUT);
> +
> + /* Clear to enable Timeout */
> + i2c_timeout &= ~HSI2C_TIMEOUT_EN;
> + writel(i2c_timeout, i2c->regs + HSI2C_TIMEOUT);
> +}
> +
> +static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
> +{
> + struct exynos5_i2c *i2c = dev_id;
> + unsigned char byte;
> +
> + if (i2c->msg->flags & I2C_M_RD) {
> + while ((readl(i2c->regs + HSI2C_FIFO_STATUS) &
> + 0x1000000) == 0) {
> + byte = (unsigned char)readl(i2c->regs + HSI2C_RX_DATA);
> + i2c->msg->buf[i2c->msg_byte_ptr++] = byte;
> + }
> +
> + if (i2c->msg_byte_ptr >= i2c->msg->len)
> + exynos5_i2c_stop(i2c);
> + } else {
> + byte = i2c->msg->buf[i2c->msg_byte_ptr++];
> + writel(byte, i2c->regs + HSI2C_TX_DATA);
> +
> + if (i2c->msg_byte_ptr >= i2c->msg->len)
> + exynos5_i2c_stop(i2c);
> + }
you never check which event generated the IRQ. Why don't you use
register address 0x24 ??
> + exynos5_disable_irq(i2c);
why are you masking IRQs here ?
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int exynos5_i2c_init(struct exynos5_i2c *i2c);
> +
> +static int exynos5_i2c_reset(struct exynos5_i2c *i2c)
> +{
> + unsigned long usi_ctl;
> +
> + usi_ctl = readl(i2c->regs + HSI2C_CTL);
> + usi_ctl |= (1u << 31);
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> + usi_ctl = readl(i2c->regs + HSI2C_CTL);
> + usi_ctl &= ~(1u << 31);
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> + exynos5_i2c_init(i2c);
> +
> + return 0;
> +}
> +
> +static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
> + struct i2c_msg *msgs, int num, int stop)
this "num" argument is useless.
> +{
> + unsigned long timeout;
> + unsigned long trans_status;
> + unsigned long usi_fifo_stat;
> + unsigned long usi_ctl;
> + unsigned long i2c_auto_conf;
> + unsigned long i2c_addr;
> + unsigned long usi_int_en;
> + unsigned long usi_fifo_ctl;
> + unsigned char byte;
> + int ret = 0;
> + int operation_mode = i2c->operation_mode;
> +
> + i2c->msg = msgs;
> + i2c->msg_byte_ptr = 0;
> +
> + init_completion(&i2c->msg_complete);
actually, init_completion() should be done on your probe(). Here you
should be calling INIT_COMPLETION() instead.
> + usi_ctl = readl(i2c->regs + HSI2C_CTL);
> + i2c_auto_conf = readl(i2c->regs + HSI2C_AUTO_CONFING);
do you really need to read these two registers ? You're programming a
new transfer, meaning that you're likely to change the entire
register...
> + exynos5_i2c_en_timeout(i2c);
> +
> + /* Set default trigger level for TXFIFO and RXFIFO */
> + usi_fifo_ctl = HSI2C_TXFIFO_TRIGGER_LEVEL |
> + HSI2C_RXFIFO_TRIGGER_LEVEL;
> + /* Enable RXFIFO and TXFIFO */
> + usi_fifo_ctl = HSI2C_RXFIFO_EN | HSI2C_TXFIFO_EN;
> +
> + writel(usi_fifo_ctl, i2c->regs + HSI2C_FIFO_CTL);
> +
> + usi_int_en = 0;
> + if (msgs->flags & I2C_M_RD) {
> + usi_ctl &= ~HSI2C_TXCHON;
> + usi_ctl |= HSI2C_RXCHON;
> +
> + i2c_auto_conf |= HSI2C_READ_WRITE;
> +
> + usi_int_en |= (HSI2C_INT_RX_ALMOSTFULL_EN |
> + HSI2C_INT_TRAILING_EN);
> + } else {
> + usi_ctl &= ~HSI2C_RXCHON;
> + usi_ctl |= HSI2C_TXCHON;
> +
> + i2c_auto_conf &= ~HSI2C_READ_WRITE;
> +
> + usi_int_en |= HSI2C_INT_TX_ALMOSTEMPTY_EN;
> + }
> +
> + if (stop == 1)
> + i2c_auto_conf |= HSI2C_STOP_AFTER_TRANS;
> + else
> + i2c_auto_conf &= ~HSI2C_STOP_AFTER_TRANS;
How about:
i2c_auto_conf |= stop ? HSI2C_STOP_AFTER_TRANS : 0; ??
> +
> +
one blank line only.
> + i2c_addr = readl(i2c->regs + HSI2C_ADDR);
why do you need to read it ? Is there more information in this register
other than the address ???
> + /* Clear Slave Address for I2C Master (Auto mode only) */
> + i2c_addr &= ~(0x3ff << 10);
> + /* Clear Slave Address for I2C Slave */
> + i2c_addr &= ~(0x3ff << 0);
> + /* Clear Master ID for I2C Master (Auto and HS mode only) */
> + if (i2c->speed_mode == HSI2C_HIGH_SPD)
> + i2c_addr &= ~(0xff << 24);
> +
> + i2c_addr |= ((msgs->addr & 0x7f) << 10);
> + writel(i2c_addr, i2c->regs + HSI2C_ADDR);
> +
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> +
> + /* Clear and set TRANS_LEN */
> + i2c_auto_conf &= ~(0xffff);
> + i2c_auto_conf |= i2c->msg->len;
> + writel(i2c_auto_conf, i2c->regs + HSI2C_AUTO_CONFING);
> +
> + /* Start data transfer in Master mode */
> + i2c_auto_conf = readl(i2c->regs + HSI2C_AUTO_CONFING);
> + i2c_auto_conf |= HSI2C_MASTER_RUN;
> + writel(i2c_auto_conf, i2c->regs + HSI2C_AUTO_CONFING);
> +
> + /* Enable appropriate interrupts */
> + if (operation_mode != HSI2C_POLLING)
> + writel(usi_int_en, i2c->regs + HSI2C_INT_ENABLE);
> +
> + ret = -EAGAIN;
> + if (msgs->flags & I2C_M_RD) {
> + if (operation_mode == HSI2C_POLLING) {
why do you want to support polling ? Do you really have situations where
IRQ wouldn't cut it ?
> + timeout = jiffies + EXYNOS5_I2C_TIMEOUT;
> + while (time_before(jiffies, timeout)) {
> + if ((readl(i2c->regs + HSI2C_FIFO_STATUS) &
> + 0x1000000) == 0) {
no magic constants, please.
> + byte = (unsigned char)readl
> + (i2c->regs + HSI2C_RX_DATA);
> + i2c->msg->buf[i2c->msg_byte_ptr++]
> + = byte;
> + }
> +
> + if (i2c->msg_byte_ptr >= i2c->msg->len) {
> + ret = 0;
> + break;
> + }
> + }
> +
> + if (ret == -EAGAIN) {
you can avoid this (and the above initialization of ret to -EAGAIN) if
you reorder this code a little bit.
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "rx timeout\n");
> + return ret;
> + }
> + } else {
> + timeout = wait_for_completion_interruptible_timeout
> + (&i2c->msg_complete, EXYNOS5_I2C_TIMEOUT);
> +
> + if (timeout == 0) {
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "rx timeout\n");
> + return ret;
> + }
> +
> + ret = 0;
> + }
if you really *must* support polling and IRQ modes, I suggest you
re-factor all of this into separate functions so that your code looks
like:
if (operation_mode == HSI2C_POLLING)
ret = exynos5_i2c_xfer_msg_polling(i2c, msgs, stop,
msg->flags & I2C_M_RD);
else
ret = exynos5_i2c_xfer_msg_irq(i2c, msgs, stop,
msg->flags & I2C_M_RD);
> + } else {
> + if (operation_mode == HSI2C_POLLING) {
> + timeout = jiffies + EXYNOS5_I2C_TIMEOUT;
> + while (time_before(jiffies, timeout) &&
> + (i2c->msg_byte_ptr < i2c->msg->len)) {
> + if ((readl(i2c->regs + HSI2C_FIFO_STATUS)
> + & 0x7f) < 64) {
> + byte = i2c->msg->buf
> + [i2c->msg_byte_ptr++];
> + writel(byte,
> + i2c->regs + HSI2C_TX_DATA);
> + }
> + }
> + } else {
> + timeout = wait_for_completion_interruptible_timeout
> + (&i2c->msg_complete, EXYNOS5_I2C_TIMEOUT);
> +
> + if (timeout == 0) {
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "tx timeout\n");
> + return ret;
> + }
> +
> + timeout = jiffies + timeout;
> + }
> + while (time_before(jiffies, timeout)) {
> + usi_fifo_stat = readl(i2c->regs + HSI2C_FIFO_STATUS);
> + trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
> + if ((usi_fifo_stat == HSI2C_FIFO_EMPTY) &&
> + ((trans_status == 0) ||
> + ((stop == 0) &&
> + (trans_status == 0x20000)))) {
> + ret = 0;
> + break;
> + }
> + }
> + if (ret == -EAGAIN) {
> + exynos5_i2c_reset(i2c);
> + dev_warn(i2c->dev, "tx timeout\n");
> + return ret;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int exynos5_i2c_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msgs, int num)
> +{
> + struct exynos5_i2c *i2c = (struct exynos5_i2c *)adap->algo_data;
> + int retry, i;
> + int ret;
> + int stop = 0;
> + struct i2c_msg *msgs_ptr = msgs;
> +
> + if (i2c->suspended) {
> + dev_err(i2c->dev, "HS-I2C is not initialzed.\n");
> + return -EIO;
> + }
> +
> + clk_prepare_enable(i2c->clk);
> +
> + for (retry = 0; retry < adap->retries; retry++) {
> + for (i = 0; i < num; i++) {
> + if (i == num - 1)
> + stop = 1;
> + ret = exynos5_i2c_xfer_msg(i2c, msgs_ptr, 1, stop);
> + msgs_ptr++;
> +
> + if (ret == -EAGAIN) {
> + msgs_ptr = msgs;
> + stop = 0;
> + break;
> + }
> + }
> + if (i == num) {
> + clk_disable_unprepare(i2c->clk);
why don't you use pm_runtime for that ? With the OMAP driver, we figured
that asynchronous PM improved transfer processing time quite a lot
because we would only cut controller's clocks when it was idle for at
least 500ms.
> + return num;
> + }
> +
> + dev_dbg(i2c->dev, "retrying transfer (%d)\n", retry);
> +
> + udelay(100);
> + }
> +
> + clk_disable_unprepare(i2c->clk);
> +
> + return -EREMOTEIO;
> +}
> +
> +static u32 exynos5_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C;
do you not support SMBUS ? Should you request for SW emulation, then ?
> +}
> +
> +static const struct i2c_algorithm exynos5_i2c_algorithm = {
> + .master_xfer = exynos5_i2c_xfer,
> + .functionality = exynos5_i2c_func,
> +};
> +
> +static int exynos5_i2c_set_timing(struct exynos5_i2c *i2c)
> +{
> + unsigned long i2c_timing_s1;
> + unsigned long i2c_timing_s2;
> + unsigned long i2c_timing_s3;
> + unsigned long i2c_timing_sla;
> + unsigned int op_clk;
> + unsigned int clkin = clk_get_rate(i2c->clk);
> + unsigned int n_clkdiv;
> + unsigned int t_start_su, t_start_hd;
> + unsigned int t_stop_su;
> + unsigned int t_data_su, t_data_hd;
> + unsigned int t_scl_l, t_scl_h;
> + unsigned int t_sr_release;
> + unsigned int t_ftl_cycle;
> + unsigned int i = 0, utemp0 = 0, utemp1 = 0, utemp2 = 0;
> +
> + if (i2c->speed_mode == HSI2C_HIGH_SPD)
> + op_clk = i2c->high_speed;
> + else
> + op_clk = i2c->fast_speed;
> +
> + /* FPCLK / FI2C =
> + * (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) + 8 + 2 * FLT_CYCLE
> + * uTemp0 = (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2)
> + * uTemp1 = (TSCLK_L + TSCLK_H + 2)
> + * uTemp2 = TSCLK_L + TSCLK_H
> + */
> + t_ftl_cycle = (readl(i2c->regs + HSI2C_CONF) >> 16) & 0x7;
> + utemp0 = (clkin / op_clk) - 8 - 2 * t_ftl_cycle;
> +
> + /* CLK_DIV max is 256 */
> + for (i = 0; i < 256; i++) {
> + utemp1 = utemp0 / (i + 1);
> + /* SCLK_L/H max is 256 / 2 */
> + if (utemp1 < 128) {
> + utemp2 = utemp1 - 2;
> + break;
> + }
> + }
> +
> + n_clkdiv = i;
> + t_scl_l = utemp2 / 2;
> + t_scl_h = utemp2 / 2;
> + t_start_su = t_scl_l;
> + t_start_hd = t_scl_l;
> + t_stop_su = t_scl_l;
> + t_data_su = t_scl_l / 2;
> + t_data_hd = t_scl_l / 2;
> + t_sr_release = utemp2;
> +
> + i2c_timing_s1 = t_start_su << 24 | t_start_hd << 16 | t_stop_su << 8;
> + i2c_timing_s2 = t_data_su << 24 | t_scl_l << 8 | t_scl_h << 0;
> + i2c_timing_s3 = n_clkdiv << 16 | t_sr_release << 0;
> + i2c_timing_sla = t_data_hd << 0;
> +
> + dev_dbg(i2c->dev, "tSTART_SU: %X, tSTART_HD: %X, tSTOP_SU: %X\n",
> + t_start_su, t_start_hd, t_stop_su);
> + dev_dbg(i2c->dev, "tDATA_SU: %X, tSCL_L: %X, tSCL_H: %X\n",
> + t_data_su, t_scl_l, t_scl_h);
> + dev_dbg(i2c->dev, "nClkDiv: %X, tSR_RELEASE: %X\n",
> + n_clkdiv, t_sr_release);
> + dev_dbg(i2c->dev, "tDATA_HD: %X\n", t_data_hd);
> +
> + if (i2c->speed_mode == HSI2C_HIGH_SPD) {
> + writel(i2c_timing_s1, i2c->regs + HSI2C_TIMING_HS1);
> + writel(i2c_timing_s2, i2c->regs + HSI2C_TIMING_HS2);
> + writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_HS3);
> + } else {
> + writel(i2c_timing_s1, i2c->regs + HSI2C_TIMING_FS1);
> + writel(i2c_timing_s2, i2c->regs + HSI2C_TIMING_FS2);
> + writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_FS3);
> + }
> + writel(i2c_timing_sla, i2c->regs + HSI2C_TIMING_SLA);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static int exynos5_i2c_parse_dt_gpio(struct exynos5_i2c *i2c)
> +{
> + int idx, gpio, ret;
> +
> + for (idx = 0; idx < 2; idx++) {
> + gpio = of_get_gpio(i2c->dev->of_node, idx);
> + if (!gpio_is_valid(gpio)) {
> + dev_err(i2c->dev, "invalid gpio[%d]: %d\n", idx, gpio);
> + goto free_gpio;
> + }
> + i2c->gpios[idx] = gpio;
> +
> + ret = gpio_request(gpio, "hsi2c-bus");
> + if (ret) {
> + dev_err(i2c->dev, "gpio [%d] request failed\n", gpio);
> + goto free_gpio;
> + }
> + }
> + return 0;
> +
> +free_gpio:
> + while (--idx >= 0)
> + gpio_free(i2c->gpios[idx]);
> + return -EINVAL;
> +}
> +
> +static void exynos5_i2c_dt_gpio_free(struct exynos5_i2c *i2c)
> +{
> + unsigned int idx;
> +
> + for (idx = 0; idx < 2; idx++)
> + gpio_free(i2c->gpios[idx]);
> +}
> +#else
> +static int exynos5_i2c_parse_dt_gpio(struct exynos5_i2c *i2c)
> +{
> + return 0;
> +}
> +
> +static void exynos5_i2c_dt_gpio_free(struct exynos5_i2c *i2c)
> +{
> +}
> +#endif
> +
> +static int exynos5_i2c_init(struct exynos5_i2c *i2c)
> +{
> + unsigned long usi_ctl = HSI2C_FUNC_MODE_I2C | HSI2C_MASTER;
> + unsigned long usi_trailing_ctl = HSI2C_TRAILING_COUNT;
> + unsigned long i2c_conf = readl(i2c->regs + HSI2C_CONF);
> +
> + if (exynos5_i2c_parse_dt_gpio(i2c))
> + return -EINVAL;
> +
> + writel(usi_ctl, i2c->regs + HSI2C_CTL);
> +
> + writel(usi_trailing_ctl, i2c->regs + HSI2C_TRAILIG_CTL);
> +
> + exynos5_i2c_set_timing(i2c);
> +
> + if (i2c->speed_mode == HSI2C_HIGH_SPD) {
> + i2c_conf |= HSI2C_HS_MODE;
> + writel(i2c_conf, i2c->regs + HSI2C_CONF);
> + }
> +
> + return 0;
> +}
> +
> +static int exynos5_i2c_probe(struct platform_device *pdev)
> +{
> + struct exynos5_i2c *i2c;
> + struct resource *res;
> + int ret;
> +
> + i2c = devm_kzalloc(&pdev->dev, sizeof(struct exynos5_i2c), GFP_KERNEL);
> + if (!i2c) {
> + dev_err(&pdev->dev, "no memory for state\n");
> + return -ENOMEM;
> + }
> +
> + if (pdev->dev.of_node) {
> + /* i2c bus number is dynamically assigned */
> + i2c->bus_number = -1;
> +
> + if (of_property_read_u32(pdev->dev.of_node,
> + "samsung,hsi2c-speed-mode", &i2c->speed_mode))
> + i2c->speed_mode = 1;
> + if (!of_property_read_u32(pdev->dev.of_node,
> + "samsung,hsi2c-hs-clk", &i2c->high_speed))
> + i2c->high_speed = 2500000;
> + if (!of_property_read_u32(pdev->dev.of_node,
> + "samsung,hsi2c-fs-clk", &i2c->fast_speed))
> + i2c->fast_speed = 400000;
> + } else {
> + dev_err(&pdev->dev, "no device node\n");
> + return -ENOENT;
> + }
> +
> + strlcpy(i2c->adap.name, "exynos5250-i2c", sizeof(i2c->adap.name));
> + i2c->adap.owner = THIS_MODULE;
> + i2c->adap.algo = &exynos5_i2c_algorithm;
> + i2c->adap.retries = 2;
> + i2c->adap.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> +
> + i2c->dev = &pdev->dev;
> + i2c->clk = clk_get(&pdev->dev, "hsi2c");
how about devm_clk_get() ?
> + if (IS_ERR(i2c->clk)) {
> + dev_err(&pdev->dev, "cannot get clock\n");
> + ret = -ENOENT;
> + goto err_noclk;
> + }
> +
> + clk_prepare_enable(i2c->clk);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "cannot find HS-I2C IO resource\n");
> + ret = -ENOENT;
> + goto err_clk;
> + }
> +
> + i2c->ioarea = request_mem_region(res->start, resource_size(res),
> + pdev->name);
please use devm_request_and_ioremap() instead.
> +
> + if (i2c->ioarea == NULL) {
> + dev_err(&pdev->dev, "cannot request HS-I2C IO\n");
> + ret = -ENXIO;
> + goto err_clk;
> + }
> +
> + i2c->regs = ioremap(res->start, resource_size(res));
> +
> + if (i2c->regs == NULL) {
> + dev_err(&pdev->dev, "cannot map HS-I2C IO\n");
> + ret = -ENXIO;
> + goto err_ioarea;
> + }
> +
> + dev_dbg(&pdev->dev, "registers %p (%p, %p)\n",
> + i2c->regs, i2c->ioarea, res);
> +
> + i2c->adap.algo_data = i2c;
> + i2c->adap.dev.parent = &pdev->dev;
> +
> + ret = exynos5_i2c_init(i2c);
> + if (ret != 0)
> + goto err_iomap;
> +
> + i2c->irq = ret = platform_get_irq(pdev, 0);
> + if (ret <= 0) {
> + dev_err(&pdev->dev, "cannot find HS-I2C IRQ\n");
> + goto err_iomap;
> + }
> +
> + ret = request_irq(i2c->irq, exynos5_i2c_irq, IRQF_DISABLED,
> + dev_name(&pdev->dev), i2c);
devm_request_irq().
> +
> + if (ret != 0) {
> + dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq);
> + goto err_iomap;
> + }
> +
> + i2c->adap.nr = i2c->bus_number;
> + i2c->adap.dev.of_node = pdev->dev.of_node;
> +
> + ret = i2c_add_numbered_adapter(&i2c->adap);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to add bus to i2c core\n");
> + goto err_irq;
> + }
> +
> + of_i2c_register_devices(&i2c->adap);
> + platform_set_drvdata(pdev, i2c);
> +
> + dev_info(&pdev->dev, "%s: Exynos5 HS-I2C adapter\n",
> + dev_name(&i2c->adap.dev));
please drop this dev_info() as it brings no useful information. The core
I2C subsystem already prints enough information when you register an
adapter.
> + clk_disable_unprepare(i2c->clk);
> + return 0;
> +
> + err_irq:
> + free_irq(i2c->irq, i2c);
> +
> + err_iomap:
> + iounmap(i2c->regs);
> +
> + err_ioarea:
> + release_resource(i2c->ioarea);
> + kfree(i2c->ioarea);
> +
> + err_clk:
> + clk_disable_unprepare(i2c->clk);
> + clk_put(i2c->clk);
> +
> + err_noclk:
> + return ret;
> +}
> +
> +static int exynos5_i2c_remove(struct platform_device *pdev)
> +{
> + struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&i2c->adap);
> + free_irq(i2c->irq, i2c);
> +
> + clk_disable_unprepare(i2c->clk);
> + clk_put(i2c->clk);
> +
> + iounmap(i2c->regs);
> +
> + release_resource(i2c->ioarea);
> + exynos5_i2c_dt_gpio_free(i2c);
> + kfree(i2c->ioarea);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int exynos5_i2c_suspend_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_lock_adapter(&i2c->adap);
> + i2c->suspended = 1;
> + i2c_unlock_adapter(&i2c->adap);
> +
> + return 0;
> +}
> +
> +static int exynos5_i2c_resume(struct device *dev)
maybe you could add _noirq suffix here too.
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_lock_adapter(&i2c->adap);
> + clk_prepare_enable(i2c->clk);
> + exynos5_i2c_init(i2c);
> + clk_disable_unprepare(i2c->clk);
> + i2c->suspended = 0;
> + i2c_unlock_adapter(&i2c->adap);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops exynos5_i2c_dev_pm_ops = {
> + .suspend_noirq = exynos5_i2c_suspend_noirq,
> + .resume_noirq = exynos5_i2c_resume,
you need to define poweroff, thaw, freeze, restore.
> +};
> +
> +#define EXYNOS5_DEV_PM_OPS (&exynos5_i2c_dev_pm_ops)
> +#else
> +#define EXYNOS5_DEV_PM_OPS NULL
> +#endif
> +
> +static struct platform_driver exynos5_i2c_driver = {
> + .probe = exynos5_i2c_probe,
> + .remove = exynos5_i2c_remove,
> + .id_table = exynos5_driver_ids,
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "exynos5-i2c",
> + .pm = EXYNOS5_DEV_PM_OPS,
> + .of_match_table = of_match_ptr(exynos5_i2c_match),
> + },
> +};
> +
> +static int __init i2c_adap_exynos5_init(void)
> +{
> + return platform_driver_register(&exynos5_i2c_driver);
> +}
> +subsys_initcall(i2c_adap_exynos5_init);
> +
> +static void __exit i2c_adap_exynos5_exit(void)
> +{
> + platform_driver_unregister(&exynos5_i2c_driver);
> +}
> +module_exit(i2c_adap_exynos5_exit);
> +
> +MODULE_DESCRIPTION("Exynos5 HS-I2C Bus driver");
> +MODULE_AUTHOR("Taekgyun Ko, <taeggyun.ko@samsung.com>");
> +MODULE_LICENSE("GPL");
looks like this should be GPL v2, judging by the comment in the
beginning of this file.
> diff --git a/drivers/i2c/busses/i2c-exynos5.h b/drivers/i2c/busses/i2c-exynos5.h
> new file mode 100644
> index 0000000..063051e
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-exynos5.h
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright (C) 2012 Samsung Electronics Co., Ltd.
> + *
> + * Exynos5 series HS-I2C Controller
> + *
> + * 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.
> +*/
> +
> +#ifndef __ASM_ARCH_REGS_HS_IIC_H
> +#define __ASM_ARCH_REGS_HS_IIC_H __FILE__
no need for the __FILE__ at the end...
> +
> +/*
> + * Register Map
> + */
this is a single line comment. Should be /* Register Map */ instead.
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121127/552ec992/attachment-0001.sig>
next prev parent reply other threads:[~2012-11-27 13:30 UTC|newest]
Thread overview: 113+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-27 13:00 [PATCH 0/3] i2c: Add High speed I2C controller driver for Exynos5 Naveen Krishna Chatradhi
2012-11-27 13:00 ` Naveen Krishna Chatradhi
2012-11-27 13:00 ` [PATCH 1/3] i2c: exynos5: add High Speed I2C controller driver Naveen Krishna Chatradhi
2012-11-27 13:00 ` Naveen Krishna Chatradhi
2012-11-27 13:23 ` Felipe Balbi [this message]
2012-11-27 13:23 ` Felipe Balbi
2012-11-27 13:23 ` Felipe Balbi
2012-11-27 13:34 ` Thomas Abraham
2012-11-27 13:34 ` Thomas Abraham
2012-11-27 13:34 ` Thomas Abraham
2012-11-28 4:23 ` Naveen Krishna Ch
2012-11-28 4:23 ` Naveen Krishna Ch
2012-11-28 4:23 ` Naveen Krishna Ch
2012-12-25 11:25 ` [PATCH 1/2] " Naveen Krishna Chatradhi
2012-12-25 11:25 ` Naveen Krishna Chatradhi
2012-12-25 11:25 ` Naveen Krishna Chatradhi
2012-12-25 11:25 ` [PATCH 2/2] i2c-exynos5: add debugfs support for registers Naveen Krishna Chatradhi
2012-12-25 11:25 ` Naveen Krishna Chatradhi
2012-12-25 11:25 ` Naveen Krishna Chatradhi
2012-12-27 22:57 ` Felipe Balbi
2012-12-27 22:57 ` Felipe Balbi
2012-12-27 22:57 ` Felipe Balbi
2012-12-27 22:59 ` Felipe Balbi
2012-12-27 22:59 ` Felipe Balbi
2012-12-27 22:59 ` Felipe Balbi
2012-12-27 22:57 ` [PATCH 1/2] i2c: exynos5: add High Speed I2C controller driver Felipe Balbi
2012-12-27 22:57 ` Felipe Balbi
2012-12-27 22:57 ` Felipe Balbi
2012-12-28 6:42 ` Naveen Krishna Ch
2012-12-28 6:42 ` Naveen Krishna Ch
2012-12-28 11:27 ` [PATCH v3] " Naveen Krishna Chatradhi
2012-12-28 11:27 ` Naveen Krishna Chatradhi
2012-12-28 11:27 ` Naveen Krishna Chatradhi
2012-12-28 16:36 ` Naveen Krishna Ch
2012-12-28 16:36 ` Naveen Krishna Ch
2012-12-28 16:36 ` Naveen Krishna Ch
2013-01-15 6:23 ` Naveen Krishna Ch
2013-01-15 6:23 ` Naveen Krishna Ch
2013-01-23 5:05 ` Naveen Krishna Ch
2013-01-23 5:05 ` Naveen Krishna Ch
2013-01-23 5:05 ` Naveen Krishna Ch
2013-02-01 15:54 ` [PATCH v4] " Naveen Krishna Chatradhi
2013-02-01 15:54 ` Naveen Krishna Chatradhi
2013-02-01 19:29 ` Wolfram Sang
2013-02-01 19:29 ` Wolfram Sang
2013-02-08 13:16 ` Grant Likely
2013-03-12 4:32 ` [PATCH] " Naveen Krishna Chatradhi
[not found] ` <1363062732-27869-1-git-send-email-ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-03-12 12:53 ` Simon Glass
2013-03-12 13:13 ` Simon Glass
2013-03-12 13:13 ` Simon Glass
2013-03-20 16:26 ` Naveen Krishna Ch
2013-03-20 16:26 ` Naveen Krishna Ch
2013-03-20 16:24 ` [RFC: PATCH v5] " Naveen Krishna Chatradhi
2013-03-20 16:24 ` Naveen Krishna Chatradhi
2013-03-25 11:52 ` Yuvaraj CD
2013-03-26 9:23 ` Wolfram Sang
2013-03-28 22:01 ` [PATCH v6] " Naveen Krishna Chatradhi
2013-03-28 23:40 ` Naveen Krishna Chatradhi
2013-03-28 23:40 ` Naveen Krishna Chatradhi
2013-04-05 4:52 ` [PATCH v7] " Naveen Krishna Chatradhi
2013-04-13 4:40 ` Naveen Krishna Ch
2013-04-13 4:40 ` Naveen Krishna Ch
2013-04-16 10:29 ` Wolfram Sang
[not found] ` <CAHfPSqCe_VJMeH3oCDc0CfcmpawMj0hN7_b3ngHDFtDUsPJLsA@mail.gmail.com>
2013-05-01 23:19 ` Fwd: " Naveen Krishna Ch
2013-05-01 23:19 ` Naveen Krishna Ch
2013-05-02 11:27 ` Wolfram Sang
2013-05-02 11:27 ` Wolfram Sang
2013-05-02 11:48 ` Naveen Krishna Ch
2013-05-02 11:48 ` Naveen Krishna Ch
2013-05-07 2:50 ` [PATCH v8] " Naveen Krishna Chatradhi
2013-05-07 2:50 ` Naveen Krishna Chatradhi
2013-05-07 12:06 ` Sachin Kamat
2013-05-17 10:10 ` [PATCH v9] " Naveen Krishna Chatradhi
2013-05-17 10:10 ` Naveen Krishna Chatradhi
2013-05-23 6:29 ` Naveen Krishna Ch
2013-05-23 6:29 ` Naveen Krishna Ch
[not found] ` <1368785452-15140-1-git-send-email-ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-06-10 8:07 ` Naveen Krishna Ch
2013-06-10 8:10 ` Naveen Krishna Ch
2013-06-10 8:10 ` Naveen Krishna Ch
2013-06-10 14:38 ` Wolfram Sang
2013-06-19 10:48 ` [PATCH v10] " Naveen Krishna Chatradhi
2013-06-19 10:48 ` Naveen Krishna Chatradhi
2013-06-19 10:48 ` Naveen Krishna Chatradhi
2013-07-01 6:17 ` Wolfram Sang
2013-07-01 6:17 ` Wolfram Sang
2013-07-01 6:17 ` Wolfram Sang
2013-07-01 10:25 ` Tomasz Figa
2013-07-01 10:25 ` Tomasz Figa
2013-07-01 10:25 ` Tomasz Figa
2013-08-15 13:12 ` Wolfram Sang
2013-08-15 13:12 ` Wolfram Sang
2013-08-16 4:58 ` Naveen Krishna Ch
2013-08-16 4:58 ` Naveen Krishna Ch
2013-08-16 4:58 ` Naveen Krishna Ch
2013-08-16 7:05 ` Wolfram Sang
2013-08-16 7:05 ` Wolfram Sang
2013-08-16 7:05 ` Wolfram Sang
2013-08-21 9:24 ` [PATCH] " Naveen Krishna Chatradhi
2013-08-21 9:24 ` Naveen Krishna Chatradhi
2013-09-08 17:03 ` Wolfram Sang
2013-09-08 17:03 ` Wolfram Sang
2013-10-11 11:43 ` Naveen Krishna Ch
2013-10-11 11:43 ` Naveen Krishna Ch
2013-10-16 5:30 ` [PATCH v12] " Naveen Krishna Chatradhi
2013-10-25 11:47 ` Naveen Krishna Ch
2013-10-25 11:47 ` Naveen Krishna Ch
2013-11-01 11:35 ` Wolfram Sang
2012-11-27 13:00 ` [PATCH 2/3] ARM: exynos5: Add gate clocks for HS-I2C Naveen Krishna Chatradhi
2012-11-27 13:00 ` Naveen Krishna Chatradhi
2012-11-27 13:00 ` Naveen Krishna Chatradhi
2012-11-27 13:00 ` [PATCH 3/3] arm: exynos5: Add HS-I2C device tree platform information Naveen Krishna Chatradhi
2012-11-27 13:00 ` Naveen Krishna Chatradhi
2012-11-27 13:00 ` Naveen Krishna Chatradhi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20121127132336.GB22556@arwen.pp.htv.fi \
--to=balbi@ti.com \
--cc=ch.naveen@samsung.com \
--cc=devicetree-discuss@lists.ozlabs.org \
--cc=grant.likely@secretlab.ca \
--cc=kgene.kim@samsung.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=naveenkrishna.ch@gmail.com \
--cc=taeggyun.ko@samsung.com \
--cc=w.sang@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.