Message-ID: <20121127132336.GB22556@arwen.pp.htv.fi>
Date:	Tue, 27 Nov 2012 15:23:36 +0200
From:	Felipe Balbi <balbi@...com>
To:	Naveen Krishna Chatradhi <ch.naveen@...sung.com>
CC:	<linux-arm-kernel@...ts.infradead.org>,
	<linux-samsung-soc@...r.kernel.org>,
	<devicetree-discuss@...ts.ozlabs.org>, <linux-i2c@...r.kernel.org>,
	<naveenkrishna.ch@...il.com>, <kgene.kim@...sung.com>,
	<grant.likely@...retlab.ca>, <w.sang@...gutronix.de>,
	<linux-kernel@...r.kernel.org>, <taeggyun.ko@...sung.com>
Subject: Re: [PATCH 1/3] i2c: exynos5: add High Speed I2C controller driver

Hi,

On Tue, Nov 27, 2012 at 06:30:34PM +0530, Naveen Krishna Chatradhi wrote:
> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
> new file mode 100644
> index 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@...sung.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

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists