Add support for Chrome OS firmware signals

Add support for Chrome OS firmware signals

[Martyn Welch]

Some Chromebooks have gpio attached to signals used to cause the firmware
to enter alternative modes of operation and/or control other device
characteristics (such as write protection on flash devices). This patch
adds a driver that exposes a read-only interface to allow these signals to
be read from user space.

In addition this patch series provides the required bindings for this to the
peach-pi Chromebook.

[PATCH 1/3] Device tree binding documentation for chromeos-firmware

[Martyn Welch]

This patch adds documentation for the chromeos-firmware binding.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 .../devicetree/bindings/misc/chromeos-firmware.txt | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/chromeos-firmware.txt

diff --git a/Documentation/devicetree/bindings/misc/chromeos-firmware.txt b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
new file mode 100644
index 0000000..8240611
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
@@ -0,0 +1,27 @@
+Device-Tree bindings for chromeos-firmware.c.
+
+Required properties:
+	- compatible = "google,gpio-firmware";
+
+Each signal is represented as a sub-node of "chromeos_firmware":
+Subnode properties:
+
+	- gpios: OF device-tree gpio specification.
+
+Example nodes:
+
+	chromeos_firmware {
+		compatible = "google,gpio-firmware";
+
+		write-protect {
+			gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
+		};
+
+		developer-switch {
+			gpios = <&gpx1 3 GPIO_ACTIVE_HIGH>;
+		};
+
+		recovery-switch {
+			gpios = <&gpx0 7 GPIO_ACTIVE_LOW>;
+		};
+	};
-- 
2.1.4

[PATCH 2/3] Add support for monitoring Chrome OS firmware signals

[Martyn Welch]

Select Chromebooks have gpio attached to signals used to cause the firmware
to enter alternative modes of operation and/or control other device
characteristics (such as write protection on flash devices). This patch
adds a driver that exposes a read-only interface to allow these signals to
be read from user space.

Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 drivers/platform/chrome/Kconfig             |  13 +++
 drivers/platform/chrome/Makefile            |   1 +
 drivers/platform/chrome/chromeos_firmware.c | 156 ++++++++++++++++++++++++++++
 3 files changed, 170 insertions(+)
 create mode 100644 drivers/platform/chrome/chromeos_firmware.c

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index d03df4a..d55ceef 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -38,6 +38,19 @@ config CHROMEOS_PSTORE
 	  If you have a supported Chromebook, choose Y or M here.
 	  The module will be called chromeos_pstore.
 
+config CHROMEOS_FIRMWARE
+	tristate "Chrome OS firmware signal monitoring"
+	depends on GPIO_SYSFS
+	---help---
+	 Many chromebooks have gpio attached to signals used to cause the
+	 firmware to enter alternative modes of operation and/or control other
+	 device characteristics (such as write protection on flash devices).
+	 This driver exposes a read-only interface to allow these signals to be
+	 read from user space.
+
+	 If you have a supported Chromebook, choose Y or M here.
+	 The module will be called chromeos_firmware.
+
 config CROS_EC_CHARDEV
         tristate "Chrome OS Embedded Controller userspace device interface"
         depends on MFD_CROS_EC
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index bc498bd..2453adf 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -1,6 +1,7 @@
 
 obj-$(CONFIG_CHROMEOS_LAPTOP)	+= chromeos_laptop.o
 obj-$(CONFIG_CHROMEOS_PSTORE)	+= chromeos_pstore.o
+obj-$(CONFIG_CHROMEOS_FIRMWARE)	+= chromeos_firmware.o
 cros_ec_devs-objs		:= cros_ec_dev.o cros_ec_sysfs.o \
 				   cros_ec_lightbar.o cros_ec_vbc.o
 obj-$(CONFIG_CROS_EC_CHARDEV)   += cros_ec_devs.o
diff --git a/drivers/platform/chrome/chromeos_firmware.c b/drivers/platform/chrome/chromeos_firmware.c
new file mode 100644
index 0000000..ab8540a
--- /dev/null
+++ b/drivers/platform/chrome/chromeos_firmware.c
@@ -0,0 +1,156 @@
+/*
+ * Copyright (C) 2015 Collabora Ltd.
+ *
+ * based on vendor driver,
+ *
+ * Copyright (C) 2011 The Chromium OS Authors
+ *
+ * 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.
+ *
+ * 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/bcd.h>
+#include <linux/gpio.h>
+#include <linux/notifier.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct chromeos_firmware_data {
+	int wp;
+	int rec;
+	int dev;
+};
+
+static int dt_gpio_init(struct platform_device *pdev, const char *of_list_name,
+			const char *gpio_desc_name, const char *sysfs_name,
+			int *gpio)
+{
+	int err;
+	enum of_gpio_flags of_flags;
+	unsigned long flags = GPIOF_DIR_IN | GPIOF_EXPORT;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *cnp;
+
+	cnp = of_get_child_by_name(np, of_list_name);
+	if (!cnp)
+		/*
+		 * We don't necessarily expect to find all of the devices, so
+		 * return without generating an error.
+		 */
+		return 0;
+
+	*gpio = of_get_named_gpio_flags(cnp, "gpios", 0, &of_flags);
+	if (!gpio_is_valid(*gpio)) {
+		err = -EINVAL;
+		goto err_prop;
+	}
+
+	if (of_flags & OF_GPIO_ACTIVE_LOW)
+		flags |= GPIOF_ACTIVE_LOW;
+
+	err = gpio_request_one(*gpio, flags, gpio_desc_name);
+	if (err)
+		goto err_prop;
+
+	err = gpio_export_link(&pdev->dev, sysfs_name, *gpio);
+	if (err)
+		goto err_gpio;
+
+	return 0;
+
+err_gpio:
+	gpio_free(*gpio);
+err_prop:
+	of_node_put(cnp);
+
+	return err;
+}
+
+static void chromeos_firmware_rem(int gpio)
+{
+	gpio_unexport(gpio);
+
+	gpio_free(gpio);
+}
+
+static int chromeos_firmware_probe(struct platform_device *pdev)
+{
+	int err;
+	struct chromeos_firmware_data *gpios;
+
+	gpios = devm_kmalloc(&pdev->dev, sizeof(gpios), GFP_KERNEL);
+	if (!gpios)
+		return -ENOMEM;
+
+	err = dt_gpio_init(pdev, "write-protect", "firmware-write-protect",
+			   "write-protect", &gpios->wp);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to init write-protect.\n");
+		goto err_wp;
+	}
+
+	err = dt_gpio_init(pdev, "recovery-switch", "firmware-recovery-switch",
+			   "recovery-switch", &gpios->rec);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to init recovery-switch.\n");
+		goto err_rec;
+	}
+
+	err = dt_gpio_init(pdev, "developer-switch",
+			   "firmware-developer-switch", "developer-switch",
+			   &gpios->dev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to init developer-switch.\n");
+		goto err_dev;
+	}
+
+	platform_set_drvdata(pdev, gpios);
+
+	return 0;
+
+err_dev:
+	chromeos_firmware_rem(gpios->rec);
+
+err_rec:
+	chromeos_firmware_rem(gpios->wp);
+err_wp:
+	return err;
+}
+
+static int chromeos_firmware_remove(struct platform_device *pdev)
+{
+	struct chromeos_firmware_data *gpios = platform_get_drvdata(pdev);
+
+	chromeos_firmware_rem(gpios->dev);
+	chromeos_firmware_rem(gpios->rec);
+	chromeos_firmware_rem(gpios->wp);
+
+	return 0;
+}
+
+static const struct of_device_id chromeos_firmware_of_match[] = {
+	{ .compatible = "google,gpio-firmware" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, chromeos_firmware_of_match);
+
+static struct platform_driver chromeos_firmware_driver = {
+	.probe = chromeos_firmware_probe,
+	.remove = chromeos_firmware_remove,
+	.driver = {
+		.name = "chromeos_firmware",
+		.of_match_table = chromeos_firmware_of_match,
+	},
+};
+module_platform_driver(chromeos_firmware_driver);
+
+MODULE_LICENSE("GPL");
-- 
2.1.4

[PATCH 3/3] Addition of binding for firmware signals on peach-pi

[Martyn Welch]

The peach pi has a GPIO connected to the firmware write protect, developer
mode and recovery mode lines. This patch adds the required nodes to the
device tree to configure the pinmuxing and allow these to be read from
user space.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Kukjin Kim <kgene@kernel.org>
Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 arch/arm/boot/dts/exynos5800-peach-pi.dts | 40 +++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
index 49a4f43..485c18f 100644
--- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
@@ -53,6 +53,25 @@
 		};
 	};
 
