[PATCH v3 0/5] ARM: sun6i: Add support for the A31 I2C controller

[PATCH v3 0/5] ARM: sun6i: Add support for the A31 I2C controller

[Maxime Ripard]

Hi everyone,

This patchset adds support the A31 i2c controller. This is mostly the
same controller as the one found in the other Allwinner SoCs, except
for the interrupts acking.

On the other SoCs using this driver, the interrupts are acked by
clearing the INT_FLAG bit in the control register, while on the A31,
the interrupt is acked by writing that bit into the control register.

The other difference is that the I2C IP is maintained in reset by a
reset controller, so we're adding optionnal support for the reset
framework in the driver to deassert the device from reset.

Thanks!
Maxime

Changes from v2:
  - Rebased on top of v3.14-rc5
  - Added Gregory Clement's Reviewed-by and Tested-by tags

Changes from v1:
  - Handle EPROBE_DEFER from the reset framework
  - Put the device back in reset at remove/failed probe
  - Document the newly introduced compatible string

Maxime Ripard (5):
  i2c: mv64xxx: Add reset deassert call
  i2c: mv64xxx: Add support for the Allwinner A31 I2C driver
  ARM: sun6i: Enable the I2C controllers
  ARM: sun6i: Enable the I2C muxing options
  ARM: sun6i: colombus: Enable the I2C controllers

 .../devicetree/bindings/i2c/i2c-mv64xxx.txt        | 20 ++++---
 arch/arm/boot/dts/sun6i-a31-colombus.dts           | 18 +++++++
 arch/arm/boot/dts/sun6i-a31.dtsi                   | 61 ++++++++++++++++++++++
 drivers/i2c/busses/Kconfig                         |  1 +
 drivers/i2c/busses/i2c-mv64xxx.c                   | 32 +++++++++++-
 5 files changed, 124 insertions(+), 8 deletions(-)

-- 
1.9.0

[PATCH v3 1/5] i2c: mv64xxx: Add reset deassert call

[Maxime Ripard]

The Allwinner A31 SoC using that IP has a reset controller maintaining
it reset unless told otherwise.

Add some optional reset support to the driver.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 .../devicetree/bindings/i2c/i2c-mv64xxx.txt         |  1 +
 drivers/i2c/busses/Kconfig                          |  1 +
 drivers/i2c/busses/i2c-mv64xxx.c                    | 21 +++++++++++++++++++--
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index 582b465..21062bc 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -16,6 +16,7 @@ Optional properties :
 
  - clock-frequency : Desired I2C bus clock frequency in Hz. If not set the
 default frequency is 100kHz
+ - resets          : phandle to the parent reset controller
 
 Examples:
 
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index f5ed031..70bcad9 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -528,6 +528,7 @@ config I2C_MPC
 config I2C_MV64XXX
 	tristate "Marvell mv64xxx I2C Controller"
 	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
+	select RESET_CONTROLLER
 	help
 	  If you say yes to this option, support will be included for the
 	  built-in I2C interface on the Marvell 64xxx line of host bridges.
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index d52d849..1bb69b6 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -17,6 +17,7 @@
 #include <linux/interrupt.h>
 #include <linux/mv643xx_i2c.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -148,6 +149,7 @@ struct mv64xxx_i2c_data {
 	bool			offload_enabled;
 /* 5us delay in order to avoid repeated start timing violation */
 	bool			errata_delay;
+	struct reset_control	*rstc;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -759,6 +761,16 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 	}
 	drv_data->irq = irq_of_parse_and_map(np, 0);
 
+	drv_data->rstc = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(drv_data->rstc)) {
+		if (PTR_ERR(drv_data->rstc) == -EPROBE_DEFER) {
+			rc = -EPROBE_DEFER;
+			goto out;
+		}
+	} else {
+		reset_control_deassert(drv_data->rstc);
+	}
+
 	/* Its not yet defined how timeouts will be specified in device tree.
 	 * So hard code the value to 1 second.
 	 */
@@ -845,7 +857,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 	}
 	if (drv_data->irq < 0) {
 		rc = -ENXIO;
-		goto exit_clk;
+		goto exit_reset;
 	}
 
 	drv_data->adapter.dev.parent = &pd->dev;
