[PATCH v2 0/7] Add cros_ec changes for newer boards

[PATCH v2 0/7] Add cros_ec changes for newer boards

[Doug Anderson]

This series adds the most critical cros_ec changes for newer boards
using cros_ec.  Specifically:
* Fixes timing/locking issues with the previously upstreamed (but
  never used upstream) cros_ec_spi driver.
* Updates the cros_ec header file to the latest version which allows
  us to use newer EC features like i2c tunneling.
* Adds an i2c tunnel driver to allow communication to the EC's i2c
  devices.

This _doesn't_ get the EC driver fully up to speed with what's in the
current Chromium OS trees.  There are a whole slew of cleanup patches
there, an addition of an LPC transport mode, and exports of functions
to userspace.  Once these patches land and we have functionality we
can continue to pick more cleanup patches.

Changes in v2:
- Update tunnel binding as per swarren
- Removed i2c20 alias for i2c tunnel

Bill Richardson (1):
  mfd: cros_ec: Sync to the latest cros_ec_commands.h from EC sources

David Hendricks (1):
  mfd: cros_ec: spi: calculate delay between transfers correctly

Doug Anderson (5):
  mfd: cros_ec: spi: Add mutex to cros_ec_spi
  mfd: cros_ec: spi: Make the cros_ec_spi timeout more reliable
  mfd: cros_ec: spi: Increase cros_ec_spi deadline from 5ms to 100ms
  i2c: ChromeOS EC tunnel driver
  ARM: tegra: Add the EC i2c tunnel to tegra124-venice2

 .../devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt |   39 +
 arch/arm/boot/dts/tegra124-venice2.dts             |   26 +
 drivers/i2c/busses/Kconfig                         |    9 +
 drivers/i2c/busses/Makefile                        |    1 +
 drivers/i2c/busses/i2c-cros-ec-tunnel.c            |  304 ++++++
 drivers/mfd/cros_ec.c                              |    7 +-
 drivers/mfd/cros_ec_spi.c                          |   67 +-
 include/linux/mfd/cros_ec.h                        |    4 +-
 include/linux/mfd/cros_ec_commands.h               | 1128 ++++++++++++++++++--
 9 files changed, 1493 insertions(+), 92 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
 create mode 100644 drivers/i2c/busses/i2c-cros-ec-tunnel.c

-- 
1.9.1.423.g4596e3a

[PATCH v2 6/7] i2c: ChromeOS EC tunnel driver

[Doug Anderson]

On ARM Chromebooks we have a few devices that are accessed by both the
AP (the main "Application Processor") and the EC (the Embedded
Controller).  These are:
* The battery (sbs-battery).
* The power management unit tps65090.

On the original Samsung ARM Chromebook these devices were on an I2C
bus that was shared between the AP and the EC and arbitrated using
some extranal GPIOs (see i2c-arb-gpio-challenge).

The original arbitration scheme worked well enough but had some
downsides:
* It was nonstandard (not using standard I2C multimaster)
* It only worked if the EC-AP communication was I2C
* It was relatively hard to debug problems (hard to tell if i2c issues
  were caused by the EC, the AP, or some device on the bus).

On the HP Chromebook 11 the design was changed to:
* The AP/EC comms were still i2c, but the battery/tps65090 were no
  longer on the bus used for AP/EC communication.  The battery was
  exposed to the AP through a limited i2c tunnel and tps65090 was
  exposed to the AP through a custom Linux driver.

On the Samsung ARM Chromebook 2 the scheme is changed yet again, now:
* The AP/EC comms are now using SPI for faster speeds.
* The EC's i2c bus is exposed to the AP through a full i2c tunnel.

The upstream "tegra124-venice2" uses the same scheme as the Samsung
ARM Chromebook 2, though it has a different set of components on the
other side of the bus.

This driver supports the scheme used by the Samsung ARM Chromebook 2.
Future patches to this driver could add support for the battery tunnel
on the HP Chromebook 11 (and perhaps could even be used to access
tps65090 on the HP Chromebook 11 instead of using a special driver,
but I haven't researched that enough).

Signed-off-by: Vincent Palatin <vpalatin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Tested-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Tested-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v2:
- Update tunnel binding as per swarren

 .../devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt |  39 +++
 drivers/i2c/busses/Kconfig                         |   9 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-cros-ec-tunnel.c            | 304 +++++++++++++++++++++
 drivers/mfd/cros_ec.c                              |   5 +
 5 files changed, 358 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
 create mode 100644 drivers/i2c/busses/i2c-cros-ec-tunnel.c

diff --git a/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt b/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
new file mode 100644
index 0000000..898f030
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
@@ -0,0 +1,39 @@
+I2C bus that tunnels through the ChromeOS EC (cros-ec)
+======================================================
+On some ChromeOS board designs we've got a connection to the EC (embedded
+controller) but no direct connection to some devices on the other side of
+the EC (like a battery and PMIC).  To get access to those devices we need
+to tunnel our i2c commands through the EC.
+
+The node for this device should be under a cros-ec node like google,cros-ec-spi
+or google,cros-ec-i2c.
+
+
+Required properties:
+- compatible: google,cros-ec-i2c-tunnel
+- google,remote-bus: The EC bus we'd like to talk to.
+
+Optional child nodes:
+- One node per I2C device connected to the tunnelled I2C bus.
+
+
+Example:
+	cros-ec@0 {
+		compatible = "google,cros-ec-spi";
+
+		...
+
+		i2c-tunnel {
+			compatible = "google,cros-ec-i2c-tunnel";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			google,remote-bus = <0>;
+
+			battery: sbs-battery@b {
+				compatible = "sbs,sbs-battery";
+				reg = <0xb>;
+				sbs,poll-retry-count = <1>;
+			};
+		};
+	}
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index c94db1c..9a0a6cc 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -993,6 +993,15 @@ config I2C_SIBYTE
 	help
 	  Supports the SiByte SOC on-chip I2C interfaces (2 channels).
 
+config I2C_CROS_EC_TUNNEL
+	tristate "ChromeOS EC tunnel I2C bus"
+	depends on MFD_CROS_EC
+	help
+	  If you say yes here you get an I2C bus that will tunnel i2c commands
+	  through to the other side of the ChromeOS EC to the i2c bus
+	  connected there. This will work whatever the interface used to
+	  talk to the EC (SPI, I2C or LPC).
+
 config SCx200_I2C
 	tristate "NatSemi SCx200 I2C using GPIO pins (DEPRECATED)"
 	depends on SCx200_GPIO
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 18d18ff..e110ca9 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_I2C_VIPERBOARD)	+= i2c-viperboard.o
 # Other I2C/SMBus bus drivers
 obj-$(CONFIG_I2C_ACORN)		+= i2c-acorn.o
 obj-$(CONFIG_I2C_BCM_KONA)	+= i2c-bcm-kona.o
+obj-$(CONFIG_I2C_CROS_EC_TUNNEL)	+= i2c-cros-ec-tunnel.o
 obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
 obj-$(CONFIG_I2C_PCA_ISA)	+= i2c-pca-isa.o
 obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
new file mode 100644
index 0000000..7e53fcb
--- /dev/null
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -0,0 +1,304 @@
+/*
+ *  Copyright (C) 2013 Google, Inc
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ * Expose an I2C passthrough to the ChromeOS EC.
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/**
+ * struct ec_i2c_device - Driver data for I2C tunnel
+ *
+ * @dev: Device node
+ * @adap: I2C adapter
+ * @ec: Pointer to EC device
+ * @remote_bus: The EC bus number we tunnel to on the other side.
+ * @request_buf: Buffer for transmitting data; we expect most transfers to fit.
+ * @response_buf: Buffer for receiving data; we expect most transfers to fit.
+ */
+
+struct ec_i2c_device {
+	struct device *dev;
+	struct i2c_adapter adap;
+	struct cros_ec_device *ec;
+
+	u16 remote_bus;
+
+	u8 request_buf[256];
+	u8 response_buf[256];
+};
+
+/**
+ * ec_i2c_construct_message - construct a message to go to the EC
+ *
+ * This function effectively stuffs the standard i2c_msg format of Linux into
+ * a format that the EC understands.
+ *
+ * @buf: The buffer to fill.  Can pass NULL to count how many bytes the message
+ *       would be.
+ * @i2c_msgs: The i2c messages to read.
+ * @num: The number of i2c messages.
+ * @bus_num: The remote bus number we want to talk to.
+ *
+ * Returns the number of bytes that the message would take up or a negative
+ * error number.
+ */
+static int ec_i2c_construct_message(u8 *buf, const struct i2c_msg i2c_msgs[],
+				    int num, u16 bus_num)
+{
+	struct ec_params_i2c_passthru *params;
+	u8 *out_data;
+	int i;
+	int size;
+
+	size = sizeof(struct ec_params_i2c_passthru);
+	size += num * sizeof(struct ec_params_i2c_passthru_msg);
+	out_data = buf + size;
+	for (i = 0; i < num; i++)
+		if (!(i2c_msgs[i].flags & I2C_M_RD))
+			size += i2c_msgs[i].len;
+
+	/* If there is no buffer, we can't build the message */
+	if (!buf)
+		return size;
+
+	params = (struct ec_params_i2c_passthru *)buf;
+	params->port = bus_num;
+	params->num_msgs = num;
+	for (i = 0; i < num; i++) {
+		const struct i2c_msg *i2c_msg = &i2c_msgs[i];
+		struct ec_params_i2c_passthru_msg *msg = &params->msg[i];
+
+		msg->len = i2c_msg->len;
+		msg->addr_flags = i2c_msg->addr;
+
+		if (i2c_msg->flags & I2C_M_TEN)
+			msg->addr_flags |= EC_I2C_FLAG_10BIT;
+
+		if (i2c_msg->flags & I2C_M_RD) {
+			msg->addr_flags |= EC_I2C_FLAG_READ;
+		} else {
+			memcpy(out_data, i2c_msg->buf, msg->len);
+			out_data += msg->len;
+		}
+	}
+
+	return size;
+}
+
+/**
+ * ec_i2c_parse_response - Parse a response from the EC
+ *
+ * We'll take the EC's response and copy it back into msgs.
+ *
+ * @buf: The buffer to parse.  Can pass NULL to count how many bytes we expect
+ *	 the response to be. Otherwise we assume that the right number of
+ *	 bytes are available.
+ * @i2c_msgs: The i2c messages to to fill up.
+ * @num: The number of i2c messages; will be modified to include the actual
+ *	 number received.
+ *
+ * Returns the number of response bytes or a negative error number.
+ */
+static int ec_i2c_parse_response(const u8 *buf, struct i2c_msg i2c_msgs[],
+				 int *num)
+{
+	const struct ec_response_i2c_passthru *resp;
+	const u8 *in_data;
+	int size;
+	int i;
+
+	size = sizeof(struct ec_response_i2c_passthru);
+	in_data = buf + size;
+	for (i = 0; i < *num; i++)
+		if (i2c_msgs[i].flags & I2C_M_RD)
+			size += i2c_msgs[i].len;
+
+	if (buf == NULL)
+		return size;
+
+	resp = (const struct ec_response_i2c_passthru *)buf;
+	if (resp->i2c_status & EC_I2C_STATUS_TIMEOUT)
+		return -ETIMEDOUT;
+	else if (resp->i2c_status & EC_I2C_STATUS_ERROR)
+		return -EREMOTEIO;
+
+	/* Other side could send us back fewer messages, but not more */
+	if (resp->num_msgs > *num)
+		return -EPROTO;
+	*num = resp->num_msgs;
+
+	for (i = 0; i < *num; i++) {
+		struct i2c_msg *i2c_msg = &i2c_msgs[i];
+
+		if (i2c_msgs[i].flags & I2C_M_RD) {
+			memcpy(i2c_msg->buf, in_data, i2c_msg->len);
+			in_data += i2c_msg->len;
+		}
+	}
+
+	return size;
+}
+
+static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
+		       int num)
+{
+	struct ec_i2c_device *bus = adap->algo_data;
+	struct device *dev = bus->dev;
+	const u16 bus_num = bus->remote_bus;
+	int request_len;
+	int response_len;
+	u8 *request = NULL;
+	u8 *response = NULL;
+	int result;
+
+	request_len = ec_i2c_construct_message(NULL, i2c_msgs, num, bus_num);
+	if (request_len < 0) {
+		dev_warn(dev, "Error constructing message %d\n", request_len);
+		result = request_len;
+		goto exit;
+	}
+	response_len = ec_i2c_parse_response(NULL, i2c_msgs, &num);
+	if (response_len < 0) {
+		/* Unexpected; no errors should come when NULL response */
+		dev_warn(dev, "Error preparing response %d\n", response_len);
+		result = response_len;
+		goto exit;
+	}
+
+	if (request_len <= ARRAY_SIZE(bus->request_buf)) {
+		request = bus->request_buf;
+	} else {
+		request = kzalloc(request_len, GFP_KERNEL);
+		if (request == NULL) {
+			result = -ENOMEM;
+			goto exit;
+		}
+	}
+	if (response_len <= ARRAY_SIZE(bus->response_buf)) {
+		response = bus->response_buf;
+	} else {
+		response = kzalloc(response_len, GFP_KERNEL);
+		if (response == NULL) {
+			result = -ENOMEM;
+			goto exit;
+		}
+	}
+
+	ec_i2c_construct_message(request, i2c_msgs, num, bus_num);
+	result = bus->ec->command_sendrecv(bus->ec, EC_CMD_I2C_PASSTHRU,
+					   request, request_len,
+					   response, response_len);
+	if (result)
+		goto exit;
+
+	result = ec_i2c_parse_response(response, i2c_msgs, &num);
+	if (result < 0)
+		goto exit;
+
+	/* Indicate success by saying how many messages were sent */
+	result = num;
+exit:
+	if (request != bus->request_buf)
+		kfree(request);
+	if (response != bus->response_buf)
+		kfree(response);
+
+	return result;
+}
+
+static u32 ec_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm ec_i2c_algorithm = {
+	.master_xfer	= ec_i2c_xfer,
+	.functionality	= ec_i2c_functionality,
+};
+
+static int ec_i2c_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct ec_i2c_device *bus = NULL;
+	u32 remote_bus;
+	int err;
+
+	dev_dbg(dev, "Probing\n");
+
+	if (!np) {
+		dev_err(dev, "no device node\n");
+		return -ENOENT;
+	}
+
+	bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
+	if (bus == NULL) {
+		dev_err(dev, "cannot allocate bus device\n");
+		return -ENOMEM;
+	}
+
+	err = of_property_read_u32(np, "google,remote-bus", &remote_bus);
+	if (err) {
+		dev_err(dev, "Couldn't read remote-bus property\n");
+		return err;
+	}
+	bus->remote_bus = remote_bus;
+
+	bus->ec = ec;
+	bus->dev = dev;
+
+	bus->adap.owner = THIS_MODULE;
+	strlcpy(bus->adap.name, "cros-ec-i2c-tunnel", sizeof(bus->adap.name));
+	bus->adap.algo = &ec_i2c_algorithm;
+	bus->adap.algo_data = bus;
+	bus->adap.dev.parent = &pdev->dev;
+	bus->adap.dev.of_node = np;
+
+	err = i2c_add_adapter(&bus->adap);
+	if (err) {
+		dev_err(dev, "cannot register i2c adapter\n");
+		return err;
+	}
+	platform_set_drvdata(pdev, bus);
+
+	dev_dbg(dev, "ChromeOS EC I2C tunnel adapter\n");
+
+	return err;
+}
+
+static int ec_i2c_remove(struct platform_device *dev)
+{
+	struct ec_i2c_device *bus = platform_get_drvdata(dev);
+
+	platform_set_drvdata(dev, NULL);
+
+	i2c_del_adapter(&bus->adap);
+
+	return 0;
+}
+
+static struct platform_driver ec_i2c_tunnel_driver = {
+	.probe = ec_i2c_probe,
+	.remove = ec_i2c_remove,
+	.driver = {
+		.name = "cros-ec-i2c-tunnel",
+	},
+};
+
+module_platform_driver(ec_i2c_tunnel_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("EC I2C tunnel driver");
+MODULE_ALIAS("platform:cros-ec-i2c-tunnel");
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index c58ab96..61bc909 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -90,6 +90,11 @@ static const struct mfd_cell cros_devs[] = {
 		.id = 1,
 		.of_compatible = "google,cros-ec-keyb",
 	},
+	{
+		.name = "cros-ec-i2c-tunnel",
+		.id = 2,
+		.of_compatible = "google,cros-ec-i2c-tunnel",
+	},
 };
 
 int cros_ec_register(struct cros_ec_device *ec_dev)