+	chromeos-firmware {
+		compatible = "google,gpio-firmware";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&wp_gpio &dev_mode &rec_mode>;
+
+		write-protect {
+			gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
+		};
+
+		developer-switch {
+			gpios = <&gpx1 3 GPIO_ACTIVE_HIGH>;
+		};
+
+		recovery-switch {
+			gpios = <&gpx0 7 GPIO_ACTIVE_LOW>;
+		};
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 
@@ -731,6 +750,13 @@
 		samsung,pin-val = <0>;
 	};
 
+	rec_mode: rec-mode {
+		samsung,pins = "gpx0-7";
+		samsung,pin-function = <0>;
+		samsung,pin-pud = <0>;
+		samsung,pin-drv = <0>;
+	};
+
 	tpm_irq: tpm-irq {
 		samsung,pins = "gpx1-0";
 		samsung,pin-function = <0>;
@@ -752,6 +778,13 @@
 		samsung,pin-drv = <0>;
 	};
 
+	dev_mode: dev-mode {
+		samsung,pins = "gpx1-3";
+		samsung,pin-function = <0>;
+		samsung,pin-pud = <3>;
+		samsung,pin-drv = <0>;
+	};
+
 	ec_irq: ec-irq {
 		samsung,pins = "gpx1-5";
 		samsung,pin-function = <0>;
@@ -773,6 +806,13 @@
 		samsung,pin-drv = <0>;
 	};
 
+	wp_gpio: wp_gpio {
+		samsung,pins = "gpx3-0";
+		samsung,pin-function = <0>;
+		samsung,pin-pud = <0>;
+		samsung,pin-drv = <0>;
+	};
+
 	max77802_irq: max77802-irq {
 		samsung,pins = "gpx3-1";
 		samsung,pin-function = <0>;
-- 
2.1.4

[PATCH v2] Add support for monitoring Chrome OS firmware signals

[Martyn Welch]

Select Chromebooks have gpio attached to signals used to cause the firmware
to enter alternative modes of operation and/or control other device
characteristics (such as write protection on flash devices). This patch
adds a driver that exposes a read-only interface to allow these signals to
be read from user space.

Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---

v2:
 - Added missing call to remove sysfs link. 

 drivers/platform/chrome/Kconfig             |  13 +++
 drivers/platform/chrome/Makefile            |   1 +
 drivers/platform/chrome/chromeos_firmware.c | 166 ++++++++++++++++++++++++++++
 3 files changed, 180 insertions(+)
 create mode 100644 drivers/platform/chrome/chromeos_firmware.c

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index d03df4a..d55ceef 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -38,6 +38,19 @@ config CHROMEOS_PSTORE
 	  If you have a supported Chromebook, choose Y or M here.
 	  The module will be called chromeos_pstore.
 
+config CHROMEOS_FIRMWARE
+	tristate "Chrome OS firmware signal monitoring"
+	depends on GPIO_SYSFS
+	---help---
+	 Many chromebooks have gpio attached to signals used to cause the
+	 firmware to enter alternative modes of operation and/or control other
+	 device characteristics (such as write protection on flash devices).
+	 This driver exposes a read-only interface to allow these signals to be
+	 read from user space.
+
+	 If you have a supported Chromebook, choose Y or M here.
+	 The module will be called chromeos_firmware.
+
 config CROS_EC_CHARDEV
         tristate "Chrome OS Embedded Controller userspace device interface"
         depends on MFD_CROS_EC
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index bc498bd..2453adf 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -1,6 +1,7 @@
 
 obj-$(CONFIG_CHROMEOS_LAPTOP)	+= chromeos_laptop.o
 obj-$(CONFIG_CHROMEOS_PSTORE)	+= chromeos_pstore.o
+obj-$(CONFIG_CHROMEOS_FIRMWARE)	+= chromeos_firmware.o
 cros_ec_devs-objs		:= cros_ec_dev.o cros_ec_sysfs.o \
 				   cros_ec_lightbar.o cros_ec_vbc.o
 obj-$(CONFIG_CROS_EC_CHARDEV)   += cros_ec_devs.o
diff --git a/drivers/platform/chrome/chromeos_firmware.c b/drivers/platform/chrome/chromeos_firmware.c
new file mode 100644
index 0000000..a08cb57
--- /dev/null
+++ b/drivers/platform/chrome/chromeos_firmware.c
@@ -0,0 +1,166 @@
+/*
+ * Copyright (C) 2015 Collabora Ltd.
+ *
+ * based on vendor driver,
+ *
+ * Copyright (C) 2011 The Chromium OS Authors
+ *
+ * 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.
+ *
+ * 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/bcd.h>
+#include <linux/gpio.h>
+#include <linux/notifier.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct chromeos_firmware_gpio_info {
+	int gpio;
+	const char *link;
+};
+
+struct chromeos_firmware_data {
+	struct chromeos_firmware_gpio_info wp;
+	struct chromeos_firmware_gpio_info rec;
+	struct chromeos_firmware_gpio_info dev;
+};
+
+static int dt_gpio_init(struct platform_device *pdev, const char *of_list_name,
+			const char *gpio_desc_name, const char *sysfs_name,
+			struct chromeos_firmware_gpio_info *gpio)
+{
+	int err;
+	enum of_gpio_flags of_flags;
+	unsigned long flags = GPIOF_DIR_IN | GPIOF_EXPORT;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *cnp;
+
+	cnp = of_get_child_by_name(np, of_list_name);
+	if (!cnp)
+		/*
+		 * We don't necessarily expect to find all of the devices, so
+		 * return without generating an error.
+		 */
+		return 0;
+
+	gpio->gpio = of_get_named_gpio_flags(cnp, "gpios", 0, &of_flags);
+	if (!gpio_is_valid(gpio->gpio)) {
+		err = -EINVAL;
+		goto err_prop;
+	}
+
+	if (of_flags & OF_GPIO_ACTIVE_LOW)
+		flags |= GPIOF_ACTIVE_LOW;
+
+	err = gpio_request_one(gpio->gpio, flags, gpio_desc_name);
+	if (err)
+		goto err_prop;
+
+	err = gpio_export_link(&pdev->dev, sysfs_name, gpio->gpio);
+	if (err)
+		goto err_gpio;
+
+	gpio->link = sysfs_name;
+
+	return 0;
+
+err_gpio:
+	gpio_free(gpio->gpio);
+err_prop:
+	of_node_put(cnp);
+
+	return err;
+}
+
+static void chromeos_firmware_rem(struct device *dev,
+				  struct chromeos_firmware_gpio_info *gpio)
+{
+	sysfs_remove_link(&dev->kobj, gpio->link);
+
+	gpio_unexport(gpio->gpio);
+
+	gpio_free(gpio->gpio);
+}
+
+static int chromeos_firmware_probe(struct platform_device *pdev)
+{
+	int err;
+	struct chromeos_firmware_data *gpios;
+
+	gpios = devm_kmalloc(&pdev->dev, sizeof(gpios), GFP_KERNEL);
+	if (!gpios)
+		return -ENOMEM;
+
+	err = dt_gpio_init(pdev, "write-protect", "firmware-write-protect",
+			   "write-protect", &gpios->wp);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to init write-protect.\n");
+		goto err_wp;
+	}
+
+	err = dt_gpio_init(pdev, "recovery-switch", "firmware-recovery-switch",
+			   "recovery-switch", &gpios->rec);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to init recovery-switch.\n");
+		goto err_rec;
+	}
+
+	err = dt_gpio_init(pdev, "developer-switch",
+			   "firmware-developer-switch", "developer-switch",
+			   &gpios->dev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to init developer-switch.\n");
+		goto err_dev;
+	}
+
+	platform_set_drvdata(pdev, gpios);
+
+	return 0;
+
+err_dev:
+	chromeos_firmware_rem(&pdev->dev, &gpios->rec);
+
+err_rec:
+	chromeos_firmware_rem(&pdev->dev, &gpios->wp);
+err_wp:
+	return err;
+}
+
+static int chromeos_firmware_remove(struct platform_device *pdev)
+{
+	struct chromeos_firmware_data *gpios = platform_get_drvdata(pdev);
+
+	chromeos_firmware_rem(&pdev->dev, &gpios->dev);
+	chromeos_firmware_rem(&pdev->dev, &gpios->rec);
+	chromeos_firmware_rem(&pdev->dev, &gpios->wp);
+
+	return 0;
+}
+
+static const struct of_device_id chromeos_firmware_of_match[] = {
+	{ .compatible = "google,gpio-firmware" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, chromeos_firmware_of_match);
+
+static struct platform_driver chromeos_firmware_driver = {
+	.probe = chromeos_firmware_probe,
+	.remove = chromeos_firmware_remove,
+	.driver = {
+		.name = "chromeos_firmware",
+		.of_match_table = chromeos_firmware_of_match,
+	},
+};
+module_platform_driver(chromeos_firmware_driver);
+
+MODULE_LICENSE("GPL");
-- 
2.1.4

Re: [PATCH 3/3] Addition of binding for firmware signals on peach-pi

[Krzysztof Kozlowski]

On 02.12.2015 04:12, Martyn Welch wrote:
> The peach pi has a GPIO connected to the firmware write protect, developer
> mode and recovery mode lines. This patch adds the required nodes to the
> device tree to configure the pinmuxing and allow these to be read from
> user space.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Kukjin Kim <kgene@kernel.org>
> Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
>  arch/arm/boot/dts/exynos5800-peach-pi.dts | 40 +++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)