@@ -865,7 +877,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 		dev_err(&drv_data->adapter.dev,
 			"mv64xxx: Can't register intr handler irq%d: %d\n",
 			drv_data->irq, rc);
-		goto exit_clk;
+		goto exit_reset;
 	} else if ((rc = i2c_add_numbered_adapter(&drv_data->adapter)) != 0) {
 		dev_err(&drv_data->adapter.dev,
 			"mv64xxx: Can't add i2c adapter, rc: %d\n", -rc);
@@ -876,6 +888,9 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 
 exit_free_irq:
 	free_irq(drv_data->irq, drv_data);
+exit_reset:
+	if (pd->dev.of_node && !IS_ERR(drv_data->rstc))
+		reset_control_assert(drv_data->rstc);
 exit_clk:
 #if defined(CONFIG_HAVE_CLK)
 	/* Not all platforms have a clk */
@@ -894,6 +909,8 @@ mv64xxx_i2c_remove(struct platform_device *dev)
 
 	i2c_del_adapter(&drv_data->adapter);
 	free_irq(drv_data->irq, drv_data);
+	if (dev->dev.of_node && !IS_ERR(drv_data->rstc))
+		reset_control_assert(drv_data->rstc);
 #if defined(CONFIG_HAVE_CLK)
 	/* Not all platforms have a clk */
 	if (!IS_ERR(drv_data->clk)) {
-- 
1.9.0

[PATCH v3 2/5] i2c: mv64xxx: Add support for the Allwinner A31 I2C driver

[Maxime Ripard]

The Allwinner A31 I2C controller is almost identical to the one used in the
other Allwinner SoCs, except for the fact that it needs to clear the interrupt
by setting the INT_FLAGS bit in the control register, instead of clearing it.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 .../devicetree/bindings/i2c/i2c-mv64xxx.txt         | 21 ++++++++++++++-------
 drivers/i2c/busses/i2c-mv64xxx.c                    | 11 +++++++++++
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index 21062bc..befd4fb 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -4,19 +4,26 @@
 Required properties :
 
  - reg             : Offset and length of the register set for the device
- - compatible      : Should be "marvell,mv64xxx-i2c" or "allwinner,sun4i-i2c"
-                     or "marvell,mv78230-i2c" or "marvell,mv78230-a0-i2c"
-                     Note: Only use "marvell,mv78230-a0-i2c" for a very rare,
-                     initial version of the SoC which had broken offload
-                     support.  Linux auto-detects this and sets it
-                     appropriately.
+ - compatible      : Should be either:
+                     - "allwinner,sun4i-i2c"
+                     - "allwinner,sun6i-a31-i2c"
+                     - "marvell,mv64xxx-i2c"
+                     - "marvell,mv78230-i2c"
+                     - "marvell,mv78230-a0-i2c"
+                       * Note: Only use "marvell,mv78230-a0-i2c" for a
+                         very rare, initial version of the SoC which
+                         had broken offload support.  Linux
+                         auto-detects this and sets it appropriately.
  - interrupts      : The interrupt number
 
 Optional properties :
 
  - clock-frequency : Desired I2C bus clock frequency in Hz. If not set the
 default frequency is 100kHz
- - resets          : phandle to the parent reset controller
+
+ - resets          : phandle to the parent reset controller. Mandatory
+                     whenever you're using the "allwinner,sun6i-a31-i2c"
+                     compatible.
 
 Examples:
 
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 1bb69b6..203a548 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -150,6 +150,7 @@ struct mv64xxx_i2c_data {
 /* 5us delay in order to avoid repeated start timing violation */
 	bool			errata_delay;
 	struct reset_control	*rstc;
+	bool			irq_clear_inverted;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -568,6 +569,11 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
 		status = readl(drv_data->reg_base + drv_data->reg_offsets.status);
 		mv64xxx_i2c_fsm(drv_data, status);
 		mv64xxx_i2c_do_action(drv_data);
+
+		if (drv_data->irq_clear_inverted)
+			writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_IFLG,
+			       drv_data->reg_base + drv_data->reg_offsets.control);
+
 		rc = IRQ_HANDLED;
 	}
 	spin_unlock_irqrestore(&drv_data->lock, flags);
@@ -687,6 +693,7 @@ static const struct i2c_algorithm mv64xxx_i2c_algo = {
  */
 static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
 	{ .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
+	{ .compatible = "allwinner,sun6i-a31-i2c", .data = &mv64xxx_i2c_regs_sun4i},
 	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
 	{ .compatible = "marvell,mv78230-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
 	{ .compatible = "marvell,mv78230-a0-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
@@ -795,6 +802,10 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 		drv_data->offload_enabled = false;
 		drv_data->errata_delay = true;
 	}
+
+	if (of_device_is_compatible(np, "allwinner,sun6i-a31-i2c"))
+		drv_data->irq_clear_inverted = true;
+
 out:
 	return rc;
 #endif
-- 
1.9.0

[PATCH v3 3/5] ARM: sun6i: Enable the I2C controllers

[Maxime Ripard]

The A31 has 4 I2C controllers that are the same than the one in the
other Allwinner SoCs, except for the fact that they are asserted in
reset by the reset unit.

Add these i2c controllers to the DTSI.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index af6f87c..22d3eae 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -359,6 +359,46 @@
 			status = "disabled";
 		};
 
+		i2c0: i2c at 01c2ac00 {
+			compatible = "allwinner,sun6i-a31-i2c";
+			reg = <0x01c2ac00 0x400>;
+			interrupts = <0 6 4>;
+			clocks = <&apb2_gates 0>;
+			clock-frequency = <100000>;
+			resets = <&apb2_rst 0>;
+			status = "disabled";
+		};
+
+		i2c1: i2c at 01c2b000 {
+			compatible = "allwinner,sun6i-a31-i2c";
+			reg = <0x01c2b000 0x400>;
+			interrupts = <0 7 4>;
+			clocks = <&apb2_gates 1>;
+			clock-frequency = <100000>;
+			resets = <&apb2_rst 1>;
+			status = "disabled";
+		};
+
+		i2c2: i2c at 01c2b400 {
+			compatible = "allwinner,sun6i-a31-i2c";
+			reg = <0x01c2b400 0x400>;
+			interrupts = <0 8 4>;
+			clocks = <&apb2_gates 2>;
+			clock-frequency = <100000>;
+			resets = <&apb2_rst 2>;
+			status = "disabled";
+		};
+
+		i2c3: i2c at 01c2b800 {
+			compatible = "allwinner,sun6i-a31-i2c";
+			reg = <0x01c2b800 0x400>;
+			interrupts = <0 9 4>;
+			clocks = <&apb2_gates 3>;
+			clock-frequency = <100000>;
+			resets = <&apb2_rst 3>;
+			status = "disabled";
+		};
+
 		spi0: spi at 01c68000 {
 			compatible = "allwinner,sun6i-a31-spi";
 			reg = <0x01c68000 0x1000>;
-- 
1.9.0

[PATCH v3 4/5] ARM: sun6i: Enable the I2C muxing options

[Maxime Ripard]

The i2c controllers have a few muxing options on the A31. Enable the
ones found in the A31 Colombus board.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 22d3eae..8d3ea29 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -257,6 +257,27 @@
 				allwinner,drive = <0>;
 				allwinner,pull = <0>;
 			};
+
+			i2c0_pins_a: i2c0 at 0 {
+				allwinner,pins = "PH14", "PH15";
+				allwinner,function = "i2c0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			i2c1_pins_a: i2c1 at 0 {
+				allwinner,pins = "PH16", "PH17";
+				allwinner,function = "i2c1";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			i2c2_pins_a: i2c2 at 0 {
+				allwinner,pins = "PH18", "PH19";
+				allwinner,function = "i2c2";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
 		};
 
 		ahb1_rst: reset at 01c202c0 {
-- 
1.9.0

[PATCH v3 5/5] ARM: sun6i: colombus: Enable the I2C controllers

[Maxime Ripard]

The A31 Colombus board has 3 I2C controllers that should be usable. However,
the first one is not working for some reason on the hardware I have been able
to test it on, while it should really be the same controller. Enable the i2c1
and i2c2 busses, and mark i2c0 as in failure in the DT.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun6i-a31-colombus.dts | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31-colombus.dts b/arch/arm/boot/dts/sun6i-a31-colombus.dts
index e5adae3..3898a7b 100644
--- a/arch/arm/boot/dts/sun6i-a31-colombus.dts
+++ b/arch/arm/boot/dts/sun6i-a31-colombus.dts
@@ -28,5 +28,23 @@
 			pinctrl-0 = <&uart0_pins_a>;
 			status = "okay";
 		};
+
+		i2c0: i2c at 01c2ac00 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2c0_pins_a>;
+			status = "fail";
+		};
+
+		i2c1: i2c at 01c2b000 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2c1_pins_a>;
+			status = "okay";
+		};
+
+		i2c2: i2c at 01c2b400 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2c2_pins_a>;
+			status = "okay";
+		};
 	};
 };
