[PATCHv2,1/6] i2c: sunxi: Add Allwinner A1X i2c driver

This patch implements a basic driver for the I2C host driver found on
the Allwinner A10, A13 and A31 SoCs.

Notable missing feature is 10-bit addressing.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 .../devicetree/bindings/i2c/i2c-sunxi.txt          |  19 +
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-sunxi.c                     | 455 +++++++++++++++++++++
 4 files changed, 485 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sunxi.txt
 create mode 100644 drivers/i2c/busses/i2c-sunxi.c

Just replying because I want to understand certain choices you make, 
absolutely not questioning your code!

On 05/26/13 12:20, Maxime Ripard wrote:
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
You forgot to add #include <linux/bitops.h> for BIT()


> +
> +static void sunxi_i2c_write(struct sunxi_i2c_dev *i2c_dev, u16 reg, u8 value)
> +{
> +	writel(value, i2c_dev->membase + reg);
Why writel? and why without (u32)value? I thought iowrite* where the 
preferred calls and in this case, wouldn't we want writeb since value is u8?

> +}
> +
> +static u32 sunxi_i2c_read(struct sunxi_i2c_dev *i2c_dev, u16 reg)
> +{
> +	return readl(i2c_dev->membase + reg);
And here, readl does match the return of u32, but aren't we always 
reading 8 bits since the TWI Data Register only uses the first 8 bits?
So wouldn't we want to return u8 and readb?

> +static int sunxi_i2c_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_i2c_dev *i2c_dev;
> +	struct device_node *np;
> +	u32 freq, div_m, div_n;
> +	struct resource res;
I feel stupid for questioning this, since it only shows my lack of 
knowledge, but
If you declare all the memory here, isn't all the data lost after 
exiting the _probe function? we pass a pointer to this memory in the 
of_address_to_resource() function so that fills it, right?

Or does after devm_ioremap_resource it no longer matter, since that 
function got what it needed and useless after?

Just asking because of the wdt driver and possibly mine that you told us 
to change.



Sorry for asking (again) maybe very obvious things. Just trying to learn.

Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sun, May 26, 2013 at 12:53:30PM +0200, Oliver Schinagl wrote:
> 
> Just replying because I want to understand certain choices you make,
> absolutely not questioning your code!
> 
> On 05/26/13 12:20, Maxime Ripard wrote:
> >+ * warranty of any kind, whether express or implied.
> >+ */
> >+
> >+#include <linux/clk.h>
> >+#include <linux/completion.h>
> You forgot to add #include <linux/bitops.h> for BIT()

Ah, yes, thanks

> >+static void sunxi_i2c_write(struct sunxi_i2c_dev *i2c_dev, u16 reg, u8 value)
> >+{
> >+	writel(value, i2c_dev->membase + reg);
> Why writel? and why without (u32)value? I thought iowrite* where the
> preferred calls and in this case, wouldn't we want writeb since
> value is u8?

You're right, value should be a u32 here, thanks for noticing.

For the iowrite* vs write*, there's no consensus, and as such no
preferred way. write* functions are doing an MMIO only access,
while iowrite functions can do MMIO and port I/O accesses.

Note that it doesn't change anything on ARM, since there's no port IO on
ARM.

> >+}
> >+
> >+static u32 sunxi_i2c_read(struct sunxi_i2c_dev *i2c_dev, u16 reg)
> >+{
> >+	return readl(i2c_dev->membase + reg);
> And here, readl does match the return of u32, but aren't we always
> reading 8 bits since the TWI Data Register only uses the first 8
> bits?
> So wouldn't we want to return u8 and readb?

They are meant to be general purpose accessors, so we shouldn't focus
only on the data register.

> 
> >+static int sunxi_i2c_probe(struct platform_device *pdev)
> >+{
> >+	struct sunxi_i2c_dev *i2c_dev;
> >+	struct device_node *np;
> >+	u32 freq, div_m, div_n;
> >+	struct resource res;
> I feel stupid for questioning this, since it only shows my lack of
> knowledge, but
> If you declare all the memory here, isn't all the data lost after
> exiting the _probe function? we pass a pointer to this memory in the
> of_address_to_resource() function so that fills it, right?
> 
> Or does after devm_ioremap_resource it no longer matter, since that
> function got what it needed and useless after?

The struct resource is only there to declare the base address and the
size of memory address. Once we have mapped it, we don't care about it
anymore.

Maxime

Hi Maxime,

On Sunday 26 of May 2013 12:20:37 Maxime Ripard wrote:
> This patch implements a basic driver for the I2C host driver found on
> the Allwinner A10, A13 and A31 SoCs.
> 
> Notable missing feature is 10-bit addressing.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  .../devicetree/bindings/i2c/i2c-sunxi.txt          |  19 +
>  drivers/i2c/busses/Kconfig                         |  10 +
>  drivers/i2c/busses/Makefile                        |   1 +
>  drivers/i2c/busses/i2c-sunxi.c                     | 455
> +++++++++++++++++++++ 4 files changed, 485 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sunxi.txt
>  create mode 100644 drivers/i2c/busses/i2c-sunxi.c
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-sunxi.txt
> b/Documentation/devicetree/bindings/i2c/i2c-sunxi.txt new file mode
> 100644
> index 0000000..40c16d0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-sunxi.txt
> @@ -0,0 +1,19 @@
> +Allwinner SoC I2C controller
> +
> +Required properties:
> +- compatible : Should be "allwinner,sun4i-i2c".
> +- reg: Should contain register location and length.
> +- interrupts: Should contain interrupt.
> +- clocks : The parent clock feeding the I2C controller.
> +
> +Recommended properties:
> +- clock-frequency : desired I2C bus clock frequency in Hz.
> +
> +Example:
> +i2c0: i2c@01c2ac00 {
> +	compatible = "allwinner,sun4i-i2c";
> +	reg = <0x01c2ac00 0x400>;
> +	interrupts = <7>;
> +	clocks = <&apb1_gates 0>;
> +	clock-frequency = <100000>;
> +};
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 631736e..1df8ba1 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -706,6 +706,16 @@ config I2C_STU300
>  	  This driver can also be built as a module. If so, the module
>  	  will be called i2c-stu300.
> 
> +config I2C_SUNXI
> +	tristate "Allwinner A1X I2C controller"
> +	depends on ARCH_SUNXI
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  I2C controller embedded in Allwinner A1X SoCs.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called i2c-sunxi.
> +
>  config I2C_TEGRA
>  	tristate "NVIDIA Tegra internal I2C controller"
>  	depends on ARCH_TEGRA
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 8f4fc23..7225818 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
>  obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
>  obj-$(CONFIG_I2C_SIRF)		+= i2c-sirf.o
>  obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
> +obj-$(CONFIG_I2C_SUNXI)		+= i2c-sunxi.o
>  obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
>  obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
>  obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
> diff --git a/drivers/i2c/busses/i2c-sunxi.c
> b/drivers/i2c/busses/i2c-sunxi.c new file mode 100644
> index 0000000..5d93f26
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-sunxi.c
> @@ -0,0 +1,455 @@
> +/*
> + * Allwinner A1X SoCs i2c controller driver.
> + *
> + * Copyright (C) 2013 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +
> +#define SUNXI_I2C_ADDR_REG		(0x00)
> +#define SUNXI_I2C_ADDR_ADDR(v)			((v & 0x7f) << 1)
> +#define SUNXI_I2C_XADDR_REG		(0x04)
> +#define SUNXI_I2C_DATA_REG		(0x08)
> +#define SUNXI_I2C_CNTR_REG		(0x0c)
> +#define SUNXI_I2C_CNTR_ASSERT_ACK		BIT(2)
> +#define SUNXI_I2C_CNTR_INT_FLAG			BIT(3)
> +#define SUNXI_I2C_CNTR_MASTER_STOP		BIT(4)
> +#define SUNXI_I2C_CNTR_MASTER_START		BIT(5)
> +#define SUNXI_I2C_CNTR_BUS_ENABLE		BIT(6)
> +#define SUNXI_I2C_CNTR_INT_ENABLE		BIT(7)
> +#define SUNXI_I2C_STA_REG		(0x10)
> +#define SUNXI_I2C_STA_BUS_ERROR			(0x00)
> +#define SUNXI_I2C_STA_START			(0x08)
> +#define SUNXI_I2C_STA_START_REPEAT		(0x10)
> +#define SUNXI_I2C_STA_MASTER_WADDR_ACK		(0x18)
> +#define SUNXI_I2C_STA_MASTER_WADDR_NAK		(0x20)
> +#define SUNXI_I2C_STA_MASTER_DATA_SENT_ACK	(0x28)
> +#define SUNXI_I2C_STA_MASTER_DATA_SENT_NAK	(0x30)
> +#define SUNXI_I2C_STA_MASTER_RADDR_ACK		(0x40)
> +#define SUNXI_I2C_STA_MASTER_RADDR_NAK		(0x48)
> +#define SUNXI_I2C_STA_MASTER_DATA_RECV_ACK	(0x50)
> +#define SUNXI_I2C_STA_MASTER_DATA_RECV_NAK	(0x58)
> +#define SUNXI_I2C_CCR_REG		(0x14)
> +#define SUNXI_I2C_CCR_DIV_N(val)		(val & 0x3)
> +#define SUNXI_I2C_CCR_DIV_M(val)		((val & 0xf) << 3)
> +#define SUNXI_I2C_SRST_REG		(0x18)
> +#define SUNXI_I2C_SRST_RESET			BIT(0)
> +#define SUNXI_I2C_EFR_REG		(0x1c)
> +#define SUNXI_I2C_LCR_REG		(0x20)
> +
> +#define SUNXI_I2C_DONE			BIT(0)
> +#define SUNXI_I2C_ERROR			BIT(1)
> +#define SUNXI_I2C_NAK			BIT(2)
> +#define SUNXI_I2C_BUS_ERROR		BIT(3)
> +
> +struct sunxi_i2c_dev {
> +	struct i2c_adapter	adapter;
> +	struct clk		*clk;
> +	struct device		*dev;
> +	struct completion	completion;
> +	unsigned int		irq;
> +	void __iomem		*membase;
> +
> +	struct i2c_msg		*msg_cur;
> +	u8			*msg_buf;
> +	size_t			msg_buf_remaining;
> +	unsigned int		msg_err;
> +};
> +
> +static void sunxi_i2c_write(struct sunxi_i2c_dev *i2c_dev, u16 reg, u8
> value) +{
> +	writel(value, i2c_dev->membase + reg);
> +}
> +
> +static u32 sunxi_i2c_read(struct sunxi_i2c_dev *i2c_dev, u16 reg)
> +{
> +	return readl(i2c_dev->membase + reg);
> +}
> +
> +/*
> + * This is where all the magic happens. The I2C controller works as a
> + * state machine, each state being a step in the i2c protocol, with
> + * the controller sending an interrupt at each state transition.
> + *
> + * The state we're in is stored in a register, which leads to a pretty
> + * huge switch statement, all of this in the interrupt handler...
> + */
> +static irqreturn_t sunxi_i2c_handler(int irq, void *data)
> +{
> +	struct sunxi_i2c_dev *i2c_dev = (struct sunxi_i2c_dev *)data;
> +	u32 status = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> +	u32 addr, val;
> +
> +	if (!(status & SUNXI_I2C_CNTR_INT_FLAG))
> +		return IRQ_NONE;
> +
> +	/* Read the current state we're in */
> +	status = sunxi_i2c_read(i2c_dev, SUNXI_I2C_STA_REG);
> +
> +	switch (status & 0xff) {
> +	/* Start condition has been transmitted */
> +	case SUNXI_I2C_STA_START:
> +	/* A repeated start condition has been transmitted */
> +	case SUNXI_I2C_STA_START_REPEAT:
> +		addr = SUNXI_I2C_ADDR_ADDR(i2c_dev->msg_cur->addr);
> +
> +		if (i2c_dev->msg_cur->flags & I2C_M_RD)
> +			addr |= 1;
> +
> +		sunxi_i2c_write(i2c_dev, SUNXI_I2C_DATA_REG, addr);
> +		break;
> +
> +	/*
> +	 * Address + Write bit have been transmitted, ACK has not been
> +	 * received. It should be treated the same way than when a
> +	 * data byte has been sent and no ACK has been received, and
> +	 * we fall through to the next case in our switch.
> +	 */
> +	case SUNXI_I2C_STA_MASTER_WADDR_NAK:
> +	/*
> +	 * Data byte has been transmitted, ACK has not been
> +	 * received. If we don't care about the NAKs, we just treat
> +	 * this case as if an ACK would have been received, and
> +	 * we fall through to the next case in our switch.
> +	 */
> +	case SUNXI_I2C_STA_MASTER_DATA_SENT_NAK:
> +		if (!(i2c_dev->msg_cur->flags & I2C_M_IGNORE_NAK)) {
> +			i2c_dev->msg_err = SUNXI_I2C_NAK;
> +			goto out;
> +		}
> +
> +	/*
> +	 * Address + Write bit have been transmitted, ACK has been
> +	 * received. In the I2C protocol sequence, we are in exactly
> +	 * the same case than when we sent some data bytes and there
> +	 * is still some data to be sent, and we fall through to the
> +	 * next case in our switch.
> +	 */
> +	case SUNXI_I2C_STA_MASTER_WADDR_ACK:
> +	/* Data byte has been transmitted, ACK has been received */
> +	case SUNXI_I2C_STA_MASTER_DATA_SENT_ACK:
> +		if (i2c_dev->msg_buf_remaining) {
> +			sunxi_i2c_write(i2c_dev, SUNXI_I2C_DATA_REG,
> +					*i2c_dev->msg_buf);
> +			i2c_dev->msg_buf++;
> +			i2c_dev->msg_buf_remaining--;
> +			break;
> +		}
> +
> +		if (i2c_dev->msg_buf_remaining == 0) {
> +			i2c_dev->msg_err = SUNXI_I2C_DONE;
> +			goto out;
> +		}
> +
> +		break;
> +
> +	/*
> +	 * Address + Read bit have been transmitted, ACK has not been
> +	 * received. If we don't care about the NAKs, we just treat
> +	 * this case as if an ACK would have been received, and fall
> +	 * through to the next case.
> +	 */
> +	case SUNXI_I2C_STA_MASTER_RADDR_NAK:
> +		if (!(i2c_dev->msg_cur->flags & I2C_M_IGNORE_NAK)) {
> +			i2c_dev->msg_err = SUNXI_I2C_NAK;
> +			goto out;
> +		}

I meant something more like:

+
+	/* Intentional fall through. */

This is enough to let the reader know that there is no break missing here 
and you can keep those nice short state descriptions from v1.

> +
> +	/*
> +	 * Address + Read bit have been transmitted, ACK has been
> +	 * received.
> +	 */
> +	case SUNXI_I2C_STA_MASTER_RADDR_ACK:
> +		/*
> +		 * We only need to send the ACK for the all the bytes
> +		 * but the last one
> +		 */
> +		if (i2c_dev->msg_buf_remaining > 1) {
> +			val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> +			sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> +					val | SUNXI_I2C_CNTR_ASSERT_ACK);
> +		}
> +
> +		break;
> +
> +	/*
> +	 * Data byte has been received, ACK has not been
> +	 * transmitted. If we don't care about the NAK and that some
> +	 * data have still to be received, we fall through to the next
> +	 * case in our switch.
> +	 */
> +	case SUNXI_I2C_STA_MASTER_DATA_RECV_NAK:
> +		if (i2c_dev->msg_buf_remaining == 1) {
> +			val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_DATA_REG);
> +			*i2c_dev->msg_buf = val & 0xff;
> +			i2c_dev->msg_buf_remaining--;
> +			i2c_dev->msg_err = SUNXI_I2C_DONE;
> +			goto out;
> +		}
> +
> +		if (!(i2c_dev->msg_cur->flags & I2C_M_IGNORE_NAK)) {
> +			i2c_dev->msg_err = SUNXI_I2C_NAK;
> +			goto out;
> +		}
> +
> +	/* Data byte has been received, ACK has been transmitted */
> +	case SUNXI_I2C_STA_MASTER_DATA_RECV_ACK:
> +		val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_DATA_REG) & 0xff;
> +		*i2c_dev->msg_buf = val;
> +		i2c_dev->msg_buf++;
> +		i2c_dev->msg_buf_remaining--;
> +
> +		/* If there's only one byte left, disable the ACK */
> +		if (i2c_dev->msg_buf_remaining == 1) {
> +			val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> +			sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> +					val & ~SUNXI_I2C_CNTR_ASSERT_ACK);
> +
> +		};
> +
> +		break;
> +
> +	case SUNXI_I2C_STA_BUS_ERROR:
> +		i2c_dev->msg_err = SUNXI_I2C_BUS_ERROR;
> +		goto out;
> +
> +	default:
> +		i2c_dev->msg_err = SUNXI_I2C_ERROR;
> +		goto out;
> +	}
> +
> +	val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> +	sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> +			val & ~SUNXI_I2C_CNTR_INT_FLAG);
> +
> +	return IRQ_HANDLED;
> +
> +out:
> +	val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> +	sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> +			val | SUNXI_I2C_CNTR_MASTER_STOP);
> +
> +	val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> +	sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> +			val & ~SUNXI_I2C_CNTR_INT_FLAG);
> +
> +	complete(&i2c_dev->completion);
> +	return IRQ_HANDLED;
> +}
> +
> +static int sunxi_i2c_xfer_msg(struct sunxi_i2c_dev *i2c_dev,
> +			      struct i2c_msg *msg)
> +{
> +	int time_left;
> +	u32 val;
> +
> +	i2c_dev->msg_cur = msg;
> +	i2c_dev->msg_buf = msg->buf;
> +	i2c_dev->msg_buf_remaining = msg->len;
> +	i2c_dev->msg_err = 0;
> +	INIT_COMPLETION(i2c_dev->completion);
> +
> +	val = sunxi_i2c_read(i2c_dev, SUNXI_I2C_CNTR_REG);
> +	val |= SUNXI_I2C_CNTR_MASTER_START;
> +	sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG, val);
> +
> +	time_left = wait_for_completion_timeout(&i2c_dev->completion,
> +						i2c_dev->adapter.timeout);
> +	if (!time_left) {
> +		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (likely(i2c_dev->msg_err == SUNXI_I2C_DONE))
> +		return 0;
> +
> +	dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev-
>msg_err);
> +
> +	return -EIO;
> +}
> +
> +static int sunxi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
> *msgs, +			  int num)
> +{
> +	struct sunxi_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
> +	int i, ret;
> +
> +	for (i = 0; i < num; i++) {
> +		ret = sunxi_i2c_xfer_msg(i2c_dev, &msgs[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return i;
> +}
> +
> +static u32 sunxi_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm sunxi_i2c_algo = {
> +	.master_xfer	= sunxi_i2c_xfer,
> +	.functionality	= sunxi_i2c_func,
> +};
> +
> +static int sunxi_i2c_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_i2c_dev *i2c_dev;
> +	struct device_node *np;
> +	u32 freq, div_m, div_n;
> +	struct resource res;

	struct resource *res;

and then...

> +	int ret;
> +
> +	np = pdev->dev.of_node;
> +	if (!np)
> +		return -EINVAL;
> +
> +	i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
> +	if (!i2c_dev)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, i2c_dev);
> +	i2c_dev->dev = &pdev->dev;
> +
> +	init_completion(&i2c_dev->completion);
> +
> +	ret = of_address_to_resource(np, 0, &res);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not get IO memory\n");
> +		return ret;
> +	}
> +
> +	i2c_dev->membase = devm_ioremap_resource(&pdev->dev, &res);

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	i2c_dev->membase = devm_ioremap_resource(&pdev->dev, res);

This way you can save yourself from parsing device tree again, one error 
check and few lines of code. Remember that of_platform_populate() creates 
all the resources of platform_devices based on reg and interrupts 
properties, so there is no point in wasting this effort in drivers by 
parsing them manually again.

> +	if (IS_ERR(i2c_dev->membase))
> +		return PTR_ERR(i2c_dev->membase);
> +
> +	i2c_dev->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(i2c_dev->clk))
> +		return PTR_ERR(i2c_dev->clk);
> +
> +	clk_prepare_enable(i2c_dev->clk);
> +
> +	sunxi_i2c_write(i2c_dev, SUNXI_I2C_SRST_REG, 
SUNXI_I2C_SRST_RESET);
> +
> +	ret = of_property_read_u32(np, "clock-frequency", &freq);
> +	if (ret < 0) {
> +		dev_warn(&pdev->dev, "Could not read clock-frequency 
property\n");
> +		freq = 100000;
> +	}
> +
> +	/*
> +	 * Set the clock dividers. we don't need to be super smart
> +	 * here, the datasheet defines the value of the factors for
> +	 * the two supported frequencies, and only the M factor
> +	 * changes between 100kHz and 400kHz.
> +	 *
> +	 * The bus clock is generated from the parent clock with two
> +	 * different dividers. It is generated as such:
> +	 *     f0 = fclk / (2 ^ DIV_N)
> +	 *     fbus = f0 / (10 * (DIV_M + 1))
> +	 *
> +	 * With DIV_N being on 3 bits, and DIV_M on 4 bits.
> +	 * So DIV_N < 8, and DIV_M < 16.
> +	 *
> +	 * However, we can generate both the supported frequencies
> +	 * with f0 = 12MHz, and only change M to get back on our
> +	 * feet.
> +	 */
> +	div_n = ilog2(clk_get_rate(i2c_dev->clk) / 12000000);
> +	if (freq == 100000)
> +		div_m = 11;
> +	else if (freq == 400000)
> +		div_m = 2;
> +	else {
> +		dev_err(&pdev->dev, "Unsupported bus frequency\n");
> +		ret = -EINVAL;
> +		goto out_clk_dis;
> +	}
> +
> +	sunxi_i2c_write(i2c_dev, SUNXI_I2C_CCR_REG,
> +			SUNXI_I2C_CCR_DIV_N(div_n) | 
SUNXI_I2C_CCR_DIV_M(div_m));
> +
> +	i2c_dev->irq = irq_of_parse_and_map(np, 0);

You don't have to go through all the parsing again here, because the 
interrupt property is already being parsed by of_platform_populate() 
creating all necessary MEM and IRQ resources.

As I suggested in review of v1, you can simply use

	i2c_dev->irq = platform_get_irq(pdev, 0);

which will just fetch the interrupt number from the resource set created 
by of_platform_populate(). Just remember to change irq field of i2c_dev to 
signed int and the following check to <= 0.

Best regards,
Tomasz

> +	if (!i2c_dev->irq) {
> +		dev_err(&pdev->dev, "No IRQ resource\n");
> +		ret = -ENODEV;
> +		goto out_clk_dis;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, i2c_dev->irq, 
sunxi_i2c_handler,
> +			       IRQF_SHARED, dev_name(&pdev->dev), 
i2c_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Could not request IRQ\n");
> +		goto out_clk_dis;
> +	}
> +
> +	i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
> +
> +	i2c_dev->adapter.owner = THIS_MODULE;
> +	strlcpy(i2c_dev->adapter.name, "sunxi I2C adapter",
> +		sizeof(i2c_dev->adapter.name));
> +	i2c_dev->adapter.algo = &sunxi_i2c_algo;
> +	i2c_dev->adapter.dev.parent = &pdev->dev;
> +
> +	sunxi_i2c_write(i2c_dev, SUNXI_I2C_CNTR_REG,
> +			SUNXI_I2C_CNTR_BUS_ENABLE | 
SUNXI_I2C_CNTR_INT_ENABLE);
> +
> +	ret = i2c_add_adapter(&i2c_dev->adapter);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register i2c adapter\n");
> +		goto out_clk_dis;
> +	}
> +
> +	return 0;
> +
> +out_clk_dis:
> +	clk_disable_unprepare(i2c_dev->clk);
> +	return ret;
> +}
> +
> +
> +static int sunxi_i2c_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&i2c_dev->adapter);
> +	clk_disable_unprepare(i2c_dev->clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sunxi_i2c_of_match[] = {
> +	{ .compatible = "allwinner,sun4i-i2c" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sunxi_i2c_of_match);
> +
> +static struct platform_driver sunxi_i2c_driver = {
> +	.probe		= sunxi_i2c_probe,
> +	.remove		= sunxi_i2c_remove,
> +	.driver		= {
> +		.name	= "i2c-sunxi",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = sunxi_i2c_of_match,
> +	},
> +};
> +module_platform_driver(sunxi_i2c_driver);
> +
> +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com");
> +MODULE_DESCRIPTION("Allwinner A1X I2C bus adapter");
> +MODULE_LICENSE("GPL");
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Oliver,

On Sunday 26 of May 2013 12:53:30 Oliver Schinagl wrote:
> Just replying because I want to understand certain choices you make,
> absolutely not questioning your code!
> 
> On 05/26/13 12:20, Maxime Ripard wrote:
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> 
> You forgot to add #include <linux/bitops.h> for BIT()
> 
> > +
> > +static void sunxi_i2c_write(struct sunxi_i2c_dev *i2c_dev, u16 reg,
> > u8 value) +{
> > +	writel(value, i2c_dev->membase + reg);
> 
> Why writel? and why without (u32)value? I thought iowrite* where the
> preferred calls and in this case, wouldn't we want writeb since value is
> u8?

Regs in ARM world are usually 32-bit wide and memory mapped, so you use 
writel for them. I believe it is the case here as well.

> > +}
> > +
> > +static u32 sunxi_i2c_read(struct sunxi_i2c_dev *i2c_dev, u16 reg)
> > +{
> > +	return readl(i2c_dev->membase + reg);
> 
> And here, readl does match the return of u32, but aren't we always
> reading 8 bits since the TWI Data Register only uses the first 8 bits?
> So wouldn't we want to return u8 and readb?
> 

Ditto. Even if only least significant 8 bits are used, the register is 32-
bit wide, so 32-bit accessor should be used. (Assuming that my previous 
comment holds true in case of this IP.)

> > +static int sunxi_i2c_probe(struct platform_device *pdev)
> > +{
> > +	struct sunxi_i2c_dev *i2c_dev;
> > +	struct device_node *np;
> > +	u32 freq, div_m, div_n;
> > +	struct resource res;
> 
> I feel stupid for questioning this, since it only shows my lack of
> knowledge, but
> If you declare all the memory here, isn't all the data lost after
> exiting the _probe function? we pass a pointer to this memory in the
> of_address_to_resource() function so that fills it, right?
> 
> Or does after devm_ioremap_resource it no longer matter, since that
> function got what it needed and useless after?
> 
> Just asking because of the wdt driver and possibly mine that you told us
> to change.

In this case struct resource is just used as a container to pass base 
address and size of area to be mapped to devm_ioremap_resource() in. It 
isn't used anymore after the function returns.

> Sorry for asking (again) maybe very obvious things. Just trying to
> learn.

No problem.

I can recommend you a great tool to browse through Linux sources:
http://lxr.free-electrons.com/source/lib/devres.c?a=arm#L107

It really helps looking up in code things that you aren't sure.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sun, May 26, 2013 at 04:38:19PM +0200, Tomasz Figa wrote:
> On Sunday 26 of May 2013 12:20:37 Maxime Ripard wrote:
> > +	/*
> > +	 * Address + Read bit have been transmitted, ACK has not been
> > +	 * received. If we don't care about the NAKs, we just treat
> > +	 * this case as if an ACK would have been received, and fall
> > +	 * through to the next case.
> > +	 */
> > +	case SUNXI_I2C_STA_MASTER_RADDR_NAK:
> > +		if (!(i2c_dev->msg_cur->flags & I2C_M_IGNORE_NAK)) {
> > +			i2c_dev->msg_err = SUNXI_I2C_NAK;
> > +			goto out;
> > +		}
> 
> I meant something more like:
> 
> +
> +	/* Intentional fall through. */
> 
> This is enough to let the reader know that there is no break missing here 
> and you can keep those nice short state descriptions from v1.

Ok, I'll do it that way

[..]

> > +static int sunxi_i2c_probe(struct platform_device *pdev)
> > +{
> > +	struct sunxi_i2c_dev *i2c_dev;
> > +	struct device_node *np;
> > +	u32 freq, div_m, div_n;
> > +	struct resource res;
> 
> 	struct resource *res;
> 
> and then...
> 
> > +	int ret;
> > +
> > +	np = pdev->dev.of_node;
> > +	if (!np)
> > +		return -EINVAL;
> > +
> > +	i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
> > +	if (!i2c_dev)
> > +		return -ENOMEM;
> > +	platform_set_drvdata(pdev, i2c_dev);
> > +	i2c_dev->dev = &pdev->dev;
> > +
> > +	init_completion(&i2c_dev->completion);
> > +
> > +	ret = of_address_to_resource(np, 0, &res);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "could not get IO memory\n");
> > +		return ret;
> > +	}
> > +
> > +	i2c_dev->membase = devm_ioremap_resource(&pdev->dev, &res);
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 	i2c_dev->membase = devm_ioremap_resource(&pdev->dev, res);
> 
> This way you can save yourself from parsing device tree again, one error 
> check and few lines of code. Remember that of_platform_populate() creates 
> all the resources of platform_devices based on reg and interrupts 
> properties, so there is no point in wasting this effort in drivers by 
> parsing them manually again.

Ah, right, I didn't thought about the double dt parsing. I'll switch to
platform_get_resource and platform_get_irq as you suggested.

Thanks!
Maxime

On 05/26/13 15:21, Maxime Ripard wrote:

>>> +static void sunxi_i2c_write(struct sunxi_i2c_dev *i2c_dev, u16 reg, u8 value)
>>> +{
>>> +	writel(value, i2c_dev->membase + reg);
>> Why writel? and why without (u32)value? I thought iowrite* where the
>> preferred calls and in this case, wouldn't we want writeb since
>> value is u8?
>
> You're right, value should be a u32 here, thanks for noticing.
>
> For the iowrite* vs write*, there's no consensus, and as such no
> preferred way. write* functions are doing an MMIO only access,
> while iowrite functions can do MMIO and port I/O accesses.
>
> Note that it doesn't change anything on ARM, since there's no port IO on
> ARM.
Ah I see, missinformation on my end. Sorry.

But why write 32 bits? The register is only 8 wide, with the rest being 
'reserved'. Then again, the register IS 32 bits wide and probably will 
haev 32 bits written to it? Correct?

>
>>> +}
>>> +
>>> +static u32 sunxi_i2c_read(struct sunxi_i2c_dev *i2c_dev, u16 reg)
>>> +{
>>> +	return readl(i2c_dev->membase + reg);
>> And here, readl does match the return of u32, but aren't we always
>> reading 8 bits since the TWI Data Register only uses the first 8
>> bits?
>> So wouldn't we want to return u8 and readb?
>
> They are meant to be general purpose accessors, so we shouldn't focus
> only on the data register.
Ah yes, of course.

>
>>
>>> +static int sunxi_i2c_probe(struct platform_device *pdev)
>>> +{
>>> +	struct sunxi_i2c_dev *i2c_dev;
>>> +	struct device_node *np;
>>> +	u32 freq, div_m, div_n;
>>> +	struct resource res;
>> I feel stupid for questioning this, since it only shows my lack of
>> knowledge, but
>> If you declare all the memory here, isn't all the data lost after
>> exiting the _probe function? we pass a pointer to this memory in the
>> of_address_to_resource() function so that fills it, right?
>>
>> Or does after devm_ioremap_resource it no longer matter, since that
>> function got what it needed and useless after?
>
> The struct resource is only there to declare the base address and the
> size of memory address. Once we have mapped it, we don't care about it
> anymore.
Thanks for clarifying that, I will adapt my driver to do the same.
>
> Maxime
>

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 05/26/13 16:44, Tomasz Figa wrote:
> Hi Oliver,
>
> I can recommend you a great tool to browse through Linux sources:
> http://lxr.free-electrons.com/source/lib/devres.c?a=arm#L107
>
> It really helps looking up in code things that you aren't sure.
>
> Best regards,
> Tomasz
>
Yeah, I'm allready a heavy user of LXR ;) But sometimes there's just 
information overload and things become hard to follow. Especially when 
there's several ways to do something.

I would have hoped that a 'ldd4' would be available which has OF support 
written in. ldd3 lacks all that.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Oliver,

On Sun, May 26, 2013 at 09:01:53PM +0200, Oliver Schinagl wrote:
> On 05/26/13 15:21, Maxime Ripard wrote:
> >For the iowrite* vs write*, there's no consensus, and as such no
> >preferred way. write* functions are doing an MMIO only access,
> >while iowrite functions can do MMIO and port I/O accesses.
> >
> >Note that it doesn't change anything on ARM, since there's no port IO on
> >ARM.
> Ah I see, missinformation on my end. Sorry.
> 
> But why write 32 bits? The register is only 8 wide, with the rest
> being 'reserved'. Then again, the register IS 32 bits wide and
> probably will haev 32 bits written to it? Correct?

Yes, the register is 32 bits wide. However, and I didn't noticed it at
first, that all the registers have the 3 upper bytes reserved.

But you'll end up writing a 32 bits value to a 32 bit register register
anyway, even if the relevant information is contained in the 8 first
bits.

Maxime