Message-ID: <20130416102902.GE16978@the-dreams.de>
Date:	Tue, 16 Apr 2013 12:29:02 +0200
From:	Wolfram Sang <wsa@...-dreams.de>
To:	Naveen Krishna Chatradhi <naveenkrishna.ch@...il.com>
Cc:	linux-i2c@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
	linux-kernel@...r.kernel.org, w.sang@...gutronix.de,
	grant.likely@...retlab.ca, devicetree-discuss@...ts.ozlabs.org,
	sjg@...omium.org, broonie@...nsource.wolfsonmicro.com,
	Naveen Krishna Chatradhi <ch.naveen@...sung.com>
Subject: Re: [PATCH v7] i2c: exynos5: add High Speed I2C controller driver

Hi,

thanks for the submission.

On Thu, Apr 04, 2013 at 09:52:01PM -0700, Naveen Krishna Chatradhi wrote:
> From: Naveen Krishna Chatradhi <ch.naveen@...sung.com>
> 
> 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.

> Driver only supports Device Tree method.
> Note: Added debugfs support for registers view, not tested.

Then leave it out, please.

> 
> Signed-off-by: Taekgyun Ko <taeggyun.ko@...sung.com>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@...sung.com>
> Reviewed-by: Simon Glass <sjg@...gle.com>
> Tested-by: Andrew Bresticker <abrestic@...gle.com>
> ---
> 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: <SDA, SCL>.
> +      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.

> +
> +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.

> +
> +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.

> +	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.

> +
> +/* 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?

> +	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.

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

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

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

> +	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, <ch.naveen@...sung.com>");
> +MODULE_AUTHOR("Taekgyun Ko, <taeggyun.ko@...sung.com>");
> +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?

Thanks,

   Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists