Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758751Ab3EAXUW (ORCPT ); Wed, 1 May 2013 19:20:22 -0400 Received: from mail-pd0-f172.google.com ([209.85.192.172]:62713 "EHLO mail-pd0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757929Ab3EAXUT (ORCPT ); Wed, 1 May 2013 19:20:19 -0400 MIME-Version: 1.0 In-Reply-To: References: <1354021236-28596-2-git-send-email-ch.naveen@samsung.com> <1365137521-31187-1-git-send-email-naveenkrishna.ch@gmail.com> <20130416102902.GE16978@the-dreams.de> From: Naveen Krishna Ch Date: Wed, 1 May 2013 16:19:58 -0700 Message-ID: Subject: Fwd: [PATCH v7] i2c: exynos5: add High Speed I2C controller driver To: linux-i2c@vger.kernel.org, "linux-samsung-soc@vger.kernel.org" , linux-kernel@vger.kernel.org, grant.likely@secretlab.ca, devicetree-discuss@lists.ozlabs.org, sjg@chromium.org, Mark Brown , Naveen Krishna Chatradhi Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 32596 Lines: 954 On 16 April 2013 15:59, Wolfram Sang wrote: > Hi, > > thanks for the submission. > > On Thu, Apr 04, 2013 at 09:52:01PM -0700, Naveen Krishna Chatradhi wrote: >> From: Naveen Krishna Chatradhi >> >> Adds support for High Speed I2C driver found in Exynos5 and >> later SoCs from Samsung. >> This driver currently supports Auto mode. > > Either explain the limitation of this mode or just leave this sentence. Sure will add bit more explanation > >> Driver only supports Device Tree method. >> Note: Added debugfs support for registers view, not tested. > > Then leave it out, please. I prefer leave it out for now. > >> >> Signed-off-by: Taekgyun Ko >> Signed-off-by: Naveen Krishna Chatradhi >> Reviewed-by: Simon Glass >> Tested-by: Andrew Bresticker >> --- >> change since v6: >> 1. clock divisor function hs split to handle the error cases >> 2. Other irq types are handled >> 3. FIFO are handled more efficiently in TX and RX >> 4. More function description added >> 5. handled the return cases in xfer_msg function >> >> .../devicetree/bindings/i2c/i2c-exynos5.txt | 50 ++ >> drivers/i2c/busses/Kconfig | 7 + >> drivers/i2c/busses/Makefile | 1 + >> drivers/i2c/busses/i2c-exynos5.c | 934 ++++++++++++++++++++ >> 4 files changed, 992 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-exynos5.txt >> create mode 100644 drivers/i2c/busses/i2c-exynos5.c >> >> diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt >> new file mode 100644 >> index 0000000..0bc9347 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt >> @@ -0,0 +1,50 @@ >> +* Samsung's High Speed I2C controller >> + >> +The Samsung's High Speed I2C controller is used to interface with I2C devices >> +at various speeds ranging from 100khz to 3.4Mhz. >> + >> +Required properties: >> + - compatible: value should be. >> + (a) "samsung,exynos5-hsi2c", for i2c compatible with exynos5 hsi2c. >> + - reg: physical base address of the controller and length of memory mapped >> + region. >> + - interrupts: interrupt number to the cpu. >> + >> + - Samsung GPIO variant (deprecated): >> + - gpios: The order of the gpios should be the following: . >> + The gpio specifier depends on the gpio controller. > > Huh? Why should we support a deprecated method with a new driver? > >> + - Pinctrl variant (preferred, if available): >> + - pinctrl-0: Pin control group to be used for this controller. >> + - pinctrl-names: Should contain only one value - "default". >> + >> +Optional properties: >> + - samsung,hs-mode: Mode of operation, High speed or Fast speed mode. If not >> + specified, default value is 0. >> + - samsung,hs-clock-freq: Desired operating frequency in Hz of the bus. >> + If not specified, the default value in Hz is 100000. >> + - samsung,fs-clock-freq: Desired operarting frequency in Hz of the bus. >> + If not specified, the default value in Hz is 100000. > > NACK! We have a generic binding for defining the bus speed. And > shouldn't hs-mode be set depending on the bus speed? > >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index adfee98..9fbfa01 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -434,6 +434,13 @@ config I2C_EG20T >> ML7213/ML7223/ML7831 is companion chip for Intel Atom E6xx series. >> ML7213/ML7223/ML7831 is completely compatible for Intel EG20T PCH. >> >> +config I2C_EXYNOS5 >> + tristate "Exynos5 high-speed I2C driver" >> + depends on ARCH_EXYNOS5 && OF >> + help >> + Say Y here to include support for High-speed I2C controller in the >> + Exynos5 based Samsung SoCs. > > s/High/high/ > >> +struct exynos5_i2c { >> + struct i2c_adapter adap; >> + unsigned int suspended:1; >> + >> + struct i2c_msg *msg; >> + struct completion msg_complete; >> + unsigned int msg_ptr; >> + unsigned int msg_len; >> + >> + unsigned int irq; >> + >> + void __iomem *regs; >> + struct clk *clk; >> + struct device *dev; >> + int state; >> + >> + /* GPIO lines for SDA/SCL*/ >> + int gpios[2]; >> + >> + /* Controller operating frequency */ >> + unsigned int clock; >> + unsigned int clk_cycle; >> + unsigned int clk_div; >> + >> + /* HSI2C Controller can operate in >> + * 1. High speed upto 3.4Mbps >> + * 2. Fast speed upto 1Mbps >> + */ >> + int speed_mode; >> +}; > > Only one space as indentation after the type, please. Will change to 1 space for all of them > >> + >> +static const struct of_device_id exynos5_i2c_match[] = { >> + { .compatible = "samsung,exynos5-hsi2c" }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, exynos5_i2c_match); >> + >> +static inline void exynos5_i2c_stop(struct exynos5_i2c *i2c) >> +{ >> + writel(0, i2c->regs + HSI2C_INT_ENABLE); >> + >> + complete(&i2c->msg_complete); >> +} > > I wonder if this needs to be a seperate function. Initial patch used exynos5_i2c_stop function several times. But, not its only once. Will remove > >> + >> +static void exynos5_i2c_en_timeout(struct exynos5_i2c *i2c) >> +{ >> + u32 i2c_timeout = readl(i2c->regs + HSI2C_TIMEOUT); >> + >> + /* Clear to enable Timeout */ >> + i2c_timeout &= ~HSI2C_TIMEOUT_EN; >> + writel(i2c_timeout, i2c->regs + HSI2C_TIMEOUT); >> +} >> + >> +static void exynos5_i2c_clr_pend_irq(struct exynos5_i2c *i2c) >> +{ >> + /* Set these bits to clear them */ > > Comment a bit obvious. Shall i remove. > >> + writel(readl(i2c->regs + HSI2C_INT_STATUS), >> + i2c->regs + HSI2C_INT_STATUS); >> + >> +} >> + >> +/* exynos5_i2c_clock_info: Calculates the clock divisor and >> + * the clock cycle length (SCLK High + SCLK Low) >> + * >> + * Returns 0 on success, -EINVAL if the cycle length cannot >> + * be calculated. >> + */ >> +static int exynos5_i2c_clock_info(struct exynos5_i2c *i2c) >> +{ >> + unsigned int op_clk = i2c->clock; >> + unsigned int clkin = clk_get_rate(i2c->clk); >> + unsigned int i, utemp0 = 0, utemp1 = 0; >> + unsigned int t_ftl_cycle; >> + >> + /* 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) >> + */ >> + 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); >> + >> + /* SCL_L and SCL_H each has max value of 255 >> + * Hence, For the clk_cycle to the have right value >> + * utemp1 has to be less then 512 and more than 4. >> + */ >> + if ((utemp1 < 512) && (utemp1 > 4)) { >> + i2c->clk_cycle = utemp1 - 2; >> + i2c->clk_div = i; >> + return 0; >> + } >> + } >> + >> + dev_warn(i2c->dev, "%s: Wrong values for clock divisor and clock cycle," >> + "aborting\n", __func__); >> + return -EINVAL; >> +} >> + >> +/* exynos5_i2c_set_timing: updates the registers with appropriate >> + * timing values calculated earlier (exynos5_i2c_clock_info) >> + * >> + * To handle the error case in calculates and to avoid the subsequent >> + * calculates the clk_div and clk_cycle are stored >> + */ >> +static void exynos5_i2c_set_timing(struct exynos5_i2c *i2c) >> +{ >> + u32 i2c_timing_s1; >> + u32 i2c_timing_s2; >> + u32 i2c_timing_s3; >> + u32 i2c_timing_sla; >> + 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; >> + >> + n_clkdiv = i2c->clk_div; >> + t_scl_l = i2c->clk_cycle / 2; >> + t_scl_h = i2c->clk_cycle / 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 = i2c->clk_cycle; >> + >> + 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); >> +} >> + >> +/* exynos5_i2c_init: configures the controller for I2C functionality >> + * Programs I2C controller for Master mode operation >> + * >> + * Note: Currently, supports AUTO mode of operation. >> + */ >> +static void exynos5_i2c_init(struct exynos5_i2c *i2c) >> +{ >> + u32 i2c_conf = readl(i2c->regs + HSI2C_CONF); >> + >> + writel((HSI2C_FUNC_MODE_I2C | HSI2C_MASTER), >> + i2c->regs + HSI2C_CTL); >> + writel(HSI2C_TRAILING_COUNT, 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 | HSI2C_AUTO_MODE, i2c->regs + HSI2C_CONF); >> +} >> + >> +static void exynos5_i2c_reset(struct exynos5_i2c *i2c) >> +{ >> + u32 i2c_ctl; >> + >> + /* Set and clear the bit for reset */ >> + i2c_ctl = readl(i2c->regs + HSI2C_CTL); >> + i2c_ctl |= HSI2C_SW_RST; >> + writel(i2c_ctl, i2c->regs + HSI2C_CTL); >> + >> + i2c_ctl = readl(i2c->regs + HSI2C_CTL); >> + i2c_ctl &= ~HSI2C_SW_RST; >> + writel(i2c_ctl, i2c->regs + HSI2C_CTL); >> + >> + /* Initialize the configure registers */ >> + exynos5_i2c_init(i2c); >> +} >> + >> +static void exynos5_i2c_master_run(struct exynos5_i2c *i2c) >> +{ >> + /* Start data transfer in Master mode */ >> + u32 i2c_auto_conf = readl(i2c->regs + HSI2C_AUTO_CONF); >> + i2c_auto_conf |= HSI2C_MASTER_RUN; >> + writel(i2c_auto_conf, i2c->regs + HSI2C_AUTO_CONF); >> +} > > I wonder if this needs to be a seperate function. This is to keep the functionality separate, but used only once. Will remove. > >> + >> +/* exynos5_i2c_irq: top level IRQ servicing routine >> + * >> + * INT_STATUS registers gives the interrupt details. Further, >> + * FIFO_STATUS or TRANS_STATUS registers are to be check for detailed >> + * state of the bus. >> + */ >> +static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id) >> +{ >> + struct exynos5_i2c *i2c = dev_id; >> + u32 fifo_level, int_status, fifo_status, trans_status; >> + unsigned char byte; >> + int len = 0; >> + >> + i2c->state = -EINVAL; >> + >> + int_status = readl(i2c->regs + HSI2C_INT_STATUS); >> + fifo_status = readl(i2c->regs + HSI2C_FIFO_STATUS); >> + >> + if (int_status & HSI2C_INT_I2C) { >> + trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS); >> + if (trans_status & HSI2C_NO_DEV_ACK) { >> + dev_dbg(i2c->dev, "No ACK from device\n"); >> + i2c->state = -ENXIO; >> + } else if (trans_status & HSI2C_NO_DEV) { >> + dev_dbg(i2c->dev, "No device\n"); >> + i2c->state = -ENXIO; >> + } else if (trans_status & HSI2C_TRANS_ABORT) { >> + dev_dbg(i2c->dev, "Deal with arbitration lose\n"); >> + i2c->state = -EAGAIN; >> + } else if (trans_status & HSI2C_TIMEOUT_AUTO) { >> + dev_dbg(i2c->dev, "Accessing device timed out\n"); >> + i2c->state = -EAGAIN; >> + } >> + } >> + /* TX_ALMOSTEMPTY can happen along with HSI2C_INT_I2C */ >> + else if (int_status & >> + (HSI2C_INT_TX_UNDERRUN | HSI2C_INT_TX_ALMOSTEMPTY)) { >> + fifo_level = HSI2C_TX_FIFO_LVL(fifo_status); >> + >> + /* To support probing the devices for detection */ >> + if (i2c->msg->len == 0) { >> + i2c->state = -ENXIO; >> + goto stop; >> + } >> + >> + /* 0x30 is the default trigger level for TX FIFO */ >> + len = 48 - fifo_level; >> + >> + if (len > i2c->msg->len) >> + len = i2c->msg->len; >> + >> + i2c->msg_len += len; >> + while (len > 0) { >> + byte = i2c->msg->buf[i2c->msg_ptr++]; >> + writel(byte, i2c->regs + HSI2C_TX_DATA); >> + len--; >> + } >> + i2c->state = 0; >> + goto stop; >> + } >> + /* If TX FIFO is full (give chance to clear) */ >> + else if (int_status & HSI2C_INT_TX_OVERRUN) >> + i2c->state = 0; >> + >> + if (int_status & (HSI2C_INT_RX_OVERRUN | HSI2C_INT_TRAILING | >> + HSI2C_INT_RX_UNDERRUN | HSI2C_INT_RX_ALMOSTFULL)) { >> + fifo_level = HSI2C_RX_FIFO_LVL(fifo_status); >> + >> + if (fifo_level >= i2c->msg->len) >> + len = i2c->msg->len; >> + else >> + len = fifo_level; >> + >> + i2c->msg_len += len; >> + while (len > 0) { >> + byte = (unsigned char) >> + readl(i2c->regs + HSI2C_RX_DATA); >> + i2c->msg->buf[i2c->msg_ptr++] = byte; >> + len--; >> + } >> + i2c->state = 0; >> + } >> + >> + >> + stop: >> + if ((i2c->msg_len == i2c->msg->len) || (i2c->state < 0)) >> + exynos5_i2c_stop(i2c); >> + >> + exynos5_i2c_clr_pend_irq(i2c); >> + >> + return IRQ_HANDLED; >> +} >> + >> +/* >> + * exynos5_i2c_wait_bus_idle >> + * >> + * Check for Master Busy bit in Trans status register >> + * Loop for a while if set. >> + * >> + * Returns -EBUSY if the bus cannot be bought to idle >> + */ >> +static int exynos5_i2c_wait_bus_idle(struct exynos5_i2c *i2c) >> +{ >> + unsigned long stop_time; >> + u32 trans_status; >> + >> + /* wait for 100 milli seconds for the bus to be idle */ >> + stop_time = jiffies + msecs_to_jiffies(100) + 1; >> + do { >> + trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS); >> + /* Return if the bus turns idle */ >> + if (!(trans_status & HSI2C_MASTER_BUSY)) >> + return 0; >> + >> + usleep_range(50, 200); >> + } while (time_before(jiffies, stop_time)); >> + >> + return -EBUSY; >> +} >> + >> +/* >> + * exynos5_i2c_message_start: Configures the bus and starts the xfer >> + * i2c: struct exynos5_i2c pointer for the current bus >> + * stop: Enables stop after transfer if set. Set for last transfer of >> + * in the list of messages. >> + * >> + * Configures the bus for read/write function >> + * Sets chip address to talk to, message length to be sent. >> + * Enables appropriate interrupts and sends start xfer command. >> + */ >> +static void exynos5_i2c_message_start(struct exynos5_i2c *i2c, int stop) >> +{ >> + u32 i2c_ctl; >> + u32 int_en = HSI2C_INT_I2C_EN; >> + u32 i2c_auto_conf = 0; >> + u32 fifo_ctl; >> + >> + exynos5_i2c_en_timeout(i2c); >> + >> + fifo_ctl = HSI2C_RXFIFO_EN | HSI2C_TXFIFO_EN | >> + HSI2C_TXFIFO_TRIGGER_LEVEL | HSI2C_RXFIFO_TRIGGER_LEVEL; >> + writel(fifo_ctl, i2c->regs + HSI2C_FIFO_CTL); >> + >> + i2c_ctl = readl(i2c->regs + HSI2C_CTL); >> + i2c_ctl &= ~(HSI2C_TXCHON | HSI2C_RXCHON); >> + >> + if (i2c->msg->flags & I2C_M_RD) { >> + i2c_ctl |= HSI2C_RXCHON; >> + >> + i2c_auto_conf |= HSI2C_READ_WRITE; >> + >> + int_en |= (HSI2C_INT_RX_ALMOSTFULL_EN | >> + HSI2C_INT_TRAILING_EN); >> + } else { >> + i2c_ctl |= HSI2C_TXCHON; >> + >> + int_en |= HSI2C_INT_TX_ALMOSTEMPTY_EN; >> + } >> + >> + if (stop == 1) >> + i2c_auto_conf |= HSI2C_STOP_AFTER_TRANS; >> + >> + writel(HSI2C_SLV_ADDR_MAS(i2c->msg->addr), i2c->regs + HSI2C_ADDR); >> + >> + writel(i2c_ctl, i2c->regs + HSI2C_CTL); >> + >> + /* In auto mode the length of xfer cannot be 0 */ >> + if (i2c->msg->len == 0) >> + i2c_auto_conf |= 0x1; > > So you send some byte then? Why not reject the message? This is to support the probing the devices (i2cdetect cases) > >> + else >> + i2c_auto_conf |= i2c->msg->len; >> + >> + writel(i2c_auto_conf, i2c->regs + HSI2C_AUTO_CONF); >> + >> + exynos5_i2c_master_run(i2c); >> + >> + writel(int_en, i2c->regs + HSI2C_INT_ENABLE); >> +} >> + >> +static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c, >> + struct i2c_msg *msgs, int stop) >> +{ >> + unsigned long timeout; >> + int ret; >> + >> + i2c->msg = msgs; >> + i2c->msg_ptr = 0; >> + i2c->msg_len = 0; >> + >> + INIT_COMPLETION(i2c->msg_complete); >> + >> + exynos5_i2c_message_start(i2c, stop); >> + >> + ret = wait_for_completion_interruptible_timeout >> + (&i2c->msg_complete, EXYNOS5_I2C_TIMEOUT); > > interruptible? Have you checked SIGINT works properly? It often causes > problems with I2C drivers. Not tested much, will do. > >> + if (ret >= 0) >> + timeout = ret; >> + else >> + return ret; >> + >> + ret = i2c->state; >> + >> + if ((timeout == 0) || (ret < 0)) { >> + exynos5_i2c_reset(i2c); >> + if (timeout == 0) { >> + dev_warn(i2c->dev, "%s timeout\n", >> + (msgs->flags & I2C_M_RD) ? "rx" : "tx"); >> + return ret; >> + } else if (ret == -EAGAIN) { >> + return ret; >> + } >> + } >> + >> + /* >> + * If this is the last message to be transfered (stop == 1) >> + * Then check if the bus can be brought back to idle. >> + * >> + * Return -EBUSY if the bus still busy. >> + */ >> + if (stop && exynos5_i2c_wait_bus_idle(i2c)) >> + return -EBUSY; >> + >> + /* Return the state as in interrupt routine */ >> + 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; >> + struct i2c_msg *msgs_ptr = msgs; >> + int retry, i = 0; >> + int ret = 0, ret_pm; >> + int stop = 0; >> + >> + if (i2c->suspended) { >> + dev_err(i2c->dev, "HS-I2C is not initialzed.\n"); >> + return -EIO; >> + } >> + >> + ret_pm = pm_runtime_get_sync(i2c->dev); >> + if (IS_ERR_VALUE(ret_pm)) { >> + ret = -EIO; >> + goto out; >> + } >> + >> + clk_prepare_enable(i2c->clk); >> + >> + for (retry = 0; retry < adap->retries; retry++) { >> + for (i = 0; i < num; i++) { >> + stop = (i == num - 1); >> + >> + ret = exynos5_i2c_xfer_msg(i2c, msgs_ptr, stop); >> + msgs_ptr++; >> + >> + if (ret == -EAGAIN) { >> + msgs_ptr = msgs; >> + break; >> + } else if (ret < 0) { >> + goto out; >> + } >> + } >> + >> + if ((i == num) && (ret != -EAGAIN)) >> + break; >> + >> + dev_dbg(i2c->dev, "retrying transfer (%d)\n", retry); >> + >> + udelay(100); >> + } >> + >> + if (i == num) { >> + /* only one message, return number of bytes transfered */ >> + if (num == 1) >> + ret = i2c->msg_len; >> + else >> + ret = num; >> + } else { >> + /* Only one message, cannot access the device */ >> + if (i == 1) >> + ret = -EREMOTEIO; >> + else >> + ret = i; >> + >> + dev_warn(i2c->dev, "xfer message failed\n"); >> + } >> + >> + out: >> + clk_disable_unprepare(i2c->clk); >> + pm_runtime_mark_last_busy(i2c->dev); >> + pm_runtime_put_autosuspend(i2c->dev); >> + return ret; >> +} >> + >> +static u32 exynos5_i2c_func(struct i2c_adapter *adap) >> +{ >> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; > > Have you checked SMBUS_QUICK works (with i2cdetect for example)? i2cdetect is working fine now. > >> +} >> + >> +static const struct i2c_algorithm exynos5_i2c_algorithm = { >> + .master_xfer = exynos5_i2c_xfer, >> + .functionality = exynos5_i2c_func, >> +}; >> + >> +/** >> + * Parse a list of GPIOs from a node property and request each one >> + * >> + * @param i2c i2c driver data >> + * @return 0 on success, -EINVAL on error, in which case no GPIOs requested >> +*/ >> +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); >> + return -EINVAL; >> + } >> + i2c->gpios[idx] = gpio; >> + >> + ret = devm_gpio_request(i2c->dev, gpio, "i2c-bus"); >> + if (ret) { >> + dev_err(i2c->dev, "gpio [%d] request failed\n", gpio); >> + return -EINVAL; >> + } >> + } >> + return 0; >> +} >> + >> +#define HSI2C_REG(regname) {.name = #regname, .offset = regname} >> +static struct debugfs_reg32 exynos5_hsi2c_regs[] = { >> + HSI2C_REG(HSI2C_CTL), HSI2C_REG(HSI2C_FIFO_CTL), >> + HSI2C_REG(HSI2C_TRAILIG_CTL), HSI2C_REG(HSI2C_CLK_CTL), >> + HSI2C_REG(HSI2C_CLK_SLOT), HSI2C_REG(HSI2C_INT_ENABLE), >> + HSI2C_REG(HSI2C_INT_STATUS), HSI2C_REG(HSI2C_ERR_STATUS), >> + HSI2C_REG(HSI2C_FIFO_STATUS), HSI2C_REG(HSI2C_TX_DATA), >> + HSI2C_REG(HSI2C_RX_DATA), HSI2C_REG(HSI2C_CONF), >> + HSI2C_REG(HSI2C_AUTO_CONF), HSI2C_REG(HSI2C_TIMEOUT), >> + HSI2C_REG(HSI2C_MANUAL_CMD), HSI2C_REG(HSI2C_TRANS_STATUS), >> + HSI2C_REG(HSI2C_TIMING_HS1), HSI2C_REG(HSI2C_TIMING_HS2), >> + HSI2C_REG(HSI2C_TIMING_HS3), HSI2C_REG(HSI2C_TIMING_FS1), >> + HSI2C_REG(HSI2C_TIMING_FS2), HSI2C_REG(HSI2C_TIMING_FS3), >> + HSI2C_REG(HSI2C_TIMING_SLA), HSI2C_REG(HSI2C_ADDR), >> +}; >> + >> +static struct debugfs_regset32 exynos5_hsi2c_regset = { >> + .regs = exynos5_hsi2c_regs, >> + .nregs = ARRAY_SIZE(exynos5_hsi2c_regs), >> +}; >> + >> +static struct dentry *exynos5_hsi2c_reg_debugfs; >> + >> +static int exynos5_i2c_probe(struct platform_device *pdev) >> +{ >> + struct device_node *np = pdev->dev.of_node; >> + struct exynos5_i2c *i2c; >> + int ret; >> + >> + if (!np) { >> + dev_err(&pdev->dev, "no device node\n"); >> + return -ENOENT; >> + } >> + >> + i2c = devm_kzalloc(&pdev->dev, sizeof(struct exynos5_i2c), GFP_KERNEL); >> + if (!i2c) { >> + dev_err(&pdev->dev, "no memory for state\n"); >> + return -ENOMEM; >> + } >> + >> + /* Mode of operation High/Fast Speed mode */ >> + if (of_get_property(np, "samsung,hs-mode", NULL)) { >> + i2c->speed_mode = 1; >> + if (of_property_read_u32(np, "samsung,hs-clock", &i2c->clock)) >> + i2c->clock = HSI2C_HS_TX_CLOCK; >> + } else { >> + i2c->speed_mode = 0; >> + if (of_property_read_u32(np, "samsung,fs-clock", &i2c->clock)) >> + i2c->clock = HSI2C_FS_TX_CLOCK; >> + } >> + >> + strlcpy(i2c->adap.name, "exynos5-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 = devm_clk_get(&pdev->dev, "hsi2c"); >> + if (IS_ERR(i2c->clk)) { >> + dev_err(&pdev->dev, "cannot get clock\n"); >> + return -ENOENT; >> + } >> + >> + clk_prepare_enable(i2c->clk); > > devm_*? Quite a few drivers have been converted to devm_* recently. > Check them as a guideline. > >> + >> + i2c->regs = of_iomap(np, 0); > > devm_ioremap_resource() This was a comment from Thomas on v1. https://lkml.org/lkml/2012/11/27/264 Kindly, suggest me which one is more optimal in this case. > >> + if (!i2c->regs) { >> + dev_err(&pdev->dev, "cannot map HS-I2C IO\n"); >> + ret = -EADDRNOTAVAIL; >> + goto err_clk; >> + } >> + >> + i2c->adap.algo_data = i2c; >> + i2c->adap.dev.parent = &pdev->dev; >> + >> + /* parse device tree and inititalise the gpio */ >> + ret = exynos5_i2c_parse_dt_gpio(i2c); >> + if (ret) >> + goto err_clk; >> + >> + /* Clear pending interrupts from u-boot or misc causes */ >> + exynos5_i2c_clr_pend_irq(i2c); >> + >> + ret = exynos5_i2c_clock_info(i2c); >> + if (ret) >> + goto err_iomap; >> + >> + exynos5_i2c_init(i2c); >> + >> + init_completion(&i2c->msg_complete); >> + >> + i2c->irq = ret = irq_of_parse_and_map(np, 0); >> + if (ret <= 0) { >> + dev_err(&pdev->dev, "cannot find HS-I2C IRQ\n"); >> + ret = -EINVAL; >> + goto err_iomap; >> + } >> + >> + ret = devm_request_irq(&pdev->dev, i2c->irq, exynos5_i2c_irq, >> + 0, dev_name(&pdev->dev), i2c); >> + >> + if (ret != 0) { >> + dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq); >> + goto err_iomap; >> + } >> + >> + /* >> + * TODO: Use private lock to avoid race conditions as >> + * mentioned in pm_runtime.txt >> + */ >> + pm_runtime_enable(i2c->dev); >> + pm_runtime_set_autosuspend_delay(i2c->dev, EXYNOS5_I2C_PM_TIMEOUT); >> + pm_runtime_use_autosuspend(i2c->dev); >> + >> + ret = pm_runtime_get_sync(i2c->dev); >> + if (IS_ERR_VALUE(ret)) >> + goto err_iomap; >> + >> + i2c->adap.nr = -1; > > core will set this (patch is in -next) Right. > >> + i2c->adap.dev.of_node = np; >> + >> + ret = i2c_add_numbered_adapter(&i2c->adap); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "failed to add bus to i2c core\n"); >> + goto err_pm; >> + } >> + >> + of_i2c_register_devices(&i2c->adap); >> + platform_set_drvdata(pdev, i2c); >> + >> + exynos5_hsi2c_reg_debugfs = debugfs_create_regset32("exynos5-hsi2c", >> + S_IFREG | S_IRUGO, >> + NULL, &exynos5_hsi2c_regset); >> + if (!exynos5_hsi2c_reg_debugfs) >> + dev_warn(&pdev->dev, "failed to create debugfs entries," >> + "but will continue."); >> + >> + clk_disable_unprepare(i2c->clk); >> + pm_runtime_mark_last_busy(i2c->dev); >> + pm_runtime_put_autosuspend(i2c->dev); >> + >> + return 0; >> + >> + err_pm: >> + pm_runtime_put(i2c->dev); >> + pm_runtime_disable(&pdev->dev); >> + err_iomap: >> + iounmap(i2c->regs); >> + err_clk: >> + clk_disable_unprepare(i2c->clk); >> + return ret; >> +} >> + >> +static int exynos5_i2c_remove(struct platform_device *pdev) >> +{ >> + struct exynos5_i2c *i2c = platform_get_drvdata(pdev); >> + int ret; >> + >> + ret = pm_runtime_get_sync(&pdev->dev); >> + if (IS_ERR_VALUE(ret)) >> + return ret; >> + >> + i2c_del_adapter(&i2c->adap); >> + >> + pm_runtime_put(&pdev->dev); >> + pm_runtime_disable(&pdev->dev); >> + >> + iounmap(i2c->regs); >> + clk_disable_unprepare(i2c->clk); >> + >> + if (exynos5_hsi2c_reg_debugfs) >> + debugfs_remove(exynos5_hsi2c_reg_debugfs); >> + >> + 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->suspended = 1; >> + >> + return 0; >> +} >> + >> +static int exynos5_i2c_resume_noirq(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct exynos5_i2c *i2c = platform_get_drvdata(pdev); >> + int ret = 0; >> + >> + clk_prepare_enable(i2c->clk); >> + >> + ret = exynos5_i2c_clock_info(i2c); >> + if (ret) { >> + clk_disable_unprepare(i2c->clk); >> + return ret; >> + } >> + >> + exynos5_i2c_init(i2c); >> + clk_disable_unprepare(i2c->clk); >> + i2c->suspended = 0; >> + >> + return 0; >> +} >> + >> +static const struct dev_pm_ops exynos5_i2c_dev_pm_ops = { >> + .suspend_noirq = exynos5_i2c_suspend_noirq, >> + .resume_noirq = exynos5_i2c_resume_noirq, >> +}; >> + >> +#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, >> + .driver = { >> + .owner = THIS_MODULE, >> + .name = "exynos5-hsi2c", >> + .pm = EXYNOS5_DEV_PM_OPS, >> + .of_match_table = 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("Naveen Krishna Chatradhi, "); >> +MODULE_AUTHOR("Taekgyun Ko, "); >> +MODULE_LICENSE("GPL"); > > Header says GPL v2 > > Also, quite some dev_dbgs in here. I haven't made up my mind yet if all > are needed, but maybe you can already check if you want to insist on all > of them. > > Since you need to resend anyway, can you fold the patch from Yuvaraj > Kumar C D into the driver if it makes sense to you? Sure, I've discussed with Yuvaraj on this, I will incorportate his changes and https://patchwork.kernel.org/patch/2464681/ and https://patchwork.kernel.org/patch/2420351/ in the next revision. > > Thanks, > > Wolfram Thanks for your valuable time and comments -- Shine bright, (: Nav :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/