-- 
1.9.0

[PATCH v3 1/5] i2c: mv64xxx: Add reset deassert call

[Wolfram Sang]

On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
> The Allwinner A31 SoC using that IP has a reset controller maintaining
> it reset unless told otherwise.
> 
> Add some optional reset support to the driver.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Applied to for-next, thanks!

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

[PATCH v3 2/5] i2c: mv64xxx: Add support for the Allwinner A31 I2C driver

[Wolfram Sang]

On Tue, Mar 04, 2014 at 05:28:38PM +0100, Maxime Ripard wrote:
> The Allwinner A31 I2C controller is almost identical to the one used in the
> other Allwinner SoCs, except for the fact that it needs to clear the interrupt
> by setting the INT_FLAGS bit in the control register, instead of clearing it.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Applied to for-next, thanks!

Still...

> +	if (of_device_is_compatible(np, "allwinner,sun6i-a31-i2c"))
> +		drv_data->irq_clear_inverted = true;

... next time an errata is needed, I think it makes sense to refactor all
these checks into one struct which can be used as match->data directly.

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

[PATCH v3 1/5] i2c: mv64xxx: Add reset deassert call

[Russell King - ARM Linux]

On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
> The Allwinner A31 SoC using that IP has a reset controller maintaining
> it reset unless told otherwise.
> 
> Add some optional reset support to the driver.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

