[RESEND2 PATCH v4 0/2] i2c: sunxi: add P2WI controller support

[RESEND2 PATCH v4 0/2] i2c: sunxi: add P2WI controller support

[Boris BREZILLON]

Hello,

Sorry for the noise, I forgot to add the linux-i2c ML in Cc.

This series adds support for the P2WI block used by some Allwinner boards
to interface with the AXP221 PMIC.

Best Regards,

Boris

Changes since v3:
- update the DT bindings doc
- fix a comment that was no longer true

Changes since v2:
- drop the initialization (switch from I2C to P2WI mode) part
- print devm_ioremap_resource err code

Changes since v1:
- implement P2WI initilization
- implement P2WI bus frequency configuration
- implement address consistency check
- fix devm_request_and_ioremap return check
- rework the macro definitions
- fix typos

[1] http://www.spinics.net/lists/linux-i2c/msg15066.html
[2] http://comments.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/8947
Boris BREZILLON (2):
  i2c: sunxi: add P2WI DT bindings documentation
  i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

 .../devicetree/bindings/i2c/i2c-sunxi-p2wi.txt     |  41 +++
 drivers/i2c/busses/Kconfig                         |  12 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-sun6i-p2wi.c                | 349 +++++++++++++++++++++
 4 files changed, 403 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt
 create mode 100644 drivers/i2c/busses/i2c-sun6i-p2wi.c

-- 
1.8.3.2

[RESEND2 PATCH v4 0/2] i2c: sunxi: add P2WI controller support

[Boris BREZILLON]

Hello,

Sorry for the noise, I forgot to add the linux-i2c ML in Cc.

This series adds support for the P2WI block used by some Allwinner boards
to interface with the AXP221 PMIC.

Best Regards,

Boris

Changes since v3:
- update the DT bindings doc
- fix a comment that was no longer true

Changes since v2:
- drop the initialization (switch from I2C to P2WI mode) part
- print devm_ioremap_resource err code

Changes since v1:
- implement P2WI initilization
- implement P2WI bus frequency configuration
- implement address consistency check
- fix devm_request_and_ioremap return check
- rework the macro definitions
- fix typos

[1] http://www.spinics.net/lists/linux-i2c/msg15066.html
[2] http://comments.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/8947
Boris BREZILLON (2):
  i2c: sunxi: add P2WI DT bindings documentation
  i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

 .../devicetree/bindings/i2c/i2c-sunxi-p2wi.txt     |  41 +++
 drivers/i2c/busses/Kconfig                         |  12 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-sun6i-p2wi.c                | 349 +++++++++++++++++++++
 4 files changed, 403 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt
 create mode 100644 drivers/i2c/busses/i2c-sun6i-p2wi.c

-- 
1.8.3.2

[RESEND2 PATCH v4 1/2] i2c: sunxi: add P2WI DT bindings documentation

[Boris BREZILLON]

P2WI (Push/Pull 2 Wire Interface) is an SMBus like bus used to communicate
with some PMICs (like the AXP221).

Document P2WI DT bindings which are pretty much the same as the one defined
for the marvell's mv64xxx controller.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 .../devicetree/bindings/i2c/i2c-sunxi-p2wi.txt     | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt b/Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt
new file mode 100644
index 0000000..6b76548
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt
@@ -0,0 +1,41 @@
+
+* Allwinner P2WI (Push/Pull 2 Wire Interface) controller
+
+Required properties :
+
+ - reg             : Offset and length of the register set for the device.
+ - compatible      : Should one of the following:
+                     - "allwinner,sun6i-a31-p2wi"
+ - interrupts      : The interrupt line connected to the P2WI peripheral.
+ - clocks          : The gate clk connected to the P2WI peripheral.
+ - resets          : The reset line connected to the P2WI peripheral.
+
+Optional properties :
+
+ - clock-frequency : Desired P2WI bus clock frequency in Hz. If not set the
+default frequency is 100kHz
+
+A P2WI may contain one child node encoding a P2WI slave device.
+
+Slave device properties:
+  Required properties:
+   - reg           : the I2C slave address used during the initialization
+                     process to switch from I2C to P2WI mode
+
+Example:
+
+	p2wi@01f03400 {
+		compatible = "allwinner,sun6i-a31-p2wi";
+		reg = <0x01f03400 0x400>;
+		interrupts = <0 39 4>;
+		clocks = <&apb0_gates 3>;
+		clock-frequency = <6000000>;
+		resets = <&apb0_rst 3>;
+
+		axp221: pmic@68 {
+			compatible = "x-powers,axp221";
+			reg = <0x68>;
+
+			/* ... */
+		};
+	};
-- 
1.8.3.2

[RESEND2 PATCH v4 1/2] i2c: sunxi: add P2WI DT bindings documentation

[Boris BREZILLON]

P2WI (Push/Pull 2 Wire Interface) is an SMBus like bus used to communicate
with some PMICs (like the AXP221).

Document P2WI DT bindings which are pretty much the same as the one defined
for the marvell's mv64xxx controller.

Signed-off-by: Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../devicetree/bindings/i2c/i2c-sunxi-p2wi.txt     | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt b/Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt
new file mode 100644
index 0000000..6b76548
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt
@@ -0,0 +1,41 @@
+
+* Allwinner P2WI (Push/Pull 2 Wire Interface) controller
+
+Required properties :
+
+ - reg             : Offset and length of the register set for the device.
+ - compatible      : Should one of the following:
+                     - "allwinner,sun6i-a31-p2wi"
+ - interrupts      : The interrupt line connected to the P2WI peripheral.
+ - clocks          : The gate clk connected to the P2WI peripheral.
+ - resets          : The reset line connected to the P2WI peripheral.
+
+Optional properties :
+
+ - clock-frequency : Desired P2WI bus clock frequency in Hz. If not set the
+default frequency is 100kHz
+
+A P2WI may contain one child node encoding a P2WI slave device.
+
+Slave device properties:
+  Required properties:
+   - reg           : the I2C slave address used during the initialization
+                     process to switch from I2C to P2WI mode
+
+Example:
+
+	p2wi@01f03400 {
+		compatible = "allwinner,sun6i-a31-p2wi";
+		reg = <0x01f03400 0x400>;
+		interrupts = <0 39 4>;
+		clocks = <&apb0_gates 3>;
+		clock-frequency = <6000000>;
+		resets = <&apb0_rst 3>;
+
+		axp221: pmic@68 {
+			compatible = "x-powers,axp221";
+			reg = <0x68>;
+
+			/* ... */
+		};
+	};
-- 
1.8.3.2

[RESEND2 PATCH v4 1/2] i2c: sunxi: add P2WI DT bindings documentation

[Boris BREZILLON]

P2WI (Push/Pull 2 Wire Interface) is an SMBus like bus used to communicate
with some PMICs (like the AXP221).

Document P2WI DT bindings which are pretty much the same as the one defined
for the marvell's mv64xxx controller.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 .../devicetree/bindings/i2c/i2c-sunxi-p2wi.txt     | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt b/Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt
new file mode 100644
index 0000000..6b76548
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-sunxi-p2wi.txt
@@ -0,0 +1,41 @@
+
+* Allwinner P2WI (Push/Pull 2 Wire Interface) controller
+
+Required properties :
+
+ - reg             : Offset and length of the register set for the device.
+ - compatible      : Should one of the following:
+                     - "allwinner,sun6i-a31-p2wi"
+ - interrupts      : The interrupt line connected to the P2WI peripheral.
+ - clocks          : The gate clk connected to the P2WI peripheral.
+ - resets          : The reset line connected to the P2WI peripheral.
+
+Optional properties :
+
+ - clock-frequency : Desired P2WI bus clock frequency in Hz. If not set the
+default frequency is 100kHz
+
+A P2WI may contain one child node encoding a P2WI slave device.
+
+Slave device properties:
+  Required properties:
+   - reg           : the I2C slave address used during the initialization
+                     process to switch from I2C to P2WI mode
+
+Example:
+
+	p2wi at 01f03400 {
+		compatible = "allwinner,sun6i-a31-p2wi";
+		reg = <0x01f03400 0x400>;
+		interrupts = <0 39 4>;
+		clocks = <&apb0_gates 3>;
+		clock-frequency = <6000000>;
+		resets = <&apb0_rst 3>;
+
+		axp221: pmic at 68 {
+			compatible = "x-powers,axp221";
+			reg = <0x68>;
+
+			/* ... */
+		};
+	};
-- 
1.8.3.2

[RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Boris BREZILLON]

The P2WI looks like an SMBus controller which only supports byte data
transfers. But, it differs from standard SMBus protocol on several
aspects:
- it supports only one slave device, and thus drop the address field
- it adds a parity bit every 8bits of data
- only one read access is required to read a byte (instead of a read
  followed by a write access in standard SMBus protocol)
- there's no Ack bit after each byte transfer

This means this bus cannot be used to interface with standard SMBus
devices (the only known device to support this interface is the AXP221
PMIC).

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/i2c/busses/Kconfig          |  12 ++
 drivers/i2c/busses/Makefile         |   1 +
 drivers/i2c/busses/i2c-sun6i-p2wi.c | 349 ++++++++++++++++++++++++++++++++++++
 3 files changed, 362 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-sun6i-p2wi.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index c94db1c..09bce1c 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -771,6 +771,18 @@ config I2C_STU300
 	  This driver can also be built as a module. If so, the module
 	  will be called i2c-stu300.
 
+config I2C_SUN6I_P2WI
+	tristate "Allwinner sun6i internal P2WI controller"
+	depends on ARCH_SUNXI
+	help
+	  If you say yes to this option, support will be included for the
+	  P2WI (Push/Pull 2 Wire Interface) controller embedded in some sunxi
+	  SOCs.
+	  The P2WI looks like an SMBus controller (which supports only byte
+	  accesses), except that it only supports one slave device.
+	  This interface is used to connect to specific PMIC devices (like the
+	  AXP221).
+
 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 18d18ff..58feeb9 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
 obj-$(CONFIG_I2C_SIRF)		+= i2c-sirf.o
 obj-$(CONFIG_I2C_ST)		+= i2c-st.o
 obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
+obj-$(CONFIG_I2C_SUN6I_P2WI)	+= i2c-sun6i-p2wi.o
 obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
 obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
 obj-$(CONFIG_I2C_WMT)		+= i2c-wmt.o
