[PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Doug Anderson]

The i2c-arbitrator driver implements the arbitration scheme that the
Embedded Controller (EC) on the ARM Chromebook expects to use for bus
multimastering.  This i2c-arbitrator driver could also be used in
other places where standard i2c bus arbitration can't be used and two
extra GPIOs are available for arbitration.

This driver is based on code that Simon Glass added to the i2c-s3c2410
driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
mux driver is as suggested by Grant Likely.  See
<https://patchwork.kernel.org/patch/1877311/> for some history.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
Changes in v1: None

 .../devicetree/bindings/i2c/i2c-arbitrator.txt     |  76 +++++++
 drivers/i2c/muxes/Kconfig                          |  11 +
 drivers/i2c/muxes/Makefile                         |   1 +
 drivers/i2c/muxes/i2c-arbitrator.c                 | 242 +++++++++++++++++++++
 4 files changed, 330 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
 create mode 100644 drivers/i2c/muxes/i2c-arbitrator.c

diff --git a/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
new file mode 100644
index 0000000..bb9cca7
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
@@ -0,0 +1,76 @@
+GPIO-based Arbitration
+======================
+This uses GPIO lines between the AP (Application Processor) and an attached
+EC (Embedded Controller) which both want to talk on the same I2C bus as master.
+
+The AP and EC each have a 'bus claim' line, which is an output that the
+other can see. These are both active low, with pull-ups enabled.
+
+- AP_CLAIM: output from AP, signalling to the EC that the AP wants the bus
+- EC_CLAIM: output from EC, signalling to the AP that the EC wants the bus
+
+
+This mechanism is used instead of standard i2c multimaster to avoid some of the
+subtle driver and silicon bugs that are often present with i2c multimaster.
+
+
+Algorithm:
+
+The basic algorithm is to assert your line when you want the bus, then make
+sure that the other side doesn't want it also. A detailed explanation is best
+done with an example.
+
+Let's say the AP wants to claim the bus. It:
+1. Asserts AP_CLAIM.
+2. Waits a little bit for the other side to notice (slew time, say 10
+   microseconds).
+3. Checks EC_CLAIM. If this is not asserted then the AP has the bus and we are
+   done.
+4. Otherwise, wait for a few milliseconds and see if EC_CLAIM is released.
+5. If not, back off, release the claim and wait for a few more milliseconds.
+6. Go back to 1 (until retry time has expired).
+
+
+Required properties:
+- compatible: i2c-arbitrator
+- bus-arbitration-gpios: Two GPIOs to use with the GPIO-based bus
+    arbitration protocol.  The first should be an output, and is used to
+    claim the I2C bus for us (AP_CLAIM).  The second should be an input and
+    signals that the other side wants to claim the bus (EC_CLAIM).
+- bus-arbitration-slew-delay-us: microseconds to wait for a GPIO to go high.
+- bus-arbitration-wait-retry-us: we'll attempt another claim after this many
+    microseconds.
+- bus-arbitration-wait-free-us: we'll give up after this many microseconds.
+- Standard I2C mux properties. See mux.txt in this directory.
+- Single I2C child bus node at reg 0. See mux.txt in this directory.
+
+
+Example:
+	i2c@12CA0000 {
+		compatible = "acme,some-i2c-device";
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+	i2c-arbitrator {
+		compatible = "i2c-arbitrator";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c-parent = <&{/i2c@12CA0000}>;
+
+		bus-arbitration-gpios = <&gpf0 3 1 0 0>, <&gpe0 4 0 3 0>;
+		bus-arbitration-slew-delay-us = <10>;
+		bus-arbitration-wait-retry-us = <3000>;
+		bus-arbitration-wait-free-us = <50000>;
+
+		i2c@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			i2c@52 {
+				// Normal i2c device
+			};
+		};
+	};
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 0be5b83..714bf16 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -5,6 +5,17 @@
 menu "Multiplexer I2C Chip support"
 	depends on I2C_MUX
 
+config I2C_ARBITRATOR
+	tristate "GPIO-based I2C arbitrator"
+	depends on GENERIC_GPIO && OF
+	help
+	  If you say yes to this option, support will be included for an
+	  i2c multimaster arbitration scheme using GPIOs (as opposed to
+	  using standard i2c multimaster mode).
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-arbitrator.
+
 config I2C_MUX_GPIO
 	tristate "GPIO-based I2C multiplexer"
 	depends on GENERIC_GPIO
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 76da869..adfe147 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -1,6 +1,7 @@
 #
 # Makefile for multiplexer I2C chip drivers.
 
+obj-$(CONFIG_I2C_ARBITRATOR)	+= i2c-arbitrator.o
 obj-$(CONFIG_I2C_MUX_GPIO)	+= i2c-mux-gpio.o
 obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
 obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c