This appears to be causing some build errors in Olof's next builder
for many of the ARM platforms which make use of this:

drivers/i2c/busses/i2c-mv64xxx.c:924: undefined reference to `reset_control_assert'
drivers/i2c/busses/i2c-mv64xxx.c:904: undefined reference to `reset_control_assert'
drivers/i2c/busses/i2c-mv64xxx.c:771: undefined reference to `devm_reset_control_get'
drivers/i2c/busses/i2c-mv64xxx.c:778: undefined reference to `reset_control_deassert'

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH v3 1/5] i2c: mv64xxx: Add reset deassert call

[Maxime Ripard]

Hi Russell,

On Fri, Mar 07, 2014 at 09:52:23AM +0000, Russell King - ARM Linux wrote:
> On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
> > The Allwinner A31 SoC using that IP has a reset controller maintaining
> > it reset unless told otherwise.
> > 
> > Add some optional reset support to the driver.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> 
> This appears to be causing some build errors in Olof's next builder
> for many of the ARM platforms which make use of this:
> 
> drivers/i2c/busses/i2c-mv64xxx.c:924: undefined reference to `reset_control_assert'
> drivers/i2c/busses/i2c-mv64xxx.c:904: undefined reference to `reset_control_assert'
> drivers/i2c/busses/i2c-mv64xxx.c:771: undefined reference to `devm_reset_control_get'
> drivers/i2c/busses/i2c-mv64xxx.c:778: undefined reference to `reset_control_deassert'

The reset framework doesn't define its functions when its not
selected, and somehow I think it was not here. What's odd is that
there is an explicit select on RESET_CONTROLLER in the Kconfig. Maybe
it's the circular dependency issue that has been reported that cause
this and Wolfram sent a patch for: http://patchwork.ozlabs.org/patch/327573/

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140307/5ce19d1e/attachment.sig>

[PATCH v3 2/5] i2c: mv64xxx: Add support for the Allwinner A31 I2C driver

[Maxime Ripard]

Hi Wolfram,