-- 
1.9.1.423.g4596e3a

[PATCH v2 7/7] ARM: tegra: Add the EC i2c tunnel to tegra124-venice2

[Doug Anderson]

This adds the EC i2c tunnel (and devices under it) to the
tegra124-venice2 device tree.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Tested-by: Andrew Bresticker <abrestic@chromium.org>
Tested-by: Stephen Warren <swarren@nvidia.com>
---
Changes in v2:
- Removed i2c20 alias for i2c tunnel

 arch/arm/boot/dts/tegra124-venice2.dts | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124-venice2.dts b/arch/arm/boot/dts/tegra124-venice2.dts
index c17283c..89cf776 100644
--- a/arch/arm/boot/dts/tegra124-venice2.dts
+++ b/arch/arm/boot/dts/tegra124-venice2.dts
@@ -813,6 +813,32 @@
 
 			google,cros-ec-spi-msg-delay = <2000>;
 
+			i2c-tunnel {
+				compatible = "google,cros-ec-i2c-tunnel";
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				google,remote-bus = <0>;
+
+				charger: bq24735@9 {
+					compatible = "ti,bq24735";
+					reg = <0x9>;
+					interrupt-parent = <&gpio>;
+					interrupts = <TEGRA_GPIO(J, 0)
+							GPIO_ACTIVE_HIGH>;
+					ti,ac-detect-gpios = <&gpio
+							TEGRA_GPIO(J, 0)
+							GPIO_ACTIVE_HIGH>;
+				};
+
+				battery: sbs-battery@b {
+					compatible = "sbs,sbs-battery";
+					reg = <0xb>;
+					sbs,i2c-retry-count = <2>;
+					sbs,poll-retry-count = <1>;
+				};
+			};
+
 			cros-ec-keyb {
 				compatible = "google,cros-ec-keyb";
 				keypad,num-rows = <8>;
-- 
1.9.1.423.g4596e3a

Re: [PATCH v2 0/7] Add cros_ec changes for newer boards

[Lee Jones]

On Tue, 22 Apr 2014, Doug Anderson wrote:

> This series adds the most critical cros_ec changes for newer boards
> using cros_ec.  Specifically:
> * Fixes timing/locking issues with the previously upstreamed (but
>   never used upstream) cros_ec_spi driver.
> * Updates the cros_ec header file to the latest version which allows
>   us to use newer EC features like i2c tunneling.
> * Adds an i2c tunnel driver to allow communication to the EC's i2c
>   devices.
> 
> This _doesn't_ get the EC driver fully up to speed with what's in the
> current Chromium OS trees.  There are a whole slew of cleanup patches
> there, an addition of an LPC transport mode, and exports of functions
> to userspace.  Once these patches land and we have functionality we
> can continue to pick more cleanup patches.
> 
> Changes in v2:
> - Update tunnel binding as per swarren
> - Removed i2c20 alias for i2c tunnel
> 
> Bill Richardson (1):
>   mfd: cros_ec: Sync to the latest cros_ec_commands.h from EC sources
> 
> David Hendricks (1):
>   mfd: cros_ec: spi: calculate delay between transfers correctly
> 
> Doug Anderson (5):
>   mfd: cros_ec: spi: Add mutex to cros_ec_spi
>   mfd: cros_ec: spi: Make the cros_ec_spi timeout more reliable
>   mfd: cros_ec: spi: Increase cros_ec_spi deadline from 5ms to 100ms
>   i2c: ChromeOS EC tunnel driver
>   ARM: tegra: Add the EC i2c tunnel to tegra124-venice2
> 
>  .../devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt |   39 +
>  arch/arm/boot/dts/tegra124-venice2.dts             |   26 +
>  drivers/i2c/busses/Kconfig                         |    9 +
>  drivers/i2c/busses/Makefile                        |    1 +
>  drivers/i2c/busses/i2c-cros-ec-tunnel.c            |  304 ++++++
>  drivers/mfd/cros_ec.c                              |    7 +-
>  drivers/mfd/cros_ec_spi.c                          |   67 +-
>  include/linux/mfd/cros_ec.h                        |    4 +-
>  include/linux/mfd/cros_ec_commands.h               | 1128 ++++++++++++++++++--
>  9 files changed, 1493 insertions(+), 92 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
>  create mode 100644 drivers/i2c/busses/i2c-cros-ec-tunnel.c

Need to wait for the ARM, DT and I2C guys to review, at which point
I'll be happy to take in and supply a branch for them to pull from if
required. If there are no _true_ dependencies and the MFD changes can
be added independently without fear of build breakages, let me know
and I'll apply them separately.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 6/7] i2c: ChromeOS EC tunnel driver

[Lee Jones]

On Tue, 22 Apr 2014, Doug Anderson wrote:

> On ARM Chromebooks we have a few devices that are accessed by both the
> AP (the main "Application Processor") and the EC (the Embedded
> Controller).  These are:
> * The battery (sbs-battery).
> * The power management unit tps65090.
> 
> On the original Samsung ARM Chromebook these devices were on an I2C
> bus that was shared between the AP and the EC and arbitrated using
> some extranal GPIOs (see i2c-arb-gpio-challenge).
> 
> The original arbitration scheme worked well enough but had some
> downsides:
> * It was nonstandard (not using standard I2C multimaster)
> * It only worked if the EC-AP communication was I2C
> * It was relatively hard to debug problems (hard to tell if i2c issues
>   were caused by the EC, the AP, or some device on the bus).
> 
> On the HP Chromebook 11 the design was changed to:
> * The AP/EC comms were still i2c, but the battery/tps65090 were no
>   longer on the bus used for AP/EC communication.  The battery was
>   exposed to the AP through a limited i2c tunnel and tps65090 was
>   exposed to the AP through a custom Linux driver.
> 
> On the Samsung ARM Chromebook 2 the scheme is changed yet again, now:
> * The AP/EC comms are now using SPI for faster speeds.
> * The EC's i2c bus is exposed to the AP through a full i2c tunnel.
> 
> The upstream "tegra124-venice2" uses the same scheme as the Samsung
> ARM Chromebook 2, though it has a different set of components on the
> other side of the bus.
> 
> This driver supports the scheme used by the Samsung ARM Chromebook 2.
> Future patches to this driver could add support for the battery tunnel
> on the HP Chromebook 11 (and perhaps could even be used to access
> tps65090 on the HP Chromebook 11 instead of using a special driver,
> but I haven't researched that enough).
> 
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Tested-by: Andrew Bresticker <abrestic@chromium.org>
> Tested-by: Stephen Warren <swarren@nvidia.com>
> ---
> Changes in v2:
> - Update tunnel binding as per swarren
> 
>  .../devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt |  39 +++
>  drivers/i2c/busses/Kconfig                         |   9 +
>  drivers/i2c/busses/Makefile                        |   1 +
>  drivers/i2c/busses/i2c-cros-ec-tunnel.c            | 304 +++++++++++++++++++++
>  drivers/mfd/cros_ec.c                              |   5 +

For the MFD changes:
  Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 0/7] Add cros_ec changes for newer boards

[Stephen Warren]

On 04/23/2014 06:32 AM, Lee Jones wrote:
> On Tue, 22 Apr 2014, Doug Anderson wrote:
> 
>> This series adds the most critical cros_ec changes for newer boards
>> using cros_ec.  Specifically:
>> * Fixes timing/locking issues with the previously upstreamed (but
>>   never used upstream) cros_ec_spi driver.
>> * Updates the cros_ec header file to the latest version which allows
>>   us to use newer EC features like i2c tunneling.
>> * Adds an i2c tunnel driver to allow communication to the EC's i2c
>>   devices.
>>
>> This _doesn't_ get the EC driver fully up to speed with what's in the
>> current Chromium OS trees.  There are a whole slew of cleanup patches
>> there, an addition of an LPC transport mode, and exports of functions
>> to userspace.  Once these patches land and we have functionality we
>> can continue to pick more cleanup patches.
...
> Need to wait for the ARM, DT and I2C guys to review, at which point
> I'll be happy to take in and supply a branch for them to pull from if
> required. If there are no _true_ dependencies and the MFD changes can
> be added independently without fear of build breakages, let me know
> and I'll apply them separately.

I believe there aren't direct dependencies between the patches. So, the
MFD patches can be applied to the MFD tree and the DT patch applied to
the Tegra tree. I'm simply waiting for the MFD patches to be applied
before applying the DT patch so that I know the DT binding definition is
fully accepted before applying a patch that uses it.

Re: [PATCH v2 0/7] Add cros_ec changes for newer boards

[Doug Anderson]

Hi,

On Wed, Apr 23, 2014 at 9:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/23/2014 06:32 AM, Lee Jones wrote:
>> On Tue, 22 Apr 2014, Doug Anderson wrote:
>>
>>> This series adds the most critical cros_ec changes for newer boards
>>> using cros_ec.  Specifically:
>>> * Fixes timing/locking issues with the previously upstreamed (but
>>>   never used upstream) cros_ec_spi driver.
>>> * Updates the cros_ec header file to the latest version which allows
>>>   us to use newer EC features like i2c tunneling.
>>> * Adds an i2c tunnel driver to allow communication to the EC's i2c
>>>   devices.
>>>
>>> This _doesn't_ get the EC driver fully up to speed with what's in the
>>> current Chromium OS trees.  There are a whole slew of cleanup patches
>>> there, an addition of an LPC transport mode, and exports of functions
>>> to userspace.  Once these patches land and we have functionality we
>>> can continue to pick more cleanup patches.
> ...
>> Need to wait for the ARM, DT and I2C guys to review, at which point
>> I'll be happy to take in and supply a branch for them to pull from if
>> required. If there are no _true_ dependencies and the MFD changes can
>> be added independently without fear of build breakages, let me know
>> and I'll apply them separately.
>
> I believe there aren't direct dependencies between the patches. So, the
> MFD patches can be applied to the MFD tree and the DT patch applied to
> the Tegra tree. I'm simply waiting for the MFD patches to be applied
> before applying the DT patch so that I know the DT binding definition is
> fully accepted before applying a patch that uses it.

All of the MFD patches are safe to apply and in pretty much arbitrary
order.  The strong dependencies in the chain are:

* We need patch #5 (mfd: cros_ec: Sync to the latest
cros_ec_commands.h from EC sources) before the i2c tunnel can compile.