Hi,

Thanks for the patch.

Few points from my side:
1. Please add a prefix to the subject: "ARM: dts:".

2. There is no need of such huge CC-list in the body of commit. This
CC-list comes from get_maintainer so there is no benefit of duplicating
it here. The CC is usually used to notify other people who might be
interested but get_maintainer does not point them.

3. I received only this third patch. I did not receive cover letter
explaining possible dependencies so I am not sure how to deal with the
patch. It looks like there are no dependencies... but maybe there are?
Is this is a new binding or no? Please provide a cover letter (if it
exists already be sure to send it to all interested parties) or send
entire patchset so the big picture could be seen.

The patch itself looks good but I'll wait with a review tag for #3.

Best regards,
Krzysztof


> 
> diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
> index 49a4f43..485c18f 100644
> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
> @@ -53,6 +53,25 @@
>  		};
>  	};
>  
> +	chromeos-firmware {
> +		compatible = "google,gpio-firmware";
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&wp_gpio &dev_mode &rec_mode>;
> +
> +		write-protect {
> +			gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		developer-switch {
> +			gpios = <&gpx1 3 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		recovery-switch {
> +			gpios = <&gpx0 7 GPIO_ACTIVE_LOW>;
> +		};
> +	};
> +
>  	gpio-keys {
>  		compatible = "gpio-keys";
>  
> @@ -731,6 +750,13 @@
>  		samsung,pin-val = <0>;
>  	};
>  
> +	rec_mode: rec-mode {
> +		samsung,pins = "gpx0-7";
> +		samsung,pin-function = <0>;
> +		samsung,pin-pud = <0>;
> +		samsung,pin-drv = <0>;
> +	};
> +
>  	tpm_irq: tpm-irq {
>  		samsung,pins = "gpx1-0";
>  		samsung,pin-function = <0>;
> @@ -752,6 +778,13 @@
>  		samsung,pin-drv = <0>;
>  	};
>  
> +	dev_mode: dev-mode {
> +		samsung,pins = "gpx1-3";
> +		samsung,pin-function = <0>;
> +		samsung,pin-pud = <3>;
> +		samsung,pin-drv = <0>;
> +	};
> +
>  	ec_irq: ec-irq {
>  		samsung,pins = "gpx1-5";
>  		samsung,pin-function = <0>;
> @@ -773,6 +806,13 @@
>  		samsung,pin-drv = <0>;
>  	};
>  
> +	wp_gpio: wp_gpio {
> +		samsung,pins = "gpx3-0";
> +		samsung,pin-function = <0>;
> +		samsung,pin-pud = <0>;
> +		samsung,pin-drv = <0>;
> +	};
> +
>  	max77802_irq: max77802-irq {
>  		samsung,pins = "gpx3-1";
>  		samsung,pin-function = <0>;
> 

Re: [PATCH v2] Add support for monitoring Chrome OS firmware signals

[Jeremiah Mahler]

Martyn,

On Tue, Dec 01, 2015 at 08:19:46PM +0000, Martyn Welch wrote:
> Select Chromebooks have gpio attached to signals used to cause the firmware
> to enter alternative modes of operation and/or control other device
[...]
> +
> +static int chromeos_firmware_probe(struct platform_device *pdev)
> +{
> +	int err;
> +	struct chromeos_firmware_data *gpios;
> +
> +	gpios = devm_kmalloc(&pdev->dev, sizeof(gpios), GFP_KERNEL);

Should this be 'sizeof(*gpios)' so it allocates enough room for the
entire struct instead of just the pointer?

[...]

-- 
- Jeremiah Mahler

Re: [PATCH 3/3] Addition of binding for firmware signals on peach-pi

[Martyn Welch]

On 01/12/15 23:51, Krzysztof Kozlowski wrote:
> On 02.12.2015 04:12, Martyn Welch wrote:
>> The peach pi has a GPIO connected to the firmware write protect, developer
>> mode and recovery mode lines. This patch adds the required nodes to the
>> device tree to configure the pinmuxing and allow these to be read from
>> user space.
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Pawel Moll <pawel.moll@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>> Cc: Kumar Gala <galak@codeaurora.org>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Kukjin Kim <kgene@kernel.org>
>> Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-samsung-soc@vger.kernel.org
>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>> ---
>>   arch/arm/boot/dts/exynos5800-peach-pi.dts | 40 +++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>
> Hi,
>
> Thanks for the patch.
>
> Few points from my side:
> 1. Please add a prefix to the subject: "ARM: dts:".
>

Ok, sorry.

> 2. There is no need of such huge CC-list in the body of commit. This
> CC-list comes from get_maintainer so there is no benefit of duplicating
> it here. The CC is usually used to notify other people who might be
> interested but get_maintainer does not point them.
>

Ok, yes these were pulled from get_maintainer.

> 3. I received only this third patch. I did not receive cover letter
> explaining possible dependencies so I am not sure how to deal with the
> patch. It looks like there are no dependencies... but maybe there are?
> Is this is a new binding or no? Please provide a cover letter (if it
> exists already be sure to send it to all interested parties) or send
> entire patchset so the big picture could be seen.
>

I'll make sure I do that next time.

The cover letter read:

Some Chromebooks have gpio attached to signals used to cause the 
firmware to enter alternative modes of operation and/or control other 
device characteristics (such as write protection on flash devices). This 
patch adds a driver that exposes a read-only interface to allow these 
signals to be read from user space.

In addition this patch series provides the required bindings for this to 
the peach-pi Chromebook.


This is a new binding, but the driver is based on functionality in the 
kernel shipped on Chromebooks. The binding has been modified based on 
the form of existing bindings in the mainline kernel.

Does that help?

Martyn

> The patch itself looks good but I'll wait with a review tag for #3.
>
> Best regards,
> Krzysztof
>
>
>>
>> diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> index 49a4f43..485c18f 100644
>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> @@ -53,6 +53,25 @@
>>   		};
>>   	};
>>
>> +	chromeos-firmware {
>> +		compatible = "google,gpio-firmware";
>> +
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&wp_gpio &dev_mode &rec_mode>;
>> +
>> +		write-protect {
>> +			gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
>> +		};
>> +
>> +		developer-switch {
>> +			gpios = <&gpx1 3 GPIO_ACTIVE_HIGH>;
>> +		};
>> +
>> +		recovery-switch {
>> +			gpios = <&gpx0 7 GPIO_ACTIVE_LOW>;
>> +		};
>> +	};
>> +
>>   	gpio-keys {
>>   		compatible = "gpio-keys";
>>
>> @@ -731,6 +750,13 @@
>>   		samsung,pin-val = <0>;
>>   	};
>>
>> +	rec_mode: rec-mode {
>> +		samsung,pins = "gpx0-7";
>> +		samsung,pin-function = <0>;
>> +		samsung,pin-pud = <0>;
>> +		samsung,pin-drv = <0>;
>> +	};
>> +
>>   	tpm_irq: tpm-irq {
>>   		samsung,pins = "gpx1-0";
>>   		samsung,pin-function = <0>;
>> @@ -752,6 +778,13 @@
>>   		samsung,pin-drv = <0>;
>>   	};
>>
>> +	dev_mode: dev-mode {
>> +		samsung,pins = "gpx1-3";
>> +		samsung,pin-function = <0>;
>> +		samsung,pin-pud = <3>;
>> +		samsung,pin-drv = <0>;
>> +	};
>> +
>>   	ec_irq: ec-irq {
>>   		samsung,pins = "gpx1-5";
>>   		samsung,pin-function = <0>;
>> @@ -773,6 +806,13 @@
>>   		samsung,pin-drv = <0>;
>>   	};
>>
>> +	wp_gpio: wp_gpio {
>> +		samsung,pins = "gpx3-0";
>> +		samsung,pin-function = <0>;
>> +		samsung,pin-pud = <0>;
>> +		samsung,pin-drv = <0>;
>> +	};
>> +
>>   	max77802_irq: max77802-irq {
>>   		samsung,pins = "gpx3-1";
>>   		samsung,pin-function = <0>;
>>
>

Re: [PATCH v2] Add support for monitoring Chrome OS firmware signals

[Martyn Welch]

On 02/12/15 06:08, Jeremiah Mahler wrote:
> Martyn,
>
> On Tue, Dec 01, 2015 at 08:19:46PM +0000, Martyn Welch wrote:
>> Select Chromebooks have gpio attached to signals used to cause the firmware
>> to enter alternative modes of operation and/or control other device
> [...]
>> +
>> +static int chromeos_firmware_probe(struct platform_device *pdev)
>> +{
>> +	int err;
>> +	struct chromeos_firmware_data *gpios;
>> +
>> +	gpios = devm_kmalloc(&pdev->dev, sizeof(gpios), GFP_KERNEL);
>
> Should this be 'sizeof(*gpios)' so it allocates enough room for the
> entire struct instead of just the pointer?
>

Yes, you're right, it should.

Martyn

[PATCH v3] Add support for monitoring Chrome OS firmware signals

[Martyn Welch]

Select Chromebooks have gpio attached to signals used to cause the firmware
to enter alternative modes of operation and/or control other device
characteristics (such as write protection on flash devices). This patch
adds a driver that exposes a read-only interface to allow these signals to
be read from user space.

Cc: Jeremiah Mahler <jmmahler@gmail.com>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---

v2:
 - Added missing call to remove sysfs link.

v3:
 - Correct malloc sizeof() usage.

 drivers/platform/chrome/Kconfig             |  13 +++
 drivers/platform/chrome/Makefile            |   1 +
 drivers/platform/chrome/chromeos_firmware.c | 166 ++++++++++++++++++++++++++++
 3 files changed, 180 insertions(+)
 create mode 100644 drivers/platform/chrome/chromeos_firmware.c

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index d03df4a..d55ceef 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -38,6 +38,19 @@ config CHROMEOS_PSTORE
 	  If you have a supported Chromebook, choose Y or M here.
 	  The module will be called chromeos_pstore.
 
+config CHROMEOS_FIRMWARE
+	tristate "Chrome OS firmware signal monitoring"
+	depends on GPIO_SYSFS
+	---help---
+	 Many chromebooks have gpio attached to signals used to cause the
+	 firmware to enter alternative modes of operation and/or control other
+	 device characteristics (such as write protection on flash devices).
+	 This driver exposes a read-only interface to allow these signals to be
+	 read from user space.
+
+	 If you have a supported Chromebook, choose Y or M here.
+	 The module will be called chromeos_firmware.
+
 config CROS_EC_CHARDEV
         tristate "Chrome OS Embedded Controller userspace device interface"
         depends on MFD_CROS_EC
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index bc498bd..2453adf 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -1,6 +1,7 @@
 
 obj-$(CONFIG_CHROMEOS_LAPTOP)	+= chromeos_laptop.o
 obj-$(CONFIG_CHROMEOS_PSTORE)	+= chromeos_pstore.o
+obj-$(CONFIG_CHROMEOS_FIRMWARE)	+= chromeos_firmware.o
 cros_ec_devs-objs		:= cros_ec_dev.o cros_ec_sysfs.o \
 				   cros_ec_lightbar.o cros_ec_vbc.o
 obj-$(CONFIG_CROS_EC_CHARDEV)   += cros_ec_devs.o
diff --git a/drivers/platform/chrome/chromeos_firmware.c b/drivers/platform/chrome/chromeos_firmware.c
new file mode 100644
index 0000000..5c2c96a
--- /dev/null
+++ b/drivers/platform/chrome/chromeos_firmware.c
@@ -0,0 +1,166 @@
+/*
+ * Copyright (C) 2015 Collabora Ltd.
+ *
+ * based on vendor driver,
+ *
+ * Copyright (C) 2011 The Chromium OS Authors
+ *
+ * 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.
+ *
+ * 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/bcd.h>
+#include <linux/gpio.h>
+#include <linux/notifier.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct chromeos_firmware_gpio_info {
+	int gpio;
+	const char *link;
+};
+
+struct chromeos_firmware_data {
+	struct chromeos_firmware_gpio_info wp;
+	struct chromeos_firmware_gpio_info rec;
+	struct chromeos_firmware_gpio_info dev;
+};
+
+static int dt_gpio_init(struct platform_device *pdev, const char *of_list_name,
+			const char *gpio_desc_name, const char *sysfs_name,
+			struct chromeos_firmware_gpio_info *gpio)
+{
+	int err;
+	enum of_gpio_flags of_flags;
+	unsigned long flags = GPIOF_DIR_IN | GPIOF_EXPORT;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *cnp;
+
+	cnp = of_get_child_by_name(np, of_list_name);
+	if (!cnp)
+		/*
+		 * We don't necessarily expect to find all of the devices, so
+		 * return without generating an error.
+		 */
+		return 0;
+
+	gpio->gpio = of_get_named_gpio_flags(cnp, "gpios", 0, &of_flags);
+	if (!gpio_is_valid(gpio->gpio)) {
+		err = -EINVAL;
+		goto err_prop;
+	}
+
+	if (of_flags & OF_GPIO_ACTIVE_LOW)
+		flags |= GPIOF_ACTIVE_LOW;
+
+	err = gpio_request_one(gpio->gpio, flags, gpio_desc_name);
+	if (err)
+		goto err_prop;
+
+	err = gpio_export_link(&pdev->dev, sysfs_name, gpio->gpio);
+	if (err)
+		goto err_gpio;
+
+	gpio->link = sysfs_name;
+
+	return 0;
+
+err_gpio:
+	gpio_free(gpio->gpio);
+err_prop:
+	of_node_put(cnp);
+
+	return err;
+}
+
+static void chromeos_firmware_rem(struct device *dev,
+				  struct chromeos_firmware_gpio_info *gpio)
+{
+	sysfs_remove_link(&dev->kobj, gpio->link);
+
+	gpio_unexport(gpio->gpio);
+
+	gpio_free(gpio->gpio);
+}
+
+static int chromeos_firmware_probe(struct platform_device *pdev)
+{
+	int err;
+	struct chromeos_firmware_data *gpios;
+
+	gpios = devm_kmalloc(&pdev->dev, sizeof(*gpios), GFP_KERNEL);
+	if (!gpios)
+		return -ENOMEM;
+
+	err = dt_gpio_init(pdev, "write-protect", "firmware-write-protect",
+			   "write-protect", &gpios->wp);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to init write-protect.\n");
+		goto err_wp;
+	}
+
+	err = dt_gpio_init(pdev, "recovery-switch", "firmware-recovery-switch",
+			   "recovery-switch", &gpios->rec);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to init recovery-switch.\n");
+		goto err_rec;
+	}
+
+	err = dt_gpio_init(pdev, "developer-switch",
+			   "firmware-developer-switch", "developer-switch",
+			   &gpios->dev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to init developer-switch.\n");
+		goto err_dev;
+	}
+
+	platform_set_drvdata(pdev, gpios);
+
+	return 0;
+
+err_dev:
+	chromeos_firmware_rem(&pdev->dev, &gpios->rec);
+
+err_rec:
+	chromeos_firmware_rem(&pdev->dev, &gpios->wp);
+err_wp:
+	return err;
+}
+
+static int chromeos_firmware_remove(struct platform_device *pdev)
+{
+	struct chromeos_firmware_data *gpios = platform_get_drvdata(pdev);
+
+	chromeos_firmware_rem(&pdev->dev, &gpios->dev);
+	chromeos_firmware_rem(&pdev->dev, &gpios->rec);
+	chromeos_firmware_rem(&pdev->dev, &gpios->wp);
+
+	return 0;
+}
+
+static const struct of_device_id chromeos_firmware_of_match[] = {
+	{ .compatible = "google,gpio-firmware" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, chromeos_firmware_of_match);
+
+static struct platform_driver chromeos_firmware_driver = {
+	.probe = chromeos_firmware_probe,
+	.remove = chromeos_firmware_remove,
+	.driver = {
+		.name = "chromeos_firmware",
+		.of_match_table = chromeos_firmware_of_match,
+	},
+};
+module_platform_driver(chromeos_firmware_driver);
+
+MODULE_LICENSE("GPL");
-- 
2.1.4

Re: [PATCH 1/3] Device tree binding documentation for chromeos-firmware

[Rob Herring]

On Tue, Dec 01, 2015 at 07:12:49PM +0000, Martyn Welch wrote:
> This patch adds documentation for the chromeos-firmware binding.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
>  .../devicetree/bindings/misc/chromeos-firmware.txt | 27 ++++++++++++++++++++++

bindings/firmware/ please.

>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/chromeos-firmware.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/chromeos-firmware.txt b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
> new file mode 100644
> index 0000000..8240611
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
> @@ -0,0 +1,27 @@
> +Device-Tree bindings for chromeos-firmware.c.

Perhaps a bit more description what this is.

What aspect of this is firmware? How does this relate to the EC?

> +
> +Required properties:
> +	- compatible = "google,gpio-firmware";

No versions?

> +
> +Each signal is represented as a sub-node of "chromeos_firmware":
> +Subnode properties:
> +
> +	- gpios: OF device-tree gpio specification.
> +
> +Example nodes:
> +
> +	chromeos_firmware {

This should go under /firmware

> +		compatible = "google,gpio-firmware";
> +
> +		write-protect {

You need to define what are valid sub nodes. The example is not 
documentation.

> +			gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		developer-switch {
> +			gpios = <&gpx1 3 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		recovery-switch {
> +			gpios = <&gpx0 7 GPIO_ACTIVE_LOW>;
> +		};
> +	};
> -- 
> 2.1.4
> 

Re: [PATCH 1/3] Device tree binding documentation for chromeos-firmware

[Martyn Welch]

On 02/12/15 15:15, Rob Herring wrote:
> On Tue, Dec 01, 2015 at 07:12:49PM +0000, Martyn Welch wrote:
>> This patch adds documentation for the chromeos-firmware binding.
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Pawel Moll <pawel.moll@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>> Cc: Kumar Gala <galak@codeaurora.org>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>> ---
>>   .../devicetree/bindings/misc/chromeos-firmware.txt | 27 ++++++++++++++++++++++
>
> bindings/firmware/ please.
>

OK.

>>   1 file changed, 27 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>
>> diff --git a/Documentation/devicetree/bindings/misc/chromeos-firmware.txt b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>> new file mode 100644
>> index 0000000..8240611
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>> @@ -0,0 +1,27 @@
>> +Device-Tree bindings for chromeos-firmware.c.
>
> Perhaps a bit more description what this is.
>
> What aspect of this is firmware? How does this relate to the EC?
>

With respect to write-protect, this line is the write protection for the 
flash which holds the bootloader.

For the developer-switch and recovery-switch, I understand that pulling 
these lines low result in the stock firmware forcing the device to boot 
into developer mode and recovery mode respectively. The device I have no 
longer runs the stock firmware, so I'm not able to confirm this, though 
I am able to drive these lines.

As far as I'm aware, none of these are related to the operation of the EC.

Will update the binding documentation.

>> +
>> +Required properties:
>> +	- compatible = "google,gpio-firmware";
>
> No versions?
>

I'm not aware of any and would rather not start inventing ones that 
aren't already there.

>> +
>> +Each signal is represented as a sub-node of "chromeos_firmware":
>> +Subnode properties:
>> +
>> +	- gpios: OF device-tree gpio specification.
>> +
>> +Example nodes:
>> +
>> +	chromeos_firmware {
>
> This should go under /firmware

Ok, will do.

>
>> +		compatible = "google,gpio-firmware";
>> +
>> +		write-protect {
>
> You need to define what are valid sub nodes. The example is not
> documentation.
>

Ok

>> +			gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
>> +		};
>> +
>> +		developer-switch {
>> +			gpios = <&gpx1 3 GPIO_ACTIVE_HIGH>;
>> +		};
>> +
>> +		recovery-switch {
>> +			gpios = <&gpx0 7 GPIO_ACTIVE_LOW>;
>> +		};
>> +	};
>> --
>> 2.1.4
>>

Re: [PATCH 1/3] Device tree binding documentation for chromeos-firmware

[Rob Herring]

On Wed, Dec 2, 2015 at 10:49 AM, Martyn Welch
<martyn.welch@collabora.co.uk> wrote:
>
>
> On 02/12/15 15:15, Rob Herring wrote:
>>
>> On Tue, Dec 01, 2015 at 07:12:49PM +0000, Martyn Welch wrote:
>>>
>>> This patch adds documentation for the chromeos-firmware binding.
>>>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>> Cc: Kumar Gala <galak@codeaurora.org>
>>> Cc: devicetree@vger.kernel.org
>>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>>> ---
>>>   .../devicetree/bindings/misc/chromeos-firmware.txt | 27
>>> ++++++++++++++++++++++
>>
>>
>> bindings/firmware/ please.
>>
>
> OK.
>
>>>   1 file changed, 27 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>> b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>> new file mode 100644
>>> index 0000000..8240611
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>> @@ -0,0 +1,27 @@
>>> +Device-Tree bindings for chromeos-firmware.c.
>>
>>
>> Perhaps a bit more description what this is.
>>
>> What aspect of this is firmware? How does this relate to the EC?
>>
>
> With respect to write-protect, this line is the write protection for the
> flash which holds the bootloader.

What is driving the write-protect? Are trying to assign ownership of
the SOC GPIOs to the bootloader/firmware? If so, I think this is all
wrong.

Rob

Re: [PATCH 1/3] Device tree binding documentation for chromeos-firmware

[Martyn Welch]

On 02/12/15 18:44, Rob Herring wrote:
> On Wed, Dec 2, 2015 at 10:49 AM, Martyn Welch
> <martyn.welch@collabora.co.uk> wrote:
>>
>>
>> On 02/12/15 15:15, Rob Herring wrote:
>>>
>>> On Tue, Dec 01, 2015 at 07:12:49PM +0000, Martyn Welch wrote:
>>>>
>>>> This patch adds documentation for the chromeos-firmware binding.
>>>>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>> Cc: devicetree@vger.kernel.org
>>>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>>>> ---
>>>>    .../devicetree/bindings/misc/chromeos-firmware.txt | 27
>>>> ++++++++++++++++++++++
>>>
>>>
>>> bindings/firmware/ please.
>>>
>>
>> OK.
>>
>>>>    1 file changed, 27 insertions(+)
>>>>    create mode 100644
>>>> Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>>> b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>>> new file mode 100644
>>>> index 0000000..8240611
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>>> @@ -0,0 +1,27 @@
>>>> +Device-Tree bindings for chromeos-firmware.c.
>>>
>>>
>>> Perhaps a bit more description what this is.
>>>
>>> What aspect of this is firmware? How does this relate to the EC?
>>>
>>
>> With respect to write-protect, this line is the write protection for the
>> flash which holds the bootloader.
>
> What is driving the write-protect? Are trying to assign ownership of
> the SOC GPIOs to the bootloader/firmware? If so, I think this is all
> wrong.
>

The lines are typically driven by a debugging board plugged into a 
socket on the Chromebooks motherboard, not by the device it's self. The 
driver exposes a read-only interface to these signals.

Martyn

Re: [PATCH 3/3] Addition of binding for firmware signals on peach-pi

[Krzysztof Kozlowski]

On 02.12.2015 18:36, Martyn Welch wrote:
> 
> 
> On 01/12/15 23:51, Krzysztof Kozlowski wrote:
>> On 02.12.2015 04:12, Martyn Welch wrote:
>>> The peach pi has a GPIO connected to the firmware write protect,
>>> developer
>>> mode and recovery mode lines. This patch adds the required nodes to the
>>> device tree to configure the pinmuxing and allow these to be read from
>>> user space.
>>>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>> Cc: Kumar Gala <galak@codeaurora.org>
>>> Cc: Russell King <linux@arm.linux.org.uk>
>>> Cc: Kukjin Kim <kgene@kernel.org>
>>> Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>> Cc: devicetree@vger.kernel.org
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-samsung-soc@vger.kernel.org
>>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>>> ---
>>>   arch/arm/boot/dts/exynos5800-peach-pi.dts | 40
>>> +++++++++++++++++++++++++++++++
>>>   1 file changed, 40 insertions(+)
>>
>> Hi,
>>
>> Thanks for the patch.
>>
>> Few points from my side:
>> 1. Please add a prefix to the subject: "ARM: dts:".
>>
> 
> Ok, sorry.
> 
>> 2. There is no need of such huge CC-list in the body of commit. This
>> CC-list comes from get_maintainer so there is no benefit of duplicating
>> it here. The CC is usually used to notify other people who might be
>> interested but get_maintainer does not point them.
>>
> 
> Ok, yes these were pulled from get_maintainer.
> 
>> 3. I received only this third patch. I did not receive cover letter
>> explaining possible dependencies so I am not sure how to deal with the
>> patch. It looks like there are no dependencies... but maybe there are?
>> Is this is a new binding or no? Please provide a cover letter (if it
>> exists already be sure to send it to all interested parties) or send
>> entire patchset so the big picture could be seen.
>>
> 
> I'll make sure I do that next time.
> 
> The cover letter read:
> 
> Some Chromebooks have gpio attached to signals used to cause the
> firmware to enter alternative modes of operation and/or control other
> device characteristics (such as write protection on flash devices). This
> patch adds a driver that exposes a read-only interface to allow these
> signals to be read from user space.
> 
> In addition this patch series provides the required bindings for this to
> the peach-pi Chromebook.
> 
> 
> This is a new binding, but the driver is based on functionality in the
> kernel shipped on Chromebooks. The binding has been modified based on
> the form of existing bindings in the mainline kernel.
> 
> Does that help?

Yes, that helps. With the changes above (subject and reduced CC-line in
commit message):
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

As there are no dependencies this should go through samsung-soc. Just
let us know about DT bindings being accepted/applied.

Best regards,
Krzysztof


> 
> Martyn
> 
>> The patch itself looks good but I'll wait with a review tag for #3.
>>
>> Best regards,
>> Krzysztof
>>
>>
>>>
>>> diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts
>>> b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>>> index 49a4f43..485c18f 100644
>>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
>>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>>> @@ -53,6 +53,25 @@
>>>           };
>>>       };
>>>
>>> +    chromeos-firmware {
>>> +        compatible = "google,gpio-firmware";
>>> +
>>> +        pinctrl-names = "default";
>>> +        pinctrl-0 = <&wp_gpio &dev_mode &rec_mode>;
>>> +
>>> +        write-protect {
>>> +            gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
>>> +        };
>>> +
>>> +        developer-switch {
>>> +            gpios = <&gpx1 3 GPIO_ACTIVE_HIGH>;
>>> +        };
>>> +
>>> +        recovery-switch {
>>> +            gpios = <&gpx0 7 GPIO_ACTIVE_LOW>;
>>> +        };
>>> +    };
>>> +
>>>       gpio-keys {
>>>           compatible = "gpio-keys";
>>>
>>> @@ -731,6 +750,13 @@
>>>           samsung,pin-val = <0>;
>>>       };
>>>
>>> +    rec_mode: rec-mode {
>>> +        samsung,pins = "gpx0-7";
>>> +        samsung,pin-function = <0>;
>>> +        samsung,pin-pud = <0>;
>>> +        samsung,pin-drv = <0>;
>>> +    };
>>> +
>>>       tpm_irq: tpm-irq {
>>>           samsung,pins = "gpx1-0";
>>>           samsung,pin-function = <0>;
>>> @@ -752,6 +778,13 @@
>>>           samsung,pin-drv = <0>;
>>>       };
>>>
>>> +    dev_mode: dev-mode {
>>> +        samsung,pins = "gpx1-3";
>>> +        samsung,pin-function = <0>;
>>> +        samsung,pin-pud = <3>;
>>> +        samsung,pin-drv = <0>;
>>> +    };
>>> +
>>>       ec_irq: ec-irq {
>>>           samsung,pins = "gpx1-5";
>>>           samsung,pin-function = <0>;
>>> @@ -773,6 +806,13 @@
>>>           samsung,pin-drv = <0>;
>>>       };
>>>
>>> +    wp_gpio: wp_gpio {
>>> +        samsung,pins = "gpx3-0";
>>> +        samsung,pin-function = <0>;
>>> +        samsung,pin-pud = <0>;
>>> +        samsung,pin-drv = <0>;
>>> +    };
>>> +
>>>       max77802_irq: max77802-irq {
>>>           samsung,pins = "gpx3-1";
>>>           samsung,pin-function = <0>;
>>>
>>
> 
> 

Re: [PATCH 1/3] Device tree binding documentation for chromeos-firmware

[Martyn Welch]

On 02/12/15 15:15, Rob Herring wrote:
> On Tue, Dec 01, 2015 at 07:12:49PM +0000, Martyn Welch wrote:
>> This patch adds documentation for the chromeos-firmware binding.
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Pawel Moll <pawel.moll@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>> Cc: Kumar Gala <galak@codeaurora.org>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>> ---
>>   .../devicetree/bindings/misc/chromeos-firmware.txt | 27 ++++++++++++++++++++++
>
> bindings/firmware/ please.
>
>>   1 file changed, 27 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>
>> diff --git a/Documentation/devicetree/bindings/misc/chromeos-firmware.txt b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>> new file mode 100644
>> index 0000000..8240611
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>> @@ -0,0 +1,27 @@

<snip>

>> +
>> +Each signal is represented as a sub-node of "chromeos_firmware":
>> +Subnode properties:
>> +
>> +	- gpios: OF device-tree gpio specification.
>> +
>> +Example nodes:
>> +
>> +	chromeos_firmware {
>
> This should go under /firmware
>

I've changed this to be:

	firmware {
		chromeos {
			...
		};
	];

Which I generally accept (assuming this is considered a part of the 
firmware) as a better way to represent this in the device tree, however 
this has the nasty side effect of causing the device tree parsing to 
avoid parsing the chromeos child and seeing it's compatible property (as 
the firmware node isn't a bus), resulting in the probe routine not being 
called.

If I add a 'compatible = "simple-bus"' property to the firmware node it 
works, but this doesn't seem quite right as I believe "simple-bus" is 
defined as a "simple memory mapped bus".

I /could/ rewrite the initialisation to call of_find_compatible_node(), 
but this seems rather hacky and inefficient. I can think of 2 other ways 
this could be resolved:

(1) As this is only tangentially related to firmware, I rename it 
something like "chromeos-signals" and make it it's own node. In essence 
this driver provides a mechanism built on top of specific GPIO (ala 
gpio-keys seems to be, after-all this has a similar use of resources to 
that).

(2) Add a compatible string something like 'compatible="logical-group";' 
to the firmware node and add that too the bus matching logic. This would 
have the advantage of solving this in the general case (I guess there 
are other instances where a grouping of things more logically rather 
than physically connected would ideally be grouped together), though I 
expect there may be some strong views regarding this approach.

Would either of those be acceptable or is there a better way of 
resolving this that I've missed?

Martyn

Re: [PATCH 1/3] Device tree binding documentation for chromeos-firmware

[Rob Herring]

On Thu, Dec 3, 2015 at 4:14 AM, Martyn Welch
<martyn.welch@collabora.co.uk> wrote:
>
> On 02/12/15 15:15, Rob Herring wrote:
>>
>> On Tue, Dec 01, 2015 at 07:12:49PM +0000, Martyn Welch wrote:
>>>
>>> This patch adds documentation for the chromeos-firmware binding.
>>>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>> Cc: Kumar Gala <galak@codeaurora.org>
>>> Cc: devicetree@vger.kernel.org
>>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>>> ---
>>>   .../devicetree/bindings/misc/chromeos-firmware.txt | 27
>>> ++++++++++++++++++++++
>>
>>
>> bindings/firmware/ please.
>>
>>>   1 file changed, 27 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>> b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>> new file mode 100644
>>> index 0000000..8240611
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>> @@ -0,0 +1,27 @@
>
>
> <snip>
>
>>> +
>>> +Each signal is represented as a sub-node of "chromeos_firmware":
>>> +Subnode properties:
>>> +
>>> +       - gpios: OF device-tree gpio specification.
>>> +
>>> +Example nodes:
>>> +
>>> +       chromeos_firmware {
>>
>>
>> This should go under /firmware
>>
>
> I've changed this to be:
>
>         firmware {
>                 chromeos {
>                         ...
>                 };
>         ];
>
> Which I generally accept (assuming this is considered a part of the
> firmware) as a better way to represent this in the device tree, however this
> has the nasty side effect of causing the device tree parsing to avoid
> parsing the chromeos child and seeing it's compatible property (as the
> firmware node isn't a bus), resulting in the probe routine not being called.
>
> If I add a 'compatible = "simple-bus"' property to the firmware node it
> works, but this doesn't seem quite right as I believe "simple-bus" is
> defined as a "simple memory mapped bus".
>
> I /could/ rewrite the initialisation to call of_find_compatible_node(), but
> this seems rather hacky and inefficient. I can think of 2 other ways this
> could be resolved:
>
> (1) As this is only tangentially related to firmware, I rename it something
> like "chromeos-signals" and make it it's own node. In essence this driver
> provides a mechanism built on top of specific GPIO (ala gpio-keys seems to
> be, after-all this has a similar use of resources to that).

I'm starting to fail to understand the relationship to firmware here...

gpio-keys are at least a thing (being a key or set of keys). Your
grouping is a rather random collection of GPIOs. Maybe you need
"gpio-switch" binding and then the function would be "label" property.

> (2) Add a compatible string something like 'compatible="logical-group";' to
> the firmware node and add that too the bus matching logic. This would have
> the advantage of solving this in the general case (I guess there are other
> instances where a grouping of things more logically rather than physically
> connected would ideally be grouped together), though I expect there may be
> some strong views regarding this approach.

Why do you need them grouped?

> Would either of those be acceptable or is there a better way of resolving
> this that I've missed?

I don't know as I still don't really understand what the h/w looks like here.

Rob

Re: [PATCH 1/3] Device tree binding documentation for chromeos-firmware

[Martyn Welch]

On 03/12/15 15:08, Rob Herring wrote:
> On Thu, Dec 3, 2015 at 4:14 AM, Martyn Welch
> <martyn.welch@collabora.co.uk> wrote:
>>
>> On 02/12/15 15:15, Rob Herring wrote:
>>>
>>> On Tue, Dec 01, 2015 at 07:12:49PM +0000, Martyn Welch wrote:
>>>>
>>>> This patch adds documentation for the chromeos-firmware binding.
>>>>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>>> Cc: Kumar Gala <galak@codeaurora.org>
>>>> Cc: devicetree@vger.kernel.org
>>>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>>>> ---
>>>>    .../devicetree/bindings/misc/chromeos-firmware.txt | 27
>>>> ++++++++++++++++++++++
>>>
>>>
>>> bindings/firmware/ please.
>>>
>>>>    1 file changed, 27 insertions(+)
>>>>    create mode 100644
>>>> Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>>> b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>>> new file mode 100644
>>>> index 0000000..8240611
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/misc/chromeos-firmware.txt
>>>> @@ -0,0 +1,27 @@
>>
>>
>> <snip>
>>
>>>> +
>>>> +Each signal is represented as a sub-node of "chromeos_firmware":
>>>> +Subnode properties:
>>>> +
>>>> +       - gpios: OF device-tree gpio specification.
>>>> +
>>>> +Example nodes:
>>>> +
>>>> +       chromeos_firmware {
>>>
>>>
>>> This should go under /firmware
>>>
>>
>> I've changed this to be:
>>
>>          firmware {
>>                  chromeos {
>>                          ...
>>                  };
>>          ];
>>
>> Which I generally accept (assuming this is considered a part of the
>> firmware) as a better way to represent this in the device tree, however this
>> has the nasty side effect of causing the device tree parsing to avoid
>> parsing the chromeos child and seeing it's compatible property (as the
>> firmware node isn't a bus), resulting in the probe routine not being called.
>>
>> If I add a 'compatible = "simple-bus"' property to the firmware node it
>> works, but this doesn't seem quite right as I believe "simple-bus" is
>> defined as a "simple memory mapped bus".
>>
>> I /could/ rewrite the initialisation to call of_find_compatible_node(), but
>> this seems rather hacky and inefficient. I can think of 2 other ways this
>> could be resolved:
>>
>> (1) As this is only tangentially related to firmware, I rename it something
>> like "chromeos-signals" and make it it's own node. In essence this driver
>> provides a mechanism built on top of specific GPIO (ala gpio-keys seems to
>> be, after-all this has a similar use of resources to that).
>
> I'm starting to fail to understand the relationship to firmware here...
>
> gpio-keys are at least a thing (being a key or set of keys). Your
> grouping is a rather random collection of GPIOs. Maybe you need
> "gpio-switch" binding and then the function would be "label" property.
>

So, something like this:

gpio-switch {
	compatible = "gpio-switch";

	pinctrl-names = "default";
	pinctrl-0 = <&wp_gpio &dev_mode &rec_mode>;

	write-protect {
		label = "write-protect";
		gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
		read-only;
	};

	developer-switch {
		label = "developer-switch";
		gpios = <&gpx1 3 GPIO_ACTIVE_HIGH>;
		read-only;
	};

	recovery-switch {
		label = "recovery-switch";
		gpios = <&gpx0 7 GPIO_ACTIVE_LOW>;
		read-only;
	};
};

(Making it much more generic in the process.)


>> (2) Add a compatible string something like 'compatible="logical-group";' to
>> the firmware node and add that too the bus matching logic. This would have
>> the advantage of solving this in the general case (I guess there are other
>> instances where a grouping of things more logically rather than physically
>> connected would ideally be grouped together), though I expect there may be
>> some strong views regarding this approach.
>
> Why do you need them grouped?
>

That's effectively what is achieved by putting this (and I assume 
anything else considered "firmware" under a firmware node isn't it? (or 
and I miss-understanding your request?)

I think it is a moot point, I'll rework as you've suggested.

Martyn