On Wed, Mar 05, 2014 at 05:34:30PM +0100, Wolfram Sang wrote:
> On Tue, Mar 04, 2014 at 05:28:38PM +0100, Maxime Ripard wrote:
> > The Allwinner A31 I2C controller is almost identical to the one used in the
> > other Allwinner SoCs, except for the fact that it needs to clear the interrupt
> > by setting the INT_FLAGS bit in the control register, instead of clearing it.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> 
> Applied to for-next, thanks!
> 
> Still...
> 
> > +	if (of_device_is_compatible(np, "allwinner,sun6i-a31-i2c"))
> > +		drv_data->irq_clear_inverted = true;
> 
> ... next time an errata is needed, I think it makes sense to refactor all
> these checks into one struct which can be used as match->data directly.

Yep, it makes sense. Thanks!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140307/9394b6de/attachment.sig>

[PATCH v3 1/5] i2c: mv64xxx: Add reset deassert call

[Russell King - ARM Linux]

On Fri, Mar 07, 2014 at 11:07:51AM +0100, Maxime Ripard wrote:
> Hi Russell,
> 
> On Fri, Mar 07, 2014 at 09:52:23AM +0000, Russell King - ARM Linux wrote:
> > On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
> > > The Allwinner A31 SoC using that IP has a reset controller maintaining
> > > it reset unless told otherwise.
> > > 
> > > Add some optional reset support to the driver.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > > Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > 
> > This appears to be causing some build errors in Olof's next builder
> > for many of the ARM platforms which make use of this:
> > 
> > drivers/i2c/busses/i2c-mv64xxx.c:924: undefined reference to `reset_control_assert'
> > drivers/i2c/busses/i2c-mv64xxx.c:904: undefined reference to `reset_control_assert'
> > drivers/i2c/busses/i2c-mv64xxx.c:771: undefined reference to `devm_reset_control_get'
> > drivers/i2c/busses/i2c-mv64xxx.c:778: undefined reference to `reset_control_deassert'
> 
> The reset framework doesn't define its functions when its not
> selected, and somehow I think it was not here. What's odd is that
> there is an explicit select on RESET_CONTROLLER in the Kconfig. Maybe
> it's the circular dependency issue that has been reported that cause
> this and Wolfram sent a patch for: http://patchwork.ozlabs.org/patch/327573/

If that patch has been taken, then yes, it will have caused the above -
because now we have Dove and Kirkwood platforms trying to build this
driver without RESET_CONTROLLER being set.

The problem with depending on RESET_CONTROLLER is that then these
platforms end up without their I2C controller - because there's nothing
which enables RESET_CONTROLLER in their configuration.

Since RESET_CONTROLLER is not required for those platforms, it really
should be optional - and I think the real fix is for the reset controller
support to provide stub functions.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH v3 1/5] i2c: mv64xxx: Add reset deassert call

[Wolfram Sang]

> Since RESET_CONTROLLER is not required for those platforms, it really
> should be optional - and I think the real fix is for the reset controller
> support to provide stub functions.

I agree; and I assumed it already does. Will resend a patch posted
earlier [1] to fix the issue. And still debugging why my build-tests
didn't get it :(

[1] https://lkml.org/lkml/2013/8/9/442

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

[linux-sunxi] Re: [PATCH v3 1/5] i2c: mv64xxx: Add reset deassert call

[Chen-Yu Tsai]

On Fri, Mar 7, 2014 at 6:34 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Mar 07, 2014 at 11:07:51AM +0100, Maxime Ripard wrote:
>> Hi Russell,
>>
>> On Fri, Mar 07, 2014 at 09:52:23AM +0000, Russell King - ARM Linux wrote:
>> > On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
>> > > The Allwinner A31 SoC using that IP has a reset controller maintaining
>> > > it reset unless told otherwise.
>> > >
>> > > Add some optional reset support to the driver.
>> > >
>> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> > > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> > > Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> >
>> > This appears to be causing some build errors in Olof's next builder
>> > for many of the ARM platforms which make use of this:
>> >
>> > drivers/i2c/busses/i2c-mv64xxx.c:924: undefined reference to `reset_control_assert'
>> > drivers/i2c/busses/i2c-mv64xxx.c:904: undefined reference to `reset_control_assert'
>> > drivers/i2c/busses/i2c-mv64xxx.c:771: undefined reference to `devm_reset_control_get'
>> > drivers/i2c/busses/i2c-mv64xxx.c:778: undefined reference to `reset_control_deassert'
>>
>> The reset framework doesn't define its functions when its not
>> selected, and somehow I think it was not here. What's odd is that
>> there is an explicit select on RESET_CONTROLLER in the Kconfig. Maybe
>> it's the circular dependency issue that has been reported that cause
>> this and Wolfram sent a patch for: http://patchwork.ozlabs.org/patch/327573/
>
> If that patch has been taken, then yes, it will have caused the above -
> because now we have Dove and Kirkwood platforms trying to build this
> driver without RESET_CONTROLLER being set.
>
> The problem with depending on RESET_CONTROLLER is that then these
> platforms end up without their I2C controller - because there's nothing
> which enables RESET_CONTROLLER in their configuration.
>
> Since RESET_CONTROLLER is not required for those platforms, it really
> should be optional - and I think the real fix is for the reset controller
> support to provide stub functions.