* As Stephen says, he shouldn't apply the device tree until we're
confident that the bindings are right.  However there's no strong
dependency otherwise.

* Patches #1 #2 and #3 are simply reliability fixes.  Those could land
at any point in time and will improve other users of cros_ec_spi (like
the keyboard on tegra124-venice2).

* Patch #4 can apply any time with no issues.  Without it large i2c
tunnel transfers won't work, but that's not a terrible problem (all
normal transfers are small).

---

All that being said, I'd request that you merge patches #1-#4 as soon
as you can and make sure you can provide a way that Wolfram can pull
them (or at least patch #4) into his i2c tree to keep them applying
when he is ready to land #5.

-Doug

Re: [PATCH v2 0/7] Add cros_ec changes for newer boards

[Doug Anderson]

Hi,

On Wed, Apr 23, 2014 at 9:32 AM, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Wed, Apr 23, 2014 at 9:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 04/23/2014 06:32 AM, Lee Jones wrote:
>>> On Tue, 22 Apr 2014, Doug Anderson wrote:
>>>
>>>> This series adds the most critical cros_ec changes for newer boards
>>>> using cros_ec.  Specifically:
>>>> * Fixes timing/locking issues with the previously upstreamed (but
>>>>   never used upstream) cros_ec_spi driver.
>>>> * Updates the cros_ec header file to the latest version which allows
>>>>   us to use newer EC features like i2c tunneling.
>>>> * Adds an i2c tunnel driver to allow communication to the EC's i2c
>>>>   devices.
>>>>
>>>> This _doesn't_ get the EC driver fully up to speed with what's in the
>>>> current Chromium OS trees.  There are a whole slew of cleanup patches
>>>> there, an addition of an LPC transport mode, and exports of functions
>>>> to userspace.  Once these patches land and we have functionality we
>>>> can continue to pick more cleanup patches.
>> ...
>>> Need to wait for the ARM, DT and I2C guys to review, at which point
>>> I'll be happy to take in and supply a branch for them to pull from if
>>> required. If there are no _true_ dependencies and the MFD changes can
>>> be added independently without fear of build breakages, let me know
>>> and I'll apply them separately.
>>
>> I believe there aren't direct dependencies between the patches. So, the
>> MFD patches can be applied to the MFD tree and the DT patch applied to
>> the Tegra tree. I'm simply waiting for the MFD patches to be applied
>> before applying the DT patch so that I know the DT binding definition is
>> fully accepted before applying a patch that uses it.
>
> All of the MFD patches are safe to apply and in pretty much arbitrary
> order.  The strong dependencies in the chain are:
>
> * We need patch #5 (mfd: cros_ec: Sync to the latest
> cros_ec_commands.h from EC sources) before the i2c tunnel can compile.
>
> * As Stephen says, he shouldn't apply the device tree until we're
> confident that the bindings are right.  However there's no strong
> dependency otherwise.
>
> * Patches #1 #2 and #3 are simply reliability fixes.  Those could land
> at any point in time and will improve other users of cros_ec_spi (like
> the keyboard on tegra124-venice2).
>
> * Patch #4 can apply any time with no issues.  Without it large i2c
> tunnel transfers won't work, but that's not a terrible problem (all
> normal transfers are small).
>
> ---
>
> All that being said, I'd request that you merge patches #1-#4 as soon
> as you can and make sure you can provide a way that Wolfram can pull
> them (or at least patch #4) into his i2c tree to keep them applying
> when he is ready to land #5.

Oops, I missed a patch.  Let me say that again.

Patch #5 (latest ec commands) can also apply at any time with no
issues, but it's needed for patch #6 (the tunnel) to compile.

All that being said, I'd request that you merge patches #1-#5 as soon
as you can and make sure you can provide a way that Wolfram can pull
them (or at least patch #5) into his i2c tree to keep them applying
when he is ready to land #6.

Re: [PATCH v2 6/7] i2c: ChromeOS EC tunnel driver

[Doug Anderson]

Wolfram,

On Tue, Apr 22, 2014 at 9:45 AM, Doug Anderson <dianders@chromium.org> wrote:
> On ARM Chromebooks we have a few devices that are accessed by both the
> AP (the main "Application Processor") and the EC (the Embedded
> Controller).  These are:
> * The battery (sbs-battery).
> * The power management unit tps65090.
>
> On the original Samsung ARM Chromebook these devices were on an I2C
> bus that was shared between the AP and the EC and arbitrated using
> some extranal GPIOs (see i2c-arb-gpio-challenge).
>
> The original arbitration scheme worked well enough but had some
> downsides:
> * It was nonstandard (not using standard I2C multimaster)
> * It only worked if the EC-AP communication was I2C
> * It was relatively hard to debug problems (hard to tell if i2c issues
>   were caused by the EC, the AP, or some device on the bus).
>
> On the HP Chromebook 11 the design was changed to:
> * The AP/EC comms were still i2c, but the battery/tps65090 were no
>   longer on the bus used for AP/EC communication.  The battery was
>   exposed to the AP through a limited i2c tunnel and tps65090 was
>   exposed to the AP through a custom Linux driver.
>
> On the Samsung ARM Chromebook 2 the scheme is changed yet again, now:
> * The AP/EC comms are now using SPI for faster speeds.
> * The EC's i2c bus is exposed to the AP through a full i2c tunnel.
>
> The upstream "tegra124-venice2" uses the same scheme as the Samsung
> ARM Chromebook 2, though it has a different set of components on the
> other side of the bus.
>
> This driver supports the scheme used by the Samsung ARM Chromebook 2.
> Future patches to this driver could add support for the battery tunnel
> on the HP Chromebook 11 (and perhaps could even be used to access
> tps65090 on the HP Chromebook 11 instead of using a special driver,
> but I haven't researched that enough).
>
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Tested-by: Andrew Bresticker <abrestic@chromium.org>
> Tested-by: Stephen Warren <swarren@nvidia.com>
> ---
> Changes in v2:
> - Update tunnel binding as per swarren
>
>  .../devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt |  39 +++
>  drivers/i2c/busses/Kconfig                         |   9 +
>  drivers/i2c/busses/Makefile                        |   1 +
>  drivers/i2c/busses/i2c-cros-ec-tunnel.c            | 304 +++++++++++++++++++++
>  drivers/mfd/cros_ec.c                              |   5 +
>  5 files changed, 358 insertions(+)

Did you have any thoughts on this i2c driver?  Please let me know and
I'd be happy to spin with changes.

-Doug

Re: [PATCH v2 0/7] Add cros_ec changes for newer boards

[Lee Jones]

[...]

> >>> Need to wait for the ARM, DT and I2C guys to review, at which point
> >>> I'll be happy to take in and supply a branch for them to pull from if
> >>> required. If there are no _true_ dependencies and the MFD changes can
> >>> be added independently without fear of build breakages, let me know
> >>> and I'll apply them separately.
> >>
> >> I believe there aren't direct dependencies between the patches. So, the
> >> MFD patches can be applied to the MFD tree and the DT patch applied to
> >> the Tegra tree. I'm simply waiting for the MFD patches to be applied
> >> before applying the DT patch so that I know the DT binding definition is
> >> fully accepted before applying a patch that uses it.
> >
> > All of the MFD patches are safe to apply and in pretty much arbitrary
> > order.  The strong dependencies in the chain are:
> >
> > * We need patch #5 (mfd: cros_ec: Sync to the latest
> > cros_ec_commands.h from EC sources) before the i2c tunnel can compile.
> >
> > * As Stephen says, he shouldn't apply the device tree until we're
> > confident that the bindings are right.  However there's no strong
> > dependency otherwise.
> >
> > * Patches #1 #2 and #3 are simply reliability fixes.  Those could land
> > at any point in time and will improve other users of cros_ec_spi (like
> > the keyboard on tegra124-venice2).
> >
> > * Patch #4 can apply any time with no issues.  Without it large i2c
> > tunnel transfers won't work, but that's not a terrible problem (all
> > normal transfers are small).
> 
> Patch #5 (latest ec commands) can also apply at any time with no
> issues, but it's needed for patch #6 (the tunnel) to compile.
> 
> All that being said, I'd request that you merge patches #1-#5 as soon
> as you can and make sure you can provide a way that Wolfram can pull
> them (or at least patch #5) into his i2c tree to keep them applying
> when he is ready to land #6.

Very well. So if I can obtain Wolfram's Ack, I can apply the MFD
changes along with patch #6 and supply him with a branch.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/7] Add cros_ec changes for newer boards

[Doug Anderson]

Lee,

On Mon, Apr 28, 2014 at 2:19 AM, Lee Jones <lee.jones@linaro.org> wrote:
> [...]
>
>> >>> Need to wait for the ARM, DT and I2C guys to review, at which point
>> >>> I'll be happy to take in and supply a branch for them to pull from if
>> >>> required. If there are no _true_ dependencies and the MFD changes can
>> >>> be added independently without fear of build breakages, let me know
>> >>> and I'll apply them separately.
>> >>
>> >> I believe there aren't direct dependencies between the patches. So, the
>> >> MFD patches can be applied to the MFD tree and the DT patch applied to
>> >> the Tegra tree. I'm simply waiting for the MFD patches to be applied
>> >> before applying the DT patch so that I know the DT binding definition is
>> >> fully accepted before applying a patch that uses it.
>> >
>> > All of the MFD patches are safe to apply and in pretty much arbitrary
>> > order.  The strong dependencies in the chain are:
>> >
>> > * We need patch #5 (mfd: cros_ec: Sync to the latest
>> > cros_ec_commands.h from EC sources) before the i2c tunnel can compile.
>> >
>> > * As Stephen says, he shouldn't apply the device tree until we're
>> > confident that the bindings are right.  However there's no strong
>> > dependency otherwise.
>> >
>> > * Patches #1 #2 and #3 are simply reliability fixes.  Those could land
>> > at any point in time and will improve other users of cros_ec_spi (like
>> > the keyboard on tegra124-venice2).
>> >
>> > * Patch #4 can apply any time with no issues.  Without it large i2c
>> > tunnel transfers won't work, but that's not a terrible problem (all
>> > normal transfers are small).
>>
>> Patch #5 (latest ec commands) can also apply at any time with no
>> issues, but it's needed for patch #6 (the tunnel) to compile.
>>
>> All that being said, I'd request that you merge patches #1-#5 as soon
>> as you can and make sure you can provide a way that Wolfram can pull
>> them (or at least patch #5) into his i2c tree to keep them applying
>> when he is ready to land #6.
>
> Very well. So if I can obtain Wolfram's Ack, I can apply the MFD
> changes along with patch #6 and supply him with a branch.

Can you explain the reason to wait for Wolfram's Ack before applying
#1 - #5?  I would think:

1. Create a topic branch.
2. Apply patches 1-5 to the topic branch
3. Merge the topic branch to your for-next branch

When Wolfram wants to take patch #6, he can either:
A. Pull your topic branch
B. If it's been long enough, patches will already be in ToT and no extra work.

If I understand correctly, using a topic branch and doing merges /
pulls means that you can provide Wolfram with stable git hashes when
he needs them and there will be no merge conflicts.

Patches #1 - #5 are bonafide bugfixes irrespective of the i2c tunnel.

-Doug

Re: [PATCH v2 0/7] Add cros_ec changes for newer boards

[Lee Jones]

> >> >>> Need to wait for the ARM, DT and I2C guys to review, at which point
> >> >>> I'll be happy to take in and supply a branch for them to pull from if
> >> >>> required. If there are no _true_ dependencies and the MFD changes can
> >> >>> be added independently without fear of build breakages, let me know
> >> >>> and I'll apply them separately.
> >> >>
> >> >> I believe there aren't direct dependencies between the patches. So, the
> >> >> MFD patches can be applied to the MFD tree and the DT patch applied to
> >> >> the Tegra tree. I'm simply waiting for the MFD patches to be applied
> >> >> before applying the DT patch so that I know the DT binding definition is
> >> >> fully accepted before applying a patch that uses it.
> >> >
> >> > All of the MFD patches are safe to apply and in pretty much arbitrary
> >> > order.  The strong dependencies in the chain are:
> >> >
> >> > * We need patch #5 (mfd: cros_ec: Sync to the latest
> >> > cros_ec_commands.h from EC sources) before the i2c tunnel can compile.
> >> >
> >> > * As Stephen says, he shouldn't apply the device tree until we're
> >> > confident that the bindings are right.  However there's no strong
> >> > dependency otherwise.
> >> >
> >> > * Patches #1 #2 and #3 are simply reliability fixes.  Those could land
> >> > at any point in time and will improve other users of cros_ec_spi (like
> >> > the keyboard on tegra124-venice2).
> >> >
> >> > * Patch #4 can apply any time with no issues.  Without it large i2c
> >> > tunnel transfers won't work, but that's not a terrible problem (all
> >> > normal transfers are small).
> >>
> >> Patch #5 (latest ec commands) can also apply at any time with no
> >> issues, but it's needed for patch #6 (the tunnel) to compile.
> >>
> >> All that being said, I'd request that you merge patches #1-#5 as soon
> >> as you can and make sure you can provide a way that Wolfram can pull
> >> them (or at least patch #5) into his i2c tree to keep them applying
> >> when he is ready to land #6.
> >
> > Very well. So if I can obtain Wolfram's Ack, I can apply the MFD
> > changes along with patch #6 and supply him with a branch.
> 
> Can you explain the reason to wait for Wolfram's Ack before applying
> #1 - #5?  I would think:
> 
> 1. Create a topic branch.
> 2. Apply patches 1-5 to the topic branch
> 3. Merge the topic branch to your for-next branch
> 
> When Wolfram wants to take patch #6, he can either:
> A. Pull your topic branch
> B. If it's been long enough, patches will already be in ToT and no extra work.
> 
> If I understand correctly, using a topic branch and doing merges /
> pulls means that you can provide Wolfram with stable git hashes when
> he needs them and there will be no merge conflicts.

I don't use TBs for MFD yet, as I've never seen the need.  The current
WoW is to only create extra branches when I have patch{es, sets} to
share.  If I start using a more TB focused methodology it will be
insinuated that the branches are stable - I like the fact that this is
_not_ the case.  Currently I am able to rebase, rework and reorder the
repo as and when I see fit, and do regularly. Except the IBs of course.

> Patches #1 - #5 are bonafide bugfixes irrespective of the i2c tunnel.

I only want to create an IB if I know it's going to be used, else I'd
prefer the patches remain transient.  Why are you so keen to rush into
having these patches applied?  They _will_ make it into v3.15, whether
they are applied immediately or after a length of time (in the case
that Wolfram does not respond).

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/7] i2c: ChromeOS EC tunnel driver

[Wolfram Sang]

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

Hi,

just a basic review to keep things rolling...

> On the original Samsung ARM Chromebook these devices were on an I2C
> bus that was shared between the AP and the EC and arbitrated using
> some extranal GPIOs (see i2c-arb-gpio-challenge).
> 
> The original arbitration scheme worked well enough but had some
> downsides:
> * It was nonstandard (not using standard I2C multimaster)
> * It only worked if the EC-AP communication was I2C
> * It was relatively hard to debug problems (hard to tell if i2c issues
>   were caused by the EC, the AP, or some device on the bus).
> 
> On the HP Chromebook 11 the design was changed to:

This paragraph would be a nice update for the gpio-arbitration docs.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt b/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

The bindings should independently be sent to the devicetree list.

> new file mode 100644
> index 0000000..898f030
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
> @@ -0,0 +1,39 @@
> +I2C bus that tunnels through the ChromeOS EC (cros-ec)
> +======================================================
> +On some ChromeOS board designs we've got a connection to the EC (embedded
> +controller) but no direct connection to some devices on the other side of
> +the EC (like a battery and PMIC).  To get access to those devices we need
> +to tunnel our i2c commands through the EC.
> +
> +The node for this device should be under a cros-ec node like google,cros-ec-spi
> +or google,cros-ec-i2c.
> +
> +
> +Required properties:
> +- compatible: google,cros-ec-i2c-tunnel
> +- google,remote-bus: The EC bus we'd like to talk to.
> +
> +Optional child nodes:
> +- One node per I2C device connected to the tunnelled I2C bus.
> +
> +
> +Example:
> +	cros-ec@0 {
> +		compatible = "google,cros-ec-spi";
> +
> +		...
> +
> +		i2c-tunnel {
> +			compatible = "google,cros-ec-i2c-tunnel";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			google,remote-bus = <0>;
> +
> +			battery: sbs-battery@b {
> +				compatible = "sbs,sbs-battery";
> +				reg = <0xb>;
> +				sbs,poll-retry-count = <1>;
> +			};
> +		};
> +	}

Can the tunnel have n busses? How to represent them then? I think the
remote-bus property should go in favor of proper sub-nodes? Wouldn't it
also be more generic to have the tunnel node seperate and reference the
tunnel-mechanism (spi here) as a phandle?

> +/**
> + * ec_i2c_construct_message - construct a message to go to the EC
> + *
> + * This function effectively stuffs the standard i2c_msg format of Linux into
> + * a format that the EC understands.
> + *
> + * @buf: The buffer to fill.  Can pass NULL to count how many bytes the message
> + *       would be.

I wonder about this NULL thing. That means the size is calculated twice.
Why not make two functions instead, one fir size calc, one for setting
up?

> +/**
> + * ec_i2c_parse_response - Parse a response from the EC
> + *
> + * We'll take the EC's response and copy it back into msgs.
> + *
> + * @buf: The buffer to parse.  Can pass NULL to count how many bytes we expect
> + *	 the response to be. Otherwise we assume that the right number of
> + *	 bytes are available.

Ditto.

> +	result = bus->ec->command_sendrecv(bus->ec, EC_CMD_I2C_PASSTHRU,
> +					   request, request_len,
> +					   response, response_len);

This function pointer should be checked against NULL in probe, I
think.

> +static int ec_i2c_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
> +	struct device *dev = &pdev->dev;
> +	struct ec_i2c_device *bus = NULL;
> +	u32 remote_bus;
> +	int err;
> +
> +	dev_dbg(dev, "Probing\n");

Drop. Device core has it already.

> +
> +	if (!np) {
> +		dev_err(dev, "no device node\n");
> +		return -ENOENT;
> +	}

Can this happen?

> +
> +	bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
> +	if (bus == NULL) {
> +		dev_err(dev, "cannot allocate bus device\n");

No need for error strings when allocating.

> +		return -ENOMEM;
> +	}
> +
> +	dev_dbg(dev, "ChromeOS EC I2C tunnel adapter\n");

Drop. Device core debug has it, too.

> +
> +	return err;
> +}
> +
> +static int ec_i2c_remove(struct platform_device *dev)
> +{
> +	struct ec_i2c_device *bus = platform_get_drvdata(dev);
> +
> +	platform_set_drvdata(dev, NULL);

Not needed.

> +
> +	i2c_del_adapter(&bus->adap);
> +
> +	return 0;
> +}
> +

Regards,

   Wolfram


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

Re: [PATCH v2 0/7] Add cros_ec changes for newer boards

[Doug Anderson]

Lee,

On Tue, Apr 29, 2014 at 1:21 AM, Lee Jones <lee.jones@linaro.org> wrote:
> I don't use TBs for MFD yet, as I've never seen the need.  The current
> WoW is to only create extra branches when I have patch{es, sets} to
> share.  If I start using a more TB focused methodology it will be
> insinuated that the branches are stable - I like the fact that this is
> _not_ the case.  Currently I am able to rebase, rework and reorder the
> repo as and when I see fit, and do regularly. Except the IBs of course.

OK.

>> Patches #1 - #5 are bonafide bugfixes irrespective of the i2c tunnel.
>
> I only want to create an IB if I know it's going to be used, else I'd
> prefer the patches remain transient.  Why are you so keen to rush into
> having these patches applied?  They _will_ make it into v3.15, whether
> they are applied immediately or after a length of time (in the case
> that Wolfram does not respond).

No strong reason, and it's actually not even a huge deal if they make
it to 3.15 or in 3.16.  Having outstanding patches simply increases
the number of things that I need to keep track of / check up on.  If
patches are good to go and reviewed I like to get them landed.

Another reason I'd love to see patches landed sooner is that it will
unblock me sending the next set of patches up.  I collected all of the
most important patches in this series, but there are a bunch of other
patches in our tree that would be nice to eventually send up.  At the
moment I'm in a position where I can dedicate a reasonable amount of
time to upstreaming.  It's likely that before long I will get sucked
into tight deadlines and will have to squeeze upstreaming in among
other priorities.

I see a response from Wolfram now, so I'll spin a V2 in the next day
or two with changes to the tunnel driver.  I'm at ELC so my hacking
time may be limited.

-Doug

Re: [PATCH v2 6/7] i2c: ChromeOS EC tunnel driver

[Doug Anderson]

Wolfram,

On Tue, Apr 29, 2014 at 5:33 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> Hi,
>
> just a basic review to keep things rolling...

Thanks for the review!  It sounds like other changes are blocked on
this one landing, so hopefully we can make some good progress!  :)

>> On the original Samsung ARM Chromebook these devices were on an I2C
>> bus that was shared between the AP and the EC and arbitrated using
>> some extranal GPIOs (see i2c-arb-gpio-challenge).
>>
>> The original arbitration scheme worked well enough but had some
>> downsides:
>> * It was nonstandard (not using standard I2C multimaster)
>> * It only worked if the EC-AP communication was I2C
>> * It was relatively hard to debug problems (hard to tell if i2c issues
>>   were caused by the EC, the AP, or some device on the bus).
>>
>> On the HP Chromebook 11 the design was changed to:
>
> This paragraph would be a nice update for the gpio-arbitration docs.

Done.  <https://patchwork.kernel.org/patch/4088551/>


>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt b/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
>
> The bindings should independently be sent to the devicetree list.
>
>> new file mode 100644
>> index 0000000..898f030
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
>> @@ -0,0 +1,39 @@
>> +I2C bus that tunnels through the ChromeOS EC (cros-ec)
>> +======================================================
>> +On some ChromeOS board designs we've got a connection to the EC (embedded
>> +controller) but no direct connection to some devices on the other side of
>> +the EC (like a battery and PMIC).  To get access to those devices we need
>> +to tunnel our i2c commands through the EC.
>> +
>> +The node for this device should be under a cros-ec node like google,cros-ec-spi
>> +or google,cros-ec-i2c.
>> +
>> +
>> +Required properties:
>> +- compatible: google,cros-ec-i2c-tunnel
>> +- google,remote-bus: The EC bus we'd like to talk to.
>> +
>> +Optional child nodes:
>> +- One node per I2C device connected to the tunnelled I2C bus.
>> +
>> +
>> +Example:
>> +     cros-ec@0 {
>> +             compatible = "google,cros-ec-spi";
>> +
>> +             ...
>> +
>> +             i2c-tunnel {
>> +                     compatible = "google,cros-ec-i2c-tunnel";
>> +                     #address-cells = <1>;
>> +                     #size-cells = <0>;
>> +
>> +                     google,remote-bus = <0>;
>> +
>> +                     battery: sbs-battery@b {
>> +                             compatible = "sbs,sbs-battery";
>> +                             reg = <0xb>;
>> +                             sbs,poll-retry-count = <1>;
>> +                     };
>> +             };
>> +     }
>
> Can the tunnel have n busses? How to represent them then? I think the
> remote-bus property should go in favor of proper sub-nodes?

I suppose it could have n busses, though today's ECs don't.

I think that the binding could later be extended to support this with:

        cros-ec@0 {
                compatible = "google,cros-ec-spi";

                ...

                i2c-tunnel0 {
                        compatible = "google,cros-ec-i2c-tunnel";
                        #address-cells = <1>;
                        #size-cells = <0>;

                        google,remote-bus = <0>;

                        battery: sbs-battery@b {
                                compatible = "sbs,sbs-battery";
                                reg = <0xb>;
                                sbs,poll-retry-count = <1>;
                        };
                };

                i2c-tunnel1 {
                        compatible = "google,cros-ec-i2c-tunnel";
                        #address-cells = <1>;
                        #size-cells = <0>;

                        google,remote-bus = <1>;

                        battery: sbs-battery@b {
                                compatible = "sbs,sbs-battery";
                                reg = <0xb>;
                                sbs,poll-retry-count = <1>;
                        };
                };
       }

...but since there's no need now, it seems like we could just leave it
with supporting 1 for now.  If we extend it like above then old device
trees will still work on newer kernels.

Note about "google,remote-bus" instead of "reg".   The main
"google,cros-ec-spi" generally has non-addressed sub-nodes.  It has no
"#address-cells" or "#size-cells".  The keyboard component, for
instance, doesn't have an "address".  You talk to it using the
keyboard commands.

...I don't think it's generally possible to have some sub nodes that
have addresses and other sub nodes that don't.  I'm happy to be
corrected, though.


Note: according to the device tree stable bindings theorem, I don't
think I'm allowed to retroactively go back and say that the keyboard
node needs a dummy "reg = <0>;" nor can I require that
"google,cros-ec-spi" add #address-cells and #size-cells.  Both device
tree bindings have been in kernel since ~3.9.


Can you give me an example of what you mean by "proper sub nodes"?


> Wouldn't it
> also be more generic to have the tunnel node seperate and reference the
> tunnel-mechanism (spi here) as a phandle?

What makes it more generic?  It's still a MFD and still only works
atop the cros_ec, so it should be under the cros_ec device, shouldn't
it?

I know use a phandle for mux, but from
<https://lkml.org/lkml/2013/2/13/517> it sounds like we were forced
into it.


>> +/**
>> + * ec_i2c_construct_message - construct a message to go to the EC
>> + *
>> + * This function effectively stuffs the standard i2c_msg format of Linux into
>> + * a format that the EC understands.
>> + *
>> + * @buf: The buffer to fill.  Can pass NULL to count how many bytes the message
>> + *       would be.
>
> I wonder about this NULL thing. That means the size is calculated twice.
> Why not make two functions instead, one fir size calc, one for setting
> up?

Done.


>> +/**
>> + * ec_i2c_parse_response - Parse a response from the EC
>> + *
>> + * We'll take the EC's response and copy it back into msgs.
>> + *
>> + * @buf: The buffer to parse.  Can pass NULL to count how many bytes we expect
>> + *    the response to be. Otherwise we assume that the right number of
>> + *    bytes are available.
>
> Ditto.

Done.


>> +     result = bus->ec->command_sendrecv(bus->ec, EC_CMD_I2C_PASSTHRU,
>> +                                        request, request_len,
>> +                                        response, response_len);
>
> This function pointer should be checked against NULL in probe, I
> think.

Done.


>> +static int ec_i2c_probe(struct platform_device *pdev)
>> +{
>> +     struct device_node *np = pdev->dev.of_node;
>> +     struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
>> +     struct device *dev = &pdev->dev;
>> +     struct ec_i2c_device *bus = NULL;
>> +     u32 remote_bus;
>> +     int err;
>> +
>> +     dev_dbg(dev, "Probing\n");
>
> Drop. Device core has it already.

Done.


>> +
>> +     if (!np) {
>> +             dev_err(dev, "no device node\n");
>> +             return -ENOENT;
>> +     }
>
> Can this happen?

Not that I know of.  I assume you want me to remove this, so done.


>> +     bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
>> +     if (bus == NULL) {
>> +             dev_err(dev, "cannot allocate bus device\n");
>
> No need for error strings when allocating.

Done.


>> +             return -ENOMEM;
>> +     }
>> +
>> +     dev_dbg(dev, "ChromeOS EC I2C tunnel adapter\n");
>
> Drop. Device core debug has it, too.

Done.


>> +
>> +     return err;
>> +}
>> +
>> +static int ec_i2c_remove(struct platform_device *dev)
>> +{
>> +     struct ec_i2c_device *bus = platform_get_drvdata(dev);
>> +
>> +     platform_set_drvdata(dev, NULL);
>
> Not needed.

Duh, thanks.  Done.


>> +     i2c_del_adapter(&bus->adap);
>> +
>> +     return 0;
>> +}
>> +

Sending up a v3 with fixes...

-Doug