new file mode 100644
index 0000000..c3bbdf7
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-arbitrator.c
@@ -0,0 +1,242 @@
+/*
+ * I2C arbitrator using GPIO API
+ *
+ * Copyright (C) 2012 Google, Inc
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/i2c.h>
+#include <linux/i2c-mux.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/of_i2c.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+
+enum {
+	I2C_ARB_GPIO_AP,		/* AP claims i2c bus */
+	I2C_ARB_GPIO_EC,		/* EC claims i2c bus */
+
+	I2C_ARB_GPIO_COUNT,
+};
+
+/**
+ * struct i2c_arbitrator_data - Driver data for i2c arbitrator
+ *
+ * @parent: Parent adapter
+ * @child: Child bus
+ * @ap_gpio: GPIO we'll use to claim.
+ * @ec_gpio: GPIO that the other side will use to claim.
+ * @slew_delay_us: microseconds to wait for a GPIO to go high.
+ * @wait_retry_us: we'll attempt another claim after this many microseconds.
+ * @wait_free_us: we'll give up after this many microseconds.
+ */
+
+struct i2c_arbitrator_data {
+	struct i2c_adapter *parent;
+	struct i2c_adapter *child;
+
+	int		ap_gpio;
+	int		ec_gpio;
+	unsigned int	slew_delay_us;
+	unsigned int	wait_retry_us;
+	unsigned int	wait_free_us;
+};
+
+
+/**
+ * i2c_arbitrator_select - claim the i2c bus
+ *
+ * Use the GPIO-based signalling protocol; return -EBUSY if we fail.
+ */
+static int i2c_arbitrator_select(struct i2c_adapter *adap, void *data, u32 chan)
+{
+	const struct i2c_arbitrator_data *arb = data;
+	unsigned long stop_retry, stop_time;
+
+	/* Start a round of trying to claim the bus */
+	stop_time = jiffies + usecs_to_jiffies(arb->wait_free_us) + 1;
+	do {
+		/* Indicate that we want to claim the bus */
+		gpio_set_value(arb->ap_gpio, 0);
+		udelay(arb->slew_delay_us);
+
+		/* Wait for the EC to release it */
+		stop_retry = jiffies + usecs_to_jiffies(arb->wait_retry_us) + 1;
+		while (time_before(jiffies, stop_retry)) {
+			if (gpio_get_value(arb->ec_gpio)) {
+				/* We got it, so return */
+				return 0;
+			}
+
+			usleep_range(50, 200);
+		}
+
+		/* It didn't release, so give up, wait, and try again */
+		gpio_set_value(arb->ap_gpio, 1);
+
+		usleep_range(arb->wait_retry_us, arb->wait_retry_us * 2);
+	} while (time_before(jiffies, stop_time));
+
+	/* Give up, release our claim */
+	gpio_set_value(arb->ap_gpio, 1);
+	udelay(arb->slew_delay_us);
+	dev_err(&adap->dev, "Could not claim bus, timeout\n");
+	return -EBUSY;
+}
+
+/**
+ * i2c_arbitrator_deselect - release the i2c bus
+ *
+ * Release the i2c bus using the GPIO-based signalling protocol.
+ */
+static int i2c_arbitrator_deselect(struct i2c_adapter *adap, void *data,
+				   u32 chan)
+{
+	const struct i2c_arbitrator_data *arb = data;
+
+	/* Release the bus and wait for the EC to notice */
+	gpio_set_value(arb->ap_gpio, 1);
+	udelay(arb->slew_delay_us);
+
+	return 0;
+}
+
+static int i2c_arbitrator_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *parent_np;
+	struct i2c_arbitrator_data *arb;
+	int ret;
+
+	/* We only support probing from device tree; no platform_data */
+	if (WARN_ON(!np))
+		return -ENODEV;
+	if (WARN_ON(pdev->dev.platform_data))
+		return -EINVAL;
+
+	arb = devm_kzalloc(&pdev->dev, sizeof(*arb), GFP_KERNEL);
+	if (WARN_ON(!arb))
+		return -ENOMEM;
+	platform_set_drvdata(pdev, arb);
+
+	/* Find our parent */
+	parent_np = of_parse_phandle(np, "i2c-parent", 0);
+	if (WARN_ON(!parent_np))
+		return -EINVAL;
+	arb->parent = of_find_i2c_adapter_by_node(parent_np);
+	if (WARN_ON(!arb->parent))
+		return -EINVAL;
+
+	/* Request GPIOs */
+	ret = of_get_named_gpio(np, "bus-arbitration-gpios", I2C_ARB_GPIO_AP);
+	if (gpio_is_valid(ret)) {
+		arb->ap_gpio = ret;
+		ret = gpio_request_one(arb->ap_gpio, GPIOF_OUT_INIT_HIGH,
+				       "bus-arbitration-ap");
+	}
+	if (WARN_ON(ret))
+		goto ap_request_failed;
+
+	ret = of_get_named_gpio(np, "bus-arbitration-gpios", I2C_ARB_GPIO_EC);
+	if (gpio_is_valid(ret)) {
+		arb->ec_gpio = ret;
+		ret = gpio_request_one(arb->ec_gpio, GPIOF_IN,
+				       "bus-arbitration-ec");
+	}
+	if (WARN_ON(ret))
+		goto ec_request_failed;
+
+	/* Arbitration parameters */
+	if (of_property_read_u32(np, "bus-arbitration-slew-delay-us",
+				 &arb->slew_delay_us))
+		arb->slew_delay_us = 10;
+	if (of_property_read_u32(np, "bus-arbitration-wait-retry-us",
+				 &arb->wait_retry_us))
+		arb->wait_retry_us = 3000;
+	if (of_property_read_u32(np, "bus-arbitration-wait-free-us",
+				 &arb->wait_free_us))
+		arb->wait_free_us = 50000;
+
+	/* Actually add the mux adapter */
+	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, 0, 0, 0,
+					 i2c_arbitrator_select,
+					 i2c_arbitrator_deselect);
+	if (WARN_ON(!arb->child)) {
+		ret = -ENODEV;
+		goto add_adapter_failed;
+	}
+
+	return 0;
+
+add_adapter_failed:
+	gpio_free(arb->ec_gpio);
+ec_request_failed:
+	gpio_free(arb->ap_gpio);
+ap_request_failed:
+	i2c_put_adapter(arb->parent);
+
+	return ret;
+}
+
+static int i2c_arbitrator_remove(struct platform_device *pdev)
+{
+	struct i2c_arbitrator_data *arb = platform_get_drvdata(pdev);
+
+	i2c_del_mux_adapter(arb->child);
+
+	gpio_free(arb->ec_gpio);
+	gpio_free(arb->ap_gpio);
+
+	platform_set_drvdata(pdev, NULL);
+	i2c_put_adapter(arb->parent);
+
+	return 0;
+}
+
+static const struct of_device_id i2c_arbitrator_of_match[] = {
+	{ .compatible = "i2c-arbitrator", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, i2c_arbitrator_of_match);
+
+static struct platform_driver i2c_arbitrator_driver = {
+	.probe	= i2c_arbitrator_probe,
+	.remove	= i2c_arbitrator_remove,
+	.driver	= {
+		.owner	= THIS_MODULE,
+		.name	= "i2c-arbitrator",
+		.of_match_table = of_match_ptr(i2c_arbitrator_of_match),
+	},
+};
+
+static int __init i2c_arbitrator_init(void)
+{
+	return platform_driver_register(&i2c_arbitrator_driver);
+}
+subsys_initcall(i2c_arbitrator_init);
+
+static void __exit i2c_arbitrator_exit(void)
+{
+	platform_driver_unregister(&i2c_arbitrator_driver);
+}
+module_exit(i2c_arbitrator_exit);
+
+MODULE_DESCRIPTION("GPIO-based I2C arbitrator driver");
+MODULE_AUTHOR("Doug Anderson <dianders@chromium.org>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:i2c-arbitrator");
-- 
1.8.1

[PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Doug Anderson]

The i2c-arbitrator driver implements the arbitration scheme that the
Embedded Controller (EC) on the ARM Chromebook expects to use for bus
multimastering.  This i2c-arbitrator driver could also be used in
other places where standard i2c bus arbitration can't be used and two
extra GPIOs are available for arbitration.

This driver is based on code that Simon Glass added to the i2c-s3c2410
driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
mux driver is as suggested by Grant Likely.  See
<https://patchwork.kernel.org/patch/1877311/> for some history.

Signed-off-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
Changes in v1: None

 .../devicetree/bindings/i2c/i2c-arbitrator.txt     |  76 +++++++
 drivers/i2c/muxes/Kconfig                          |  11 +
 drivers/i2c/muxes/Makefile                         |   1 +
 drivers/i2c/muxes/i2c-arbitrator.c                 | 242 +++++++++++++++++++++
 4 files changed, 330 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
 create mode 100644 drivers/i2c/muxes/i2c-arbitrator.c

diff --git a/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
new file mode 100644
index 0000000..bb9cca7
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
@@ -0,0 +1,76 @@
+GPIO-based Arbitration
+======================
+This uses GPIO lines between the AP (Application Processor) and an attached
+EC (Embedded Controller) which both want to talk on the same I2C bus as master.
+
+The AP and EC each have a 'bus claim' line, which is an output that the
+other can see. These are both active low, with pull-ups enabled.
+
+- AP_CLAIM: output from AP, signalling to the EC that the AP wants the bus
+- EC_CLAIM: output from EC, signalling to the AP that the EC wants the bus
+
+
+This mechanism is used instead of standard i2c multimaster to avoid some of the
+subtle driver and silicon bugs that are often present with i2c multimaster.
+
+
+Algorithm:
+
+The basic algorithm is to assert your line when you want the bus, then make
+sure that the other side doesn't want it also. A detailed explanation is best
+done with an example.
+
+Let's say the AP wants to claim the bus. It:
+1. Asserts AP_CLAIM.
+2. Waits a little bit for the other side to notice (slew time, say 10
+   microseconds).
+3. Checks EC_CLAIM. If this is not asserted then the AP has the bus and we are
+   done.
+4. Otherwise, wait for a few milliseconds and see if EC_CLAIM is released.
+5. If not, back off, release the claim and wait for a few more milliseconds.
+6. Go back to 1 (until retry time has expired).
+
+
+Required properties:
+- compatible: i2c-arbitrator
+- bus-arbitration-gpios: Two GPIOs to use with the GPIO-based bus
+    arbitration protocol.  The first should be an output, and is used to
+    claim the I2C bus for us (AP_CLAIM).  The second should be an input and
+    signals that the other side wants to claim the bus (EC_CLAIM).
+- bus-arbitration-slew-delay-us: microseconds to wait for a GPIO to go high.
+- bus-arbitration-wait-retry-us: we'll attempt another claim after this many
+    microseconds.
+- bus-arbitration-wait-free-us: we'll give up after this many microseconds.
+- Standard I2C mux properties. See mux.txt in this directory.
+- Single I2C child bus node at reg 0. See mux.txt in this directory.
+
+
+Example:
+	i2c@12CA0000 {
+		compatible = "acme,some-i2c-device";
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+	i2c-arbitrator {
+		compatible = "i2c-arbitrator";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c-parent = <&{/i2c@12CA0000}>;
+
+		bus-arbitration-gpios = <&gpf0 3 1 0 0>, <&gpe0 4 0 3 0>;
+		bus-arbitration-slew-delay-us = <10>;
+		bus-arbitration-wait-retry-us = <3000>;
+		bus-arbitration-wait-free-us = <50000>;
+
+		i2c@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			i2c@52 {
+				// Normal i2c device
+			};
+		};
+	};
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 0be5b83..714bf16 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -5,6 +5,17 @@
 menu "Multiplexer I2C Chip support"
 	depends on I2C_MUX
 
+config I2C_ARBITRATOR
+	tristate "GPIO-based I2C arbitrator"
+	depends on GENERIC_GPIO && OF
+	help
+	  If you say yes to this option, support will be included for an
+	  i2c multimaster arbitration scheme using GPIOs (as opposed to
+	  using standard i2c multimaster mode).
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-arbitrator.
+
 config I2C_MUX_GPIO
 	tristate "GPIO-based I2C multiplexer"
 	depends on GENERIC_GPIO
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 76da869..adfe147 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -1,6 +1,7 @@
 #
 # Makefile for multiplexer I2C chip drivers.
 
+obj-$(CONFIG_I2C_ARBITRATOR)	+= i2c-arbitrator.o
 obj-$(CONFIG_I2C_MUX_GPIO)	+= i2c-mux-gpio.o
 obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
 obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c
new file mode 100644
index 0000000..c3bbdf7
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-arbitrator.c
@@ -0,0 +1,242 @@
+/*
+ * I2C arbitrator using GPIO API
+ *
+ * Copyright (C) 2012 Google, Inc
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/i2c.h>
+#include <linux/i2c-mux.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/of_i2c.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+
+enum {
+	I2C_ARB_GPIO_AP,		/* AP claims i2c bus */
+	I2C_ARB_GPIO_EC,		/* EC claims i2c bus */
+
+	I2C_ARB_GPIO_COUNT,
+};
+
+/**
+ * struct i2c_arbitrator_data - Driver data for i2c arbitrator
+ *
+ * @parent: Parent adapter
+ * @child: Child bus
+ * @ap_gpio: GPIO we'll use to claim.
+ * @ec_gpio: GPIO that the other side will use to claim.
+ * @slew_delay_us: microseconds to wait for a GPIO to go high.
+ * @wait_retry_us: we'll attempt another claim after this many microseconds.
+ * @wait_free_us: we'll give up after this many microseconds.
+ */
+
+struct i2c_arbitrator_data {
+	struct i2c_adapter *parent;
+	struct i2c_adapter *child;
+
+	int		ap_gpio;
+	int		ec_gpio;
+	unsigned int	slew_delay_us;
+	unsigned int	wait_retry_us;
+	unsigned int	wait_free_us;
+};
+
+
+/**
+ * i2c_arbitrator_select - claim the i2c bus
+ *
+ * Use the GPIO-based signalling protocol; return -EBUSY if we fail.
+ */
+static int i2c_arbitrator_select(struct i2c_adapter *adap, void *data, u32 chan)
+{
+	const struct i2c_arbitrator_data *arb = data;
+	unsigned long stop_retry, stop_time;
+
+	/* Start a round of trying to claim the bus */
+	stop_time = jiffies + usecs_to_jiffies(arb->wait_free_us) + 1;
+	do {
+		/* Indicate that we want to claim the bus */
+		gpio_set_value(arb->ap_gpio, 0);
+		udelay(arb->slew_delay_us);
+
+		/* Wait for the EC to release it */
+		stop_retry = jiffies + usecs_to_jiffies(arb->wait_retry_us) + 1;
+		while (time_before(jiffies, stop_retry)) {
+			if (gpio_get_value(arb->ec_gpio)) {
+				/* We got it, so return */
+				return 0;
+			}
+
+			usleep_range(50, 200);
+		}
+
+		/* It didn't release, so give up, wait, and try again */
+		gpio_set_value(arb->ap_gpio, 1);
+
+		usleep_range(arb->wait_retry_us, arb->wait_retry_us * 2);
+	} while (time_before(jiffies, stop_time));
+
+	/* Give up, release our claim */
+	gpio_set_value(arb->ap_gpio, 1);
+	udelay(arb->slew_delay_us);
+	dev_err(&adap->dev, "Could not claim bus, timeout\n");
+	return -EBUSY;
+}
+
+/**
+ * i2c_arbitrator_deselect - release the i2c bus
+ *
+ * Release the i2c bus using the GPIO-based signalling protocol.
+ */
+static int i2c_arbitrator_deselect(struct i2c_adapter *adap, void *data,
+				   u32 chan)
+{
+	const struct i2c_arbitrator_data *arb = data;
+
+	/* Release the bus and wait for the EC to notice */
+	gpio_set_value(arb->ap_gpio, 1);
+	udelay(arb->slew_delay_us);
+
+	return 0;
+}
+
+static int i2c_arbitrator_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *parent_np;
+	struct i2c_arbitrator_data *arb;
+	int ret;
+
+	/* We only support probing from device tree; no platform_data */
+	if (WARN_ON(!np))
+		return -ENODEV;
+	if (WARN_ON(pdev->dev.platform_data))
+		return -EINVAL;
+
+	arb = devm_kzalloc(&pdev->dev, sizeof(*arb), GFP_KERNEL);
+	if (WARN_ON(!arb))
+		return -ENOMEM;
+	platform_set_drvdata(pdev, arb);
+
+	/* Find our parent */
+	parent_np = of_parse_phandle(np, "i2c-parent", 0);
+	if (WARN_ON(!parent_np))
+		return -EINVAL;
+	arb->parent = of_find_i2c_adapter_by_node(parent_np);
+	if (WARN_ON(!arb->parent))
+		return -EINVAL;
+
+	/* Request GPIOs */
+	ret = of_get_named_gpio(np, "bus-arbitration-gpios", I2C_ARB_GPIO_AP);
+	if (gpio_is_valid(ret)) {
+		arb->ap_gpio = ret;
+		ret = gpio_request_one(arb->ap_gpio, GPIOF_OUT_INIT_HIGH,
+				       "bus-arbitration-ap");
+	}
+	if (WARN_ON(ret))
+		goto ap_request_failed;
+
+	ret = of_get_named_gpio(np, "bus-arbitration-gpios", I2C_ARB_GPIO_EC);
+	if (gpio_is_valid(ret)) {
+		arb->ec_gpio = ret;
+		ret = gpio_request_one(arb->ec_gpio, GPIOF_IN,
+				       "bus-arbitration-ec");
+	}
+	if (WARN_ON(ret))
+		goto ec_request_failed;
+
+	/* Arbitration parameters */
+	if (of_property_read_u32(np, "bus-arbitration-slew-delay-us",
+				 &arb->slew_delay_us))
+		arb->slew_delay_us = 10;
+	if (of_property_read_u32(np, "bus-arbitration-wait-retry-us",
+				 &arb->wait_retry_us))
+		arb->wait_retry_us = 3000;
+	if (of_property_read_u32(np, "bus-arbitration-wait-free-us",
+				 &arb->wait_free_us))
+		arb->wait_free_us = 50000;
+
+	/* Actually add the mux adapter */
+	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, 0, 0, 0,
+					 i2c_arbitrator_select,
+					 i2c_arbitrator_deselect);
+	if (WARN_ON(!arb->child)) {
+		ret = -ENODEV;
+		goto add_adapter_failed;
+	}
+
+	return 0;
+
+add_adapter_failed:
+	gpio_free(arb->ec_gpio);
+ec_request_failed:
+	gpio_free(arb->ap_gpio);
+ap_request_failed:
+	i2c_put_adapter(arb->parent);
+
+	return ret;
+}
+
+static int i2c_arbitrator_remove(struct platform_device *pdev)
+{
+	struct i2c_arbitrator_data *arb = platform_get_drvdata(pdev);
+
+	i2c_del_mux_adapter(arb->child);
+
+	gpio_free(arb->ec_gpio);
+	gpio_free(arb->ap_gpio);
+
+	platform_set_drvdata(pdev, NULL);
+	i2c_put_adapter(arb->parent);
+
+	return 0;
+}
+
+static const struct of_device_id i2c_arbitrator_of_match[] = {
+	{ .compatible = "i2c-arbitrator", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, i2c_arbitrator_of_match);
+
+static struct platform_driver i2c_arbitrator_driver = {
+	.probe	= i2c_arbitrator_probe,
+	.remove	= i2c_arbitrator_remove,
+	.driver	= {
+		.owner	= THIS_MODULE,
+		.name	= "i2c-arbitrator",
+		.of_match_table = of_match_ptr(i2c_arbitrator_of_match),
+	},
+};
+
+static int __init i2c_arbitrator_init(void)
+{
+	return platform_driver_register(&i2c_arbitrator_driver);
+}
+subsys_initcall(i2c_arbitrator_init);
+
+static void __exit i2c_arbitrator_exit(void)
+{
+	platform_driver_unregister(&i2c_arbitrator_driver);
+}
+module_exit(i2c_arbitrator_exit);
+
+MODULE_DESCRIPTION("GPIO-based I2C arbitrator driver");
+MODULE_AUTHOR("Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:i2c-arbitrator");
-- 
1.8.1

[PATCH v1 2/4] ARM: dts: Add i2c-arbitrator bus for exynos5250-snow

[Doug Anderson]

We need to use the i2c-arbitrator to talk to any of the devices on i2c
bus 4 on exynos5250-snow so that we don't confuse the embedded
controller (EC).  Add the i2c-arbitrator to the device tree.  As we
add future devices (keyboard, sbs, tps65090) we'll add them on top of
this.

The arbitrated bus is numbered 104 simply as a convenience to make it
easier for people poking around to guess that it might have something
to do with the physical bus 4.

The addition is split between the cros5250-common and the snow device
tree file since not all cros5250-class devices use arbitration.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v1: None

 arch/arm/boot/dts/cros5250-common.dtsi |  5 ++++-
 arch/arm/boot/dts/exynos5250-snow.dts  | 24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/cros5250-common.dtsi b/arch/arm/boot/dts/cros5250-common.dtsi
index 46c0980..f451375 100644
--- a/arch/arm/boot/dts/cros5250-common.dtsi
+++ b/arch/arm/boot/dts/cros5250-common.dtsi
@@ -58,7 +58,10 @@
 	};
 
 	i2c@12CA0000 {
-		status = "disabled";
+		samsung,i2c-sda-delay = <100>;
+		samsung,i2c-max-bus-freq = <66000>;
+		gpios = <&gpa2 0 3 3 0>,
+			<&gpa2 1 3 3 0>;
 	};
 
 	i2c@12CB0000 {
diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
index 17dd951..69a87a0 100644
--- a/arch/arm/boot/dts/exynos5250-snow.dts
+++ b/arch/arm/boot/dts/exynos5250-snow.dts
@@ -16,6 +16,10 @@
 	model = "Google Snow";
 	compatible = "google,snow", "samsung,exynos5250";
 
+	aliases {
+		i2c104 = &i2c_104;
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 
@@ -29,6 +33,26 @@
 		};
 	};
 