Philipp Zabel suggested that adding a _optional variant that provides stubs
and doesn't depend on RESET_CONTROLLER is probably better. This keeps the
compile time checks for drivers requiring it.

See: https://lkml.org/lkml/2014/1/10/220

I ended up dropping my patch though.


Cheers,
ChenYu

[linux-sunxi] Re: [PATCH v3 1/5] i2c: mv64xxx: Add reset deassert call

[Wolfram Sang]

> > Since RESET_CONTROLLER is not required for those platforms, it really
> > should be optional - and I think the real fix is for the reset controller
> > support to provide stub functions.
> 
> Philipp Zabel suggested that adding a _optional variant that provides stubs
> and doesn't depend on RESET_CONTROLLER is probably better. This keeps the
> compile time checks for drivers requiring it.
> 
> See: https://lkml.org/lkml/2014/1/10/220
> 
> I ended up dropping my patch though.

Thanks for the pointer. Well, looks like I need to revert the offending
i2c patches then until this issue is fixed? We can't have
RESET_CONTROLLER (circular dependency) and we can't skip it (build
failures).

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

[PATCH] i2c: mv64xxx: Fix circular dependencies warning and compilation breakage

[Maxime Ripard]

This patch fixes the circular dependency introduced by commit 370136bc67c3
("i2c: mv64xxx: Add reset deassert call"):

drivers/video/Kconfig:42:error: recursive dependency detected!

Since the reset framework doesn't define dummy stubs whenever
CONFIG_RESET_CONTROLLER is not defined, it's the only sane way to have a driver
that compiles in any cases.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/i2c/busses/Kconfig       |  1 -
 drivers/i2c/busses/i2c-mv64xxx.c | 22 +++++++++++++---------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 70bcad9..f5ed031 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -528,7 +528,6 @@ config I2C_MPC
 config I2C_MV64XXX
 	tristate "Marvell mv64xxx I2C Controller"
 	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
-	select RESET_CONTROLLER
 	help
 	  If you say yes to this option, support will be included for the
 	  built-in I2C interface on the Marvell 64xxx line of host bridges.
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 203a548..a1dc99b 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -768,14 +768,16 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 	}
 	drv_data->irq = irq_of_parse_and_map(np, 0);
 