diff --git a/drivers/i2c/busses/i2c-sun6i-p2wi.c b/drivers/i2c/busses/i2c-sun6i-p2wi.c
new file mode 100644
index 0000000..10f78d2
--- /dev/null
+++ b/drivers/i2c/busses/i2c-sun6i-p2wi.c
@@ -0,0 +1,349 @@
+/*
+ * P2WI (Push-Pull Two Wire Interface) bus driver.
+ *
+ * Author: Boris BREZILLON <boris.brezillon@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/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/reset.h>
+
+
+/* P2WI registers */
+#define P2WI_CTRL		0x0
+#define P2WI_CCR		0x4
+#define P2WI_INTE		0x8
+#define P2WI_INTS		0xc
+#define P2WI_DADDR0		0x10
+#define P2WI_DADDR1		0x14
+#define P2WI_DLEN		0x18
+#define P2WI_DATA0		0x1c
+#define P2WI_DATA1		0x20
+#define P2WI_LCR		0x24
+#define P2WI_PMCR		0x28
+
+/* CTRL fields */
+#define P2WI_CTRL_START_TRANS		BIT(7)
+#define P2WI_CTRL_ABORT_TRANS		BIT(6)
+#define P2WI_CTRL_GLOBAL_INT_ENB	BIT(1)
+#define P2WI_CTRL_SOFT_RST		BIT(0)
+
+/* CLK CTRL fields */
+#define P2WI_CCR_SDA_OUT_DELAY(v)	(((v) & 0x7) << 8)
+#define P2WI_CCR_MAX_CLK_DIV		0xff
+#define P2WI_CCR_CLK_DIV(v)		((v) & P2WI_CCR_MAX_CLK_DIV)
+
+/* STATUS fields */
+#define P2WI_INTS_TRANS_ERR_ID(v)	(((v) >> 8) & 0xff)
+#define P2WI_INTS_LOAD_BSY		BIT(2)
+#define P2WI_INTS_TRANS_ERR		BIT(1)
+#define P2WI_INTS_TRANS_OVER		BIT(0)
+
+/* DATA LENGTH fields*/
+#define P2WI_DLEN_READ			BIT(4)
+#define P2WI_DLEN_DATA_LENGTH(v)	((v - 1) & 0x7)
+
+/* LINE CTRL fields*/
+#define P2WI_LCR_SCL_STATE		BIT(5)
+#define P2WI_LCR_SDA_STATE		BIT(4)
+#define P2WI_LCR_SCL_CTL		BIT(3)
+#define P2WI_LCR_SCL_CTL_EN		BIT(2)
+#define P2WI_LCR_SDA_CTL		BIT(1)
+#define P2WI_LCR_SDA_CTL_EN		BIT(0)
+
+/* PMU MODE CTRL fields */
+#define P2WI_PMCR_PMU_INIT_SEND		BIT(31)
+#define P2WI_PMCR_PMU_INIT_DATA(v)	(((v) & 0xff) << 16)
+#define P2WI_PMCR_PMU_MODE_REG(v)	(((v) & 0xff) << 8)
+#define P2WI_PMCR_PMU_DEV_ADDR(v)	((v) & 0xff)
+
+#define P2WI_MAX_FREQ			6000000
+
+struct p2wi {
+	struct i2c_adapter adapter;
+	struct completion complete;
+	unsigned int irq;
+	unsigned int status;
+	void __iomem *regs;
+	struct clk *clk;
+	struct reset_control *rstc;
+	int slave_addr;
+};
+
+static irqreturn_t p2wi_interrupt(int irq, void *dev_id)
+{
+	struct p2wi *p2wi = dev_id;
+	unsigned long status;
+
+	status = readl(p2wi->regs + P2WI_INTS);
+	p2wi->status = status;
+
+	/* Clear interrupts */
+	status &= (P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR |
+		   P2WI_INTS_TRANS_OVER);
+	writel(status, p2wi->regs + P2WI_INTS);
+
+	complete(&p2wi->complete);
+
+	return IRQ_HANDLED;
+}
+
+static u32 p2wi_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_SMBUS_BYTE_DATA;
+}
+
+static int p2wi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+			   unsigned short flags, char read_write,
+			   u8 command, int size, union i2c_smbus_data *data)
+{
+	struct p2wi *p2wi = i2c_get_adapdata(adap);
+	unsigned long dlen = P2WI_DLEN_DATA_LENGTH(1);
+
+	if (addr > 0xff ||
+	    (p2wi->slave_addr >= 0 && addr != p2wi->slave_addr)) {
+		dev_err(&adap->dev, "invalid P2WI address\n");
+		return -EINVAL;
+	}
+
+	if (!data)
+		return -EINVAL;
+
+	writel(command, p2wi->regs + P2WI_DADDR0);
+
+	if (read_write == I2C_SMBUS_READ)
+		dlen |= P2WI_DLEN_READ;
+	else
+		writel(data->byte, p2wi->regs + P2WI_DATA0);
+
+	writel(dlen, p2wi->regs + P2WI_DLEN);
+
+	if (readl(p2wi->regs + P2WI_CTRL) & P2WI_CTRL_START_TRANS) {
+		dev_err(&adap->dev, "P2WI bus busy\n");
+		return -EBUSY;
+	}
+
+	reinit_completion(&p2wi->complete);
+
+	writel(P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR | P2WI_INTS_TRANS_OVER,
+	       p2wi->regs + P2WI_INTE);
+
+	writel(P2WI_CTRL_START_TRANS | P2WI_CTRL_GLOBAL_INT_ENB,
+	       p2wi->regs + P2WI_CTRL);
+
+	wait_for_completion(&p2wi->complete);
+
+	if (p2wi->status & P2WI_INTS_LOAD_BSY) {
+		dev_err(&adap->dev, "P2WI bus busy\n");
+		return -EBUSY;
+	}
+
+	if (p2wi->status & P2WI_INTS_TRANS_ERR) {
+		dev_err(&adap->dev, "P2WI bus xfer error\n");
+		return -ENXIO;
+	}
+
+	if (read_write == I2C_SMBUS_READ)
+		data->byte = readl(p2wi->regs + P2WI_DATA0);
+
+	return 0;
+}
+
+static const struct i2c_algorithm p2wi_algo = {
+	.smbus_xfer = p2wi_smbus_xfer,
+	.functionality = p2wi_functionality,
+};
+
+static const struct of_device_id p2wi_of_match_table[] = {
+	{ .compatible = "allwinner,sun6i-a31-p2wi" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, p2wi_of_match_table);
+
+static int p2wi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *childnp;
+	unsigned long parent_clk_freq;
+	u32 clk_freq = 100000;
+	struct resource *r;
+	struct p2wi *p2wi;
+	u32 slave_addr;
+	int clk_div;
+	int ret;
+
+	of_property_read_u32(np, "clock-frequency", &clk_freq);
+	if (clk_freq > P2WI_MAX_FREQ) {
+		dev_err(dev,
+			"required clock-frequency (%u Hz) is too high (max = 6MHz)",
+			clk_freq);
+		return -EINVAL;
+	}
+
+	if (of_get_child_count(np) > 1) {
+		dev_err(dev, "P2WI only supports one slave device\n");
+		return -EINVAL;
+	}
+
+	p2wi = devm_kzalloc(dev, sizeof(struct p2wi), GFP_KERNEL);
+	if (!p2wi) {
+		dev_err(dev, "failed to allocate p2wi struct\n");
+		return -ENOMEM;
+	}
+
+	p2wi->slave_addr = -1;
+
+	/*
+	 * Authorize a p2wi node without any children to be able to use an
+	 * i2c-dev from userpace.
+	 * In this case the slave_addr is set to -1 and won't be checked when
+	 * launching a P2WI transfer.
+	 */
+	childnp = of_get_next_available_child(np, NULL);
+	if (childnp) {
+		ret = of_property_read_u32(childnp, "reg", &slave_addr);
+		if (ret || slave_addr > 0xff) {
+			dev_err(dev, "invalid slave address on node %s\n",
+				childnp->full_name);
+			return -EINVAL;
+		}
+
+		p2wi->slave_addr = slave_addr;
+	}
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	p2wi->regs = devm_ioremap_resource(dev, r);
+	if (IS_ERR(p2wi->regs)) {
+		ret = PTR_ERR(p2wi->regs);
+		dev_err(dev, "failed to retrieve iomem resource: %d\n", ret);
+		return ret;
+	}
+
+	snprintf(p2wi->adapter.name, sizeof(p2wi->adapter.name), pdev->name);
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		dev_err(dev, "failed to retrieve irq: %d\n", ret);
+		return ret;
+	}
+	p2wi->irq = ret;
+
+	p2wi->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(p2wi->clk)) {
+		ret = PTR_ERR(p2wi->clk);
+		dev_err(dev, "failed to retrieve clk: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(p2wi->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clk: %d\n", ret);
+		return ret;
+	}
+
+	parent_clk_freq = clk_get_rate(p2wi->clk);
+
+	p2wi->rstc = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(p2wi->rstc)) {
+		ret = PTR_ERR(p2wi->rstc);
+		dev_err(dev, "failed to retrieve reset controller: %d\n",
+			ret);
+		goto err_clk_disable;
+	}
+
+	ret = reset_control_deassert(p2wi->rstc);
+	if (ret) {
+		dev_err(dev, "failed to deassert reset line: %d\n",
+			ret);
+		goto err_clk_disable;
+	}
+
+	init_completion(&p2wi->complete);
+	p2wi->adapter.dev.parent = dev;
+	p2wi->adapter.algo = &p2wi_algo;
+	p2wi->adapter.owner = THIS_MODULE;
+	p2wi->adapter.dev.of_node = pdev->dev.of_node;
+	platform_set_drvdata(pdev, p2wi);
+	i2c_set_adapdata(&p2wi->adapter, p2wi);
+
+	ret = devm_request_irq(dev, p2wi->irq, p2wi_interrupt, 0, pdev->name,
+			       p2wi);
+	if (ret) {
+		dev_err(dev, "can't register interrupt handler irq%d: %d\n",
+			p2wi->irq, ret);
+		goto err_reset_assert;
+	}
+
+	writel(P2WI_CTRL_SOFT_RST, p2wi->regs + P2WI_CTRL);
+
+	clk_div = parent_clk_freq / clk_freq;
+	if (!clk_div) {
+		dev_warn(dev,
+			 "clock-frequency is too high, setting it to %lu Hz\n",
+			 parent_clk_freq);
+		clk_div = 1;
+	} else if (clk_div > P2WI_CCR_MAX_CLK_DIV) {
+		dev_warn(dev,
+			 "clock-frequency is too low, setting it to %lu Hz\n",
+			 parent_clk_freq / P2WI_CCR_MAX_CLK_DIV);
+		clk_div = P2WI_CCR_MAX_CLK_DIV;
+	}
+
+	writel(P2WI_CCR_SDA_OUT_DELAY(1) | P2WI_CCR_CLK_DIV(clk_div),
+	       p2wi->regs + P2WI_CCR);
+
+	ret = i2c_add_adapter(&p2wi->adapter);
+	if (!ret)
+		return 0;
+
+err_reset_assert:
+	reset_control_assert(p2wi->rstc);
+
+err_clk_disable:
+	clk_disable_unprepare(p2wi->clk);
+
+	return ret;
+}
+
+static int p2wi_remove(struct platform_device *dev)
+{
+	struct p2wi *p2wi = platform_get_drvdata(dev);
+
+	reset_control_assert(p2wi->rstc);
+	clk_disable_unprepare(p2wi->clk);
+	i2c_del_adapter(&p2wi->adapter);
+
+	return 0;
+}
+
+static struct platform_driver p2wi_driver = {
+	.probe	= p2wi_probe,
+	.remove	= p2wi_remove,
+	.driver	= {
+		.owner = THIS_MODULE,
+		.name = "i2c-sunxi-p2wi",
+		.of_match_table = p2wi_of_match_table,
+	},
+};
+module_platform_driver(p2wi_driver);
+
+MODULE_AUTHOR("Boris BREZILLON <boris.brezillon@free-electrons.com>");
+MODULE_DESCRIPTION("Allwinner P2WI driver");
+MODULE_LICENSE("GPL");
-- 
1.8.3.2

[RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Boris BREZILLON]

The P2WI looks like an SMBus controller which only supports byte data
transfers. But, it differs from standard SMBus protocol on several
aspects:
- it supports only one slave device, and thus drop the address field
- it adds a parity bit every 8bits of data
- only one read access is required to read a byte (instead of a read
  followed by a write access in standard SMBus protocol)
- there's no Ack bit after each byte transfer

This means this bus cannot be used to interface with standard SMBus
devices (the only known device to support this interface is the AXP221
PMIC).

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/i2c/busses/Kconfig          |  12 ++
 drivers/i2c/busses/Makefile         |   1 +
 drivers/i2c/busses/i2c-sun6i-p2wi.c | 349 ++++++++++++++++++++++++++++++++++++
 3 files changed, 362 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-sun6i-p2wi.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index c94db1c..09bce1c 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -771,6 +771,18 @@ config I2C_STU300
 	  This driver can also be built as a module. If so, the module
 	  will be called i2c-stu300.
 
+config I2C_SUN6I_P2WI
+	tristate "Allwinner sun6i internal P2WI controller"
+	depends on ARCH_SUNXI
+	help
+	  If you say yes to this option, support will be included for the
+	  P2WI (Push/Pull 2 Wire Interface) controller embedded in some sunxi
+	  SOCs.
+	  The P2WI looks like an SMBus controller (which supports only byte
+	  accesses), except that it only supports one slave device.
+	  This interface is used to connect to specific PMIC devices (like the
+	  AXP221).
+
 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 18d18ff..58feeb9 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
 obj-$(CONFIG_I2C_SIRF)		+= i2c-sirf.o
 obj-$(CONFIG_I2C_ST)		+= i2c-st.o
 obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
+obj-$(CONFIG_I2C_SUN6I_P2WI)	+= i2c-sun6i-p2wi.o
 obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
 obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
 obj-$(CONFIG_I2C_WMT)		+= i2c-wmt.o
diff --git a/drivers/i2c/busses/i2c-sun6i-p2wi.c b/drivers/i2c/busses/i2c-sun6i-p2wi.c
new file mode 100644
index 0000000..10f78d2
--- /dev/null
+++ b/drivers/i2c/busses/i2c-sun6i-p2wi.c
@@ -0,0 +1,349 @@
+/*
+ * P2WI (Push-Pull Two Wire Interface) bus driver.
+ *
+ * Author: Boris BREZILLON <boris.brezillon@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/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/reset.h>
+
+
+/* P2WI registers */
+#define P2WI_CTRL		0x0
+#define P2WI_CCR		0x4
+#define P2WI_INTE		0x8
+#define P2WI_INTS		0xc
+#define P2WI_DADDR0		0x10
+#define P2WI_DADDR1		0x14
+#define P2WI_DLEN		0x18
+#define P2WI_DATA0		0x1c
+#define P2WI_DATA1		0x20
+#define P2WI_LCR		0x24
+#define P2WI_PMCR		0x28
+
+/* CTRL fields */
+#define P2WI_CTRL_START_TRANS		BIT(7)
+#define P2WI_CTRL_ABORT_TRANS		BIT(6)
+#define P2WI_CTRL_GLOBAL_INT_ENB	BIT(1)
+#define P2WI_CTRL_SOFT_RST		BIT(0)
+
+/* CLK CTRL fields */
+#define P2WI_CCR_SDA_OUT_DELAY(v)	(((v) & 0x7) << 8)
+#define P2WI_CCR_MAX_CLK_DIV		0xff
+#define P2WI_CCR_CLK_DIV(v)		((v) & P2WI_CCR_MAX_CLK_DIV)
+
+/* STATUS fields */
+#define P2WI_INTS_TRANS_ERR_ID(v)	(((v) >> 8) & 0xff)
+#define P2WI_INTS_LOAD_BSY		BIT(2)
+#define P2WI_INTS_TRANS_ERR		BIT(1)
+#define P2WI_INTS_TRANS_OVER		BIT(0)
+
+/* DATA LENGTH fields*/
+#define P2WI_DLEN_READ			BIT(4)
+#define P2WI_DLEN_DATA_LENGTH(v)	((v - 1) & 0x7)
+
+/* LINE CTRL fields*/
+#define P2WI_LCR_SCL_STATE		BIT(5)
+#define P2WI_LCR_SDA_STATE		BIT(4)
+#define P2WI_LCR_SCL_CTL		BIT(3)
+#define P2WI_LCR_SCL_CTL_EN		BIT(2)
+#define P2WI_LCR_SDA_CTL		BIT(1)
+#define P2WI_LCR_SDA_CTL_EN		BIT(0)
+
+/* PMU MODE CTRL fields */
+#define P2WI_PMCR_PMU_INIT_SEND		BIT(31)
+#define P2WI_PMCR_PMU_INIT_DATA(v)	(((v) & 0xff) << 16)
+#define P2WI_PMCR_PMU_MODE_REG(v)	(((v) & 0xff) << 8)
+#define P2WI_PMCR_PMU_DEV_ADDR(v)	((v) & 0xff)
+
+#define P2WI_MAX_FREQ			6000000
+
+struct p2wi {
+	struct i2c_adapter adapter;
+	struct completion complete;
+	unsigned int irq;
+	unsigned int status;
+	void __iomem *regs;
+	struct clk *clk;
+	struct reset_control *rstc;
+	int slave_addr;
+};
+
+static irqreturn_t p2wi_interrupt(int irq, void *dev_id)
+{
+	struct p2wi *p2wi = dev_id;
+	unsigned long status;
+
+	status = readl(p2wi->regs + P2WI_INTS);
+	p2wi->status = status;
+
+	/* Clear interrupts */
+	status &= (P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR |
+		   P2WI_INTS_TRANS_OVER);
+	writel(status, p2wi->regs + P2WI_INTS);
+
+	complete(&p2wi->complete);
+
+	return IRQ_HANDLED;
+}
+
+static u32 p2wi_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_SMBUS_BYTE_DATA;
+}
+
+static int p2wi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+			   unsigned short flags, char read_write,
+			   u8 command, int size, union i2c_smbus_data *data)
+{
+	struct p2wi *p2wi = i2c_get_adapdata(adap);
+	unsigned long dlen = P2WI_DLEN_DATA_LENGTH(1);
+
+	if (addr > 0xff ||
+	    (p2wi->slave_addr >= 0 && addr != p2wi->slave_addr)) {
+		dev_err(&adap->dev, "invalid P2WI address\n");
+		return -EINVAL;
+	}
+
+	if (!data)
+		return -EINVAL;
+
+	writel(command, p2wi->regs + P2WI_DADDR0);
+
+	if (read_write == I2C_SMBUS_READ)
+		dlen |= P2WI_DLEN_READ;
+	else
+		writel(data->byte, p2wi->regs + P2WI_DATA0);
+
+	writel(dlen, p2wi->regs + P2WI_DLEN);
+
+	if (readl(p2wi->regs + P2WI_CTRL) & P2WI_CTRL_START_TRANS) {
+		dev_err(&adap->dev, "P2WI bus busy\n");
+		return -EBUSY;
+	}
+
+	reinit_completion(&p2wi->complete);
+
+	writel(P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR | P2WI_INTS_TRANS_OVER,
+	       p2wi->regs + P2WI_INTE);
+
+	writel(P2WI_CTRL_START_TRANS | P2WI_CTRL_GLOBAL_INT_ENB,
+	       p2wi->regs + P2WI_CTRL);
+
+	wait_for_completion(&p2wi->complete);
+
+	if (p2wi->status & P2WI_INTS_LOAD_BSY) {
+		dev_err(&adap->dev, "P2WI bus busy\n");
+		return -EBUSY;
+	}
+
+	if (p2wi->status & P2WI_INTS_TRANS_ERR) {
+		dev_err(&adap->dev, "P2WI bus xfer error\n");
+		return -ENXIO;
+	}
+
+	if (read_write == I2C_SMBUS_READ)
+		data->byte = readl(p2wi->regs + P2WI_DATA0);
+
+	return 0;
+}
+
+static const struct i2c_algorithm p2wi_algo = {
+	.smbus_xfer = p2wi_smbus_xfer,
+	.functionality = p2wi_functionality,
+};
+
+static const struct of_device_id p2wi_of_match_table[] = {
+	{ .compatible = "allwinner,sun6i-a31-p2wi" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, p2wi_of_match_table);
+
+static int p2wi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *childnp;
+	unsigned long parent_clk_freq;
+	u32 clk_freq = 100000;
+	struct resource *r;
+	struct p2wi *p2wi;
+	u32 slave_addr;
+	int clk_div;
+	int ret;
+
+	of_property_read_u32(np, "clock-frequency", &clk_freq);
+	if (clk_freq > P2WI_MAX_FREQ) {
+		dev_err(dev,
+			"required clock-frequency (%u Hz) is too high (max = 6MHz)",
+			clk_freq);
+		return -EINVAL;
+	}
+
+	if (of_get_child_count(np) > 1) {
+		dev_err(dev, "P2WI only supports one slave device\n");
+		return -EINVAL;
+	}
+
+	p2wi = devm_kzalloc(dev, sizeof(struct p2wi), GFP_KERNEL);
+	if (!p2wi) {
+		dev_err(dev, "failed to allocate p2wi struct\n");
+		return -ENOMEM;
+	}
+
+	p2wi->slave_addr = -1;
+
+	/*
+	 * Authorize a p2wi node without any children to be able to use an
+	 * i2c-dev from userpace.
+	 * In this case the slave_addr is set to -1 and won't be checked when
+	 * launching a P2WI transfer.
+	 */
+	childnp = of_get_next_available_child(np, NULL);
+	if (childnp) {
+		ret = of_property_read_u32(childnp, "reg", &slave_addr);
+		if (ret || slave_addr > 0xff) {
+			dev_err(dev, "invalid slave address on node %s\n",
+				childnp->full_name);
+			return -EINVAL;
+		}
+
+		p2wi->slave_addr = slave_addr;
+	}
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	p2wi->regs = devm_ioremap_resource(dev, r);
+	if (IS_ERR(p2wi->regs)) {
+		ret = PTR_ERR(p2wi->regs);
+		dev_err(dev, "failed to retrieve iomem resource: %d\n", ret);
+		return ret;
+	}
+
+	snprintf(p2wi->adapter.name, sizeof(p2wi->adapter.name), pdev->name);
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		dev_err(dev, "failed to retrieve irq: %d\n", ret);
+		return ret;
+	}
+	p2wi->irq = ret;
+
+	p2wi->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(p2wi->clk)) {
+		ret = PTR_ERR(p2wi->clk);
+		dev_err(dev, "failed to retrieve clk: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(p2wi->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clk: %d\n", ret);
+		return ret;
+	}
+
+	parent_clk_freq = clk_get_rate(p2wi->clk);
+
+	p2wi->rstc = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(p2wi->rstc)) {
+		ret = PTR_ERR(p2wi->rstc);
+		dev_err(dev, "failed to retrieve reset controller: %d\n",
+			ret);
+		goto err_clk_disable;
+	}
+
+	ret = reset_control_deassert(p2wi->rstc);
+	if (ret) {
+		dev_err(dev, "failed to deassert reset line: %d\n",
+			ret);
+		goto err_clk_disable;
+	}
+
+	init_completion(&p2wi->complete);
+	p2wi->adapter.dev.parent = dev;
+	p2wi->adapter.algo = &p2wi_algo;
+	p2wi->adapter.owner = THIS_MODULE;
+	p2wi->adapter.dev.of_node = pdev->dev.of_node;
+	platform_set_drvdata(pdev, p2wi);
+	i2c_set_adapdata(&p2wi->adapter, p2wi);
+
+	ret = devm_request_irq(dev, p2wi->irq, p2wi_interrupt, 0, pdev->name,
+			       p2wi);
+	if (ret) {
+		dev_err(dev, "can't register interrupt handler irq%d: %d\n",
+			p2wi->irq, ret);
+		goto err_reset_assert;
+	}
+
+	writel(P2WI_CTRL_SOFT_RST, p2wi->regs + P2WI_CTRL);
+
+	clk_div = parent_clk_freq / clk_freq;
+	if (!clk_div) {
+		dev_warn(dev,
+			 "clock-frequency is too high, setting it to %lu Hz\n",
+			 parent_clk_freq);
+		clk_div = 1;
+	} else if (clk_div > P2WI_CCR_MAX_CLK_DIV) {
+		dev_warn(dev,
+			 "clock-frequency is too low, setting it to %lu Hz\n",
+			 parent_clk_freq / P2WI_CCR_MAX_CLK_DIV);
+		clk_div = P2WI_CCR_MAX_CLK_DIV;
+	}
+
+	writel(P2WI_CCR_SDA_OUT_DELAY(1) | P2WI_CCR_CLK_DIV(clk_div),
+	       p2wi->regs + P2WI_CCR);
+
+	ret = i2c_add_adapter(&p2wi->adapter);
+	if (!ret)
+		return 0;
+
+err_reset_assert:
+	reset_control_assert(p2wi->rstc);
+
+err_clk_disable:
+	clk_disable_unprepare(p2wi->clk);
+
+	return ret;
+}
+
+static int p2wi_remove(struct platform_device *dev)
+{
+	struct p2wi *p2wi = platform_get_drvdata(dev);
+
+	reset_control_assert(p2wi->rstc);
+	clk_disable_unprepare(p2wi->clk);
+	i2c_del_adapter(&p2wi->adapter);
+
+	return 0;
+}
+
+static struct platform_driver p2wi_driver = {
+	.probe	= p2wi_probe,
+	.remove	= p2wi_remove,
+	.driver	= {
+		.owner = THIS_MODULE,
+		.name = "i2c-sunxi-p2wi",
+		.of_match_table = p2wi_of_match_table,
+	},
+};
+module_platform_driver(p2wi_driver);
+
+MODULE_AUTHOR("Boris BREZILLON <boris.brezillon@free-electrons.com>");
+MODULE_DESCRIPTION("Allwinner P2WI driver");
+MODULE_LICENSE("GPL");
-- 
1.8.3.2

Re: [RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 7073 bytes --]

On Tue, Jun 03, 2014 at 10:49:52AM +0200, Boris BREZILLON wrote:
> The P2WI looks like an SMBus controller which only supports byte data
> transfers. But, it differs from standard SMBus protocol on several
> aspects:
> - it supports only one slave device, and thus drop the address field
> - it adds a parity bit every 8bits of data
> - only one read access is required to read a byte (instead of a read
>   followed by a write access in standard SMBus protocol)
> - there's no Ack bit after each byte transfer
> 
> This means this bus cannot be used to interface with standard SMBus
> devices (the only known device to support this interface is the AXP221
> PMIC).

Good description. Should be a comment at the top of the driver to spread
the word.

> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/i2c/busses/Kconfig          |  12 ++
>  drivers/i2c/busses/Makefile         |   1 +
>  drivers/i2c/busses/i2c-sun6i-p2wi.c | 349 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 362 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-sun6i-p2wi.c

...

> +struct p2wi {
> +	struct i2c_adapter adapter;
> +	struct completion complete;
> +	unsigned int irq;

Can be a local variable in probe.

> +	unsigned int status;
> +	void __iomem *regs;
> +	struct clk *clk;
> +	struct reset_control *rstc;
> +	int slave_addr;
> +};
> +
> +static irqreturn_t p2wi_interrupt(int irq, void *dev_id)
> +{
> +	struct p2wi *p2wi = dev_id;
> +	unsigned long status;
> +
> +	status = readl(p2wi->regs + P2WI_INTS);
> +	p2wi->status = status;
> +
> +	/* Clear interrupts */
> +	status &= (P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR |
> +		   P2WI_INTS_TRANS_OVER);
> +	writel(status, p2wi->regs + P2WI_INTS);
> +
> +	complete(&p2wi->complete);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static u32 p2wi_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_SMBUS_BYTE_DATA;
> +}
> +
> +static int p2wi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> +			   unsigned short flags, char read_write,
> +			   u8 command, int size, union i2c_smbus_data *data)
> +{
> +	struct p2wi *p2wi = i2c_get_adapdata(adap);
> +	unsigned long dlen = P2WI_DLEN_DATA_LENGTH(1);
> +
> +	if (addr > 0xff ||

Why 0xff? Does the PMIC support that? I2C addresses are 7-bit. You
won't even have a slave device if it has an illegal i2c address, so this
shouldn't happen.

> +	    (p2wi->slave_addr >= 0 && addr != p2wi->slave_addr)) {
> +		dev_err(&adap->dev, "invalid P2WI address\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	writel(command, p2wi->regs + P2WI_DADDR0);
> +
> +	if (read_write == I2C_SMBUS_READ)
> +		dlen |= P2WI_DLEN_READ;
> +	else
> +		writel(data->byte, p2wi->regs + P2WI_DATA0);
> +
> +	writel(dlen, p2wi->regs + P2WI_DLEN);
> +
> +	if (readl(p2wi->regs + P2WI_CTRL) & P2WI_CTRL_START_TRANS) {
> +		dev_err(&adap->dev, "P2WI bus busy\n");
> +		return -EBUSY;
> +	}
> +
> +	reinit_completion(&p2wi->complete);
> +
> +	writel(P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR | P2WI_INTS_TRANS_OVER,
> +	       p2wi->regs + P2WI_INTE);
> +
> +	writel(P2WI_CTRL_START_TRANS | P2WI_CTRL_GLOBAL_INT_ENB,
> +	       p2wi->regs + P2WI_CTRL);
> +
> +	wait_for_completion(&p2wi->complete);
> +
> +	if (p2wi->status & P2WI_INTS_LOAD_BSY) {
> +		dev_err(&adap->dev, "P2WI bus busy\n");
> +		return -EBUSY;
> +	}
> +
> +	if (p2wi->status & P2WI_INTS_TRANS_ERR) {
> +		dev_err(&adap->dev, "P2WI bus xfer error\n");
> +		return -ENXIO;
> +	}
> +
> +	if (read_write == I2C_SMBUS_READ)
> +		data->byte = readl(p2wi->regs + P2WI_DATA0);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_algorithm p2wi_algo = {
> +	.smbus_xfer = p2wi_smbus_xfer,
> +	.functionality = p2wi_functionality,
> +};
> +
> +static const struct of_device_id p2wi_of_match_table[] = {
> +	{ .compatible = "allwinner,sun6i-a31-p2wi" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, p2wi_of_match_table);
> +
> +static int p2wi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *childnp;
> +	unsigned long parent_clk_freq;
> +	u32 clk_freq = 100000;
> +	struct resource *r;
> +	struct p2wi *p2wi;
> +	u32 slave_addr;
> +	int clk_div;
> +	int ret;
> +
> +	of_property_read_u32(np, "clock-frequency", &clk_freq);
> +	if (clk_freq > P2WI_MAX_FREQ) {
> +		dev_err(dev,
> +			"required clock-frequency (%u Hz) is too high (max = 6MHz)",
> +			clk_freq);
> +		return -EINVAL;
> +	}
> +
> +	if (of_get_child_count(np) > 1) {
> +		dev_err(dev, "P2WI only supports one slave device\n");
> +		return -EINVAL;
> +	}
> +
> +	p2wi = devm_kzalloc(dev, sizeof(struct p2wi), GFP_KERNEL);
> +	if (!p2wi) {
> +		dev_err(dev, "failed to allocate p2wi struct\n");

No error strings for OOM.

> +		return -ENOMEM;
> +	}
> +
> +	p2wi->slave_addr = -1;
> +
> +	/*
> +	 * Authorize a p2wi node without any children to be able to use an
> +	 * i2c-dev from userpace.
> +	 * In this case the slave_addr is set to -1 and won't be checked when
> +	 * launching a P2WI transfer.
> +	 */
> +	childnp = of_get_next_available_child(np, NULL);
> +	if (childnp) {
> +		ret = of_property_read_u32(childnp, "reg", &slave_addr);
> +		if (ret || slave_addr > 0xff) {

Again: Is 8 bit range important here? Otherwise I'd leave the check to the
core.

> +			dev_err(dev, "invalid slave address on node %s\n",
> +				childnp->full_name);
> +			return -EINVAL;
> +		}
> +
> +		p2wi->slave_addr = slave_addr;
> +	}
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	p2wi->regs = devm_ioremap_resource(dev, r);
> +	if (IS_ERR(p2wi->regs)) {
> +		ret = PTR_ERR(p2wi->regs);
> +		dev_err(dev, "failed to retrieve iomem resource: %d\n", ret);

devm_ioremap_resource prints errors on its own.

> +		return ret;
> +	}
> +
> +	snprintf(p2wi->adapter.name, sizeof(p2wi->adapter.name), pdev->name);
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to retrieve irq: %d\n", ret);
> +		return ret;
> +	}
> +	p2wi->irq = ret;
> +
> +	p2wi->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(p2wi->clk)) {
> +		ret = PTR_ERR(p2wi->clk);
> +		dev_err(dev, "failed to retrieve clk: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(p2wi->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	parent_clk_freq = clk_get_rate(p2wi->clk);
> +
> +	p2wi->rstc = devm_reset_control_get(dev, NULL);
> +	if (IS_ERR(p2wi->rstc)) {
> +		ret = PTR_ERR(p2wi->rstc);
> +		dev_err(dev, "failed to retrieve reset controller: %d\n",
> +			ret);

My general suggestion: Don't be too strict on the 80 char limit. IMO this dangling
'ret' is not more readable.

> +		goto err_clk_disable;
> +	}

Regards,

   Wolfram

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Wolfram Sang]

On Tue, Jun 03, 2014 at 10:49:52AM +0200, Boris BREZILLON wrote:
> The P2WI looks like an SMBus controller which only supports byte data
> transfers. But, it differs from standard SMBus protocol on several
> aspects:
> - it supports only one slave device, and thus drop the address field
> - it adds a parity bit every 8bits of data
> - only one read access is required to read a byte (instead of a read
>   followed by a write access in standard SMBus protocol)
> - there's no Ack bit after each byte transfer
> 
> This means this bus cannot be used to interface with standard SMBus
> devices (the only known device to support this interface is the AXP221
> PMIC).

Good description. Should be a comment at the top of the driver to spread
the word.

> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/i2c/busses/Kconfig          |  12 ++
>  drivers/i2c/busses/Makefile         |   1 +
>  drivers/i2c/busses/i2c-sun6i-p2wi.c | 349 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 362 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-sun6i-p2wi.c

...

> +struct p2wi {
> +	struct i2c_adapter adapter;
> +	struct completion complete;
> +	unsigned int irq;

Can be a local variable in probe.

> +	unsigned int status;
> +	void __iomem *regs;
> +	struct clk *clk;
> +	struct reset_control *rstc;
> +	int slave_addr;
> +};
> +
> +static irqreturn_t p2wi_interrupt(int irq, void *dev_id)
> +{
> +	struct p2wi *p2wi = dev_id;
> +	unsigned long status;
> +
> +	status = readl(p2wi->regs + P2WI_INTS);
> +	p2wi->status = status;
> +
> +	/* Clear interrupts */
> +	status &= (P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR |
> +		   P2WI_INTS_TRANS_OVER);
> +	writel(status, p2wi->regs + P2WI_INTS);
> +
> +	complete(&p2wi->complete);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static u32 p2wi_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_SMBUS_BYTE_DATA;
> +}
> +
> +static int p2wi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> +			   unsigned short flags, char read_write,
> +			   u8 command, int size, union i2c_smbus_data *data)
> +{
> +	struct p2wi *p2wi = i2c_get_adapdata(adap);
> +	unsigned long dlen = P2WI_DLEN_DATA_LENGTH(1);
> +
> +	if (addr > 0xff ||

Why 0xff? Does the PMIC support that? I2C addresses are 7-bit. You
won't even have a slave device if it has an illegal i2c address, so this
shouldn't happen.

> +	    (p2wi->slave_addr >= 0 && addr != p2wi->slave_addr)) {
> +		dev_err(&adap->dev, "invalid P2WI address\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	writel(command, p2wi->regs + P2WI_DADDR0);
> +
> +	if (read_write == I2C_SMBUS_READ)
> +		dlen |= P2WI_DLEN_READ;
> +	else
> +		writel(data->byte, p2wi->regs + P2WI_DATA0);
> +
> +	writel(dlen, p2wi->regs + P2WI_DLEN);
> +
> +	if (readl(p2wi->regs + P2WI_CTRL) & P2WI_CTRL_START_TRANS) {
> +		dev_err(&adap->dev, "P2WI bus busy\n");
> +		return -EBUSY;
> +	}
> +
> +	reinit_completion(&p2wi->complete);
> +
> +	writel(P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR | P2WI_INTS_TRANS_OVER,
> +	       p2wi->regs + P2WI_INTE);
> +
> +	writel(P2WI_CTRL_START_TRANS | P2WI_CTRL_GLOBAL_INT_ENB,
> +	       p2wi->regs + P2WI_CTRL);
> +
> +	wait_for_completion(&p2wi->complete);
> +
> +	if (p2wi->status & P2WI_INTS_LOAD_BSY) {
> +		dev_err(&adap->dev, "P2WI bus busy\n");
> +		return -EBUSY;
> +	}
> +
> +	if (p2wi->status & P2WI_INTS_TRANS_ERR) {
> +		dev_err(&adap->dev, "P2WI bus xfer error\n");
> +		return -ENXIO;
> +	}
> +
> +	if (read_write == I2C_SMBUS_READ)
> +		data->byte = readl(p2wi->regs + P2WI_DATA0);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_algorithm p2wi_algo = {
> +	.smbus_xfer = p2wi_smbus_xfer,
> +	.functionality = p2wi_functionality,
> +};
> +
> +static const struct of_device_id p2wi_of_match_table[] = {
> +	{ .compatible = "allwinner,sun6i-a31-p2wi" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, p2wi_of_match_table);
> +
> +static int p2wi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *childnp;
> +	unsigned long parent_clk_freq;
> +	u32 clk_freq = 100000;
> +	struct resource *r;
> +	struct p2wi *p2wi;
> +	u32 slave_addr;
> +	int clk_div;
> +	int ret;
> +
> +	of_property_read_u32(np, "clock-frequency", &clk_freq);
> +	if (clk_freq > P2WI_MAX_FREQ) {
> +		dev_err(dev,
> +			"required clock-frequency (%u Hz) is too high (max = 6MHz)",
> +			clk_freq);
> +		return -EINVAL;
> +	}
> +
> +	if (of_get_child_count(np) > 1) {
> +		dev_err(dev, "P2WI only supports one slave device\n");
> +		return -EINVAL;
> +	}
> +
> +	p2wi = devm_kzalloc(dev, sizeof(struct p2wi), GFP_KERNEL);
> +	if (!p2wi) {
> +		dev_err(dev, "failed to allocate p2wi struct\n");

No error strings for OOM.

> +		return -ENOMEM;
> +	}
> +
> +	p2wi->slave_addr = -1;
> +
> +	/*
> +	 * Authorize a p2wi node without any children to be able to use an
> +	 * i2c-dev from userpace.
> +	 * In this case the slave_addr is set to -1 and won't be checked when
> +	 * launching a P2WI transfer.
> +	 */
> +	childnp = of_get_next_available_child(np, NULL);
> +	if (childnp) {
> +		ret = of_property_read_u32(childnp, "reg", &slave_addr);
> +		if (ret || slave_addr > 0xff) {

Again: Is 8 bit range important here? Otherwise I'd leave the check to the
core.

> +			dev_err(dev, "invalid slave address on node %s\n",
> +				childnp->full_name);
> +			return -EINVAL;
> +		}
> +
> +		p2wi->slave_addr = slave_addr;
> +	}
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	p2wi->regs = devm_ioremap_resource(dev, r);
> +	if (IS_ERR(p2wi->regs)) {
> +		ret = PTR_ERR(p2wi->regs);
> +		dev_err(dev, "failed to retrieve iomem resource: %d\n", ret);

devm_ioremap_resource prints errors on its own.

> +		return ret;
> +	}
> +
> +	snprintf(p2wi->adapter.name, sizeof(p2wi->adapter.name), pdev->name);
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to retrieve irq: %d\n", ret);
> +		return ret;
> +	}
> +	p2wi->irq = ret;
> +
> +	p2wi->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(p2wi->clk)) {
> +		ret = PTR_ERR(p2wi->clk);
> +		dev_err(dev, "failed to retrieve clk: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(p2wi->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	parent_clk_freq = clk_get_rate(p2wi->clk);
> +
> +	p2wi->rstc = devm_reset_control_get(dev, NULL);
> +	if (IS_ERR(p2wi->rstc)) {
> +		ret = PTR_ERR(p2wi->rstc);
> +		dev_err(dev, "failed to retrieve reset controller: %d\n",
> +			ret);

My general suggestion: Don't be too strict on the 80 char limit. IMO this dangling
'ret' is not more readable.

> +		goto err_clk_disable;
> +	}

Regards,

   Wolfram
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140610/cabb5e19/attachment.sig>

Re: [RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Boris BREZILLON]

Hello Wolfram,

On 10/06/2014 10:38, Wolfram Sang wrote:
> On Tue, Jun 03, 2014 at 10:49:52AM +0200, Boris BREZILLON wrote:
>> The P2WI looks like an SMBus controller which only supports byte data
>> transfers. But, it differs from standard SMBus protocol on several
>> aspects:
>> - it supports only one slave device, and thus drop the address field
>> - it adds a parity bit every 8bits of data
>> - only one read access is required to read a byte (instead of a read
>>   followed by a write access in standard SMBus protocol)
>> - there's no Ack bit after each byte transfer
>>
>> This means this bus cannot be used to interface with standard SMBus
>> devices (the only known device to support this interface is the AXP221
>> PMIC).
> Good description. Should be a comment at the top of the driver to spread
> the word.

Sure, I'll copy this description in the driver.

>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> ---
>>  drivers/i2c/busses/Kconfig          |  12 ++
>>  drivers/i2c/busses/Makefile         |   1 +
>>  drivers/i2c/busses/i2c-sun6i-p2wi.c | 349 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 362 insertions(+)
>>  create mode 100644 drivers/i2c/busses/i2c-sun6i-p2wi.c
> ...
>
>> +struct p2wi {
>> +	struct i2c_adapter adapter;
>> +	struct completion complete;
>> +	unsigned int irq;
> Can be a local variable in probe.

Yes, I'll remove it from this structure.

>
>> +	unsigned int status;
>> +	void __iomem *regs;
>> +	struct clk *clk;
>> +	struct reset_control *rstc;
>> +	int slave_addr;
>> +};
>> +
>> +static irqreturn_t p2wi_interrupt(int irq, void *dev_id)
>> +{
>> +	struct p2wi *p2wi = dev_id;
>> +	unsigned long status;
>> +
>> +	status = readl(p2wi->regs + P2WI_INTS);
>> +	p2wi->status = status;
>> +
>> +	/* Clear interrupts */
>> +	status &= (P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR |
>> +		   P2WI_INTS_TRANS_OVER);
>> +	writel(status, p2wi->regs + P2WI_INTS);
>> +
>> +	complete(&p2wi->complete);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static u32 p2wi_functionality(struct i2c_adapter *adap)
>> +{
>> +	return I2C_FUNC_SMBUS_BYTE_DATA;
>> +}
>> +
>> +static int p2wi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>> +			   unsigned short flags, char read_write,
>> +			   u8 command, int size, union i2c_smbus_data *data)
>> +{
>> +	struct p2wi *p2wi = i2c_get_adapdata(adap);
>> +	unsigned long dlen = P2WI_DLEN_DATA_LENGTH(1);
>> +
>> +	if (addr > 0xff ||
> Why 0xff? Does the PMIC support that? I2C addresses are 7-bit. You
> won't even have a slave device if it has an illegal i2c address, so this
> shouldn't happen.

The P2WI protocol supports 8bits addresses, hence I added this 0xff check.
Anyway, the PMIC I use (AXP221) is assigned the 0x68 address, and I
don't think there are a lot of P2WI compatible devices in the wild, so
we can just assume 7bits addresses are fine and rely on the core code
checks.

>
>> +	    (p2wi->slave_addr >= 0 && addr != p2wi->slave_addr)) {
>> +		dev_err(&adap->dev, "invalid P2WI address\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!data)
>> +		return -EINVAL;
>> +
>> +	writel(command, p2wi->regs + P2WI_DADDR0);
>> +
>> +	if (read_write == I2C_SMBUS_READ)
>> +		dlen |= P2WI_DLEN_READ;
>> +	else
>> +		writel(data->byte, p2wi->regs + P2WI_DATA0);
>> +
>> +	writel(dlen, p2wi->regs + P2WI_DLEN);
>> +
>> +	if (readl(p2wi->regs + P2WI_CTRL) & P2WI_CTRL_START_TRANS) {
>> +		dev_err(&adap->dev, "P2WI bus busy\n");
>> +		return -EBUSY;
>> +	}
>> +
>> +	reinit_completion(&p2wi->complete);
>> +
>> +	writel(P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR | P2WI_INTS_TRANS_OVER,
>> +	       p2wi->regs + P2WI_INTE);
>> +
>> +	writel(P2WI_CTRL_START_TRANS | P2WI_CTRL_GLOBAL_INT_ENB,
>> +	       p2wi->regs + P2WI_CTRL);
>> +
>> +	wait_for_completion(&p2wi->complete);
>> +
>> +	if (p2wi->status & P2WI_INTS_LOAD_BSY) {
>> +		dev_err(&adap->dev, "P2WI bus busy\n");
>> +		return -EBUSY;
>> +	}
>> +
>> +	if (p2wi->status & P2WI_INTS_TRANS_ERR) {
>> +		dev_err(&adap->dev, "P2WI bus xfer error\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	if (read_write == I2C_SMBUS_READ)
>> +		data->byte = readl(p2wi->regs + P2WI_DATA0);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_algorithm p2wi_algo = {
>> +	.smbus_xfer = p2wi_smbus_xfer,
>> +	.functionality = p2wi_functionality,
>> +};
>> +
>> +static const struct of_device_id p2wi_of_match_table[] = {
>> +	{ .compatible = "allwinner,sun6i-a31-p2wi" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, p2wi_of_match_table);
>> +
>> +static int p2wi_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct device_node *childnp;
>> +	unsigned long parent_clk_freq;
>> +	u32 clk_freq = 100000;
>> +	struct resource *r;
>> +	struct p2wi *p2wi;
>> +	u32 slave_addr;
>> +	int clk_div;
>> +	int ret;
>> +
>> +	of_property_read_u32(np, "clock-frequency", &clk_freq);
>> +	if (clk_freq > P2WI_MAX_FREQ) {
>> +		dev_err(dev,
>> +			"required clock-frequency (%u Hz) is too high (max = 6MHz)",
>> +			clk_freq);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (of_get_child_count(np) > 1) {
>> +		dev_err(dev, "P2WI only supports one slave device\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	p2wi = devm_kzalloc(dev, sizeof(struct p2wi), GFP_KERNEL);
>> +	if (!p2wi) {
>> +		dev_err(dev, "failed to allocate p2wi struct\n");
> No error strings for OOM.

I'll drop this line.

>
>> +		return -ENOMEM;
>> +	}
>> +
>> +	p2wi->slave_addr = -1;
>> +
>> +	/*
>> +	 * Authorize a p2wi node without any children to be able to use an
>> +	 * i2c-dev from userpace.
>> +	 * In this case the slave_addr is set to -1 and won't be checked when
>> +	 * launching a P2WI transfer.
>> +	 */
>> +	childnp = of_get_next_available_child(np, NULL);
>> +	if (childnp) {
>> +		ret = of_property_read_u32(childnp, "reg", &slave_addr);
>> +		if (ret || slave_addr > 0xff) {
> Again: Is 8 bit range important here? Otherwise I'd leave the check to the
> core.
>
>> +			dev_err(dev, "invalid slave address on node %s\n",
>> +				childnp->full_name);
>> +			return -EINVAL;
>> +		}
>> +
>> +		p2wi->slave_addr = slave_addr;
>> +	}
>> +
>> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	p2wi->regs = devm_ioremap_resource(dev, r);
>> +	if (IS_ERR(p2wi->regs)) {
>> +		ret = PTR_ERR(p2wi->regs);
>> +		dev_err(dev, "failed to retrieve iomem resource: %d\n", ret);
> devm_ioremap_resource prints errors on its own.

Ditto

>
>> +		return ret;
>> +	}
>> +
>> +	snprintf(p2wi->adapter.name, sizeof(p2wi->adapter.name), pdev->name);
>> +	ret = platform_get_irq(pdev, 0);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to retrieve irq: %d\n", ret);
>> +		return ret;
>> +	}
>> +	p2wi->irq = ret;
>> +
>> +	p2wi->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(p2wi->clk)) {
>> +		ret = PTR_ERR(p2wi->clk);
>> +		dev_err(dev, "failed to retrieve clk: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(p2wi->clk);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	parent_clk_freq = clk_get_rate(p2wi->clk);
>> +
>> +	p2wi->rstc = devm_reset_control_get(dev, NULL);
>> +	if (IS_ERR(p2wi->rstc)) {
>> +		ret = PTR_ERR(p2wi->rstc);
>> +		dev_err(dev, "failed to retrieve reset controller: %d\n",
>> +			ret);
> My general suggestion: Don't be too strict on the 80 char limit. IMO this dangling
> 'ret' is not more readable.

Okay, I'll fix that.


Thanks for your review.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Boris BREZILLON]

Hello Wolfram,

On 10/06/2014 10:38, Wolfram Sang wrote:
> On Tue, Jun 03, 2014 at 10:49:52AM +0200, Boris BREZILLON wrote:
>> The P2WI looks like an SMBus controller which only supports byte data
>> transfers. But, it differs from standard SMBus protocol on several
>> aspects:
>> - it supports only one slave device, and thus drop the address field
>> - it adds a parity bit every 8bits of data
>> - only one read access is required to read a byte (instead of a read
>>   followed by a write access in standard SMBus protocol)
>> - there's no Ack bit after each byte transfer
>>
>> This means this bus cannot be used to interface with standard SMBus
>> devices (the only known device to support this interface is the AXP221
>> PMIC).
> Good description. Should be a comment at the top of the driver to spread
> the word.

Sure, I'll copy this description in the driver.

>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> ---
>>  drivers/i2c/busses/Kconfig          |  12 ++
>>  drivers/i2c/busses/Makefile         |   1 +
>>  drivers/i2c/busses/i2c-sun6i-p2wi.c | 349 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 362 insertions(+)
>>  create mode 100644 drivers/i2c/busses/i2c-sun6i-p2wi.c
> ...
>
>> +struct p2wi {
>> +	struct i2c_adapter adapter;
>> +	struct completion complete;
>> +	unsigned int irq;
> Can be a local variable in probe.

Yes, I'll remove it from this structure.

>
>> +	unsigned int status;
>> +	void __iomem *regs;
>> +	struct clk *clk;
>> +	struct reset_control *rstc;
>> +	int slave_addr;
>> +};
>> +
>> +static irqreturn_t p2wi_interrupt(int irq, void *dev_id)
>> +{
>> +	struct p2wi *p2wi = dev_id;
>> +	unsigned long status;
>> +
>> +	status = readl(p2wi->regs + P2WI_INTS);
>> +	p2wi->status = status;
>> +
>> +	/* Clear interrupts */
>> +	status &= (P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR |
>> +		   P2WI_INTS_TRANS_OVER);
>> +	writel(status, p2wi->regs + P2WI_INTS);
>> +
>> +	complete(&p2wi->complete);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static u32 p2wi_functionality(struct i2c_adapter *adap)
>> +{
>> +	return I2C_FUNC_SMBUS_BYTE_DATA;
>> +}
>> +
>> +static int p2wi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>> +			   unsigned short flags, char read_write,
>> +			   u8 command, int size, union i2c_smbus_data *data)
>> +{
>> +	struct p2wi *p2wi = i2c_get_adapdata(adap);
>> +	unsigned long dlen = P2WI_DLEN_DATA_LENGTH(1);
>> +
>> +	if (addr > 0xff ||
> Why 0xff? Does the PMIC support that? I2C addresses are 7-bit. You
> won't even have a slave device if it has an illegal i2c address, so this
> shouldn't happen.

The P2WI protocol supports 8bits addresses, hence I added this 0xff check.
Anyway, the PMIC I use (AXP221) is assigned the 0x68 address, and I
don't think there are a lot of P2WI compatible devices in the wild, so
we can just assume 7bits addresses are fine and rely on the core code
checks.

>
>> +	    (p2wi->slave_addr >= 0 && addr != p2wi->slave_addr)) {
>> +		dev_err(&adap->dev, "invalid P2WI address\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!data)
>> +		return -EINVAL;
>> +
>> +	writel(command, p2wi->regs + P2WI_DADDR0);
>> +
>> +	if (read_write == I2C_SMBUS_READ)
>> +		dlen |= P2WI_DLEN_READ;
>> +	else
>> +		writel(data->byte, p2wi->regs + P2WI_DATA0);
>> +
>> +	writel(dlen, p2wi->regs + P2WI_DLEN);
>> +
>> +	if (readl(p2wi->regs + P2WI_CTRL) & P2WI_CTRL_START_TRANS) {
>> +		dev_err(&adap->dev, "P2WI bus busy\n");
>> +		return -EBUSY;
>> +	}
>> +
>> +	reinit_completion(&p2wi->complete);
>> +
>> +	writel(P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR | P2WI_INTS_TRANS_OVER,
>> +	       p2wi->regs + P2WI_INTE);
>> +
>> +	writel(P2WI_CTRL_START_TRANS | P2WI_CTRL_GLOBAL_INT_ENB,
>> +	       p2wi->regs + P2WI_CTRL);
>> +
>> +	wait_for_completion(&p2wi->complete);
>> +
>> +	if (p2wi->status & P2WI_INTS_LOAD_BSY) {
>> +		dev_err(&adap->dev, "P2WI bus busy\n");
>> +		return -EBUSY;
>> +	}
>> +
>> +	if (p2wi->status & P2WI_INTS_TRANS_ERR) {
>> +		dev_err(&adap->dev, "P2WI bus xfer error\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	if (read_write == I2C_SMBUS_READ)
>> +		data->byte = readl(p2wi->regs + P2WI_DATA0);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_algorithm p2wi_algo = {
>> +	.smbus_xfer = p2wi_smbus_xfer,
>> +	.functionality = p2wi_functionality,
>> +};
>> +
>> +static const struct of_device_id p2wi_of_match_table[] = {
>> +	{ .compatible = "allwinner,sun6i-a31-p2wi" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, p2wi_of_match_table);
>> +
>> +static int p2wi_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct device_node *childnp;
>> +	unsigned long parent_clk_freq;
>> +	u32 clk_freq = 100000;
>> +	struct resource *r;
>> +	struct p2wi *p2wi;
>> +	u32 slave_addr;
>> +	int clk_div;
>> +	int ret;
>> +
>> +	of_property_read_u32(np, "clock-frequency", &clk_freq);
>> +	if (clk_freq > P2WI_MAX_FREQ) {
>> +		dev_err(dev,
>> +			"required clock-frequency (%u Hz) is too high (max = 6MHz)",
>> +			clk_freq);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (of_get_child_count(np) > 1) {
>> +		dev_err(dev, "P2WI only supports one slave device\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	p2wi = devm_kzalloc(dev, sizeof(struct p2wi), GFP_KERNEL);
>> +	if (!p2wi) {
>> +		dev_err(dev, "failed to allocate p2wi struct\n");
> No error strings for OOM.

I'll drop this line.

>
>> +		return -ENOMEM;
>> +	}
>> +
>> +	p2wi->slave_addr = -1;
>> +
>> +	/*
>> +	 * Authorize a p2wi node without any children to be able to use an
>> +	 * i2c-dev from userpace.
>> +	 * In this case the slave_addr is set to -1 and won't be checked when
>> +	 * launching a P2WI transfer.
>> +	 */
>> +	childnp = of_get_next_available_child(np, NULL);
>> +	if (childnp) {
>> +		ret = of_property_read_u32(childnp, "reg", &slave_addr);
>> +		if (ret || slave_addr > 0xff) {
> Again: Is 8 bit range important here? Otherwise I'd leave the check to the
> core.
>
>> +			dev_err(dev, "invalid slave address on node %s\n",
>> +				childnp->full_name);
>> +			return -EINVAL;
>> +		}
>> +
>> +		p2wi->slave_addr = slave_addr;
>> +	}
>> +
>> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	p2wi->regs = devm_ioremap_resource(dev, r);
>> +	if (IS_ERR(p2wi->regs)) {
>> +		ret = PTR_ERR(p2wi->regs);
>> +		dev_err(dev, "failed to retrieve iomem resource: %d\n", ret);
> devm_ioremap_resource prints errors on its own.

Ditto

>
>> +		return ret;
>> +	}
>> +
>> +	snprintf(p2wi->adapter.name, sizeof(p2wi->adapter.name), pdev->name);
>> +	ret = platform_get_irq(pdev, 0);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to retrieve irq: %d\n", ret);
>> +		return ret;
>> +	}
>> +	p2wi->irq = ret;
>> +
>> +	p2wi->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(p2wi->clk)) {
>> +		ret = PTR_ERR(p2wi->clk);
>> +		dev_err(dev, "failed to retrieve clk: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(p2wi->clk);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	parent_clk_freq = clk_get_rate(p2wi->clk);
>> +
>> +	p2wi->rstc = devm_reset_control_get(dev, NULL);
>> +	if (IS_ERR(p2wi->rstc)) {
>> +		ret = PTR_ERR(p2wi->rstc);
>> +		dev_err(dev, "failed to retrieve reset controller: %d\n",
>> +			ret);
> My general suggestion: Don't be too strict on the 80 char limit. IMO this dangling
> 'ret' is not more readable.

Okay, I'll fix that.


Thanks for your review.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Paul Carpenter]

Wolfram Sang wrote:
> On Tue, Jun 03, 2014 at 10:49:52AM +0200, Boris BREZILLON wrote:
>> The P2WI looks like an SMBus controller which only supports byte data
>> transfers. But, it differs from standard SMBus protocol on several
>> aspects:
>> - it supports only one slave device, and thus drop the address field
>> - it adds a parity bit every 8bits of data
>> - only one read access is required to read a byte (instead of a read
>>   followed by a write access in standard SMBus protocol)

Minor quibble should be
"(instead of a write followed by a read access in standard SMBus
   protocol)"

That being similar to EEPROM and other types of devices write internal
address followed by read access from internal address.

>> - there's no Ack bit after each byte transfer
>>
>> This means this bus cannot be used to interface with standard SMBus
>> devices (the only known device to support this interface is the AXP221
>> PMIC).
> 
> Good description. Should be a comment at the top of the driver to spread
> the word.

-- 
Paul Carpenter          | paul@pcserviceselectronics.co.uk
<http://www.pcserviceselectronics.co.uk/>    PC Services
<http://www.pcserviceselectronics.co.uk/pi/>  Raspberry Pi Add-ons
<http://www.pcserviceselectronics.co.uk/fonts/> Timing Diagram Font
<http://www.badweb.org.uk/> For those web sites you hate

Re: [RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Paul Carpenter]

Wolfram Sang wrote:
> On Tue, Jun 03, 2014 at 10:49:52AM +0200, Boris BREZILLON wrote:
>> The P2WI looks like an SMBus controller which only supports byte data
>> transfers. But, it differs from standard SMBus protocol on several
>> aspects:
>> - it supports only one slave device, and thus drop the address field
>> - it adds a parity bit every 8bits of data
>> - only one read access is required to read a byte (instead of a read
>>   followed by a write access in standard SMBus protocol)

Minor quibble should be
"(instead of a write followed by a read access in standard SMBus
   protocol)"

That being similar to EEPROM and other types of devices write internal
address followed by read access from internal address.

>> - there's no Ack bit after each byte transfer
>>
>> This means this bus cannot be used to interface with standard SMBus
>> devices (the only known device to support this interface is the AXP221
>> PMIC).
> 
> Good description. Should be a comment at the top of the driver to spread
> the word.

-- 
Paul Carpenter          | paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org
<http://www.pcserviceselectronics.co.uk/>    PC Services
<http://www.pcserviceselectronics.co.uk/pi/>  Raspberry Pi Add-ons
<http://www.pcserviceselectronics.co.uk/fonts/> Timing Diagram Font
<http://www.badweb.org.uk/> For those web sites you hate

Re: [RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Boris BREZILLON]

On 10/06/2014 10:54, Boris BREZILLON wrote:
> Hello Wolfram,
>
> On 10/06/2014 10:38, Wolfram Sang wrote:
>> On Tue, Jun 03, 2014 at 10:49:52AM +0200, Boris BREZILLON wrote:
>>> The P2WI looks like an SMBus controller which only supports byte data
>>> transfers. But, it differs from standard SMBus protocol on several
>>> aspects:
>>> - it supports only one slave device, and thus drop the address field
>>> - it adds a parity bit every 8bits of data
>>> - only one read access is required to read a byte (instead of a read
>>>   followed by a write access in standard SMBus protocol)
>>> - there's no Ack bit after each byte transfer
>>>
>>> This means this bus cannot be used to interface with standard SMBus
>>> devices (the only known device to support this interface is the AXP221
>>> PMIC).
>> Good description. Should be a comment at the top of the driver to spread
>> the word.
> Sure, I'll copy this description in the driver.
>
>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
>>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> ---
>>>  drivers/i2c/busses/Kconfig          |  12 ++
>>>  drivers/i2c/busses/Makefile         |   1 +
>>>  drivers/i2c/busses/i2c-sun6i-p2wi.c | 349 ++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 362 insertions(+)
>>>  create mode 100644 drivers/i2c/busses/i2c-sun6i-p2wi.c
>> ...
>>
>>> +struct p2wi {
>>> +	struct i2c_adapter adapter;
>>> +	struct completion complete;
>>> +	unsigned int irq;
>> Can be a local variable in probe.
> Yes, I'll remove it from this structure.
>
>>> +	unsigned int status;
>>> +	void __iomem *regs;
>>> +	struct clk *clk;
>>> +	struct reset_control *rstc;
>>> +	int slave_addr;
>>> +};
>>> +
>>> +static irqreturn_t p2wi_interrupt(int irq, void *dev_id)
>>> +{
>>> +	struct p2wi *p2wi = dev_id;
>>> +	unsigned long status;
>>> +
>>> +	status = readl(p2wi->regs + P2WI_INTS);
>>> +	p2wi->status = status;
>>> +
>>> +	/* Clear interrupts */
>>> +	status &= (P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR |
>>> +		   P2WI_INTS_TRANS_OVER);
>>> +	writel(status, p2wi->regs + P2WI_INTS);
>>> +
>>> +	complete(&p2wi->complete);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static u32 p2wi_functionality(struct i2c_adapter *adap)
>>> +{
>>> +	return I2C_FUNC_SMBUS_BYTE_DATA;
>>> +}
>>> +
>>> +static int p2wi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>>> +			   unsigned short flags, char read_write,
>>> +			   u8 command, int size, union i2c_smbus_data *data)
>>> +{
>>> +	struct p2wi *p2wi = i2c_get_adapdata(adap);
>>> +	unsigned long dlen = P2WI_DLEN_DATA_LENGTH(1);
>>> +
>>> +	if (addr > 0xff ||
>> Why 0xff? Does the PMIC support that? I2C addresses are 7-bit. You
>> won't even have a slave device if it has an illegal i2c address, so this
>> shouldn't happen.
> The P2WI protocol supports 8bits addresses, hence I added this 0xff check.
> Anyway, the PMIC I use (AXP221) is assigned the 0x68 address, and I
> don't think there are a lot of P2WI compatible devices in the wild, so
> we can just assume 7bits addresses are fine and rely on the core code
> checks.

My bad, the P2WI protocol does not have any address concept.
The 0xff value come from the P2WI_PMCR_PMU_DEV_ADDR field which is
specified to be 8 bits large.
Anyway, this does not change the fact that we can remove this check.

>
>>> +	    (p2wi->slave_addr >= 0 && addr != p2wi->slave_addr)) {
>>> +		dev_err(&adap->dev, "invalid P2WI address\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (!data)
>>> +		return -EINVAL;
>>> +
>>> +	writel(command, p2wi->regs + P2WI_DADDR0);
>>> +
>>> +	if (read_write == I2C_SMBUS_READ)
>>> +		dlen |= P2WI_DLEN_READ;
>>> +	else
>>> +		writel(data->byte, p2wi->regs + P2WI_DATA0);
>>> +
>>> +	writel(dlen, p2wi->regs + P2WI_DLEN);
>>> +
>>> +	if (readl(p2wi->regs + P2WI_CTRL) & P2WI_CTRL_START_TRANS) {
>>> +		dev_err(&adap->dev, "P2WI bus busy\n");
>>> +		return -EBUSY;
>>> +	}
>>> +
>>> +	reinit_completion(&p2wi->complete);
>>> +
>>> +	writel(P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR | P2WI_INTS_TRANS_OVER,
>>> +	       p2wi->regs + P2WI_INTE);
>>> +
>>> +	writel(P2WI_CTRL_START_TRANS | P2WI_CTRL_GLOBAL_INT_ENB,
>>> +	       p2wi->regs + P2WI_CTRL);
>>> +
>>> +	wait_for_completion(&p2wi->complete);
>>> +
>>> +	if (p2wi->status & P2WI_INTS_LOAD_BSY) {
>>> +		dev_err(&adap->dev, "P2WI bus busy\n");
>>> +		return -EBUSY;
>>> +	}
>>> +
>>> +	if (p2wi->status & P2WI_INTS_TRANS_ERR) {
>>> +		dev_err(&adap->dev, "P2WI bus xfer error\n");
>>> +		return -ENXIO;
>>> +	}
>>> +
>>> +	if (read_write == I2C_SMBUS_READ)
>>> +		data->byte = readl(p2wi->regs + P2WI_DATA0);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct i2c_algorithm p2wi_algo = {
>>> +	.smbus_xfer = p2wi_smbus_xfer,
>>> +	.functionality = p2wi_functionality,
>>> +};
>>> +
>>> +static const struct of_device_id p2wi_of_match_table[] = {
>>> +	{ .compatible = "allwinner,sun6i-a31-p2wi" },
>>> +	{}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, p2wi_of_match_table);
>>> +
>>> +static int p2wi_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct device_node *np = dev->of_node;
>>> +	struct device_node *childnp;
>>> +	unsigned long parent_clk_freq;
>>> +	u32 clk_freq = 100000;
>>> +	struct resource *r;
>>> +	struct p2wi *p2wi;
>>> +	u32 slave_addr;
>>> +	int clk_div;
>>> +	int ret;
>>> +
>>> +	of_property_read_u32(np, "clock-frequency", &clk_freq);
>>> +	if (clk_freq > P2WI_MAX_FREQ) {
>>> +		dev_err(dev,
>>> +			"required clock-frequency (%u Hz) is too high (max = 6MHz)",
>>> +			clk_freq);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (of_get_child_count(np) > 1) {
>>> +		dev_err(dev, "P2WI only supports one slave device\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	p2wi = devm_kzalloc(dev, sizeof(struct p2wi), GFP_KERNEL);
>>> +	if (!p2wi) {
>>> +		dev_err(dev, "failed to allocate p2wi struct\n");
>> No error strings for OOM.
> I'll drop this line.
>
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	p2wi->slave_addr = -1;
>>> +
>>> +	/*
>>> +	 * Authorize a p2wi node without any children to be able to use an
>>> +	 * i2c-dev from userpace.
>>> +	 * In this case the slave_addr is set to -1 and won't be checked when
>>> +	 * launching a P2WI transfer.
>>> +	 */
>>> +	childnp = of_get_next_available_child(np, NULL);
>>> +	if (childnp) {
>>> +		ret = of_property_read_u32(childnp, "reg", &slave_addr);
>>> +		if (ret || slave_addr > 0xff) {
>> Again: Is 8 bit range important here? Otherwise I'd leave the check to the
>> core.
>>
>>> +			dev_err(dev, "invalid slave address on node %s\n",
>>> +				childnp->full_name);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		p2wi->slave_addr = slave_addr;
>>> +	}
>>> +
>>> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	p2wi->regs = devm_ioremap_resource(dev, r);
>>> +	if (IS_ERR(p2wi->regs)) {
>>> +		ret = PTR_ERR(p2wi->regs);
>>> +		dev_err(dev, "failed to retrieve iomem resource: %d\n", ret);
>> devm_ioremap_resource prints errors on its own.
> Ditto
>
>>> +		return ret;
>>> +	}
>>> +
>>> +	snprintf(p2wi->adapter.name, sizeof(p2wi->adapter.name), pdev->name);
>>> +	ret = platform_get_irq(pdev, 0);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "failed to retrieve irq: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +	p2wi->irq = ret;
>>> +
>>> +	p2wi->clk = devm_clk_get(dev, NULL);
>>> +	if (IS_ERR(p2wi->clk)) {
>>> +		ret = PTR_ERR(p2wi->clk);
>>> +		dev_err(dev, "failed to retrieve clk: %d\n",
>>> +			ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = clk_prepare_enable(p2wi->clk);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to enable clk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	parent_clk_freq = clk_get_rate(p2wi->clk);
>>> +
>>> +	p2wi->rstc = devm_reset_control_get(dev, NULL);
>>> +	if (IS_ERR(p2wi->rstc)) {
>>> +		ret = PTR_ERR(p2wi->rstc);
>>> +		dev_err(dev, "failed to retrieve reset controller: %d\n",
>>> +			ret);
>> My general suggestion: Don't be too strict on the 80 char limit. IMO this dangling
>> 'ret' is not more readable.
> Okay, I'll fix that.
>
>
> Thanks for your review.
>
> Best Regards,
>
> Boris
>

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Boris BREZILLON]

On 10/06/2014 10:54, Boris BREZILLON wrote:
> Hello Wolfram,
>
> On 10/06/2014 10:38, Wolfram Sang wrote:
>> On Tue, Jun 03, 2014 at 10:49:52AM +0200, Boris BREZILLON wrote:
>>> The P2WI looks like an SMBus controller which only supports byte data
>>> transfers. But, it differs from standard SMBus protocol on several
>>> aspects:
>>> - it supports only one slave device, and thus drop the address field
>>> - it adds a parity bit every 8bits of data
>>> - only one read access is required to read a byte (instead of a read
>>>   followed by a write access in standard SMBus protocol)
>>> - there's no Ack bit after each byte transfer
>>>
>>> This means this bus cannot be used to interface with standard SMBus
>>> devices (the only known device to support this interface is the AXP221
>>> PMIC).
>> Good description. Should be a comment at the top of the driver to spread
>> the word.
> Sure, I'll copy this description in the driver.
>
>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
>>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> ---
>>>  drivers/i2c/busses/Kconfig          |  12 ++
>>>  drivers/i2c/busses/Makefile         |   1 +
>>>  drivers/i2c/busses/i2c-sun6i-p2wi.c | 349 ++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 362 insertions(+)
>>>  create mode 100644 drivers/i2c/busses/i2c-sun6i-p2wi.c
>> ...
>>
>>> +struct p2wi {
>>> +	struct i2c_adapter adapter;
>>> +	struct completion complete;
>>> +	unsigned int irq;
>> Can be a local variable in probe.
> Yes, I'll remove it from this structure.
>
>>> +	unsigned int status;
>>> +	void __iomem *regs;
>>> +	struct clk *clk;
>>> +	struct reset_control *rstc;
>>> +	int slave_addr;
>>> +};
>>> +
>>> +static irqreturn_t p2wi_interrupt(int irq, void *dev_id)
>>> +{
>>> +	struct p2wi *p2wi = dev_id;
>>> +	unsigned long status;
>>> +
>>> +	status = readl(p2wi->regs + P2WI_INTS);
>>> +	p2wi->status = status;
>>> +
>>> +	/* Clear interrupts */
>>> +	status &= (P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR |
>>> +		   P2WI_INTS_TRANS_OVER);
>>> +	writel(status, p2wi->regs + P2WI_INTS);
>>> +
>>> +	complete(&p2wi->complete);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static u32 p2wi_functionality(struct i2c_adapter *adap)
>>> +{
>>> +	return I2C_FUNC_SMBUS_BYTE_DATA;
>>> +}
>>> +
>>> +static int p2wi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>>> +			   unsigned short flags, char read_write,
>>> +			   u8 command, int size, union i2c_smbus_data *data)
>>> +{
>>> +	struct p2wi *p2wi = i2c_get_adapdata(adap);
>>> +	unsigned long dlen = P2WI_DLEN_DATA_LENGTH(1);
>>> +
>>> +	if (addr > 0xff ||
>> Why 0xff? Does the PMIC support that? I2C addresses are 7-bit. You
>> won't even have a slave device if it has an illegal i2c address, so this
>> shouldn't happen.
> The P2WI protocol supports 8bits addresses, hence I added this 0xff check.
> Anyway, the PMIC I use (AXP221) is assigned the 0x68 address, and I
> don't think there are a lot of P2WI compatible devices in the wild, so
> we can just assume 7bits addresses are fine and rely on the core code
> checks.

My bad, the P2WI protocol does not have any address concept.
The 0xff value come from the P2WI_PMCR_PMU_DEV_ADDR field which is
specified to be 8 bits large.
Anyway, this does not change the fact that we can remove this check.

>
>>> +	    (p2wi->slave_addr >= 0 && addr != p2wi->slave_addr)) {
>>> +		dev_err(&adap->dev, "invalid P2WI address\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (!data)
>>> +		return -EINVAL;
>>> +
>>> +	writel(command, p2wi->regs + P2WI_DADDR0);
>>> +
>>> +	if (read_write == I2C_SMBUS_READ)
>>> +		dlen |= P2WI_DLEN_READ;
>>> +	else
>>> +		writel(data->byte, p2wi->regs + P2WI_DATA0);
>>> +
>>> +	writel(dlen, p2wi->regs + P2WI_DLEN);
>>> +
>>> +	if (readl(p2wi->regs + P2WI_CTRL) & P2WI_CTRL_START_TRANS) {
>>> +		dev_err(&adap->dev, "P2WI bus busy\n");
>>> +		return -EBUSY;
>>> +	}
>>> +
>>> +	reinit_completion(&p2wi->complete);
>>> +
>>> +	writel(P2WI_INTS_LOAD_BSY | P2WI_INTS_TRANS_ERR | P2WI_INTS_TRANS_OVER,
>>> +	       p2wi->regs + P2WI_INTE);
>>> +
>>> +	writel(P2WI_CTRL_START_TRANS | P2WI_CTRL_GLOBAL_INT_ENB,
>>> +	       p2wi->regs + P2WI_CTRL);
>>> +
>>> +	wait_for_completion(&p2wi->complete);
>>> +
>>> +	if (p2wi->status & P2WI_INTS_LOAD_BSY) {
>>> +		dev_err(&adap->dev, "P2WI bus busy\n");
>>> +		return -EBUSY;
>>> +	}
>>> +
>>> +	if (p2wi->status & P2WI_INTS_TRANS_ERR) {
>>> +		dev_err(&adap->dev, "P2WI bus xfer error\n");
>>> +		return -ENXIO;
>>> +	}
>>> +
>>> +	if (read_write == I2C_SMBUS_READ)
>>> +		data->byte = readl(p2wi->regs + P2WI_DATA0);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct i2c_algorithm p2wi_algo = {
>>> +	.smbus_xfer = p2wi_smbus_xfer,
>>> +	.functionality = p2wi_functionality,
>>> +};
>>> +
>>> +static const struct of_device_id p2wi_of_match_table[] = {
>>> +	{ .compatible = "allwinner,sun6i-a31-p2wi" },
>>> +	{}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, p2wi_of_match_table);
>>> +
>>> +static int p2wi_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct device_node *np = dev->of_node;
>>> +	struct device_node *childnp;
>>> +	unsigned long parent_clk_freq;
>>> +	u32 clk_freq = 100000;
>>> +	struct resource *r;
>>> +	struct p2wi *p2wi;
>>> +	u32 slave_addr;
>>> +	int clk_div;
>>> +	int ret;
>>> +
>>> +	of_property_read_u32(np, "clock-frequency", &clk_freq);
>>> +	if (clk_freq > P2WI_MAX_FREQ) {
>>> +		dev_err(dev,
>>> +			"required clock-frequency (%u Hz) is too high (max = 6MHz)",
>>> +			clk_freq);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (of_get_child_count(np) > 1) {
>>> +		dev_err(dev, "P2WI only supports one slave device\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	p2wi = devm_kzalloc(dev, sizeof(struct p2wi), GFP_KERNEL);
>>> +	if (!p2wi) {
>>> +		dev_err(dev, "failed to allocate p2wi struct\n");
>> No error strings for OOM.
> I'll drop this line.
>
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	p2wi->slave_addr = -1;
>>> +
>>> +	/*
>>> +	 * Authorize a p2wi node without any children to be able to use an
>>> +	 * i2c-dev from userpace.
>>> +	 * In this case the slave_addr is set to -1 and won't be checked when
>>> +	 * launching a P2WI transfer.
>>> +	 */
>>> +	childnp = of_get_next_available_child(np, NULL);
>>> +	if (childnp) {
>>> +		ret = of_property_read_u32(childnp, "reg", &slave_addr);
>>> +		if (ret || slave_addr > 0xff) {
>> Again: Is 8 bit range important here? Otherwise I'd leave the check to the
>> core.
>>
>>> +			dev_err(dev, "invalid slave address on node %s\n",
>>> +				childnp->full_name);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		p2wi->slave_addr = slave_addr;
>>> +	}
>>> +
>>> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	p2wi->regs = devm_ioremap_resource(dev, r);
>>> +	if (IS_ERR(p2wi->regs)) {
>>> +		ret = PTR_ERR(p2wi->regs);
>>> +		dev_err(dev, "failed to retrieve iomem resource: %d\n", ret);
>> devm_ioremap_resource prints errors on its own.
> Ditto
>
>>> +		return ret;
>>> +	}
>>> +
>>> +	snprintf(p2wi->adapter.name, sizeof(p2wi->adapter.name), pdev->name);
>>> +	ret = platform_get_irq(pdev, 0);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "failed to retrieve irq: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +	p2wi->irq = ret;
>>> +
>>> +	p2wi->clk = devm_clk_get(dev, NULL);
>>> +	if (IS_ERR(p2wi->clk)) {
>>> +		ret = PTR_ERR(p2wi->clk);
>>> +		dev_err(dev, "failed to retrieve clk: %d\n",
>>> +			ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = clk_prepare_enable(p2wi->clk);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to enable clk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	parent_clk_freq = clk_get_rate(p2wi->clk);
>>> +
>>> +	p2wi->rstc = devm_reset_control_get(dev, NULL);
>>> +	if (IS_ERR(p2wi->rstc)) {
>>> +		ret = PTR_ERR(p2wi->rstc);
>>> +		dev_err(dev, "failed to retrieve reset controller: %d\n",
>>> +			ret);
>> My general suggestion: Don't be too strict on the 80 char limit. IMO this dangling
>> 'ret' is not more readable.
> Okay, I'll fix that.
>
>
> Thanks for your review.
>
> Best Regards,
>
> Boris
>

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Boris BREZILLON]

Hello Paul,

On 10/06/2014 10:56, Paul Carpenter wrote:
> Wolfram Sang wrote:
>> On Tue, Jun 03, 2014 at 10:49:52AM +0200, Boris BREZILLON wrote:
>>> The P2WI looks like an SMBus controller which only supports byte data
>>> transfers. But, it differs from standard SMBus protocol on several
>>> aspects:
>>> - it supports only one slave device, and thus drop the address field
>>> - it adds a parity bit every 8bits of data
>>> - only one read access is required to read a byte (instead of a read
>>>   followed by a write access in standard SMBus protocol)
>
> Minor quibble should be
> "(instead of a write followed by a read access in standard SMBus
>   protocol)"
>

Fixed.

Thanks for pointing this out.

Best Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RESEND2 PATCH v4 2/2] i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support

[Boris BREZILLON]

Hello Paul,

On 10/06/2014 10:56, Paul Carpenter wrote:
> Wolfram Sang wrote:
>> On Tue, Jun 03, 2014 at 10:49:52AM +0200, Boris BREZILLON wrote:
>>> The P2WI looks like an SMBus controller which only supports byte data
>>> transfers. But, it differs from standard SMBus protocol on several
>>> aspects:
>>> - it supports only one slave device, and thus drop the address field
>>> - it adds a parity bit every 8bits of data
>>> - only one read access is required to read a byte (instead of a read
>>>   followed by a write access in standard SMBus protocol)
>
> Minor quibble should be
> "(instead of a write followed by a read access in standard SMBus
>   protocol)"
>

Fixed.

Thanks for pointing this out.

Best Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com