+	i2c-arbitrator {
+		compatible = "i2c-arbitrator";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c-parent = <&{/i2c@12CA0000}>;
+
+		bus-arbitration-gpios = <&gpf0 3 1 0 0>, <&gpe0 4 0 3 0>;
+		bus-arbitration-slew-delay-us = <10>;
+		bus-arbitration-wait-retry-us = <3000>;
+		bus-arbitration-wait-free-us = <50000>;
+
+		/* Use ID 104 as a hint that we're on physical bus 4 */
+		i2c_104: i2c@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+	};
+
 	/*
 	 * On Snow we've got SIP WiFi and so can keep drive strengths low to
 	 * reduce EMI.
-- 
1.8.1

[PATCH v1 2/4] ARM: dts: Add i2c-arbitrator bus for exynos5250-snow

[Doug Anderson]

We need to use the i2c-arbitrator to talk to any of the devices on i2c
bus 4 on exynos5250-snow so that we don't confuse the embedded
controller (EC).  Add the i2c-arbitrator to the device tree.  As we
add future devices (keyboard, sbs, tps65090) we'll add them on top of
this.

The arbitrated bus is numbered 104 simply as a convenience to make it
easier for people poking around to guess that it might have something
to do with the physical bus 4.

The addition is split between the cros5250-common and the snow device
tree file since not all cros5250-class devices use arbitration.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v1: None

 arch/arm/boot/dts/cros5250-common.dtsi |  5 ++++-
 arch/arm/boot/dts/exynos5250-snow.dts  | 24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/cros5250-common.dtsi b/arch/arm/boot/dts/cros5250-common.dtsi
index 46c0980..f451375 100644
--- a/arch/arm/boot/dts/cros5250-common.dtsi
+++ b/arch/arm/boot/dts/cros5250-common.dtsi
@@ -58,7 +58,10 @@
 	};
 
 	i2c at 12CA0000 {
-		status = "disabled";
+		samsung,i2c-sda-delay = <100>;
+		samsung,i2c-max-bus-freq = <66000>;
+		gpios = <&gpa2 0 3 3 0>,
+			<&gpa2 1 3 3 0>;
 	};
 
 	i2c at 12CB0000 {
diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
index 17dd951..69a87a0 100644
--- a/arch/arm/boot/dts/exynos5250-snow.dts
+++ b/arch/arm/boot/dts/exynos5250-snow.dts
@@ -16,6 +16,10 @@
 	model = "Google Snow";
 	compatible = "google,snow", "samsung,exynos5250";
 
+	aliases {
+		i2c104 = &i2c_104;
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 
@@ -29,6 +33,26 @@
 		};
 	};
 
+	i2c-arbitrator {
+		compatible = "i2c-arbitrator";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c-parent = <&{/i2c@12CA0000}>;
+
+		bus-arbitration-gpios = <&gpf0 3 1 0 0>, <&gpe0 4 0 3 0>;
+		bus-arbitration-slew-delay-us = <10>;
+		bus-arbitration-wait-retry-us = <3000>;
+		bus-arbitration-wait-free-us = <50000>;
+
+		/* Use ID 104 as a hint that we're on physical bus 4 */
+		i2c_104: i2c at 0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+	};
+
 	/*
 	 * On Snow we've got SIP WiFi and so can keep drive strengths low to
 	 * reduce EMI.
-- 
1.8.1

[PATCH v1 3/4] ARM: dts: Add sbs-battery for exynos5250-snow

[Doug Anderson]

Now that we have i2c-arbitrator in place on bus 4 we can add the
sbs-battery driver.  Future devices will be added onto bus 4 once
drivers are in good shape.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v1: None

 arch/arm/boot/dts/exynos5250-snow.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
index 69a87a0..b651958 100644
--- a/arch/arm/boot/dts/exynos5250-snow.dts
+++ b/arch/arm/boot/dts/exynos5250-snow.dts
@@ -50,6 +50,12 @@
 			reg = <0>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+
+			battery: sbs-battery@b {
+				compatible = "sbs,sbs-battery";
+				reg = <0xb>;
+				sbs,poll-retry-count = <1>;
+			};
 		};
 	};
 
-- 
1.8.1

[PATCH v1 3/4] ARM: dts: Add sbs-battery for exynos5250-snow

[Doug Anderson]

Now that we have i2c-arbitrator in place on bus 4 we can add the
sbs-battery driver.  Future devices will be added onto bus 4 once
drivers are in good shape.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v1: None

 arch/arm/boot/dts/exynos5250-snow.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
index 69a87a0..b651958 100644
--- a/arch/arm/boot/dts/exynos5250-snow.dts
+++ b/arch/arm/boot/dts/exynos5250-snow.dts
@@ -50,6 +50,12 @@
 			reg = <0>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+
+			battery: sbs-battery at b {
+				compatible = "sbs,sbs-battery";
+				reg = <0xb>;
+				sbs,poll-retry-count = <1>;
+			};
 		};
 	};
 
-- 
1.8.1

[PATCH v1 4/4] i2c-mux: i2c_add_mux_adapter() should use -1 for auto bus num

[Doug Anderson]

The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
we don't want to force the bus number of the adapter.  This is
non-ideal because:
* 0 is actually a valid bus number to request
* i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
  -1 to mean the same thing.  That means extra logic in
  i2c_add_mux_adapter().

Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
accordingly.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Notes:
- If there's a good reason that force_nr uses 0 for auto then feel
  free to drop this patch.  I've place it at the end of the series to
  make it easy to just drop it.

 drivers/i2c/i2c-mux.c               | 10 +++-------
 drivers/i2c/muxes/i2c-arbitrator.c  |  2 +-
 drivers/i2c/muxes/i2c-mux-gpio.c    |  2 +-
 drivers/i2c/muxes/i2c-mux-pca9541.c |  2 +-
 drivers/i2c/muxes/i2c-mux-pca954x.c |  2 +-
 drivers/i2c/muxes/i2c-mux-pinctrl.c |  4 ++--
 include/linux/i2c-mux.h             |  2 +-
 7 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index d94e0ce..8ad1a56 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -103,7 +103,7 @@ static unsigned int i2c_mux_parent_classes(struct i2c_adapter *parent)
 
 struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 				struct device *mux_dev,
-				void *mux_priv, u32 force_nr, u32 chan_id,
+				void *mux_priv, int force_nr, u32 chan_id,
 				unsigned int class,
 				int (*select) (struct i2c_adapter *,
 					       void *, u32),
@@ -168,12 +168,8 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 		}
 	}
 
-	if (force_nr) {
-		priv->adap.nr = force_nr;
-		ret = i2c_add_numbered_adapter(&priv->adap);
-	} else {
-		ret = i2c_add_adapter(&priv->adap);
-	}
+	priv->adap.nr = force_nr;
+	ret = i2c_add_numbered_adapter(&priv->adap);
 	if (ret < 0) {
 		dev_err(&parent->dev,
 			"failed to add mux-adapter (error=%d)\n",
diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c
index c3bbdf7..89d0d06 100644
--- a/drivers/i2c/muxes/i2c-arbitrator.c
+++ b/drivers/i2c/muxes/i2c-arbitrator.c
@@ -173,7 +173,7 @@ static int i2c_arbitrator_probe(struct platform_device *pdev)
 		arb->wait_free_us = 50000;
 
 	/* Actually add the mux adapter */