-	drv_data->rstc = devm_reset_control_get(dev, NULL);
-	if (IS_ERR(drv_data->rstc)) {
-		if (PTR_ERR(drv_data->rstc) == -EPROBE_DEFER) {
-			rc = -EPROBE_DEFER;
-			goto out;
+	if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) {
+		drv_data->rstc = devm_reset_control_get(dev, NULL);
+		if (IS_ERR(drv_data->rstc)) {
+			if (PTR_ERR(drv_data->rstc) == -EPROBE_DEFER) {
+				rc = -EPROBE_DEFER;
+				goto out;
+			}
+		} else {
+			reset_control_deassert(drv_data->rstc);
 		}
-	} else {
-		reset_control_deassert(drv_data->rstc);
 	}
 
 	/* Its not yet defined how timeouts will be specified in device tree.
@@ -900,7 +902,8 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 exit_free_irq:
 	free_irq(drv_data->irq, drv_data);
 exit_reset:
-	if (pd->dev.of_node && !IS_ERR(drv_data->rstc))
+	if (pd->dev.of_node && IS_ENABLED(CONFIG_RESET_CONTROLLER) &&
+	    !IS_ERR(drv_data->rstc))
 		reset_control_assert(drv_data->rstc);
 exit_clk:
 #if defined(CONFIG_HAVE_CLK)
@@ -920,7 +923,8 @@ mv64xxx_i2c_remove(struct platform_device *dev)
 
 	i2c_del_adapter(&drv_data->adapter);
 	free_irq(drv_data->irq, drv_data);
-	if (dev->dev.of_node && !IS_ERR(drv_data->rstc))
+	if (dev->dev.of_node && IS_ENABLED(CONFIG_RESET_CONTROLLER) &&
+	    !IS_ERR(drv_data->rstc))
 		reset_control_assert(drv_data->rstc);
 #if defined(CONFIG_HAVE_CLK)
 	/* Not all platforms have a clk */
-- 
1.9.0

[linux-sunxi] Re: [PATCH v3 1/5] i2c: mv64xxx: Add reset deassert call

[Maxime Ripard]

Hi,

On Fri, Mar 07, 2014 at 12:18:58PM +0100, Wolfram Sang wrote:
> 
> > > Since RESET_CONTROLLER is not required for those platforms, it really
> > > should be optional - and I think the real fix is for the reset controller
> > > support to provide stub functions.
> > 
> > Philipp Zabel suggested that adding a _optional variant that provides stubs
> > and doesn't depend on RESET_CONTROLLER is probably better. This keeps the
> > compile time checks for drivers requiring it.
> > 
> > See: https://lkml.org/lkml/2014/1/10/220
> > 
> > I ended up dropping my patch though.
> 
> Thanks for the pointer. Well, looks like I need to revert the offending
> i2c patches then until this issue is fixed? We can't have
> RESET_CONTROLLER (circular dependency) and we can't skip it (build
> failures).
> 

I just sent a fix in reply to your mail that should fix the issue
without having to revert the patches.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140307/6635e2ba/attachment.sig>

[PATCH] i2c: mv64xxx: Fix circular dependencies warning and compilation breakage

[Wolfram Sang]

New patch -> new thread, please.

> This patch fixes the circular dependency introduced by commit 370136bc67c3
> ("i2c: mv64xxx: Add reset deassert call"):
> 
> drivers/video/Kconfig:42:error: recursive dependency detected!

Please base it on i2c-next. I already applied my patch.
Your patch fixes the build error discovered by it.

> Since the reset framework doesn't define dummy stubs whenever
> CONFIG_RESET_CONTROLLER is not defined, it's the only sane way to have a driver
> that compiles in any cases.

Paragraph needs reformat. And please drop "sane". #ifdefs are not sane.
Fixing the reset framework would be sane.

> @@ -900,7 +902,8 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>  exit_free_irq:
>  	free_irq(drv_data->irq, drv_data);
>  exit_reset:
> -	if (pd->dev.of_node && !IS_ERR(drv_data->rstc))
> +	if (pd->dev.of_node && IS_ENABLED(CONFIG_RESET_CONTROLLER) &&
> +	    !IS_ERR(drv_data->rstc))

Why don't you simply set rstc to some ERR_PTR above if not
RESET_CONTROLLER?

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

[PATCH] i2c: mv64xxx: Fix circular dependencies warning and compilation breakage

[Wolfram Sang]

> Why don't you simply set rstc to some ERR_PTR above if not
> RESET_CONTROLLER?

I know why. Sorry for the noise.


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

[PATCH] i2c: mv64xxx: Fix circular dependencies warning and compilation breakage

[Maxime Ripard]

On Fri, Mar 07, 2014 at 03:24:05PM +0100, Wolfram Sang wrote:
> 
> New patch -> new thread, please.
> 
> > This patch fixes the circular dependency introduced by commit 370136bc67c3
> > ("i2c: mv64xxx: Add reset deassert call"):
> > 
> > drivers/video/Kconfig:42:error: recursive dependency detected!
> 
> Please base it on i2c-next. I already applied my patch.
> Your patch fixes the build error discovered by it.

I didn't realise that. I will.

> > Since the reset framework doesn't define dummy stubs whenever
> > CONFIG_RESET_CONTROLLER is not defined, it's the only sane way to have a driver
> > that compiles in any cases.
> 
> Paragraph needs reformat. And please drop "sane". #ifdefs are not sane.
> Fixing the reset framework would be sane.

Ok..

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140307/c66db7b9/attachment.sig>