-	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, 0, 0, 0,
+	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, -1, 0, 0,
 					 i2c_arbitrator_select,
 					 i2c_arbitrator_deselect);
 	if (WARN_ON(!arb->child)) {
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 9f50ef0..301ed0b 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < mux->data.n_values; i++) {
-		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
+		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;
 		unsigned int class = mux->data.classes ? mux->data.classes[i] : 0;
 
 		mux->adap[i] = i2c_add_mux_adapter(parent, &pdev->dev, mux, nr,
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index f3b8f9a..a58b3c2 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -350,7 +350,7 @@ static int pca9541_probe(struct i2c_client *client,
 
 	/* Create mux adapter */
 
-	force = 0;
+	force = -1;
 	if (pdata)
 		force = pdata->modes[0].adap_id;
 	data->mux_adap = i2c_add_mux_adapter(adap, &client->dev, client,
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 8e43872..4663ce6 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -215,7 +215,7 @@ static int pca954x_probe(struct i2c_client *client,
 
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {
-		force = 0;			  /* dynamic adap number */
+		force = -1;			  /* dynamic adap number */
 		class = 0;			  /* no class by default */
 		if (pdata) {
 			if (num < pdata->num_modes) {
diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
index a43c0ce..401ff5d 100644
--- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
@@ -217,8 +217,8 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < mux->pdata->bus_count; i++) {
-		u32 bus = mux->pdata->base_bus_num ?
-				(mux->pdata->base_bus_num + i) : 0;
+		int bus = mux->pdata->base_bus_num ?
+				(mux->pdata->base_bus_num + i) : -1;
 
 		mux->busses[i] = i2c_add_mux_adapter(mux->parent, &pdev->dev,
 						     mux, bus, i, 0,
diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
index 40cb05a..a7bfb55 100644
--- a/include/linux/i2c-mux.h
+++ b/include/linux/i2c-mux.h
@@ -35,7 +35,7 @@
  */
 struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 				struct device *mux_dev,
-				void *mux_priv, u32 force_nr, u32 chan_id,
+				void *mux_priv, int force_nr, u32 chan_id,
 				unsigned int class,
 				int (*select) (struct i2c_adapter *,
 					       void *mux_dev, u32 chan_id),
-- 
1.8.1

[PATCH v1 4/4] i2c-mux: i2c_add_mux_adapter() should use -1 for auto bus num

[Doug Anderson]

The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
we don't want to force the bus number of the adapter.  This is
non-ideal because:
* 0 is actually a valid bus number to request
* i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
  -1 to mean the same thing.  That means extra logic in
  i2c_add_mux_adapter().

Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
accordingly.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Notes:
- If there's a good reason that force_nr uses 0 for auto then feel
  free to drop this patch.  I've place it at the end of the series to
  make it easy to just drop it.

 drivers/i2c/i2c-mux.c               | 10 +++-------
 drivers/i2c/muxes/i2c-arbitrator.c  |  2 +-
 drivers/i2c/muxes/i2c-mux-gpio.c    |  2 +-
 drivers/i2c/muxes/i2c-mux-pca9541.c |  2 +-
 drivers/i2c/muxes/i2c-mux-pca954x.c |  2 +-
 drivers/i2c/muxes/i2c-mux-pinctrl.c |  4 ++--
 include/linux/i2c-mux.h             |  2 +-
 7 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index d94e0ce..8ad1a56 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -103,7 +103,7 @@ static unsigned int i2c_mux_parent_classes(struct i2c_adapter *parent)
 
 struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 				struct device *mux_dev,
-				void *mux_priv, u32 force_nr, u32 chan_id,
+				void *mux_priv, int force_nr, u32 chan_id,
 				unsigned int class,
 				int (*select) (struct i2c_adapter *,
 					       void *, u32),
@@ -168,12 +168,8 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 		}
 	}
 
-	if (force_nr) {
-		priv->adap.nr = force_nr;
-		ret = i2c_add_numbered_adapter(&priv->adap);
-	} else {
-		ret = i2c_add_adapter(&priv->adap);
-	}
+	priv->adap.nr = force_nr;
+	ret = i2c_add_numbered_adapter(&priv->adap);
 	if (ret < 0) {
 		dev_err(&parent->dev,
 			"failed to add mux-adapter (error=%d)\n",
diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c
index c3bbdf7..89d0d06 100644
--- a/drivers/i2c/muxes/i2c-arbitrator.c
+++ b/drivers/i2c/muxes/i2c-arbitrator.c
@@ -173,7 +173,7 @@ static int i2c_arbitrator_probe(struct platform_device *pdev)
 		arb->wait_free_us = 50000;
 
 	/* Actually add the mux adapter */
-	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, 0, 0, 0,
+	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, -1, 0, 0,
 					 i2c_arbitrator_select,
 					 i2c_arbitrator_deselect);
 	if (WARN_ON(!arb->child)) {
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 9f50ef0..301ed0b 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < mux->data.n_values; i++) {
-		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
+		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;
 		unsigned int class = mux->data.classes ? mux->data.classes[i] : 0;
 
 		mux->adap[i] = i2c_add_mux_adapter(parent, &pdev->dev, mux, nr,
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index f3b8f9a..a58b3c2 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -350,7 +350,7 @@ static int pca9541_probe(struct i2c_client *client,
 
 	/* Create mux adapter */
 
-	force = 0;
+	force = -1;
 	if (pdata)
 		force = pdata->modes[0].adap_id;
 	data->mux_adap = i2c_add_mux_adapter(adap, &client->dev, client,
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 8e43872..4663ce6 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -215,7 +215,7 @@ static int pca954x_probe(struct i2c_client *client,
 
 	/* Now create an adapter for each channel */
 	for (num = 0; num < chips[data->type].nchans; num++) {
-		force = 0;			  /* dynamic adap number */
+		force = -1;			  /* dynamic adap number */
 		class = 0;			  /* no class by default */
 		if (pdata) {
 			if (num < pdata->num_modes) {
diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
index a43c0ce..401ff5d 100644
--- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
@@ -217,8 +217,8 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < mux->pdata->bus_count; i++) {
-		u32 bus = mux->pdata->base_bus_num ?
-				(mux->pdata->base_bus_num + i) : 0;
+		int bus = mux->pdata->base_bus_num ?
+				(mux->pdata->base_bus_num + i) : -1;
 
 		mux->busses[i] = i2c_add_mux_adapter(mux->parent, &pdev->dev,
 						     mux, bus, i, 0,
diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
index 40cb05a..a7bfb55 100644
--- a/include/linux/i2c-mux.h
+++ b/include/linux/i2c-mux.h
@@ -35,7 +35,7 @@
  */
 struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 				struct device *mux_dev,
-				void *mux_priv, u32 force_nr, u32 chan_id,
+				void *mux_priv, int force_nr, u32 chan_id,
 				unsigned int class,
 				int (*select) (struct i2c_adapter *,
 					       void *mux_dev, u32 chan_id),
-- 
1.8.1

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Olof Johansson]

Hi,

The driver pieces have been reviewed a bit already, but I have a question on
the bindings below.

On Wed, Feb 13, 2013 at 10:02:09AM -0800, Doug Anderson wrote:
> The i2c-arbitrator driver implements the arbitration scheme that the
> Embedded Controller (EC) on the ARM Chromebook expects to use for bus
> multimastering.  This i2c-arbitrator driver could also be used in
> other places where standard i2c bus arbitration can't be used and two
> extra GPIOs are available for arbitration.
> 
> This driver is based on code that Simon Glass added to the i2c-s3c2410
> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
> mux driver is as suggested by Grant Likely.  See
> <https://patchwork.kernel.org/patch/1877311/> for some history.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> ---
> Changes in v1: None
> 
>  .../devicetree/bindings/i2c/i2c-arbitrator.txt     |  76 +++++++
>  drivers/i2c/muxes/Kconfig                          |  11 +
>  drivers/i2c/muxes/Makefile                         |   1 +
>  drivers/i2c/muxes/i2c-arbitrator.c                 | 242 +++++++++++++++++++++
>  4 files changed, 330 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
>  create mode 100644 drivers/i2c/muxes/i2c-arbitrator.c
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
> new file mode 100644
> index 0000000..bb9cca7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
> @@ -0,0 +1,76 @@
> +GPIO-based Arbitration
> +======================
> +This uses GPIO lines between the AP (Application Processor) and an attached
> +EC (Embedded Controller) which both want to talk on the same I2C bus as master.
> +
> +The AP and EC each have a 'bus claim' line, which is an output that the
> +other can see. These are both active low, with pull-ups enabled.
> +
> +- AP_CLAIM: output from AP, signalling to the EC that the AP wants the bus
> +- EC_CLAIM: output from EC, signalling to the AP that the EC wants the bus
> +
> +
> +This mechanism is used instead of standard i2c multimaster to avoid some of the
> +subtle driver and silicon bugs that are often present with i2c multimaster.
> +
> +
> +Algorithm:
> +
> +The basic algorithm is to assert your line when you want the bus, then make
> +sure that the other side doesn't want it also. A detailed explanation is best
> +done with an example.
> +
> +Let's say the AP wants to claim the bus. It:
> +1. Asserts AP_CLAIM.
> +2. Waits a little bit for the other side to notice (slew time, say 10
> +   microseconds).
> +3. Checks EC_CLAIM. If this is not asserted then the AP has the bus and we are
> +   done.
> +4. Otherwise, wait for a few milliseconds and see if EC_CLAIM is released.
> +5. If not, back off, release the claim and wait for a few more milliseconds.
> +6. Go back to 1 (until retry time has expired).
> +
> +
> +Required properties:
> +- compatible: i2c-arbitrator
> +- bus-arbitration-gpios: Two GPIOs to use with the GPIO-based bus
> +    arbitration protocol.  The first should be an output, and is used to
> +    claim the I2C bus for us (AP_CLAIM).  The second should be an input and
> +    signals that the other side wants to claim the bus (EC_CLAIM).
> +- bus-arbitration-slew-delay-us: microseconds to wait for a GPIO to go high.
> +- bus-arbitration-wait-retry-us: we'll attempt another claim after this many
> +    microseconds.
> +- bus-arbitration-wait-free-us: we'll give up after this many microseconds.
> +- Standard I2C mux properties. See mux.txt in this directory.
> +- Single I2C child bus node at reg 0. See mux.txt in this directory.
> +
> +
> +Example:
> +	i2c@12CA0000 {
> +		compatible = "acme,some-i2c-device";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +
> +	i2c-arbitrator {
> +		compatible = "i2c-arbitrator";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		i2c-parent = <&{/i2c@12CA0000}>;

Why use this custom i2c-parent property? The arbitrator should just be
under the i2c controller in the device tree, and thus parent would be
derived from the topology there.


-Olof

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Olof Johansson]

Hi,

The driver pieces have been reviewed a bit already, but I have a question on
the bindings below.

On Wed, Feb 13, 2013 at 10:02:09AM -0800, Doug Anderson wrote:
> The i2c-arbitrator driver implements the arbitration scheme that the
> Embedded Controller (EC) on the ARM Chromebook expects to use for bus
> multimastering.  This i2c-arbitrator driver could also be used in
> other places where standard i2c bus arbitration can't be used and two
> extra GPIOs are available for arbitration.
> 
> This driver is based on code that Simon Glass added to the i2c-s3c2410
> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
> mux driver is as suggested by Grant Likely.  See
> <https://patchwork.kernel.org/patch/1877311/> for some history.
> 
> Signed-off-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
> Changes in v1: None
> 
>  .../devicetree/bindings/i2c/i2c-arbitrator.txt     |  76 +++++++
>  drivers/i2c/muxes/Kconfig                          |  11 +
>  drivers/i2c/muxes/Makefile                         |   1 +
>  drivers/i2c/muxes/i2c-arbitrator.c                 | 242 +++++++++++++++++++++
>  4 files changed, 330 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
>  create mode 100644 drivers/i2c/muxes/i2c-arbitrator.c
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
> new file mode 100644
> index 0000000..bb9cca7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
> @@ -0,0 +1,76 @@
> +GPIO-based Arbitration
> +======================
> +This uses GPIO lines between the AP (Application Processor) and an attached
> +EC (Embedded Controller) which both want to talk on the same I2C bus as master.
> +
> +The AP and EC each have a 'bus claim' line, which is an output that the
> +other can see. These are both active low, with pull-ups enabled.
> +
> +- AP_CLAIM: output from AP, signalling to the EC that the AP wants the bus
> +- EC_CLAIM: output from EC, signalling to the AP that the EC wants the bus
> +
> +
> +This mechanism is used instead of standard i2c multimaster to avoid some of the
> +subtle driver and silicon bugs that are often present with i2c multimaster.
> +
> +
> +Algorithm:
> +
> +The basic algorithm is to assert your line when you want the bus, then make
> +sure that the other side doesn't want it also. A detailed explanation is best
> +done with an example.
> +
> +Let's say the AP wants to claim the bus. It:
> +1. Asserts AP_CLAIM.
> +2. Waits a little bit for the other side to notice (slew time, say 10
> +   microseconds).
> +3. Checks EC_CLAIM. If this is not asserted then the AP has the bus and we are
> +   done.
> +4. Otherwise, wait for a few milliseconds and see if EC_CLAIM is released.
> +5. If not, back off, release the claim and wait for a few more milliseconds.
> +6. Go back to 1 (until retry time has expired).
> +
> +
> +Required properties:
> +- compatible: i2c-arbitrator
> +- bus-arbitration-gpios: Two GPIOs to use with the GPIO-based bus
> +    arbitration protocol.  The first should be an output, and is used to
> +    claim the I2C bus for us (AP_CLAIM).  The second should be an input and
> +    signals that the other side wants to claim the bus (EC_CLAIM).
> +- bus-arbitration-slew-delay-us: microseconds to wait for a GPIO to go high.
> +- bus-arbitration-wait-retry-us: we'll attempt another claim after this many
> +    microseconds.
> +- bus-arbitration-wait-free-us: we'll give up after this many microseconds.
> +- Standard I2C mux properties. See mux.txt in this directory.
> +- Single I2C child bus node at reg 0. See mux.txt in this directory.
> +
> +
> +Example:
> +	i2c@12CA0000 {
> +		compatible = "acme,some-i2c-device";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +
> +	i2c-arbitrator {
> +		compatible = "i2c-arbitrator";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		i2c-parent = <&{/i2c@12CA0000}>;

Why use this custom i2c-parent property? The arbitrator should just be
under the i2c controller in the device tree, and thus parent would be
derived from the topology there.


-Olof

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Olof Johansson]

On Wed, Feb 13, 2013 at 10:45 AM, Olof Johansson <olof@lixom.net> wrote:

>> +             i2c-parent = <&{/i2c@12CA0000}>;
>
> Why use this custom i2c-parent property? The arbitrator should just be
> under the i2c controller in the device tree, and thus parent would be
> derived from the topology there.

Ah. Reading up on the discussions for the other mux bindings, there's
the issue of the arbitrator not being addressable/accessible via i2c.

Sigh, awkward and annoying, since it breaks the topography for the
hardware apart due to the arbitrator. But I guess there's no good
alternative. :(


-Olof

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Olof Johansson]

On Wed, Feb 13, 2013 at 10:45 AM, Olof Johansson <olof@lixom.net> wrote:

>> +             i2c-parent = <&{/i2c@12CA0000}>;
>
> Why use this custom i2c-parent property? The arbitrator should just be
> under the i2c controller in the device tree, and thus parent would be
> derived from the topology there.

Ah. Reading up on the discussions for the other mux bindings, there's
the issue of the arbitrator not being addressable/accessible via i2c.

Sigh, awkward and annoying, since it breaks the topography for the
hardware apart due to the arbitrator. But I guess there's no good
alternative. :(


-Olof

Re: [PATCH v1 4/4] i2c-mux: i2c_add_mux_adapter() should use -1 for auto bus num

[Guenter Roeck]

On Wed, Feb 13, 2013 at 10:02:12AM -0800, Doug Anderson wrote:
> The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
> we don't want to force the bus number of the adapter.  This is
> non-ideal because:
> * 0 is actually a valid bus number to request
> * i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
>   -1 to mean the same thing.  That means extra logic in
>   i2c_add_mux_adapter().
> 
> Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
> accordingly.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Notes:
> - If there's a good reason that force_nr uses 0 for auto then feel
>   free to drop this patch.  I've place it at the end of the series to
>   make it easy to just drop it.
> 
>  drivers/i2c/i2c-mux.c               | 10 +++-------
>  drivers/i2c/muxes/i2c-arbitrator.c  |  2 +-
>  drivers/i2c/muxes/i2c-mux-gpio.c    |  2 +-
>  drivers/i2c/muxes/i2c-mux-pca9541.c |  2 +-
>  drivers/i2c/muxes/i2c-mux-pca954x.c |  2 +-
>  drivers/i2c/muxes/i2c-mux-pinctrl.c |  4 ++--
>  include/linux/i2c-mux.h             |  2 +-
>  7 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index d94e0ce..8ad1a56 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -103,7 +103,7 @@ static unsigned int i2c_mux_parent_classes(struct i2c_adapter *parent)
>  
>  struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
>  				struct device *mux_dev,
> -				void *mux_priv, u32 force_nr, u32 chan_id,
> +				void *mux_priv, int force_nr, u32 chan_id,
>  				unsigned int class,
>  				int (*select) (struct i2c_adapter *,
>  					       void *, u32),
> @@ -168,12 +168,8 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
>  		}
>  	}
>  
> -	if (force_nr) {
> -		priv->adap.nr = force_nr;
> -		ret = i2c_add_numbered_adapter(&priv->adap);
> -	} else {
> -		ret = i2c_add_adapter(&priv->adap);
> -	}
> +	priv->adap.nr = force_nr;
> +	ret = i2c_add_numbered_adapter(&priv->adap);
>  	if (ret < 0) {
>  		dev_err(&parent->dev,
>  			"failed to add mux-adapter (error=%d)\n",
> diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c
> index c3bbdf7..89d0d06 100644
> --- a/drivers/i2c/muxes/i2c-arbitrator.c
> +++ b/drivers/i2c/muxes/i2c-arbitrator.c
> @@ -173,7 +173,7 @@ static int i2c_arbitrator_probe(struct platform_device *pdev)
>  		arb->wait_free_us = 50000;
>  
>  	/* Actually add the mux adapter */
> -	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, 0, 0, 0,
> +	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, -1, 0, 0,
>  					 i2c_arbitrator_select,
>  					 i2c_arbitrator_deselect);
>  	if (WARN_ON(!arb->child)) {
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 9f50ef0..301ed0b 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	for (i = 0; i < mux->data.n_values; i++) {
> -		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
> +		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;
>  		unsigned int class = mux->data.classes ? mux->data.classes[i] : 0;
>  
>  		mux->adap[i] = i2c_add_mux_adapter(parent, &pdev->dev, mux, nr,
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index f3b8f9a..a58b3c2 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -350,7 +350,7 @@ static int pca9541_probe(struct i2c_client *client,
>  
>  	/* Create mux adapter */
>  
> -	force = 0;
> +	force = -1;
>  	if (pdata)
>  		force = pdata->modes[0].adap_id;
>  	data->mux_adap = i2c_add_mux_adapter(adap, &client->dev, client,
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 8e43872..4663ce6 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -215,7 +215,7 @@ static int pca954x_probe(struct i2c_client *client,
>  
>  	/* Now create an adapter for each channel */
>  	for (num = 0; num < chips[data->type].nchans; num++) {
> -		force = 0;			  /* dynamic adap number */
> +		force = -1;			  /* dynamic adap number */
>  		class = 0;			  /* no class by default */
>  		if (pdata) {
>  			if (num < pdata->num_modes) {
> diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> index a43c0ce..401ff5d 100644
> --- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> @@ -217,8 +217,8 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
>  	}
>  
>  	for (i = 0; i < mux->pdata->bus_count; i++) {
> -		u32 bus = mux->pdata->base_bus_num ?
> -				(mux->pdata->base_bus_num + i) : 0;
> +		int bus = mux->pdata->base_bus_num ?
> +				(mux->pdata->base_bus_num + i) : -1;

Maybe this should be:
		int bus = mux->pdata->base_bus_num >= 0 ?
				(mux->pdata->base_bus_num + i) : -1;

Otherwise you still can not assign bus number 0 as 1st bus. Also, if base_bus_num
ends up being provided as -1, the result would be pretty much unpredictable.

Thanks,
Guenter

Re: [PATCH v1 4/4] i2c-mux: i2c_add_mux_adapter() should use -1 for auto bus num

[Guenter Roeck]

On Wed, Feb 13, 2013 at 10:02:12AM -0800, Doug Anderson wrote:
> The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
> we don't want to force the bus number of the adapter.  This is
> non-ideal because:
> * 0 is actually a valid bus number to request
> * i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
>   -1 to mean the same thing.  That means extra logic in
>   i2c_add_mux_adapter().
> 
> Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
> accordingly.
> 
> Signed-off-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> Notes:
> - If there's a good reason that force_nr uses 0 for auto then feel
>   free to drop this patch.  I've place it at the end of the series to
>   make it easy to just drop it.
> 
>  drivers/i2c/i2c-mux.c               | 10 +++-------
>  drivers/i2c/muxes/i2c-arbitrator.c  |  2 +-
>  drivers/i2c/muxes/i2c-mux-gpio.c    |  2 +-
>  drivers/i2c/muxes/i2c-mux-pca9541.c |  2 +-
>  drivers/i2c/muxes/i2c-mux-pca954x.c |  2 +-
>  drivers/i2c/muxes/i2c-mux-pinctrl.c |  4 ++--
>  include/linux/i2c-mux.h             |  2 +-
>  7 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index d94e0ce..8ad1a56 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -103,7 +103,7 @@ static unsigned int i2c_mux_parent_classes(struct i2c_adapter *parent)
>  
>  struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
>  				struct device *mux_dev,
> -				void *mux_priv, u32 force_nr, u32 chan_id,
> +				void *mux_priv, int force_nr, u32 chan_id,
>  				unsigned int class,
>  				int (*select) (struct i2c_adapter *,
>  					       void *, u32),
> @@ -168,12 +168,8 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
>  		}
>  	}
>  
> -	if (force_nr) {
> -		priv->adap.nr = force_nr;
> -		ret = i2c_add_numbered_adapter(&priv->adap);
> -	} else {
> -		ret = i2c_add_adapter(&priv->adap);
> -	}
> +	priv->adap.nr = force_nr;
> +	ret = i2c_add_numbered_adapter(&priv->adap);
>  	if (ret < 0) {
>  		dev_err(&parent->dev,
>  			"failed to add mux-adapter (error=%d)\n",
> diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c
> index c3bbdf7..89d0d06 100644
> --- a/drivers/i2c/muxes/i2c-arbitrator.c
> +++ b/drivers/i2c/muxes/i2c-arbitrator.c
> @@ -173,7 +173,7 @@ static int i2c_arbitrator_probe(struct platform_device *pdev)
>  		arb->wait_free_us = 50000;
>  
>  	/* Actually add the mux adapter */
> -	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, 0, 0, 0,
> +	arb->child = i2c_add_mux_adapter(arb->parent, &pdev->dev, arb, -1, 0, 0,
>  					 i2c_arbitrator_select,
>  					 i2c_arbitrator_deselect);
>  	if (WARN_ON(!arb->child)) {
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 9f50ef0..301ed0b 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	for (i = 0; i < mux->data.n_values; i++) {
> -		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
> +		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;
>  		unsigned int class = mux->data.classes ? mux->data.classes[i] : 0;
>  
>  		mux->adap[i] = i2c_add_mux_adapter(parent, &pdev->dev, mux, nr,
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index f3b8f9a..a58b3c2 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -350,7 +350,7 @@ static int pca9541_probe(struct i2c_client *client,
>  
>  	/* Create mux adapter */
>  
> -	force = 0;
> +	force = -1;
>  	if (pdata)
>  		force = pdata->modes[0].adap_id;
>  	data->mux_adap = i2c_add_mux_adapter(adap, &client->dev, client,
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 8e43872..4663ce6 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -215,7 +215,7 @@ static int pca954x_probe(struct i2c_client *client,
>  
>  	/* Now create an adapter for each channel */
>  	for (num = 0; num < chips[data->type].nchans; num++) {
> -		force = 0;			  /* dynamic adap number */
> +		force = -1;			  /* dynamic adap number */
>  		class = 0;			  /* no class by default */
>  		if (pdata) {
>  			if (num < pdata->num_modes) {
> diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> index a43c0ce..401ff5d 100644
> --- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> @@ -217,8 +217,8 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
>  	}
>  
>  	for (i = 0; i < mux->pdata->bus_count; i++) {
> -		u32 bus = mux->pdata->base_bus_num ?
> -				(mux->pdata->base_bus_num + i) : 0;
> +		int bus = mux->pdata->base_bus_num ?
> +				(mux->pdata->base_bus_num + i) : -1;

Maybe this should be:
		int bus = mux->pdata->base_bus_num >= 0 ?
				(mux->pdata->base_bus_num + i) : -1;

Otherwise you still can not assign bus number 0 as 1st bus. Also, if base_bus_num
ends up being provided as -1, the result would be pretty much unpredictable.

Thanks,
Guenter

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Stephen Warren]

On 02/13/2013 11:02 AM, Doug Anderson wrote:
> The i2c-arbitrator driver implements the arbitration scheme that the
> Embedded Controller (EC) on the ARM Chromebook expects to use for bus
> multimastering.  This i2c-arbitrator driver could also be used in
> other places where standard i2c bus arbitration can't be used and two
> extra GPIOs are available for arbitration.
> 
> This driver is based on code that Simon Glass added to the i2c-s3c2410
> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
> mux driver is as suggested by Grant Likely.  See
> <https://patchwork.kernel.org/patch/1877311/> for some history.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt

> +This mechanism is used instead of standard i2c multimaster to avoid some of the
> +subtle driver and silicon bugs that are often present with i2c multimaster.

Being really pick here, but I2C should be capitalized in free-form text.
There are a few other places where the comment applies.

> +Required properties:
> +- compatible: i2c-arbitrator

That seems pretty generic. What if there are other arbitration schemes?

> +- bus-arbitration-gpios: Two GPIOs to use with the GPIO-based bus
> +    arbitration protocol.  The first should be an output, and is used to
> +    claim the I2C bus for us (AP_CLAIM).  The second should be an input and
> +    signals that the other side wants to claim the bus (EC_CLAIM).

Is it worth using two separate properties here, so they each get a
unique name. That way, nobody has the remember which order the two GPIOs
come in.

> diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c

> +enum {
> +	I2C_ARB_GPIO_AP,		/* AP claims i2c bus */
> +	I2C_ARB_GPIO_EC,		/* EC claims i2c bus */
> +
> +	I2C_ARB_GPIO_COUNT,
> +};

Oh, I see from later code that those are indices into the
bus-arbitration-gpios DT property. I thought they were states in some
state machine at first. A comment might help here, if you continue to
use one property.

> +static int i2c_arbitrator_select(struct i2c_adapter *adap, void *data, u32 chan)
> +{
> +	const struct i2c_arbitrator_data *arb = data;
> +	unsigned long stop_retry, stop_time;
> +
> +	/* Start a round of trying to claim the bus */
> +	stop_time = jiffies + usecs_to_jiffies(arb->wait_free_us) + 1;
> +	do {
> +		/* Indicate that we want to claim the bus */
> +		gpio_set_value(arb->ap_gpio, 0);

The GPIO signals appear to be active low in the code. Instead, I think
it'd make more sense to extract the polarity of the GPIO from DT, using
of_get_named_gpio_flags().

> +static int i2c_arbitrator_probe(struct platform_device *pdev)

> +	/* Request GPIOs */
> +	ret = of_get_named_gpio(np, "bus-arbitration-gpios", I2C_ARB_GPIO_AP);
> +	if (gpio_is_valid(ret)) {
> +		arb->ap_gpio = ret;
> +		ret = gpio_request_one(arb->ap_gpio, GPIOF_OUT_INIT_HIGH,
> +				       "bus-arbitration-ap");
> +	}
> +	if (WARN_ON(ret))
> +		goto ap_request_failed;

you could use devm_gpio_request_one() and save some cleanup logic.

> +	/* Arbitration parameters */
> +	if (of_property_read_u32(np, "bus-arbitration-slew-delay-us",
> +				 &arb->slew_delay_us))
> +		arb->slew_delay_us = 10;

The DT binding document says that property is required. Either the code
should error out here, or the document updated to indicate that the
properties are optional, and specify what the defaults are.

> +static int i2c_arbitrator_remove(struct platform_device *pdev)

> +	platform_set_drvdata(pdev, NULL);

You shouldn't have to do that; nothing should care what the pdata value
is while the device isn't probed anyway.

> +static int __init i2c_arbitrator_init(void)
> +{
> +	return platform_driver_register(&i2c_arbitrator_driver);
> +}
> +subsys_initcall(i2c_arbitrator_init);
> +
> +static void __exit i2c_arbitrator_exit(void)
> +{
> +	platform_driver_unregister(&i2c_arbitrator_driver);
> +}
> +module_exit(i2c_arbitrator_exit);

You should be able to replace all that with:

module_platform_driver(&i2c_arbitrator_driver);

> +MODULE_LICENSE("GPL");

The header says GPL v2 only, so "GPL v2".

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Stephen Warren]

On 02/13/2013 11:02 AM, Doug Anderson wrote:
> The i2c-arbitrator driver implements the arbitration scheme that the
> Embedded Controller (EC) on the ARM Chromebook expects to use for bus
> multimastering.  This i2c-arbitrator driver could also be used in
> other places where standard i2c bus arbitration can't be used and two
> extra GPIOs are available for arbitration.
> 
> This driver is based on code that Simon Glass added to the i2c-s3c2410
> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
> mux driver is as suggested by Grant Likely.  See
> <https://patchwork.kernel.org/patch/1877311/> for some history.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt

> +This mechanism is used instead of standard i2c multimaster to avoid some of the
> +subtle driver and silicon bugs that are often present with i2c multimaster.

Being really pick here, but I2C should be capitalized in free-form text.
There are a few other places where the comment applies.

> +Required properties:
> +- compatible: i2c-arbitrator

That seems pretty generic. What if there are other arbitration schemes?

> +- bus-arbitration-gpios: Two GPIOs to use with the GPIO-based bus
> +    arbitration protocol.  The first should be an output, and is used to
> +    claim the I2C bus for us (AP_CLAIM).  The second should be an input and
> +    signals that the other side wants to claim the bus (EC_CLAIM).

Is it worth using two separate properties here, so they each get a
unique name. That way, nobody has the remember which order the two GPIOs
come in.

> diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c

> +enum {
> +	I2C_ARB_GPIO_AP,		/* AP claims i2c bus */
> +	I2C_ARB_GPIO_EC,		/* EC claims i2c bus */
> +
> +	I2C_ARB_GPIO_COUNT,
> +};

Oh, I see from later code that those are indices into the
bus-arbitration-gpios DT property. I thought they were states in some
state machine at first. A comment might help here, if you continue to
use one property.

> +static int i2c_arbitrator_select(struct i2c_adapter *adap, void *data, u32 chan)
> +{
> +	const struct i2c_arbitrator_data *arb = data;
> +	unsigned long stop_retry, stop_time;
> +
> +	/* Start a round of trying to claim the bus */
> +	stop_time = jiffies + usecs_to_jiffies(arb->wait_free_us) + 1;
> +	do {
> +		/* Indicate that we want to claim the bus */
> +		gpio_set_value(arb->ap_gpio, 0);

The GPIO signals appear to be active low in the code. Instead, I think
it'd make more sense to extract the polarity of the GPIO from DT, using
of_get_named_gpio_flags().

> +static int i2c_arbitrator_probe(struct platform_device *pdev)

> +	/* Request GPIOs */
> +	ret = of_get_named_gpio(np, "bus-arbitration-gpios", I2C_ARB_GPIO_AP);
> +	if (gpio_is_valid(ret)) {
> +		arb->ap_gpio = ret;
> +		ret = gpio_request_one(arb->ap_gpio, GPIOF_OUT_INIT_HIGH,
> +				       "bus-arbitration-ap");
> +	}
> +	if (WARN_ON(ret))
> +		goto ap_request_failed;

you could use devm_gpio_request_one() and save some cleanup logic.

> +	/* Arbitration parameters */
> +	if (of_property_read_u32(np, "bus-arbitration-slew-delay-us",
> +				 &arb->slew_delay_us))
> +		arb->slew_delay_us = 10;

The DT binding document says that property is required. Either the code
should error out here, or the document updated to indicate that the
properties are optional, and specify what the defaults are.

> +static int i2c_arbitrator_remove(struct platform_device *pdev)

> +	platform_set_drvdata(pdev, NULL);

You shouldn't have to do that; nothing should care what the pdata value
is while the device isn't probed anyway.

> +static int __init i2c_arbitrator_init(void)
> +{
> +	return platform_driver_register(&i2c_arbitrator_driver);
> +}
> +subsys_initcall(i2c_arbitrator_init);
> +
> +static void __exit i2c_arbitrator_exit(void)
> +{
> +	platform_driver_unregister(&i2c_arbitrator_driver);
> +}
> +module_exit(i2c_arbitrator_exit);

You should be able to replace all that with:

module_platform_driver(&i2c_arbitrator_driver);

> +MODULE_LICENSE("GPL");

The header says GPL v2 only, so "GPL v2".

Re: [PATCH v1 2/4] ARM: dts: Add i2c-arbitrator bus for exynos5250-snow

[Stephen Warren]

On 02/13/2013 11:02 AM, Doug Anderson wrote:
> We need to use the i2c-arbitrator to talk to any of the devices on i2c
> bus 4 on exynos5250-snow so that we don't confuse the embedded
> controller (EC).  Add the i2c-arbitrator to the device tree.  As we
> add future devices (keyboard, sbs, tps65090) we'll add them on top of
> this.
> 
> The arbitrated bus is numbered 104 simply as a convenience to make it
> easier for people poking around to guess that it might have something
> to do with the physical bus 4.
> 
> The addition is split between the cros5250-common and the snow device
> tree file since not all cros5250-class devices use arbitration.

> diff --git a/arch/arm/boot/dts/cros5250-common.dtsi b/arch/arm/boot/dts/cros5250-common.dtsi

>  	i2c@12CA0000 {
> -		status = "disabled";
> +		samsung,i2c-sda-delay = <100>;
> +		samsung,i2c-max-bus-freq = <66000>;

Shouldn't that use the standard clock-frequency property?

> diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts

> +	i2c-arbitrator {
> +		compatible = "i2c-arbitrator";
> +		#address-cells = <1>;
> +		#size-cells = <0>;

> +		/* Use ID 104 as a hint that we're on physical bus 4 */
> +		i2c_104: i2c@0 {

Does something use that hint? It sounds a little odd.

[PATCH v1 2/4] ARM: dts: Add i2c-arbitrator bus for exynos5250-snow

[Stephen Warren]

On 02/13/2013 11:02 AM, Doug Anderson wrote:
> We need to use the i2c-arbitrator to talk to any of the devices on i2c
> bus 4 on exynos5250-snow so that we don't confuse the embedded
> controller (EC).  Add the i2c-arbitrator to the device tree.  As we
> add future devices (keyboard, sbs, tps65090) we'll add them on top of
> this.
> 
> The arbitrated bus is numbered 104 simply as a convenience to make it
> easier for people poking around to guess that it might have something
> to do with the physical bus 4.
> 
> The addition is split between the cros5250-common and the snow device
> tree file since not all cros5250-class devices use arbitration.

> diff --git a/arch/arm/boot/dts/cros5250-common.dtsi b/arch/arm/boot/dts/cros5250-common.dtsi

>  	i2c at 12CA0000 {
> -		status = "disabled";
> +		samsung,i2c-sda-delay = <100>;
> +		samsung,i2c-max-bus-freq = <66000>;

Shouldn't that use the standard clock-frequency property?

> diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts

> +	i2c-arbitrator {
> +		compatible = "i2c-arbitrator";
> +		#address-cells = <1>;
> +		#size-cells = <0>;

> +		/* Use ID 104 as a hint that we're on physical bus 4 */
> +		i2c_104: i2c at 0 {

Does something use that hint? It sounds a little odd.

Re: [PATCH v1 4/4] i2c-mux: i2c_add_mux_adapter() should use -1 for auto bus num

[Stephen Warren]

On 02/13/2013 11:02 AM, Doug Anderson wrote:
> The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
> we don't want to force the bus number of the adapter.  This is
> non-ideal because:
> * 0 is actually a valid bus number to request
> * i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
>   -1 to mean the same thing.  That means extra logic in
>   i2c_add_mux_adapter().
> 
> Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
> accordingly.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Notes:
> - If there's a good reason that force_nr uses 0 for auto then feel
>   free to drop this patch.  I've place it at the end of the series to
>   make it easy to just drop it.

IIRC (and I only vaguely do...) it's because:

> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 9f50ef0..301ed0b 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	for (i = 0; i < mux->data.n_values; i++) {
> -		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
> +		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;

Here, mux->data.base_nr is platform data (or copied directly from it),
and any field in a platform data struct stored in a global variable not
explicitly initialized would be 0, hence 0 would typically mean "no
explicit bus number desired". Since a mux can't exist without a parent
I2C bus, it's unlikely anyone would want a mux to be I2C bus 0, but
rather the parent to have that number.

Re: [PATCH v1 4/4] i2c-mux: i2c_add_mux_adapter() should use -1 for auto bus num

[Stephen Warren]

On 02/13/2013 11:02 AM, Doug Anderson wrote:
> The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
> we don't want to force the bus number of the adapter.  This is
> non-ideal because:
> * 0 is actually a valid bus number to request
> * i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
>   -1 to mean the same thing.  That means extra logic in
>   i2c_add_mux_adapter().
> 
> Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
> accordingly.
> 
> Signed-off-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> Notes:
> - If there's a good reason that force_nr uses 0 for auto then feel
>   free to drop this patch.  I've place it at the end of the series to
>   make it easy to just drop it.

IIRC (and I only vaguely do...) it's because:

> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 9f50ef0..301ed0b 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	for (i = 0; i < mux->data.n_values; i++) {
> -		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
> +		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;

Here, mux->data.base_nr is platform data (or copied directly from it),
and any field in a platform data struct stored in a global variable not
explicitly initialized would be 0, hence 0 would typically mean "no
explicit bus number desired". Since a mux can't exist without a parent
I2C bus, it's unlikely anyone would want a mux to be I2C bus 0, but
rather the parent to have that number.

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Doug Anderson]

Stephen,

Thank you for the review.  Comments below (including changes I have
done locally).  I probably won't have time to test / repost until
tomorrow.


On Wed, Feb 13, 2013 at 1:02 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 02/13/2013 11:02 AM, Doug Anderson wrote:
>> The i2c-arbitrator driver implements the arbitration scheme that the
>> Embedded Controller (EC) on the ARM Chromebook expects to use for bus
>> multimastering.  This i2c-arbitrator driver could also be used in
>> other places where standard i2c bus arbitration can't be used and two
>> extra GPIOs are available for arbitration.
>>
>> This driver is based on code that Simon Glass added to the i2c-s3c2410
>> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
>> mux driver is as suggested by Grant Likely.  See
>> <https://patchwork.kernel.org/patch/1877311/> for some history.
>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
>
>> +This mechanism is used instead of standard i2c multimaster to avoid some of the
>> +subtle driver and silicon bugs that are often present with i2c multimaster.
>
> Being really pick here, but I2C should be capitalized in free-form text.
> There are a few other places where the comment applies.

Done.


>> +Required properties:
>> +- compatible: i2c-arbitrator
>
> That seems pretty generic. What if there are other arbitration schemes?

OK, going to go with i2c-arbitrator-cros-ec.  Hopefully that sounds OK.


>> +- bus-arbitration-gpios: Two GPIOs to use with the GPIO-based bus
>> +    arbitration protocol.  The first should be an output, and is used to
>> +    claim the I2C bus for us (AP_CLAIM).  The second should be an input and
>> +    signals that the other side wants to claim the bus (EC_CLAIM).
>
> Is it worth using two separate properties here, so they each get a
> unique name. That way, nobody has the remember which order the two GPIOs
> come in.

Yes, I think it's better too.  Done.


>> diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c
>
>> +enum {
>> +     I2C_ARB_GPIO_AP,                /* AP claims i2c bus */
>> +     I2C_ARB_GPIO_EC,                /* EC claims i2c bus */
>> +
>> +     I2C_ARB_GPIO_COUNT,
>> +};
>
> Oh, I see from later code that those are indices into the
> bus-arbitration-gpios DT property. I thought they were states in some
> state machine at first. A comment might help here, if you continue to
> use one property.

Removed this bit of code.


>> +static int i2c_arbitrator_select(struct i2c_adapter *adap, void *data, u32 chan)
>> +{
>> +     const struct i2c_arbitrator_data *arb = data;
>> +     unsigned long stop_retry, stop_time;
>> +
>> +     /* Start a round of trying to claim the bus */
>> +     stop_time = jiffies + usecs_to_jiffies(arb->wait_free_us) + 1;
>> +     do {
>> +             /* Indicate that we want to claim the bus */
>> +             gpio_set_value(arb->ap_gpio, 0);
>
> The GPIO signals appear to be active low in the code. Instead, I think
> it'd make more sense to extract the polarity of the GPIO from DT, using
> of_get_named_gpio_flags().

A little torn here.  It adds a bunch of complexity to the code to
handle this case and there are no known or anticipated users.  I only
wish that the GPIO polarity could be more hidden from drivers (add
functions like gpio_assert, gpio_deassert, etc)...

In any case, I've done it.  I used the "!!" trick a lot to convert
"zero/non-zero" to "0/1" to at least reduce the lines of code a
little.  I think this is common enough that it helps readability
rather than hurting it.


>> +static int i2c_arbitrator_probe(struct platform_device *pdev)
>
>> +     /* Request GPIOs */
>> +     ret = of_get_named_gpio(np, "bus-arbitration-gpios", I2C_ARB_GPIO_AP);
>> +     if (gpio_is_valid(ret)) {
>> +             arb->ap_gpio = ret;
>> +             ret = gpio_request_one(arb->ap_gpio, GPIOF_OUT_INIT_HIGH,
>> +                                    "bus-arbitration-ap");
>> +     }
>> +     if (WARN_ON(ret))
>> +             goto ap_request_failed;
>
> you could use devm_gpio_request_one() and save some cleanup logic.

Whoops, didn't realize that was there.  Done.


>> +     /* Arbitration parameters */
>> +     if (of_property_read_u32(np, "bus-arbitration-slew-delay-us",
>> +                              &arb->slew_delay_us))
>> +             arb->slew_delay_us = 10;
>
> The DT binding document says that property is required. Either the code
> should error out here, or the document updated to indicate that the
> properties are optional, and specify what the defaults are.

Done.  Now optional.


>> +static int i2c_arbitrator_remove(struct platform_device *pdev)
>
>> +     platform_set_drvdata(pdev, NULL);
>
> You shouldn't have to do that; nothing should care what the pdata value
> is while the device isn't probed anyway.

Done.  Code was stolen from i2c_mux_gpio_remove(), so perhaps we
should remove it there too.  I'll send up a quick cleanup patch
tomorrow.


>> +static int __init i2c_arbitrator_init(void)
>> +{
>> +     return platform_driver_register(&i2c_arbitrator_driver);
>> +}
>> +subsys_initcall(i2c_arbitrator_init);
>> +
>> +static void __exit i2c_arbitrator_exit(void)
>> +{
>> +     platform_driver_unregister(&i2c_arbitrator_driver);
>> +}
>> +module_exit(i2c_arbitrator_exit);
>
> You should be able to replace all that with:
>
> module_platform_driver(&i2c_arbitrator_driver);

Sigh.  Yeah, I had that.  ...but it ends up getting initted
significantly later in the init process and that was uncovering bugs
in other drivers where they weren't expressing their dependencies
properly.  I was going to try to fix those bugs separately but it
seemed to make some sense to prioritize this bus a little bit anyway
by using subsys_initcall().  ...but maybe that's just wrong.

Unless you think it's a bug or terrible form to use subsys_initcall()
I'd rather leave that.  Is that OK?


>> +MODULE_LICENSE("GPL");
>
> The header says GPL v2 only, so "GPL v2".

Done.

Re: [PATCH v1 2/4] ARM: dts: Add i2c-arbitrator bus for exynos5250-snow

[Doug Anderson]

Stephen,


On Wed, Feb 13, 2013 at 1:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/13/2013 11:02 AM, Doug Anderson wrote:
>> We need to use the i2c-arbitrator to talk to any of the devices on i2c
>> bus 4 on exynos5250-snow so that we don't confuse the embedded
>> controller (EC).  Add the i2c-arbitrator to the device tree.  As we
>> add future devices (keyboard, sbs, tps65090) we'll add them on top of
>> this.
>>
>> The arbitrated bus is numbered 104 simply as a convenience to make it
>> easier for people poking around to guess that it might have something
>> to do with the physical bus 4.
>>
>> The addition is split between the cros5250-common and the snow device
>> tree file since not all cros5250-class devices use arbitration.
>
>> diff --git a/arch/arm/boot/dts/cros5250-common.dtsi b/arch/arm/boot/dts/cros5250-common.dtsi
>
>>       i2c@12CA0000 {
>> -             status = "disabled";
>> +             samsung,i2c-sda-delay = <100>;
>> +             samsung,i2c-max-bus-freq = <66000>;
>
> Shouldn't that use the standard clock-frequency property?

At the moment this is the way that you specify it for the i2c
controller we're using...


>> diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
>
>> +     i2c-arbitrator {
>> +             compatible = "i2c-arbitrator";
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>
>> +             /* Use ID 104 as a hint that we're on physical bus 4 */
>> +             i2c_104: i2c@0 {
>
> Does something use that hint? It sounds a little odd.

The i2c bus numbering patches will end up creating "/dev/i2c-104".
...so the hint is really for developers who are poking around.  It's
definitely a strange number and that's part of the point.  Right now
there's nothing preventing someone from using 'i2cdetect' or 'i2cget'
on bus 4 and bypassing arbitration.  My hope was that having something
crazy like "104" would help someone realize that it's a bad idea.

...besides, if I don't come up with _some_ number then this will get
assigned the first unused ID which could be extra confusing.

-Doug

[PATCH v1 2/4] ARM: dts: Add i2c-arbitrator bus for exynos5250-snow

[Doug Anderson]

Stephen,


On Wed, Feb 13, 2013 at 1:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/13/2013 11:02 AM, Doug Anderson wrote:
>> We need to use the i2c-arbitrator to talk to any of the devices on i2c
>> bus 4 on exynos5250-snow so that we don't confuse the embedded
>> controller (EC).  Add the i2c-arbitrator to the device tree.  As we
>> add future devices (keyboard, sbs, tps65090) we'll add them on top of
>> this.
>>
>> The arbitrated bus is numbered 104 simply as a convenience to make it
>> easier for people poking around to guess that it might have something
>> to do with the physical bus 4.
>>
>> The addition is split between the cros5250-common and the snow device
>> tree file since not all cros5250-class devices use arbitration.
>
>> diff --git a/arch/arm/boot/dts/cros5250-common.dtsi b/arch/arm/boot/dts/cros5250-common.dtsi
>
>>       i2c at 12CA0000 {
>> -             status = "disabled";
>> +             samsung,i2c-sda-delay = <100>;
>> +             samsung,i2c-max-bus-freq = <66000>;
>
> Shouldn't that use the standard clock-frequency property?

At the moment this is the way that you specify it for the i2c
controller we're using...


>> diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
>
>> +     i2c-arbitrator {
>> +             compatible = "i2c-arbitrator";
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>
>> +             /* Use ID 104 as a hint that we're on physical bus 4 */
>> +             i2c_104: i2c at 0 {
>
> Does something use that hint? It sounds a little odd.

The i2c bus numbering patches will end up creating "/dev/i2c-104".
...so the hint is really for developers who are poking around.  It's
definitely a strange number and that's part of the point.  Right now
there's nothing preventing someone from using 'i2cdetect' or 'i2cget'
on bus 4 and bypassing arbitration.  My hope was that having something
crazy like "104" would help someone realize that it's a bad idea.

...besides, if I don't come up with _some_ number then this will get
assigned the first unused ID which could be extra confusing.

-Doug

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Stephen Warren]

On 02/13/2013 05:34 PM, Doug Anderson wrote:
> On Wed, Feb 13, 2013 at 1:02 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 02/13/2013 11:02 AM, Doug Anderson wrote:
>>> The i2c-arbitrator driver implements the arbitration scheme that the
>>> Embedded Controller (EC) on the ARM Chromebook expects to use for bus
>>> multimastering.  This i2c-arbitrator driver could also be used in
>>> other places where standard i2c bus arbitration can't be used and two
>>> extra GPIOs are available for arbitration.
>>>
>>> This driver is based on code that Simon Glass added to the i2c-s3c2410
>>> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
>>> mux driver is as suggested by Grant Likely.  See
>>> <https://patchwork.kernel.org/patch/1877311/> for some history.
>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt
...
>>> +Required properties:
>>> +- compatible: i2c-arbitrator
>>
>> That seems pretty generic. What if there are other arbitration schemes?
> 
> OK, going to go with i2c-arbitrator-cros-ec.  Hopefully that sounds OK.

That seems fine. The mechanism seems potentially a little more generic
than just for cros-ec though, but I guess there's no harm naming it
after the first implementation. That why "compatible" allows multiple
entries anyway.

>>> +static int i2c_arbitrator_select(struct i2c_adapter *adap, void *data, u32 chan)
>>> +{
>>> +     const struct i2c_arbitrator_data *arb = data;
>>> +     unsigned long stop_retry, stop_time;
>>> +
>>> +     /* Start a round of trying to claim the bus */
>>> +     stop_time = jiffies + usecs_to_jiffies(arb->wait_free_us) + 1;
>>> +     do {
>>> +             /* Indicate that we want to claim the bus */
>>> +             gpio_set_value(arb->ap_gpio, 0);
>>
>> The GPIO signals appear to be active low in the code. Instead, I think
>> it'd make more sense to extract the polarity of the GPIO from DT, using
>> of_get_named_gpio_flags().
> 
> A little torn here.  It adds a bunch of complexity to the code to
> handle this case and there are no known or anticipated users.  I only
> wish that the GPIO polarity could be more hidden from drivers (add
> functions like gpio_assert, gpio_deassert, etc)...

Yes, that would be nice. Alex, LinusW?

> In any case, I've done it.  I used the "!!" trick a lot to convert
> "zero/non-zero" to "0/1" to at least reduce the lines of code a
> little.  I think this is common enough that it helps readability
> rather than hurting it.

>>> +static int __init i2c_arbitrator_init(void)
>>> +{
>>> +     return platform_driver_register(&i2c_arbitrator_driver);
>>> +}
>>> +subsys_initcall(i2c_arbitrator_init);
>>> +
>>> +static void __exit i2c_arbitrator_exit(void)
>>> +{
>>> +     platform_driver_unregister(&i2c_arbitrator_driver);
>>> +}
>>> +module_exit(i2c_arbitrator_exit);
>>
>> You should be able to replace all that with:
>>
>> module_platform_driver(&i2c_arbitrator_driver);
> 
> Sigh.  Yeah, I had that.  ...but it ends up getting initted
> significantly later in the init process and that was uncovering bugs
> in other drivers where they weren't expressing their dependencies
> properly.  I was going to try to fix those bugs separately but it
> seemed to make some sense to prioritize this bus a little bit anyway
> by using subsys_initcall().  ...but maybe that's just wrong.
> 
> Unless you think it's a bug or terrible form to use subsys_initcall()
> I'd rather leave that.  Is that OK?

It's certainly a bug if it doesn't work correctly as
module_platform_driver(). It'll have to be fixed sometime.

I don't think it's a big enough issue for me to object to the patch
providing it gets fixed sometime, but I've certainly seem other people
push back harder on using subsys_initcall for expressing dependencies;
it's not very extensible - what happens if there's another bug in some
other driver that requires an even earlier level of initcall?

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Doug Anderson]

Stephen,

On Wed, Feb 13, 2013 at 4:41 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> OK, going to go with i2c-arbitrator-cros-ec.  Hopefully that sounds OK.
>
> That seems fine. The mechanism seems potentially a little more generic
> than just for cros-ec though, but I guess there's no harm naming it
> after the first implementation. That why "compatible" allows multiple
> entries anyway.

Yup, that was my thought.


>>> You should be able to replace all that with:
>>>
>>> module_platform_driver(&i2c_arbitrator_driver);
>>
>> Sigh.  Yeah, I had that.  ...but it ends up getting initted
>> significantly later in the init process and that was uncovering bugs
>> in other drivers where they weren't expressing their dependencies
>> properly.  I was going to try to fix those bugs separately but it
>> seemed to make some sense to prioritize this bus a little bit anyway
>> by using subsys_initcall().  ...but maybe that's just wrong.
>>
>> Unless you think it's a bug or terrible form to use subsys_initcall()
>> I'd rather leave that.  Is that OK?
>
> It's certainly a bug if it doesn't work correctly as
> module_platform_driver(). It'll have to be fixed sometime.
>
> I don't think it's a big enough issue for me to object to the patch
> providing it gets fixed sometime, but I've certainly seem other people
> push back harder on using subsys_initcall for expressing dependencies;
> it's not very extensible - what happens if there's another bug in some
> other driver that requires an even earlier level of initcall?

I don't like it either.  I'll give it a few hours tomorrow and
hopefully I can track down the problem.  If I can't track it down or
if I come up with a really good justification for why it's needed I'll
leave it with subsys_initcall() unless someone gives me a big nak.

-Doug

Re: [PATCH v1 2/4] ARM: dts: Add i2c-arbitrator bus for exynos5250-snow

[Stephen Warren]

On 02/13/2013 05:38 PM, Doug Anderson wrote:
> Stephen,
> 
> 
> On Wed, Feb 13, 2013 at 1:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/13/2013 11:02 AM, Doug Anderson wrote:
>>> We need to use the i2c-arbitrator to talk to any of the devices on i2c
>>> bus 4 on exynos5250-snow so that we don't confuse the embedded
>>> controller (EC).  Add the i2c-arbitrator to the device tree.  As we
>>> add future devices (keyboard, sbs, tps65090) we'll add them on top of
>>> this.
>>>
>>> The arbitrated bus is numbered 104 simply as a convenience to make it
>>> easier for people poking around to guess that it might have something
>>> to do with the physical bus 4.
>>>
>>> The addition is split between the cros5250-common and the snow device
>>> tree file since not all cros5250-class devices use arbitration.

>>> diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
>>
>>> +     i2c-arbitrator {
>>> +             compatible = "i2c-arbitrator";
>>> +             #address-cells = <1>;
>>> +             #size-cells = <0>;
>>
>>> +             /* Use ID 104 as a hint that we're on physical bus 4 */
>>> +             i2c_104: i2c@0 {
>>
>> Does something use that hint? It sounds a little odd.
> 
> The i2c bus numbering patches will end up creating "/dev/i2c-104".

Oh sorry, I see this is just the alias doing it's job. I'd misread that
as the reg value being 104 and driving the bus ID.

[PATCH v1 2/4] ARM: dts: Add i2c-arbitrator bus for exynos5250-snow

[Stephen Warren]

On 02/13/2013 05:38 PM, Doug Anderson wrote:
> Stephen,
> 
> 
> On Wed, Feb 13, 2013 at 1:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/13/2013 11:02 AM, Doug Anderson wrote:
>>> We need to use the i2c-arbitrator to talk to any of the devices on i2c
>>> bus 4 on exynos5250-snow so that we don't confuse the embedded
>>> controller (EC).  Add the i2c-arbitrator to the device tree.  As we
>>> add future devices (keyboard, sbs, tps65090) we'll add them on top of
>>> this.
>>>
>>> The arbitrated bus is numbered 104 simply as a convenience to make it
>>> easier for people poking around to guess that it might have something
>>> to do with the physical bus 4.
>>>
>>> The addition is split between the cros5250-common and the snow device
>>> tree file since not all cros5250-class devices use arbitration.

>>> diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
>>
>>> +     i2c-arbitrator {
>>> +             compatible = "i2c-arbitrator";
>>> +             #address-cells = <1>;
>>> +             #size-cells = <0>;
>>
>>> +             /* Use ID 104 as a hint that we're on physical bus 4 */
>>> +             i2c_104: i2c at 0 {
>>
>> Does something use that hint? It sounds a little odd.
> 
> The i2c bus numbering patches will end up creating "/dev/i2c-104".

Oh sorry, I see this is just the alias doing it's job. I'd misread that
as the reg value being 104 and driving the bus ID.

Re: [PATCH v1 4/4] i2c-mux: i2c_add_mux_adapter() should use -1 for auto bus num

[Jean Delvare]

On Wed, 13 Feb 2013 14:09:08 -0700, Stephen Warren wrote:
> On 02/13/2013 11:02 AM, Doug Anderson wrote:
> > The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
> > we don't want to force the bus number of the adapter.  This is
> > non-ideal because:
> > * 0 is actually a valid bus number to request
> > * i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
> >   -1 to mean the same thing.  That means extra logic in
> >   i2c_add_mux_adapter().
> > 
> > Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
> > accordingly.
> > 
> > Signed-off-by: Doug Anderson <dianders@chromium.org>
> > ---
> > Notes:
> > - If there's a good reason that force_nr uses 0 for auto then feel
> >   free to drop this patch.  I've place it at the end of the series to
> >   make it easy to just drop it.
> 
> IIRC (and I only vaguely do...) it's because:
> 
> > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> > index 9f50ef0..301ed0b 100644
> > --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> > @@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	for (i = 0; i < mux->data.n_values; i++) {
> > -		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
> > +		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;
> 
> Here, mux->data.base_nr is platform data (or copied directly from it),
> and any field in a platform data struct stored in a global variable not
> explicitly initialized would be 0, hence 0 would typically mean "no
> explicit bus number desired". Since a mux can't exist without a parent
> I2C bus, it's unlikely anyone would want a mux to be I2C bus 0, but
> rather the parent to have that number.

Yes, as I recall this is exactly the reason why the current code is the
way it is.

-- 
Jean Delvare

Re: [PATCH v1 4/4] i2c-mux: i2c_add_mux_adapter() should use -1 for auto bus num

[Jean Delvare]

On Wed, 13 Feb 2013 14:09:08 -0700, Stephen Warren wrote:
> On 02/13/2013 11:02 AM, Doug Anderson wrote:
> > The force_nr parameter to i2c_add_mux_adapter() uses 0 to signify that
> > we don't want to force the bus number of the adapter.  This is
> > non-ideal because:
> > * 0 is actually a valid bus number to request
> > * i2c_add_numbered_adapter() (which i2c_add_mux_adapter() calls) uses
> >   -1 to mean the same thing.  That means extra logic in
> >   i2c_add_mux_adapter().
> > 
> > Fix i2c_add_mux_adapter() to use -1 and update all mux drivers
> > accordingly.
> > 
> > Signed-off-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > ---
> > Notes:
> > - If there's a good reason that force_nr uses 0 for auto then feel
> >   free to drop this patch.  I've place it at the end of the series to
> >   make it easy to just drop it.
> 
> IIRC (and I only vaguely do...) it's because:
> 
> > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> > index 9f50ef0..301ed0b 100644
> > --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> > @@ -208,7 +208,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	for (i = 0; i < mux->data.n_values; i++) {
> > -		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
> > +		int nr = mux->data.base_nr ? (mux->data.base_nr + i) : -1;
> 
> Here, mux->data.base_nr is platform data (or copied directly from it),
> and any field in a platform data struct stored in a global variable not
> explicitly initialized would be 0, hence 0 would typically mean "no
> explicit bus number desired". Since a mux can't exist without a parent
> I2C bus, it's unlikely anyone would want a mux to be I2C bus 0, but
> rather the parent to have that number.

Yes, as I recall this is exactly the reason why the current code is the
way it is.

-- 
Jean Delvare

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Linus Walleij]

On Thu, Feb 14, 2013 at 1:41 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 02/13/2013 05:34 PM, Doug Anderson wrote:

>> A little torn here.  It adds a bunch of complexity to the code to
>> handle this case and there are no known or anticipated users.  I only
>> wish that the GPIO polarity could be more hidden from drivers (add
>> functions like gpio_assert, gpio_deassert, etc)...
>
> Yes, that would be nice. Alex, LinusW?

OK so good point since Alex is rewriting the way the external
API to GPIOs is done.

So this is one of the evolutionary artifacts of the GPIO subsystem:
that it has this concept of clients having to know if they want to
drive the line low or high.

Either way somewhere in the system the knowledge of whether
the low or high state counts as asserted must be stored.

The same goes for inputs really: for example we have a
platform data flag for drivers/mmc/host/mmci.c that tells
whether a card is inserted when the line goes low or high. And
this varies between platforms.

So that would lead to gpio_get_value() being rewritten
as gpio_is_asserted() as well.

We all agree that the meaning of a certain GPIO pins
high vs low state as asserter or deasserted is a machine-
specific thing, so it need to come from device tree or platform
data.

So what this is really all about is whether that knowledge
should be part of the consumer DT node/platform data
or the producer DT node/platform data.

I.e. in the MMCI case whether we encode that into the
DT node/pdata for the GPIO controller or the MMCI
controller.

A bit like whether to eat eggs from the big or little end
you could say :-)

But changing it would have very drastic effects.
Consider this snippet from
arch/arm/boot/dts/snowball.dts:

// External Micro SD slot
sdi0_per1@80126000 {
      arm,primecell-periphid = <0x10480180>;
      max-frequency = <50000000>;
      bus-width = <4>;
      mmc-cap-mmc-highspeed;
      vmmc-supply = <&ab8500_ldo_aux3_reg>;

      cd-gpios  = <&gpio6 26 0x4>; // 218
      cd-inverted;

      status = "okay";
};

Note property "cd-inverted".

It states whether the GPIO value is active high (default) or
active low (this flag set). Ironically the binding document is
incomplete but we have to support device trees like this
going forward.

How do I make sure that this device tree continue to work
as expected if we change the semantic of the GPIO subsystem
to only provide gpio_is_asserted()?

You would have to include a function call to the GPIO core
and tell it what is asserted and what is not, like
gpiod_set_assertion_polarity() so the driver can also
tell the GPIO subsystem what is asserted and what is not,
rather than encoding that at the GPIO end of things.

So all of a sudden *both* the consumers and the providers
can define assertion sematics for the pins. What happens
if they are in disagreement?

So I don't know if that is such a good idea, it just makes
everything much more complex to handle for questionable
gain.

Another issue would be with things like bit-banged buses,
I don't thing an I2C bus with inverted semantics is that
very useful, and it makes things hard to debug for the
user, it's much more clear if the bitbang is driving the
line high or low than if it's asserting or deasserting it.

Yours,
Linus Walleij

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Stephen Warren]

On 02/14/2013 03:01 AM, Linus Walleij wrote:
> On Thu, Feb 14, 2013 at 1:41 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 02/13/2013 05:34 PM, Doug Anderson wrote:
> 
>>> A little torn here.  It adds a bunch of complexity to the code to
>>> handle this case and there are no known or anticipated users.  I only
>>> wish that the GPIO polarity could be more hidden from drivers (add
>>> functions like gpio_assert, gpio_deassert, etc)...
>>
>> Yes, that would be nice. Alex, LinusW?
> 
> OK so good point since Alex is rewriting the way the external
> API to GPIOs is done.
> 
> So this is one of the evolutionary artifacts of the GPIO subsystem:
> that it has this concept of clients having to know if they want to
> drive the line low or high.
> 
> Either way somewhere in the system the knowledge of whether
> the low or high state counts as asserted must be stored.
> 
> The same goes for inputs really: for example we have a
> platform data flag for drivers/mmc/host/mmci.c that tells
> whether a card is inserted when the line goes low or high. And
> this varies between platforms.
> 
> So that would lead to gpio_get_value() being rewritten
> as gpio_is_asserted() as well.
> 
> We all agree that the meaning of a certain GPIO pins
> high vs low state as asserter or deasserted is a machine-
> specific thing, so it need to come from device tree or platform
> data.
> 
> So what this is really all about is whether that knowledge
> should be part of the consumer DT node/platform data
> or the producer DT node/platform data.
> 
> I.e. in the MMCI case whether we encode that into the
> DT node/pdata for the GPIO controller or the MMCI
> controller.
> 
> A bit like whether to eat eggs from the big or little end
> you could say :-)
> 
> But changing it would have very drastic effects.
> Consider this snippet from
> arch/arm/boot/dts/snowball.dts:
> 
> // External Micro SD slot
> sdi0_per1@80126000 {
>       arm,primecell-periphid = <0x10480180>;
>       max-frequency = <50000000>;
>       bus-width = <4>;
>       mmc-cap-mmc-highspeed;
>       vmmc-supply = <&ab8500_ldo_aux3_reg>;
> 
>       cd-gpios  = <&gpio6 26 0x4>; // 218
>       cd-inverted;
> 
>       status = "okay";
> };
> 
> Note property "cd-inverted".
> 
> It states whether the GPIO value is active high (default) or
> active low (this flag set). Ironically the binding document is
> incomplete but we have to support device trees like this
> going forward.
> 
> How do I make sure that this device tree continue to work
> as expected if we change the semantic of the GPIO subsystem
> to only provide gpio_is_asserted()?
> 
> You would have to include a function call to the GPIO core
> and tell it what is asserted and what is not, like
> gpiod_set_assertion_polarity() so the driver can also
> tell the GPIO subsystem what is asserted and what is not,
> rather than encoding that at the GPIO end of things.
> 
> So all of a sudden *both* the consumers and the providers
> can define assertion sematics for the pins. What happens
> if they are in disagreement?

I think it's actually binding-specific.

Either a binding assumed that the GPIO specifier might not include an
inversion flag, and hence included its own alternative (cd-inverted), or
it assumed that the GPIO specifier would always include this flag, and
hence relied purely on the GPIO flags.

Drivers are written to support specific bindings, and hence they know
which case they fall into.

Hence, I think we want something like:

Case 1: Just use GPIO specifier flags:

gpio = of_get_named_gpio_flags(np, name, index, &flags);
gpio_request_with_flags(gpio, flags);

Case 2: Just use binding-specific property:

gpio = of_get_named_gpio(np, name, index);
flags = 0;
if (of_property_read_bool(np, name))
    flags |= FLAG_INVERTED;
gpio_request_with_flags(gpio, flags);

Or something like that.

That seems simple enough?

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Doug Anderson]

On Wed, Feb 13, 2013 at 4:54 PM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>>> You should be able to replace all that with:
>>>>
>>>> module_platform_driver(&i2c_arbitrator_driver);
>>>
>>> Sigh.  Yeah, I had that.  ...but it ends up getting initted
>>> significantly later in the init process and that was uncovering bugs
>>> in other drivers where they weren't expressing their dependencies
>>> properly.  I was going to try to fix those bugs separately but it
>>> seemed to make some sense to prioritize this bus a little bit anyway
>>> by using subsys_initcall().  ...but maybe that's just wrong.
>>>
>>> Unless you think it's a bug or terrible form to use subsys_initcall()
>>> I'd rather leave that.  Is that OK?
>>
>> It's certainly a bug if it doesn't work correctly as
>> module_platform_driver(). It'll have to be fixed sometime.
>>
>> I don't think it's a big enough issue for me to object to the patch
>> providing it gets fixed sometime, but I've certainly seem other people
>> push back harder on using subsys_initcall for expressing dependencies;
>> it's not very extensible - what happens if there's another bug in some
>> other driver that requires an even earlier level of initcall?
>
> I don't like it either.  I'll give it a few hours tomorrow and
> hopefully I can track down the problem.  If I can't track it down or
> if I come up with a really good justification for why it's needed I'll
> leave it with subsys_initcall() unless someone gives me a big nak.

OK, so I dug into my problems here a little bit.  All of the problems
are with a private branch that includes stuff not fully upstream,
but...

The problem is that we've got a regulator (tps65090) on this bus.
Right now the first code that wants to use tps65090 runs from the
set_power() callback of "platform-lcd".  It looks like:
   lcd_fet = regulator_get(NULL, "lcd_vdd");

...and "platform-lcd" is instantiated really early via
platform_device_register() for some reason.

I tried to fix it by moving platform-lcd to actually be instantiated
via the device tree (with platform data populated through
of_platform_populate).  I then hooked up regulators through the device
tree:

    platform-lcd {
        compatible = "platform-lcd";
        vcd_led-supply = <&vcd_led>;
        lcd_vdd-supply = <&lcd_vdd>;
    };

...I verified that these worked (needed a small mod to tps65090) when
I used subsys_initcall().  AKA: I could rename lcd_vdd-supply to
doug-supply and then change the code to ask for doug-supply and it
worked so I know that the device tree regulator association worked.

...but when I moved to module_platform_driver() then things still broke.

[    1.510000] platform-lcd supply lcd_vdd not found, using dummy regulator

I was sorta hoping that there would be some magic where
regulator_get() would find the device tree node for the regulator and
then resolve the chain.  ...but maybe that's a pipe dream.


Is there some better way I should be expressing dependencies?  Do I
need to try to hack something together with -EAGAIN (ick!)?  I will
point out that the i2c bus that we're arbitrating on also registers
with subsys_initcall().


In any case, I've spent my few hours today.  It's not a waste of time
since the above learnings were good, but I haven't actually found a
way to avoid the subsys_initcall().  Unless someone can point to what
I'm missing, I'll send another patch up with subsys_initcall(),
hopefully by the end of the day...


Thanks for listening!

-Doug

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Stephen Warren]

On 02/14/2013 02:40 PM, Doug Anderson wrote:
> On Wed, Feb 13, 2013 at 4:54 PM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>>>> You should be able to replace all that with:
>>>>>
>>>>> module_platform_driver(&i2c_arbitrator_driver);
...
> OK, so I dug into my problems here a little bit.  All of the problems
> are with a private branch that includes stuff not fully upstream,
> but...
> 
> The problem is that we've got a regulator (tps65090) on this bus.
> Right now the first code that wants to use tps65090 runs from the
> set_power() callback of "platform-lcd".  It looks like:
>    lcd_fet = regulator_get(NULL, "lcd_vdd");
> 
> ...and "platform-lcd" is instantiated really early via
> platform_device_register() for some reason.
> 
> I tried to fix it by moving platform-lcd to actually be instantiated
> via the device tree (with platform data populated through
> of_platform_populate).  I then hooked up regulators through the device
> tree:

It shouldn't matter when the platform-lcd device is instantiated, so
doing it via a board file vs. a device tree shouldn't make much difference.

...
> ...but when I moved to module_platform_driver() then things still broke.
> 
> [    1.510000] platform-lcd supply lcd_vdd not found, using dummy regulator

What prints that? I assume that's some error-handling logic in the
platform-lcd driver. It's probably not detecting an -EPROBE_DEFFERED
return from regulator_get() correctly, and hence proceeding with the
probe() when it should simply return and let the kernel retry the
probe() later.

> I was sorta hoping that there would be some magic where
> regulator_get() would find the device tree node for the regulator and
> then resolve the chain.  ...but maybe that's a pipe dream.

regulator_get() won't forcibly probe() the whole dependency chain. The
idea is that if a driver tries to get a resource, and it fails with
-EPROBE_DEFER, the requesting driver should fail its own probe() with
that same error code, and the driver core will retry the failed probe()
later when the resource is hopefully available.

> Is there some better way I should be expressing dependencies?  Do I
> need to try to hack something together with -EAGAIN (ick!)?

Yes, basically. -EPROBE_DEFER specifically, although that might be an
alias for -EGAIN; I can't remember which way that went.

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Doug Anderson]

Stephen,

On Thu, Feb 14, 2013 at 3:35 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> [    1.510000] platform-lcd supply lcd_vdd not found, using dummy regulator
>
> What prints that? I assume that's some error-handling logic in the
> platform-lcd driver. It's probably not detecting an -EPROBE_DEFFERED
> return from regulator_get() correctly, and hence proceeding with the
> probe() when it should simply return and let the kernel retry the
> probe() later.

It's printed by "core.c" in drivers/regulator.

    pr_warn("%s supply %s not found, using dummy regulator\n",
             devname, id);
    rdev = dummy_regulator_rdev;
    goto found;

We can avoid that logic with has_full_constraints.  That will be some
work to get done but also should be done at some point in time.  Once
we use has_full_constraints we'll get ERR_PTR(-EPROBE_DEFER);

...but then we're SOL since the function callback that is setting this
regulator has this prototype:

    void (*set_power)(struct plat_lcd_data *, unsigned int power);

...notice a lack of an error return.  ...and actually a lack of pretty
much everything.  platform-lcd needs some lovin.


>> Is there some better way I should be expressing dependencies?  Do I
>> need to try to hack something together with -EAGAIN (ick!)?
>
> Yes, basically. -EPROBE_DEFER specifically, although that might be an
> alias for -EGAIN; I can't remember which way that went.

Fair enough; if deferring and hoping is the way that it's done then
I'll need to live with it.

...since there are a lot of yaks to shave to actually get
-EPROBE_DEFER to work (fix so that has_full_constraints works and also
fix so that platform-lcd can handle failure of lcd_set_power), I'm
still planning to send up the patch with subsys_initcall as soon as I
can.

I have filed a bug in the Chromium OS tracker to track at least the
issues with platform-lcd: http://crosbug.com/38971


-Doug

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Stephen Warren]

On 02/14/2013 04:59 PM, Doug Anderson wrote:
> Stephen,
> 
> On Thu, Feb 14, 2013 at 3:35 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>> [    1.510000] platform-lcd supply lcd_vdd not found, using dummy regulator
>>
>> What prints that? I assume that's some error-handling logic in the
>> platform-lcd driver. It's probably not detecting an -EPROBE_DEFFERED
>> return from regulator_get() correctly, and hence proceeding with the
>> probe() when it should simply return and let the kernel retry the
>> probe() later.
> 
> It's printed by "core.c" in drivers/regulator.
> 
>     pr_warn("%s supply %s not found, using dummy regulator\n",
>              devname, id);
>     rdev = dummy_regulator_rdev;
>     goto found;
> 
> We can avoid that logic with has_full_constraints.  That will be some
> work to get done but also should be done at some point in time.  Once
> we use has_full_constraints we'll get ERR_PTR(-EPROBE_DEFER);

That flag is normally set automatically:

static int __init regulator_init_complete(void)
...
        /*
         * Since DT doesn't provide an idiomatic mechanism for
         * enabling full constraints and since it's much more natural
         * with DT to provide them just assume that a DT enabled
         * system has full constraints.
         */
        if (of_have_populated_dt())
                has_full_constraints = true;

Is of_have_populated_dt() not returning true for you? Perhaps when using
the initcall, your regulator_get()s are happening before
regulator_init_complete() gets called, which is the source of the problem?

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Doug Anderson]

Stephen,

On Thu, Feb 14, 2013 at 4:16 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> We can avoid that logic with has_full_constraints.  That will be some
>> work to get done but also should be done at some point in time.  Once
>> we use has_full_constraints we'll get ERR_PTR(-EPROBE_DEFER);
>
> That flag is normally set automatically:
>
> static int __init regulator_init_complete(void)
> ...
>         /*
>          * Since DT doesn't provide an idiomatic mechanism for
>          * enabling full constraints and since it's much more natural
>          * with DT to provide them just assume that a DT enabled
>          * system has full constraints.
>          */
>         if (of_have_populated_dt())
>                 has_full_constraints = true;
>
> Is of_have_populated_dt() not returning true for you? Perhaps when using
> the initcall, your regulator_get()s are happening before
> regulator_init_complete() gets called, which is the source of the problem?

The regulator_get() calls are before regulator_init_complete() by
quite a margin.  The regulator_init_complete() is a late_initcall, so
that's not too surprising.  Are we not supposed to access regulators
until after late_init()?  That seems like quite a limitation...


Note: I did send up a v2 of the patch series that you probably saw.
It still uses subsys_initcall().  I can easily send up a v3 if you
would like.  Worst case I can carry a local patch to hack the
arbitrator driver to use subsys_initcall() until we fix everything
else.


One other argument for using subsys_initcall(), though.  Using
module_platform_driver() means that we end up going through
device_initcall().  I would argue that since we're providing a bus
we're much more of a "subsystem" than a "device".  ...or is that a BS
argument?

Certainly if you end up building this code as a module then it'll get
called late anyway, but hopefully you wouldn't do that on a system
that had really critical components on the provided bus?


Let me know if you're happy with v2 or if you'd like me to make a rev.
 Thanks!  :)

-Doug

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 789 bytes --]

On Thu, Feb 14, 2013 at 01:40:49PM -0800, Doug Anderson wrote:
> On Wed, Feb 13, 2013 at 4:54 PM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:

> ...but when I moved to module_platform_driver() then things still broke.

> [    1.510000] platform-lcd supply lcd_vdd not found, using dummy regulator

> I was sorta hoping that there would be some magic where
> regulator_get() would find the device tree node for the regulator and
> then resolve the chain.  ...but maybe that's a pipe dream.

You shouldn't have dummy regulators enabled at all, that'll break a
bunch of stuff including dependency resolution.  They're a crutch to
help get things booting not something you should be using in production.
If they're not enabled regulator_get() will defer the probe.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 454 bytes --]

On Thu, Feb 14, 2013 at 05:14:35PM -0800, Doug Anderson wrote:

> The regulator_get() calls are before regulator_init_complete() by
> quite a margin.  The regulator_init_complete() is a late_initcall, so
> that's not too surprising.  Are we not supposed to access regulators
> until after late_init()?  That seems like quite a limitation...

No, they should work fine at all points (the worst that'll happen is
that you might get a few extra deferrals).

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Linus Walleij]

On Fri, Feb 15, 2013 at 11:24 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Thu, Feb 14, 2013 at 01:40:49PM -0800, Doug Anderson wrote:
>> On Wed, Feb 13, 2013 at 4:54 PM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
>> ...but when I moved to module_platform_driver() then things still broke.
>
>> [    1.510000] platform-lcd supply lcd_vdd not found, using dummy regulator
>
>> I was sorta hoping that there would be some magic where
>> regulator_get() would find the device tree node for the regulator and
>> then resolve the chain.  ...but maybe that's a pipe dream.
>
> You shouldn't have dummy regulators enabled at all, that'll break a
> bunch of stuff including dependency resolution.  They're a crutch to
> help get things booting not something you should be using in production.
> If they're not enabled regulator_get() will defer the probe.

Hm, totally unrelated but Mark, should we consider marking the
kernel tainted when the dummy regulators are in use?
(And likewise for the dummy pinctrl I guess.)

We could use some TAINT_INCONSISTENT_RESOURCES
or something.

I was thinking it could help to draw attention to resolving the
platforms that stick dummy stuff in them and never get rid of
it.

But maybe this would be too rude?

Yours,
Linus Walleij

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Doug Anderson]

Mark / Stephen,

On Fri, Feb 15, 2013 at 2:24 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> You shouldn't have dummy regulators enabled at all, that'll break a
> bunch of stuff including dependency resolution.  They're a crutch to
> help get things booting not something you should be using in production.
> If they're not enabled regulator_get() will defer the probe.

Argh, that'll learn me.  :P  The CONFIG_REGULATOR_DUMMY=y was only
present in the "work in progress" tree that I was using to test
integration.  It's not present anywhere else.  ...so I guess that
mystery is solved.

We'll still need to figure out how to plumb EPROBE_DEFER handling into
a few more places to get the issue fully solved, but at least a bunch
of mysteries (to me) are solved.

I think v3 of the patch series is in pretty good shape till the next
person takes a gander at it.  ;)

-Doug

Re: [PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 364 bytes --]

On Fri, Feb 15, 2013 at 08:56:42PM +0100, Linus Walleij wrote:

> Hm, totally unrelated but Mark, should we consider marking the
> kernel tainted when the dummy regulators are in use?
> (And likewise for the dummy pinctrl I guess.)

It's not a bad idea, though since it tends not to result in kernel
crashes we wouldn't see the taint report as often as we'd like.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss