[PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

The MinnowBoard (http://www.minnowboard.org) is an Intel Atom (E6xx) plus EG20T
PCH development board. It uses a few GPIO lines for specific purposes and
exposes the rest to the user.

Request the dedicated GPIO lines:
	HWID
	LVDS_DETECT
	PHY_RESET
	LED0
	LED1

Setup platform drivers for the MinnowBoard LEDs using the leds-gpio
driver. Setup led0 and led1 with heartbeat and mmc0 default triggers
respectively.

GPIO lines SUS[0-4] are dual purpose, either for LVDS signaling or as
user GPIO. Determine which via the LVDS_DETECT signal and enable or
disable them accordingly.

Provide a minimal public interface:
	minnow_detect()
	minnow_lvds_detect()
	minnow_hwid()
	minnow_phy_reset()

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: Matthew Garrett <matthew.garrett@nebula.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/Kconfig            |  20 ++++
 drivers/platform/x86/Makefile           |   1 +
 drivers/platform/x86/minnowboard-gpio.h |  60 ++++++++++
 drivers/platform/x86/minnowboard.c      | 193 ++++++++++++++++++++++++++++++++
 include/linux/minnowboard.h             |  37 ++++++
 5 files changed, 311 insertions(+)
 create mode 100644 drivers/platform/x86/minnowboard-gpio.h
 create mode 100644 drivers/platform/x86/minnowboard.c
 create mode 100644 include/linux/minnowboard.h

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 8577261..154dbf6 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -15,6 +15,26 @@ menuconfig X86_PLATFORM_DEVICES
 
 if X86_PLATFORM_DEVICES
 
+config MINNOWBOARD
+	tristate "MinnowBoard GPIO and LVDS support"
+	depends on LPC_SCH
+	depends on GPIO_SCH
+	depends on GPIO_PCH
+	depends on LEDS_GPIO
+	default n
+	---help---
+	  This driver configures the MinnowBoard fixed functionality GPIO lines.
+
+	  It ensures that the E6XX SUS GPIOs muxed with LVDS signals (SUS[0:2])
+	  are disabled if the LVDS_DETECT signal is asserted.
+
+	  If LED_TRIGGER* are enabled, LED0 will use the heartbeat trigger and
+	  LED1 will use the mmc0 trigger.
+
+	  The Minnow Hardware ID is read from the GPIO HWID lines and logged.
+
+	  If you have a MinnowBoard, say Y or M here.
+
 config ACER_WMI
 	tristate "Acer WMI Laptop Extras"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index ef0ec74..45ede1c 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -2,6 +2,7 @@
 # Makefile for linux/drivers/platform/x86
 # x86 Platform-Specific Drivers
 #
+obj-$(CONFIG_MINNOWBOARD)	+= minnowboard.o
 obj-$(CONFIG_ASUS_LAPTOP)	+= asus-laptop.o
 obj-$(CONFIG_ASUS_WMI)		+= asus-wmi.o
 obj-$(CONFIG_ASUS_NB_WMI)	+= asus-nb-wmi.o
diff --git a/drivers/platform/x86/minnowboard-gpio.h b/drivers/platform/x86/minnowboard-gpio.h
new file mode 100644
index 0000000..ccc8361
--- /dev/null
+++ b/drivers/platform/x86/minnowboard-gpio.h
@@ -0,0 +1,60 @@
+/*
+ * MinnowBoard Linux platform driver
+ * Copyright (c) 2013, Intel Corporation.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Author: Darren Hart <dvhart@linux.intel.com>
+ */
+
+/* MinnowBoard GPIO definitions */
+#define GPIO_BTN0 0
+#define GPIO_BTN1 1
+#define GPIO_BTN2 2
+#define GPIO_BTN3 3
+
+#define GPIO_PROG_VOLTAGE 4
+
+/*
+ * If !LVDS_DETECT, the AUX lines are available as GPIO,
+ * otherwise they are used for LVDS signals.
+ */
+#define GPIO_AUX0 5
+#define GPIO_AUX1 6
+#define GPIO_AUX2 7
+#define GPIO_AUX3 8
+#define GPIO_AUX4 9
+
+#define GPIO_LED0 10
+#define GPIO_LED1 11
+
+#define GPIO_USB_VBUS_DETECT 12
+
+#define GPIO_PHY_RESET 13
+
+#define GPIO_PCH0 244
+#define GPIO_PCH1 245
+#define GPIO_PCH2 246
+#define GPIO_PCH3 247
+#define GPIO_PCH4 248
+#define GPIO_PCH5 249
+#define GPIO_PCH6 250
+#define GPIO_PCH7 251
+
+#define GPIO_HWID0 252
+#define GPIO_HWID1 253
+#define GPIO_HWID2 254
+
+#define GPIO_LVDS_DETECT 255
diff --git a/drivers/platform/x86/minnowboard.c b/drivers/platform/x86/minnowboard.c
new file mode 100644
index 0000000..73d59c1
--- /dev/null
+++ b/drivers/platform/x86/minnowboard.c
@@ -0,0 +1,193 @@
+/*
+ * MinnowBoard Linux platform driver
+ * Copyright (c) 2013, Intel Corporation.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Author: Darren Hart <dvhart@linux.intel.com>
+ */
+
+#define pr_fmt(fmt) "MinnowBoard: " fmt
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/dmi.h>
+#include <linux/input.h>
+#include <linux/gpio.h>
+#include <linux/leds.h>
+#include <linux/gpio-sch.h>
+#include <linux/delay.h>
+#include <linux/minnowboard.h>
+#include "minnowboard-gpio.h"
+
+static int minnow_hwid_val = -1;
+
+/* leds-gpio platform device structures */
+static const struct gpio_led minnow_leds[] = {
+	{ .name = "minnow_led0", .gpio = GPIO_LED0, .active_low = 0,
+	  .retain_state_suspended = 1, .default_state = LEDS_GPIO_DEFSTATE_ON,
+	  .default_trigger = "heartbeat"},
+	{ .name = "minnow_led1", .gpio = GPIO_LED1, .active_low = 0,
+	  .retain_state_suspended = 1, .default_state = LEDS_GPIO_DEFSTATE_ON,
+	  .default_trigger = "mmc0"},
+};
+
+static struct gpio_led_platform_data minnow_leds_platform_data = {
+	.num_leds = ARRAY_SIZE(minnow_leds),
+	.leds = (void *) minnow_leds,
+};
+
+static struct platform_device minnow_gpio_leds = {
+	.name =	"leds-gpio",
+	.id = -1,
+	.dev = {
+		.platform_data = &minnow_leds_platform_data,
+	},
+};
+
+static struct gpio hwid_gpios[] = {
+	{ GPIO_HWID0, GPIOF_DIR_IN | GPIOF_EXPORT, "minnow_gpio_hwid0" },
+	{ GPIO_HWID1, GPIOF_DIR_IN | GPIOF_EXPORT, "minnow_gpio_hwid1" },
+	{ GPIO_HWID2, GPIOF_DIR_IN | GPIOF_EXPORT, "minnow_gpio_hwid2" },
+};
+
+int minnow_hwid(void)
+{
+	/* This should never be called prior to minnow_init_module() */
+	WARN_ON_ONCE(minnow_hwid_val == -1);
+	return minnow_hwid_val;
+}
+EXPORT_SYMBOL_GPL(minnow_hwid);
+
+bool minnow_detect(void)
+{
+	const char *cmp;
+
+	cmp = dmi_get_system_info(DMI_BOARD_NAME);
+	if (cmp && strstr(cmp, "MinnowBoard"))
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(minnow_detect);
+
+bool minnow_lvds_detect(void)
+{
+	return !!gpio_get_value(GPIO_LVDS_DETECT);
+}
+EXPORT_SYMBOL_GPL(minnow_lvds_detect);
+
+void minnow_phy_reset(void)
+{
+	/*
+	 * Hold reset for a little over 1ms and allow some time after to ensure
+	 * the PHY has fully woken up.
+	 */
+	gpio_set_value(GPIO_PHY_RESET, 0);
+	usleep_range(1250, 1500);
+	gpio_set_value(GPIO_PHY_RESET, 1);
+	usleep_range(1250, 1500);
+}
+EXPORT_SYMBOL_GPL(minnow_phy_reset);
+
+static int __init minnow_module_init(void)
+{
+	int err, val, i;
+
+	err = -ENODEV;
+	if (!minnow_detect())
+		goto out;
+
+#ifdef MODULE
+/* Load any implicit dependencies that are not built-in */
+#ifdef CONFIG_LPC_SCH_MODULE
+	if (request_module("lpc_sch"))
+		goto out;
+#endif
+#ifdef CONFIG_GPIO_SCH_MODULE
+	if (request_module("gpio-sch"))
+		goto out;
+#endif
+#ifdef CONFIG_GPIO_PCH_MODULE
+	if (request_module("gpio-pch"))
+		goto out;
+#endif
+#endif
+
+	/* HWID GPIOs */
+	err = gpio_request_array(hwid_gpios, ARRAY_SIZE(hwid_gpios));
+	if (err) {
+		pr_err("Failed to request hwid GPIO lines\n");
+		goto out;
+	}
+	minnow_hwid_val = (!!gpio_get_value(GPIO_HWID0)) |
+			  (!!gpio_get_value(GPIO_HWID1) << 1) |
+			  (!!gpio_get_value(GPIO_HWID2) << 2);
+
+	pr_info("Hardware ID: %d\n", minnow_hwid_val);
+
+	err = gpio_request_one(GPIO_LVDS_DETECT, GPIOF_DIR_IN | GPIOF_EXPORT,
+			       "minnow_lvds_detect");
+	if (err) {
+		pr_err("Failed to request LVDS_DETECT GPIO line (%d)\n",
+		       GPIO_LVDS_DETECT);
+		goto out;
+	}
+
+	/* Disable the GPIO lines if LVDS is detected */
+	val = minnow_lvds_detect() ? 1 : 0;
+	pr_info("Aux GPIO lines %s\n", val ? "Disabled" : "Enabled");
+	for (i = 0; i < 5; i++)
+		sch_gpio_resume_set_enable(i, !val);
+
+	/* Reserve the AR8031 PHY's ETH_RESET GPIO line */
+	err = gpio_request_one(GPIO_PHY_RESET,
+			       GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT,
+			       "minnow_phy_reset");
+	if (err) {
+		pr_err("Failed to request PHY_RESET GPIO line (%d)\n",
+		       GPIO_PHY_RESET);
+		goto out_lvds;
+	}
+
+	/* GPIO LEDs */
+	err = platform_device_register(&minnow_gpio_leds);
+	if (err) {
+		pr_err("Failed to register leds-gpio platform device\n");
+		goto out_phy;
+	}
+	goto out;
+
+ out_phy:
+	gpio_free(GPIO_PHY_RESET);
+
+ out_lvds:
+	gpio_free(GPIO_LVDS_DETECT);
+
+ out:
+	return err;
+}
+
+static void __exit minnow_module_exit(void)
+{
+	gpio_free(GPIO_LVDS_DETECT);
+	gpio_free(GPIO_PHY_RESET);
+	platform_device_unregister(&minnow_gpio_leds);
+}
+
+module_init(minnow_module_init);
+module_exit(minnow_module_exit);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/minnowboard.h b/include/linux/minnowboard.h
new file mode 100644
index 0000000..d3608b8
--- /dev/null
+++ b/include/linux/minnowboard.h
@@ -0,0 +1,37 @@
+/*
+ * MinnowBoard Linux platform driver
+ * Copyright (c) 2013, Intel Corporation.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Author: Darren Hart <dvhart@linux.intel.com>
+ */
+
+#ifndef _LINUX_MINNOWBOARD_H
+#define _LINUX_MINNOWBOARD_H
+
+#if defined(CONFIG_MINNOWBOARD) || defined(CONFIG_MINNOWBOARD_MODULE)
+bool minnow_detect(void);
+bool minnow_lvds_detect(void);
+int minnow_hwid(void);
+void minnow_phy_reset(void);
+#else
+#define minnow_detect() (false)
+#define minnow_lvds_detect() (false)
+#define minnow_hwid() (-1)
+#define minnow_phy_reset() do { } while (0)
+#endif /* MINNOWBOARD */
+
+#endif /* _LINUX_MINNOWBOARD_H */
-- 
1.8.1.2

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Olof Johansson]

Hi,

On Tue, Jun 25, 2013 at 06:53:24PM -0700, Darren Hart wrote:
> The MinnowBoard (http://www.minnowboard.org) is an Intel Atom (E6xx) plus EG20T
> PCH development board. It uses a few GPIO lines for specific purposes and
> exposes the rest to the user.
> 
> Request the dedicated GPIO lines:
> 	HWID
> 	LVDS_DETECT
> 	PHY_RESET
> 	LED0
> 	LED1
> 
> Setup platform drivers for the MinnowBoard LEDs using the leds-gpio
> driver. Setup led0 and led1 with heartbeat and mmc0 default triggers
> respectively.
> 
> GPIO lines SUS[0-4] are dual purpose, either for LVDS signaling or as
> user GPIO. Determine which via the LVDS_DETECT signal and enable or
> disable them accordingly.
> 
> Provide a minimal public interface:
> 	minnow_detect()
> 	minnow_lvds_detect()
> 	minnow_hwid()
> 	minnow_phy_reset()
> 
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> Cc: Matthew Garrett <matthew.garrett@nebula.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: platform-driver-x86@vger.kernel.org
> ---
>  drivers/platform/x86/Kconfig            |  20 ++++
>  drivers/platform/x86/Makefile           |   1 +
>  drivers/platform/x86/minnowboard-gpio.h |  60 ++++++++++
>  drivers/platform/x86/minnowboard.c      | 193 ++++++++++++++++++++++++++++++++
>  include/linux/minnowboard.h             |  37 ++++++
>  5 files changed, 311 insertions(+)
>  create mode 100644 drivers/platform/x86/minnowboard-gpio.h
>  create mode 100644 drivers/platform/x86/minnowboard.c
>  create mode 100644 include/linux/minnowboard.h

Hmmmm. x86 boardfiles arriving under drivers/platform. 

The main concern is that this won't really scale if more vendors add
variations of this board -- needing code changes for each and every one
of them. Given that it's an open platform encouraging derivatives, that seems
like a slippery slope.

It's really unfortunate that this information couldn't be passed in from
firmware. We've been working so hard for so long on ARM now to move away
from board files, it's such a pity to add them on another major platform.

> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 8577261..154dbf6 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -15,6 +15,26 @@ menuconfig X86_PLATFORM_DEVICES
>  
>  if X86_PLATFORM_DEVICES
>  
> +config MINNOWBOARD
> +	tristate "MinnowBoard GPIO and LVDS support"
> +	depends on LPC_SCH
> +	depends on GPIO_SCH
> +	depends on GPIO_PCH
> +	depends on LEDS_GPIO
> +	default n

No need to default n. 'n' is the default default. :)

> +	---help---
> +	  This driver configures the MinnowBoard fixed functionality GPIO lines.
> +
> +	  It ensures that the E6XX SUS GPIOs muxed with LVDS signals (SUS[0:2])
> +	  are disabled if the LVDS_DETECT signal is asserted.
> +
> +	  If LED_TRIGGER* are enabled, LED0 will use the heartbeat trigger and
> +	  LED1 will use the mmc0 trigger.
> +
> +	  The Minnow Hardware ID is read from the GPIO HWID lines and logged.
> +
> +	  If you have a MinnowBoard, say Y or M here.
> +
>  config ACER_WMI
>  	tristate "Acer WMI Laptop Extras"
>  	depends on ACPI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index ef0ec74..45ede1c 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -2,6 +2,7 @@
>  # Makefile for linux/drivers/platform/x86
>  # x86 Platform-Specific Drivers
>  #
> +obj-$(CONFIG_MINNOWBOARD)	+= minnowboard.o
>  obj-$(CONFIG_ASUS_LAPTOP)	+= asus-laptop.o
>  obj-$(CONFIG_ASUS_WMI)		+= asus-wmi.o
>  obj-$(CONFIG_ASUS_NB_WMI)	+= asus-nb-wmi.o
> diff --git a/drivers/platform/x86/minnowboard-gpio.h b/drivers/platform/x86/minnowboard-gpio.h
> new file mode 100644
> index 0000000..ccc8361
> --- /dev/null
> +++ b/drivers/platform/x86/minnowboard-gpio.h
> @@ -0,0 +1,60 @@
> +/*
> + * MinnowBoard Linux platform driver
> + * Copyright (c) 2013, Intel Corporation.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + * Author: Darren Hart <dvhart@linux.intel.com>
> + */
> +
> +/* MinnowBoard GPIO definitions */
> +#define GPIO_BTN0 0
> +#define GPIO_BTN1 1
> +#define GPIO_BTN2 2
> +#define GPIO_BTN3 3
> +
> +#define GPIO_PROG_VOLTAGE 4
> +
> +/*
> + * If !LVDS_DETECT, the AUX lines are available as GPIO,
> + * otherwise they are used for LVDS signals.
> + */
> +#define GPIO_AUX0 5
> +#define GPIO_AUX1 6
> +#define GPIO_AUX2 7
> +#define GPIO_AUX3 8
> +#define GPIO_AUX4 9
> +
> +#define GPIO_LED0 10
> +#define GPIO_LED1 11
> +
> +#define GPIO_USB_VBUS_DETECT 12
> +
> +#define GPIO_PHY_RESET 13
> +
> +#define GPIO_PCH0 244
> +#define GPIO_PCH1 245
> +#define GPIO_PCH2 246
> +#define GPIO_PCH3 247
> +#define GPIO_PCH4 248
> +#define GPIO_PCH5 249
> +#define GPIO_PCH6 250
> +#define GPIO_PCH7 251
> +
> +#define GPIO_HWID0 252
> +#define GPIO_HWID1 253
> +#define GPIO_HWID2 254
> +
> +#define GPIO_LVDS_DETECT 255

It looks like at least gpio-pch.c uses dynamic gpio numbers, which makes it
hard to define these as static numbers since they might move around depending
on module load order, etc.

Looks like gpio-sch has hardcoded base 0, so the low-numbered ones might be OK
for now.

> diff --git a/drivers/platform/x86/minnowboard.c b/drivers/platform/x86/minnowboard.c
> new file mode 100644
> index 0000000..73d59c1
> --- /dev/null
> +++ b/drivers/platform/x86/minnowboard.c
> @@ -0,0 +1,193 @@
> +/*
> + * MinnowBoard Linux platform driver
> + * Copyright (c) 2013, Intel Corporation.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + * Author: Darren Hart <dvhart@linux.intel.com>
> + */
> +
> +#define pr_fmt(fmt) "MinnowBoard: " fmt
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/dmi.h>
> +#include <linux/input.h>
> +#include <linux/gpio.h>
> +#include <linux/leds.h>
> +#include <linux/gpio-sch.h>
> +#include <linux/delay.h>
> +#include <linux/minnowboard.h>
> +#include "minnowboard-gpio.h"
> +
> +static int minnow_hwid_val = -1;
> +
> +/* leds-gpio platform device structures */
> +static const struct gpio_led minnow_leds[] = {
> +	{ .name = "minnow_led0", .gpio = GPIO_LED0, .active_low = 0,
> +	  .retain_state_suspended = 1, .default_state = LEDS_GPIO_DEFSTATE_ON,
> +	  .default_trigger = "heartbeat"},
> +	{ .name = "minnow_led1", .gpio = GPIO_LED1, .active_low = 0,
> +	  .retain_state_suspended = 1, .default_state = LEDS_GPIO_DEFSTATE_ON,
> +	  .default_trigger = "mmc0"},
> +};
> +
> +static struct gpio_led_platform_data minnow_leds_platform_data = {
> +	.num_leds = ARRAY_SIZE(minnow_leds),
> +	.leds = (void *) minnow_leds,
> +};
> +
> +static struct platform_device minnow_gpio_leds = {
> +	.name =	"leds-gpio",
> +	.id = -1,
> +	.dev = {
> +		.platform_data = &minnow_leds_platform_data,
> +	},
> +};
> +
> +static struct gpio hwid_gpios[] = {
> +	{ GPIO_HWID0, GPIOF_DIR_IN | GPIOF_EXPORT, "minnow_gpio_hwid0" },
> +	{ GPIO_HWID1, GPIOF_DIR_IN | GPIOF_EXPORT, "minnow_gpio_hwid1" },
> +	{ GPIO_HWID2, GPIOF_DIR_IN | GPIOF_EXPORT, "minnow_gpio_hwid2" },
> +};
> +
> +int minnow_hwid(void)
> +{
> +	/* This should never be called prior to minnow_init_module() */
> +	WARN_ON_ONCE(minnow_hwid_val == -1);
> +	return minnow_hwid_val;
> +}
> +EXPORT_SYMBOL_GPL(minnow_hwid);

I don't see this used anywhere, so it's hard to tell what the expected use is.
I'd just remove it until first user comes along.

> +bool minnow_detect(void)
> +{
> +	const char *cmp;
> +
> +	cmp = dmi_get_system_info(DMI_BOARD_NAME);
> +	if (cmp && strstr(cmp, "MinnowBoard"))
> +		return true;
> +
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(minnow_detect);
>
> +bool minnow_lvds_detect(void)
> +{
> +	return !!gpio_get_value(GPIO_LVDS_DETECT);
> +}
> +EXPORT_SYMBOL_GPL(minnow_lvds_detect);

These two are only used in the other subfiles.

The scope of those files are really narrow, and you end up exporting a set of
global functions for it. I suggest you just bake them all together -- there's
a small amount of flexibility lost w.r.t. keeping some as modules and others
built-in, but the code isn't very large.

> +void minnow_phy_reset(void)
> +{
> +	/*
> +	 * Hold reset for a little over 1ms and allow some time after to ensure
> +	 * the PHY has fully woken up.
> +	 */
> +	gpio_set_value(GPIO_PHY_RESET, 0);
> +	usleep_range(1250, 1500);
> +	gpio_set_value(GPIO_PHY_RESET, 1);
> +	usleep_range(1250, 1500);
> +}
> +EXPORT_SYMBOL_GPL(minnow_phy_reset);

What phy? USB? SATA? Ethernet?

I wonder if you'd be better off just putting the GPIO line in the table in the
driver instead of the reset function pointer, and have this done from there
instead of exporting function pointers from board code to drivers.

> +static int __init minnow_module_init(void)
> +{
> +	int err, val, i;
> +
> +	err = -ENODEV;
> +	if (!minnow_detect())
> +		goto out;
> +
> +#ifdef MODULE
> +/* Load any implicit dependencies that are not built-in */
> +#ifdef CONFIG_LPC_SCH_MODULE
> +	if (request_module("lpc_sch"))

Less ifdefs with:

if (IS_MODULE(....) && request_module(..))

> +	/* HWID GPIOs */
> +	err = gpio_request_array(hwid_gpios, ARRAY_SIZE(hwid_gpios));
> +	if (err) {
> +		pr_err("Failed to request hwid GPIO lines\n");
> +		goto out;
> +	}
> +	minnow_hwid_val = (!!gpio_get_value(GPIO_HWID0)) |
> +			  (!!gpio_get_value(GPIO_HWID1) << 1) |
> +			  (!!gpio_get_value(GPIO_HWID2) << 2);
> +
> +	pr_info("Hardware ID: %d\n", minnow_hwid_val);
> +
> +	err = gpio_request_one(GPIO_LVDS_DETECT, GPIOF_DIR_IN | GPIOF_EXPORT,
> +			       "minnow_lvds_detect");
> +	if (err) {
> +		pr_err("Failed to request LVDS_DETECT GPIO line (%d)\n",
> +		       GPIO_LVDS_DETECT);
> +		goto out;
> +	}
> +
> +	/* Disable the GPIO lines if LVDS is detected */
> +	val = minnow_lvds_detect() ? 1 : 0;
> +	pr_info("Aux GPIO lines %s\n", val ? "Disabled" : "Enabled");
> +	for (i = 0; i < 5; i++)
> +		sch_gpio_resume_set_enable(i, !val);
> +
> +	/* Reserve the AR8031 PHY's ETH_RESET GPIO line */
> +	err = gpio_request_one(GPIO_PHY_RESET,
> +			       GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT,
> +			       "minnow_phy_reset");
> +	if (err) {
> +		pr_err("Failed to request PHY_RESET GPIO line (%d)\n",
> +		       GPIO_PHY_RESET);
> +		goto out_lvds;
> +	}
> +
> +	/* GPIO LEDs */
> +	err = platform_device_register(&minnow_gpio_leds);
> +	if (err) {
> +		pr_err("Failed to register leds-gpio platform device\n");
> +		goto out_phy;
> +	}
> +	goto out;
> +
> + out_phy:
> +	gpio_free(GPIO_PHY_RESET);
> +
> + out_lvds:
> +	gpio_free(GPIO_LVDS_DETECT);
> +
> + out:
> +	return err;
> +}
> +
> +static void __exit minnow_module_exit(void)
> +{
> +	gpio_free(GPIO_LVDS_DETECT);
> +	gpio_free(GPIO_PHY_RESET);
> +	platform_device_unregister(&minnow_gpio_leds);
> +}
> +
> +module_init(minnow_module_init);
> +module_exit(minnow_module_exit);
> +
> +MODULE_LICENSE("GPL");

You probably want a MODULE_DEVICE_TABLE(dmi, ...) here?

> diff --git a/include/linux/minnowboard.h b/include/linux/minnowboard.h
> new file mode 100644
> index 0000000..d3608b8
> --- /dev/null
> +++ b/include/linux/minnowboard.h
> @@ -0,0 +1,37 @@
> +/*
> + * MinnowBoard Linux platform driver
> + * Copyright (c) 2013, Intel Corporation.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + * Author: Darren Hart <dvhart@linux.intel.com>
> + */
> +
> +#ifndef _LINUX_MINNOWBOARD_H
> +#define _LINUX_MINNOWBOARD_H
> +
> +#if defined(CONFIG_MINNOWBOARD) || defined(CONFIG_MINNOWBOARD_MODULE)
> +bool minnow_detect(void);
> +bool minnow_lvds_detect(void);
> +int minnow_hwid(void);
> +void minnow_phy_reset(void);
> +#else
> +#define minnow_detect() (false)
> +#define minnow_lvds_detect() (false)
> +#define minnow_hwid() (-1)
> +#define minnow_phy_reset() do { } while (0)
> +#endif /* MINNOWBOARD */

Besides the phy_reset() export, the rest of these don't leave this
subdirectory. It'd be nice not to expose this very board-specific stuff to the
rest of the kernel -- temptation tends to be too great to resist adding more
and more things over time.


-Olof

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Tue, 2013-06-25 at 21:00 -0700, Olof Johansson wrote:
> Hi,

Hey Olof, thanks for the review!

David M, search for "minnow_phy_reset" for your bit :-)

> 
> On Tue, Jun 25, 2013 at 06:53:24PM -0700, Darren Hart wrote:
> > The MinnowBoard (http://www.minnowboard.org) is an Intel Atom (E6xx) plus EG20T
> > PCH development board. It uses a few GPIO lines for specific purposes and
> > exposes the rest to the user.
> > 
> > Request the dedicated GPIO lines:
> > 	HWID
> > 	LVDS_DETECT
> > 	PHY_RESET
> > 	LED0
> > 	LED1
> > 
> > Setup platform drivers for the MinnowBoard LEDs using the leds-gpio
> > driver. Setup led0 and led1 with heartbeat and mmc0 default triggers
> > respectively.
> > 
> > GPIO lines SUS[0-4] are dual purpose, either for LVDS signaling or as
> > user GPIO. Determine which via the LVDS_DETECT signal and enable or
> > disable them accordingly.
> > 
> > Provide a minimal public interface:
> > 	minnow_detect()
> > 	minnow_lvds_detect()
> > 	minnow_hwid()
> > 	minnow_phy_reset()
> > 
> > Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> > Cc: Matthew Garrett <matthew.garrett@nebula.com>
> > Cc: Grant Likely <grant.likely@linaro.org>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: platform-driver-x86@vger.kernel.org
> > ---
> >  drivers/platform/x86/Kconfig            |  20 ++++
> >  drivers/platform/x86/Makefile           |   1 +
> >  drivers/platform/x86/minnowboard-gpio.h |  60 ++++++++++
> >  drivers/platform/x86/minnowboard.c      | 193 ++++++++++++++++++++++++++++++++
> >  include/linux/minnowboard.h             |  37 ++++++
> >  5 files changed, 311 insertions(+)
> >  create mode 100644 drivers/platform/x86/minnowboard-gpio.h
> >  create mode 100644 drivers/platform/x86/minnowboard.c
> >  create mode 100644 include/linux/minnowboard.h
> 
> Hmmmm. x86 boardfiles arriving under drivers/platform. 


Indeed, I hate myself for this :-)

Here was my rationale, feel free to pick it apart:

1) I need time, possibly a couple of months, to get proper ACPI support
for these drivers into the firmware. Then I can rewrite these as ACPI
drivers as is proper for an x86 board. I've already started down this
path.

2) I felt the value of getting something upstream, even if it isn't
perfect, before the board ships was preferable to only having it in
what is effectively a vendor tree (linux-yocto_3.8 standard/minnow) and
risking various other implementations popping up and confusing the
situation.

3) I at least wanted to fix the pch_gbe support which is currently tied
up with these platform drivers. I considered pushing the
minnow_phy_reset into pch_gbe, but I previously was scolded for putting
too much board-specific knowledge into that driver.


> The main concern is that this won't really scale if more vendors add
> variations of this board -- needing code changes for each and every one
> of them. Given that it's an open platform encouraging derivatives, that seems
> like a slippery slope.


+100000


> It's really unfortunate that this information couldn't be passed in from
> firmware. We've been working so hard for so long on ARM now to move away
> from board files, it's such a pity to add them on another major platform.


Nod, see above. If this trumps the rationale above, it might just make
sense to push the phy reset into the pch_gbe driver and hold off on
these. Alternatively, we can choose to accept that this is transitional
with the understanding that drivers/platform/x86/minnow* *will* go
away.


> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 8577261..154dbf6 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -15,6 +15,26 @@ menuconfig X86_PLATFORM_DEVICES
> >  
> >  if X86_PLATFORM_DEVICES
> >  
> > +config MINNOWBOARD
> > +	tristate "MinnowBoard GPIO and LVDS support"
> > +	depends on LPC_SCH
> > +	depends on GPIO_SCH
> > +	depends on GPIO_PCH
> > +	depends on LEDS_GPIO
> > +	default n
> 
> No need to default n. 'n' is the default default. :)


Check. Thanks.

...

> > +#define GPIO_PCH0 244
> > +#define GPIO_PCH1 245
> > +#define GPIO_PCH2 246
> > +#define GPIO_PCH3 247
> > +#define GPIO_PCH4 248
> > +#define GPIO_PCH5 249
> > +#define GPIO_PCH6 250
> > +#define GPIO_PCH7 251
> > +
> > +#define GPIO_HWID0 252
> > +#define GPIO_HWID1 253
> > +#define GPIO_HWID2 254
> > +
> > +#define GPIO_LVDS_DETECT 255
> 
> It looks like at least gpio-pch.c uses dynamic gpio numbers, which makes it
> hard to define these as static numbers since they might move around depending
> on module load order, etc.


Oh dear, not something I've experienced. OK, I'll go look into how to
properly handle that case. Any pointers?

...

> > +int minnow_hwid(void)
> > +{
> > +	/* This should never be called prior to minnow_init_module() */
> > +	WARN_ON_ONCE(minnow_hwid_val == -1);
> > +	return minnow_hwid_val;
> > +}
> > +EXPORT_SYMBOL_GPL(minnow_hwid);
> 
> I don't see this used anywhere, so it's hard to tell what the expected use is.
> I'd just remove it until first user comes along.


OK, can do.


> > +bool minnow_detect(void)
> > +{
> > +	const char *cmp;
> > +
> > +	cmp = dmi_get_system_info(DMI_BOARD_NAME);
> > +	if (cmp && strstr(cmp, "MinnowBoard"))
> > +		return true;
> > +
> > +	return false;
> > +}
> > +EXPORT_SYMBOL_GPL(minnow_detect);
> >
> > +bool minnow_lvds_detect(void)
> > +{
> > +	return !!gpio_get_value(GPIO_LVDS_DETECT);
> > +}
> > +EXPORT_SYMBOL_GPL(minnow_lvds_detect);
> 
> These two are only used in the other subfiles.
> 
> The scope of those files are really narrow, and you end up exporting a set of
> global functions for it. I suggest you just bake them all together -- there's
> a small amount of flexibility lost w.r.t. keeping some as modules and others
> built-in, but the code isn't very large.


I had them together initially. The reason I broke them up was to
separate fixed functionality from example code. minnowboard.c sets up
the API and any fixed things, like the GPIO LEDs. Those can be easily
configured through LED triggers, so no need to allow for a different
kind of driver.

minnowboard-gpio.c provides easy user access to the GPIO, but would
conflict with a properly written driver for a device driven with the
GPIO. This one is for prototyping and experimentation, but shouldn't be
considered as a "permanent" driver like minnowboard.c is.

minnowboard-keys.c is example code and could possibly be considered a
"permanent" driver once the ACPI version makes it a bit more
configurable by specifying the key codes and such outside the kernel
driver.

minnow_detect() can be replaced with DMI_MATCH() anywhere, so perhaps
this should just go away once the drivers are ACPI'ified.

minnow_lvds_detect().... that's a long story. I only really need it in
minnowboard-gpio.c to make sure I only export the GPIO if they are not
being used for LVDS. I could move it to that driver and remove the
export altogether.


> 
> > +void minnow_phy_reset(void)
> > +{
> > +	/*
> > +	 * Hold reset for a little over 1ms and allow some time after to ensure
> > +	 * the PHY has fully woken up.
> > +	 */
> > +	gpio_set_value(GPIO_PHY_RESET, 0);
> > +	usleep_range(1250, 1500);
> > +	gpio_set_value(GPIO_PHY_RESET, 1);
> > +	usleep_range(1250, 1500);
> > +}
> > +EXPORT_SYMBOL_GPL(minnow_phy_reset);
> 
> What phy? USB? SATA? Ethernet?

Ethernet, yes.

> 
> I wonder if you'd be better off just putting the GPIO line in the table in the
> driver instead of the reset function pointer, and have this done from there
> instead of exporting function pointers from board code to drivers.


I bike-shed'ed this until I was blue and decided I should leave it up
to David Miller as he was likely to have an opinion on consolidation of
functionality versus board-agnostic code.

David, would you prefer I set the GPIO from the PCI Subsystem ID and
move the reset function into the pch_gbe driver?


> 
> > +static int __init minnow_module_init(void)
> > +{
> > +	int err, val, i;
> > +
> > +	err = -ENODEV;
> > +	if (!minnow_detect())
> > +		goto out;
> > +
> > +#ifdef MODULE
> > +/* Load any implicit dependencies that are not built-in */
> > +#ifdef CONFIG_LPC_SCH_MODULE
> > +	if (request_module("lpc_sch"))
> 
> Less ifdefs with:
> 
> if (IS_MODULE(....) && request_module(..))


Shiny. Can do. Thanks.


> > +
> > +module_init(minnow_module_init);
> > +module_exit(minnow_module_exit);
> > +
> > +MODULE_LICENSE("GPL");
> 
> You probably want a MODULE_DEVICE_TABLE(dmi, ...) here?


See what happens when core kernel people are allowed to write driver
code? How does this relate to converting this over to an ACPI device
driver? I guess I would replace the above with
MODULE_DEVICE_TABLE(acpi, ...) ? If I do the above.... is this still an
evil board-file? What makes the acpi method of discover superior to
setting up linux-hotplug via dmi?

I think there is precious piece of learning to be had here (for me I
mean)...


> > diff --git a/include/linux/minnowboard.h b/include/linux/minnowboard.h
...
> > +#ifndef _LINUX_MINNOWBOARD_H
> > +#define _LINUX_MINNOWBOARD_H
> > +
> > +#if defined(CONFIG_MINNOWBOARD) || defined(CONFIG_MINNOWBOARD_MODULE)
> > +bool minnow_detect(void);
> > +bool minnow_lvds_detect(void);
> > +int minnow_hwid(void);
> > +void minnow_phy_reset(void);
> > +#else
> > +#define minnow_detect() (false)
> > +#define minnow_lvds_detect() (false)
> > +#define minnow_hwid() (-1)
> > +#define minnow_phy_reset() do { } while (0)
> > +#endif /* MINNOWBOARD */
> 
> Besides the phy_reset() export, the rest of these don't leave this
> subdirectory. It'd be nice not to expose this very board-specific stuff to the
> rest of the kernel -- temptation tends to be too great to resist adding more
> and more things over time.

Good point. I'll address in V2 after we bat around some of the above a
bit.

Thank you for taking the time Olof!

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Matthew Garrett]

On Tue, 2013-06-25 at 21:43 -0700, Darren Hart wrote:

> 1) I need time, possibly a couple of months, to get proper ACPI support
> for these drivers into the firmware. Then I can rewrite these as ACPI
> drivers as is proper for an x86 board. I've already started down this
> path.

A couple of months pushes you back one kernel release. It's not a huge
deal. I think I side with Olof here - the kernel developers have pushed
back against hardcoded and NIHed ARM device descriptors, and given that
we have a perfectly reasonable standard in the X86 world (ie, ACPI), I'm
not enthusiastic about merging something that's (a) going to be
superseded in the near future and (b) may end up serving as an example
to others who think this is ok.

Do these boards currently boot any other OSes?

> See what happens when core kernel people are allowed to write driver
> code? How does this relate to converting this over to an ACPI device
> driver? I guess I would replace the above with
> MODULE_DEVICE_TABLE(acpi, ...) ? If I do the above.... is this still an
> evil board-file? What makes the acpi method of discover superior to
> setting up linux-hotplug via dmi?

MODULE_DEVICE_TABLE is invisible to the running driver, it just exports
metadata that udev will use to decide whether to load the driver. If a
driver is intended to deal with a specific board, DMI makes sense. If
it's intended to deal with a specific ACPI device (ie, something with a
_HID that defines the programming model), ACPI makes sense.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Wed, 2013-06-26 at 04:52 +0000, Matthew Garrett wrote:
> On Tue, 2013-06-25 at 21:43 -0700, Darren Hart wrote:
> 
> > 1) I need time, possibly a couple of months, to get proper ACPI support
> > for these drivers into the firmware. Then I can rewrite these as ACPI
> > drivers as is proper for an x86 board. I've already started down this
> > path.
> 
> A couple of months pushes you back one kernel release. It's not a huge
> deal. I think I side with Olof here - the kernel developers have pushed
> back against hardcoded and NIHed ARM device descriptors, and given that
> we have a perfectly reasonable standard in the X86 world (ie, ACPI), I'm
> not enthusiastic about merging something that's (a) going to be
> superseded in the near future and (b) may end up serving as an example
> to others who think this is ok.


My biggest concern is fragmentation after the boards start to ship. I
recognize this comes under the "lack of planning on your part doesn't
constitute an emergency on my part" heading (although it really wasn't
a lack of planning). Now that this is out here and people can see where
it's going, if we choose to nack these and wait for a better
implementation, that alone may accomplish the same goal.


> Do these boards currently boot any other OSes?


Currently the linux-yocto_3.8 standard/minnow Linux kernel is the only
thing known to boot on it. This is what will ship with the device.


> > See what happens when core kernel people are allowed to write driver
> > code? How does this relate to converting this over to an ACPI device
> > driver? I guess I would replace the above with
> > MODULE_DEVICE_TABLE(acpi, ...) ? If I do the above.... is this still an
> > evil board-file? What makes the acpi method of discover superior to
> > setting up linux-hotplug via dmi?
> 
> MODULE_DEVICE_TABLE is invisible to the running driver, it just exports
> metadata that udev will use to decide whether to load the driver. If a
> driver is intended to deal with a specific board, DMI makes sense. If
> it's intended to deal with a specific ACPI device (ie, something with a
> _HID that defines the programming model), ACPI makes sense.

What are you referring to with "programming model" here?

The three drivers in question:
minnowboard.c
minnowboard-gpio.c
minnowboard-keys.c

are all board-specific. They map GPIO to their fixed functions and
provide an API for board-specific queries (minnowboard.c), they provide
example uses (minnowboard-gpio and minnowboard-keys) which aid in
experimentation and the development of new drivers.

Which of these make sense as ACPI devices in your opinion?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Matthew Garrett]

On Tue, 2013-06-25 at 22:32 -0700, Darren Hart wrote:

> are all board-specific. They map GPIO to their fixed functions and
> provide an API for board-specific queries (minnowboard.c), they provide
> example uses (minnowboard-gpio and minnowboard-keys) which aid in
> experimentation and the development of new drivers.
> 
> Which of these make sense as ACPI devices in your opinion?

Does the firmware contain a corresponding ACPI device with a specific
_HID that defines the programming model? If so, it should have an ACPI
driver. If not, it shouldn't.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Wed, 2013-06-26 at 05:36 +0000, Matthew Garrett wrote:
> On Tue, 2013-06-25 at 22:32 -0700, Darren Hart wrote:
> 
> > are all board-specific. They map GPIO to their fixed functions and
> > provide an API for board-specific queries (minnowboard.c), they provide
> > example uses (minnowboard-gpio and minnowboard-keys) which aid in
> > experimentation and the development of new drivers.
> > 
> > Which of these make sense as ACPI devices in your opinion?
> 
> Does the firmware contain a corresponding ACPI device with a specific
> _HID that defines the programming model? If so, it should have an ACPI
> driver. If not, it shouldn't.

It does not currently. However, the firmware is being developed along
with the hardware and the OS. We have the opportunity to do whatever
makes the most sense.

My next-steps here were to get a basic ACPI ID setup for each of the
three drivers just so they could be enumerated over ACPI rather than
just loaded manually. Beyond that I'll be looking to Len and Rafael for
guidance.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Linus Walleij]

On Wed, Jun 26, 2013 at 3:53 AM, Darren Hart <dvhart@linux.intel.com> wrote:

> Provide a minimal public interface:
>         minnow_detect()
>         minnow_lvds_detect()
>         minnow_hwid()
>         minnow_phy_reset()

So instead of these calling drivers issueing gpio_request() themselves
to obtain a resource, they make a function call to this proxy that issue
gpio_request() for them.

This is generally not how we do things. A driver should request its
GPIO just as it requests its regulator or clock or IRQ line or anything
else. Centralizing resource handling is not a good idea IMO, it's better
that each driver request it's GPIO pin(s) and do the stuff it needs
with them.

This of course creates the problem of associating the GPIOs to a
driver and how it should look that up, which I guess ACPI can do,
isn't that what acpi_find_gpio() is for?

Yours,
Linus Walleij

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Thu, 2013-06-27 at 11:14 +0200, Linus Walleij wrote:
> On Wed, Jun 26, 2013 at 3:53 AM, Darren Hart <dvhart@linux.intel.com> wrote:
> 
> > Provide a minimal public interface:
> >         minnow_detect()
> >         minnow_lvds_detect()
> >         minnow_hwid()
> >         minnow_phy_reset()
> 
> So instead of these calling drivers issueing gpio_request() themselves
> to obtain a resource, they make a function call to this proxy that issue
> gpio_request() for them.
> 
> This is generally not how we do things. A driver should request its
> GPIO just as it requests its regulator or clock or IRQ line or anything
> else.

I'll fix this for minnow_phy_reset() by moving the reset routine into
the pch_gbe driver and have it request the GPIO, using the pci_id
structure to determine the GPIO line.

I'll drop minnow_lvds_detect() and work toward the firmware managing
this aspect instead.

minnow_detect() doesn't access GPIO.

minnow_hwid() just returns an int that the minnowboard platform driver
read from the GPIO. This seems like a proper abstraction to me. Do you
object to this one as well?

>  Centralizing resource handling is not a good idea IMO, it's better
> that each driver request it's GPIO pin(s) and do the stuff it needs
> with them.

Understood. Since minnow_hwid() is not currently used anywhere anyway,
I'll take Olof's advice and just drop it. We can always add it back if
it becomes necessary.

> This of course creates the problem of associating the GPIOs to a
> driver and how it should look that up, which I guess ACPI can do,
> isn't that what acpi_find_gpio() is for?

The only one that remains is pch_gbe and we have a PCI Subsystem ID
there that we can use. Once we determine it is a MinnowBoard, we can
look the GPIO up by chip and offset. If this changes in future board
revs, we'll need minnow_hwid(), or we'll have to figure something out
either in the PCI config space or via ACPI as you suggested.

Thanks Linus, appreciate all the feedback.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Mark Brown]

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

On Thu, Jun 27, 2013 at 10:43:38PM -0700, Darren Hart wrote:

> minnow_hwid() just returns an int that the minnowboard platform driver
> read from the GPIO. This seems like a proper abstraction to me. Do you
> object to this one as well?

We should really have a subsystem for this too - the general idea idea
of identifying boards, fit options and so on by looking at things like
GPIOs or numbers in flash is really common.

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

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Linus Walleij]

On Thu, Jul 4, 2013 at 6:26 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jun 27, 2013 at 10:43:38PM -0700, Darren Hart wrote:
>
>> minnow_hwid() just returns an int that the minnowboard platform driver
>> read from the GPIO. This seems like a proper abstraction to me. Do you
>> object to this one as well?
>
> We should really have a subsystem for this too - the general idea idea
> of identifying boards, fit options and so on by looking at things like
> GPIOs or numbers in flash is really common.

Would it then be a bus following the pattern we chiseled out for
the soc bus? (Greg, Lee & Arnd architectured this.)

There we needed a struct device * on an overarching level to
tie in the sysfs entries reading out the SoC properties. But
it would be the same thing with in-kernel accessors for these
properties.

So it would be the same pattern above with a board bus, in
DT syntax:

board {
     soc {
     };
};

Yours,
Linus Walleij

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Mark Brown]

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

On Sat, Jul 20, 2013 at 07:37:54PM +0200, Linus Walleij wrote:
> On Thu, Jul 4, 2013 at 6:26 PM, Mark Brown <broonie@kernel.org> wrote:

> > We should really have a subsystem for this too - the general idea idea
> > of identifying boards, fit options and so on by looking at things like
> > GPIOs or numbers in flash is really common.

> Would it then be a bus following the pattern we chiseled out for
> the soc bus? (Greg, Lee & Arnd architectured this.)

I'd expect it to be a bus, yes.

> There we needed a struct device * on an overarching level to
> tie in the sysfs entries reading out the SoC properties. But
> it would be the same thing with in-kernel accessors for these
> properties.

I'd expect us to end up with devices for the modules doing the mapping.
I'm not sure what the accessors you're thinking about would be, this
should hopefully be transparent to devices sitting on the boards
otherwise it seems like there's not that much point really.

> So it would be the same pattern above with a board bus, in
> DT syntax:

> board {
>      soc {
>      };
> };

I'm not 100% sure what this means, sorry?  If you're saying it'd be
something that sits within the master board for the system (whatever
that happens to be) then yes.

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

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Mon, 2013-07-22 at 00:41 +0100, Mark Brown wrote:
> On Sat, Jul 20, 2013 at 07:37:54PM +0200, Linus Walleij wrote:
> > On Thu, Jul 4, 2013 at 6:26 PM, Mark Brown <broonie@kernel.org> wrote:
> 
> > > We should really have a subsystem for this too - the general idea idea
> > > of identifying boards, fit options and so on by looking at things like
> > > GPIOs or numbers in flash is really common.
> 
> > Would it then be a bus following the pattern we chiseled out for
> > the soc bus? (Greg, Lee & Arnd architectured this.)
> 
> I'd expect it to be a bus, yes.

It seems to me the platform bus already provides everything we need. We
can just make a platform driver, say platform-id-gpio.c which can get
it's platform data from OF or ACPI (_PRP proposal from kernel summit
last week for example). Why would a separate bus type need to be
defined?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Mark Brown]

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

On Wed, Oct 30, 2013 at 02:18:00PM +0000, Darren Hart wrote:
> On Mon, 2013-07-22 at 00:41 +0100, Mark Brown wrote:

> > I'd expect it to be a bus, yes.

> It seems to me the platform bus already provides everything we need. We
> can just make a platform driver, say platform-id-gpio.c which can get
> it's platform data from OF or ACPI (_PRP proposal from kernel summit
> last week for example). Why would a separate bus type need to be
> defined?

For the identifier space.  We ought to be able to register handling for
plugin boards separately to notifying the system of their existance in a
similar fashion to how we register drivers for anything else.

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

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Grant Likely]

On Thu, Jul 4, 2013 at 5:26 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jun 27, 2013 at 10:43:38PM -0700, Darren Hart wrote:
>
>> minnow_hwid() just returns an int that the minnowboard platform driver
>> read from the GPIO. This seems like a proper abstraction to me. Do you
>> object to this one as well?
>
> We should really have a subsystem for this too - the general idea idea
> of identifying boards, fit options and so on by looking at things like
> GPIOs or numbers in flash is really common.

And yet this is a platform with ACPI. I would expect the ACPI to
identify the board, not a custom driver. The newest ACPI spec adds a
lot of nice useful things like GPIO and SPI bindings. Talk to Al Stone
about the progress his team has made on adding GPIO support to ACPICA.

This driver shouldn't be merged into mainline. Keep it as an
out-of-tree patch until the proper solution is implemented. That
shouldn't be too onerous since we now have available not one, but two
mechanisms for describing exactly what you want to do; ACPI or FDT.

g.

Re: [PATCH 4/8] minnowboard: Add base platform driver for the MinnowBoard

[Darren Hart]

On Mon, 2013-07-22 at 01:09 +0100, Grant Likely wrote:
> On Thu, Jul 4, 2013 at 5:26 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Jun 27, 2013 at 10:43:38PM -0700, Darren Hart wrote:
> >
> >> minnow_hwid() just returns an int that the minnowboard platform driver
> >> read from the GPIO. This seems like a proper abstraction to me. Do you
> >> object to this one as well?
> >
> > We should really have a subsystem for this too - the general idea idea
> > of identifying boards, fit options and so on by looking at things like
> > GPIOs or numbers in flash is really common.
> 
> And yet this is a platform with ACPI. I would expect the ACPI to
> identify the board, not a custom driver. The newest ACPI spec adds a
> lot of nice useful things like GPIO and SPI bindings. Talk to Al Stone
> about the progress his team has made on adding GPIO support to ACPICA.
> 
> This driver shouldn't be merged into mainline. Keep it as an
> out-of-tree patch until the proper solution is implemented. That
> shouldn't be too onerous since we now have available not one, but two
> mechanisms for describing exactly what you want to do; ACPI or FDT.

Yes, that is the current plan. I have isolated the necessary patches for
bug fixes (merged, serial support (merged) and networking support (under
review). I am exploring different options with ACPI for how to best
handle the GPIO (LEDs and Keys) and the Lure device descriptions.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

[PATCH 5/8] minnowboard-gpio: Export MinnowBoard expansion GPIO

[Darren Hart]

Request and export the user-configurable GPIO lines to sysfs. This provides a
label readable in /debugfs/gpio and a simple interface for experimenting with
GPIO on the MinnowBoard.

This is separate from the minnowboard driver to provide users with the
flexibility to write kernel drivers for their own devices using these GPIO
lines.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: Matthew Garrett <matthew.garrett@nebula.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/Kconfig            |  18 +++++
 drivers/platform/x86/Makefile           |   1 +
 drivers/platform/x86/minnowboard-gpio.c | 115 ++++++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+)
 create mode 100644 drivers/platform/x86/minnowboard-gpio.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 154dbf6..c8755cb 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -35,6 +35,24 @@ config MINNOWBOARD
 
 	  If you have a MinnowBoard, say Y or M here.
 
+if MINNOWBOARD
+config MINNOWBOARD_GPIO
+	tristate "MinnowBoard Expansion GPIO"
+	depends on MINNOWBOARD
+	default n
+	---help---
+	  Export the EG20T (gpio-pch) lines on the expansion connector to sysfs
+	  for easy manipulation from userspace. These will be named
+	  "minnow_gpio_pch[0-7]". If LVDS is not in use, export the E6XX
+	  (gpio-sch) lines on the expansion connector to sysfs, these will be
+	  named "minnow_gpio_aux[0-4]".
+
+	  If you have a MinnowBoard, and want to experiment with the GPIO,
+	  say Y or M here.
+
+endif # MINNOWBOARD
+
+
 config ACER_WMI
 	tristate "Acer WMI Laptop Extras"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 45ede1c..4dac9f0 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -3,6 +3,7 @@
 # x86 Platform-Specific Drivers
 #
 obj-$(CONFIG_MINNOWBOARD)	+= minnowboard.o
+obj-$(CONFIG_MINNOWBOARD_GPIO)	+= minnowboard-gpio.o
 obj-$(CONFIG_ASUS_LAPTOP)	+= asus-laptop.o
 obj-$(CONFIG_ASUS_WMI)		+= asus-wmi.o
 obj-$(CONFIG_ASUS_NB_WMI)	+= asus-nb-wmi.o
diff --git a/drivers/platform/x86/minnowboard-gpio.c b/drivers/platform/x86/minnowboard-gpio.c
new file mode 100644
index 0000000..c33c438
--- /dev/null
+++ b/drivers/platform/x86/minnowboard-gpio.c
@@ -0,0 +1,115 @@
+/*
+ * MinnowBoard Linux platform driver
+ * Copyright (c) 2013, Intel Corporation.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Author: Darren Hart <dvhart@linux.intel.com>
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/gpio.h>
+#include <linux/gpio_keys.h>
+#include <linux/input.h>
+#include <linux/minnowboard.h>
+#include "minnowboard-gpio.h"
+
+static struct gpio expansion_gpios[] = {
+	{ GPIO_PCH0, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch0" },
+	{ GPIO_PCH1, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch1" },
+	{ GPIO_PCH2, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch2" },
+	{ GPIO_PCH3, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch3" },
+	{ GPIO_PCH4, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch4" },
+	{ GPIO_PCH5, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch5" },
+	{ GPIO_PCH6, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch6" },
+	{ GPIO_PCH7, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_pch7" },
+};
+
+static struct gpio expansion_aux_gpios[] = {
+	{ GPIO_AUX0, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_aux0" },
+	{ GPIO_AUX1, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_aux1" },
+	{ GPIO_AUX2, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_aux2" },
+	{ GPIO_AUX3, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_aux3" },
+	{ GPIO_AUX4, GPIOF_DIR_IN | GPIOF_EXPORT | GPIOF_EXPORT_CHANGEABLE,
+	  "minnow_gpio_aux4" },
+};
+
+static int __init minnow_gpio_module_init(void)
+{
+	int err;
+
+	err = -ENODEV;
+	if (!minnow_detect())
+		goto out;
+
+#ifdef MODULE
+#ifdef CONFIG_MINNOWBOARD_MODULE
+	if (request_module("minnowboard"))
+		goto out;
+#endif
+#endif
+
+	/* Auxillary Expansion GPIOs */
+	if (!minnow_lvds_detect()) {
+		pr_debug("LVDS_DETECT not asserted, configuring Aux GPIO lines\n");
+		err = gpio_request_array(expansion_aux_gpios,
+					 ARRAY_SIZE(expansion_aux_gpios));
+		if (err) {
+			pr_err("Failed to request expansion aux GPIO lines\n");
+			goto out;
+		}
+	} else {
+		pr_debug("LVDS_DETECT asserted, ignoring aux GPIO lines\n");
+	}
+
+	/* Expansion GPIOs */
+	err = gpio_request_array(expansion_gpios, ARRAY_SIZE(expansion_gpios));
+	if (err) {
+		pr_err("Failed to request expansion GPIO lines\n");
+		if (minnow_lvds_detect())
+			gpio_free_array(expansion_aux_gpios,
+					ARRAY_SIZE(expansion_aux_gpios));
+		goto out;
+	}
+
+ out:
+	return err;
+}
+
+static void __exit minnow_gpio_module_exit(void)
+{
+	if (minnow_lvds_detect())
+		gpio_free_array(expansion_aux_gpios,
+				ARRAY_SIZE(expansion_aux_gpios));
+	gpio_free_array(expansion_gpios, ARRAY_SIZE(expansion_gpios));
+}
+
+module_init(minnow_gpio_module_init);
+module_exit(minnow_gpio_module_exit);
+
+MODULE_LICENSE("GPL");
-- 
1.8.1.2

Re: [PATCH 5/8] minnowboard-gpio: Export MinnowBoard expansion GPIO

[Andy Shevchenko]

On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> Request and export the user-configurable GPIO lines to sysfs. This provides a
> label readable in /debugfs/gpio and a simple interface for experimenting with
> GPIO on the MinnowBoard.
> 
> This is separate from the minnowboard driver to provide users with the
> flexibility to write kernel drivers for their own devices using these GPIO
> lines.

Few comments below.

> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -35,6 +35,24 @@ config MINNOWBOARD
>  
>  	  If you have a MinnowBoard, say Y or M here.
>  
> +if MINNOWBOARD
> +config MINNOWBOARD_GPIO
> +	tristate "MinnowBoard Expansion GPIO"
> +	depends on MINNOWBOARD
> +	default n

Like you already had been told you don't need to have default n.

> --- /dev/null
> +++ b/drivers/platform/x86/minnowboard-gpio.c

> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio_keys.h>
> +#include <linux/input.h>
> +#include <linux/minnowboard.h>

+ empty line here?

> +#include "minnowboard-gpio.h"

> +static int __init minnow_gpio_module_init(void)
> +{
> +	int err;
> +
> +	err = -ENODEV;
> +	if (!minnow_detect())
> +		goto out;
> +
> +#ifdef MODULE
> +#ifdef CONFIG_MINNOWBOARD_MODULE

And less ifdefs with IS_MODULE().

> +	if (request_module("minnowboard"))
> +		goto out;
> +#endif
> +#endif
> +
> +	/* Auxillary Expansion GPIOs */
> +	if (!minnow_lvds_detect()) {
> +		pr_debug("LVDS_DETECT not asserted, configuring Aux GPIO lines\n");
> +		err = gpio_request_array(expansion_aux_gpios,
> +					 ARRAY_SIZE(expansion_aux_gpios));
> +		if (err) {
> +			pr_err("Failed to request expansion aux GPIO lines\n");
> +			goto out;
> +		}
> +	} else {
> +		pr_debug("LVDS_DETECT asserted, ignoring aux GPIO lines\n");
> +	}
> +
> +	/* Expansion GPIOs */
> +	err = gpio_request_array(expansion_gpios, ARRAY_SIZE(expansion_gpios));
> +	if (err) {
> +		pr_err("Failed to request expansion GPIO lines\n");
> +		if (minnow_lvds_detect())
> +			gpio_free_array(expansion_aux_gpios,
> +					ARRAY_SIZE(expansion_aux_gpios));
> +		goto out;
> +	}
> +
> + out:
> +	return err;

Are you planning to add something else to 'out' path?
Otherwise I think it will look better if you do return instead of
[useless] gotos.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 5/8] minnowboard-gpio: Export MinnowBoard expansion GPIO

[Darren Hart]

On Wed, 2013-06-26 at 10:55 +0300, Andy Shevchenko wrote:
> On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> > Request and export the user-configurable GPIO lines to sysfs. This provides a
> > label readable in /debugfs/gpio and a simple interface for experimenting with
> > GPIO on the MinnowBoard.
> > 
> > This is separate from the minnowboard driver to provide users with the
> > flexibility to write kernel drivers for their own devices using these GPIO
> > lines.
> 
> Few comments below.
> 
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -35,6 +35,24 @@ config MINNOWBOARD
> >  
> >  	  If you have a MinnowBoard, say Y or M here.
> >  
> > +if MINNOWBOARD
> > +config MINNOWBOARD_GPIO
> > +	tristate "MinnowBoard Expansion GPIO"
> > +	depends on MINNOWBOARD
> > +	default n
> 
> Like you already had been told you don't need to have default n.
> 
> > --- /dev/null
> > +++ b/drivers/platform/x86/minnowboard-gpio.c
> 
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio_keys.h>
> > +#include <linux/input.h>
> > +#include <linux/minnowboard.h>
> 
> + empty line here?
> 
> > +#include "minnowboard-gpio.h"
> 
> > +static int __init minnow_gpio_module_init(void)
> > +{
> > +	int err;
> > +
> > +	err = -ENODEV;
> > +	if (!minnow_detect())
> > +		goto out;
> > +
> > +#ifdef MODULE
> > +#ifdef CONFIG_MINNOWBOARD_MODULE
> 
> And less ifdefs with IS_MODULE().
> 

All good to here and consistent with Olof's comments. Thanks, I will
include in V2.

> > +	if (request_module("minnowboard"))
> > +		goto out;
> > +#endif
> > +#endif
> > +
> > +	/* Auxillary Expansion GPIOs */
> > +	if (!minnow_lvds_detect()) {
> > +		pr_debug("LVDS_DETECT not asserted, configuring Aux GPIO lines\n");
> > +		err = gpio_request_array(expansion_aux_gpios,
> > +					 ARRAY_SIZE(expansion_aux_gpios));
> > +		if (err) {
> > +			pr_err("Failed to request expansion aux GPIO lines\n");
> > +			goto out;
> > +		}
> > +	} else {
> > +		pr_debug("LVDS_DETECT asserted, ignoring aux GPIO lines\n");
> > +	}
> > +
> > +	/* Expansion GPIOs */
> > +	err = gpio_request_array(expansion_gpios, ARRAY_SIZE(expansion_gpios));
> > +	if (err) {
> > +		pr_err("Failed to request expansion GPIO lines\n");
> > +		if (minnow_lvds_detect())
> > +			gpio_free_array(expansion_aux_gpios,
> > +					ARRAY_SIZE(expansion_aux_gpios));
> > +		goto out;
> > +	}
> > +
> > + out:
> > +	return err;
> 
> Are you planning to add something else to 'out' path?
> Otherwise I think it will look better if you do return instead of
> [useless] gotos.

I suppose this is a matter of preference. I am allergic to multiple
return points. However, your argument is consistent with CodingStyle
Chapter 7 in that it states "and some common work such as cleanup has to
be done." If that "and" is a required sort of &&, then I should change
it. Do others have a strong opinion here?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 5/8] minnowboard-gpio: Export MinnowBoard expansion GPIO

[Andy Shevchenko]

On Wed, 2013-06-26 at 09:21 -0700, Darren Hart wrote: 
> On Wed, 2013-06-26 at 10:55 +0300, Andy Shevchenko wrote:

> > > + out:
> > > +	return err;
> > 
> > Are you planning to add something else to 'out' path?
> > Otherwise I think it will look better if you do return instead of
> > [useless] gotos.
> 
> I suppose this is a matter of preference. I am allergic to multiple
> return points. However, your argument is consistent with CodingStyle
> Chapter 7 in that it states "and some common work such as cleanup has to
> be done." If that "and" is a required sort of &&, then I should change
> it. Do others have a strong opinion here?

There was recently similar discussion. Author finally agreed to change:
http://www.spinics.net/lists/arm-kernel/msg252108.html

"I did say in the changelog I opted for goto over return. But since
everybody keeps preferring returns..."

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 5/8] minnowboard-gpio: Export MinnowBoard expansion GPIO

[Darren Hart]

On Thu, 2013-06-27 at 11:18 +0300, Andy Shevchenko wrote:
> On Wed, 2013-06-26 at 09:21 -0700, Darren Hart wrote: 
> > On Wed, 2013-06-26 at 10:55 +0300, Andy Shevchenko wrote:
> 
> > > > + out:
> > > > +	return err;
> > > 
> > > Are you planning to add something else to 'out' path?
> > > Otherwise I think it will look better if you do return instead of
> > > [useless] gotos.
> > 
> > I suppose this is a matter of preference. I am allergic to multiple
> > return points. However, your argument is consistent with CodingStyle
> > Chapter 7 in that it states "and some common work such as cleanup has to
> > be done." If that "and" is a required sort of &&, then I should change
> > it. Do others have a strong opinion here?
> 
> There was recently similar discussion. Author finally agreed to change:
> http://www.spinics.net/lists/arm-kernel/msg252108.html
> 
> "I did say in the changelog I opted for goto over return. But since
> everybody keeps preferring returns..."

OK, I'll fix that up in V2 and use that model in the future. Thank you
for being persistent.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

[PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Darren Hart]

Configure the four buttons tied to the E6XX GPIO lines on the
MinnowBoard as keys using the gpio-keys-polled platform driver. From
left to right, bind them to LEFT, DOWN, UP, RIGHT, similar to the VI
directional keys.

This is separate from the minnowboard driver to provide users with the
flexibility to write kernel drivers for their own devices using these GPIO
lines.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: Matthew Garrett <matthew.garrett@nebula.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: platform-driver-x86@vger.kernel.org
---
 drivers/platform/x86/Kconfig            |  14 +++++
 drivers/platform/x86/Makefile           |   1 +
 drivers/platform/x86/minnowboard-keys.c | 108 ++++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+)
 create mode 100644 drivers/platform/x86/minnowboard-keys.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index c8755cb..b9ff98c 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -50,6 +50,20 @@ config MINNOWBOARD_GPIO
 	  If you have a MinnowBoard, and want to experiment with the GPIO,
 	  say Y or M here.
 
+config MINNOWBOARD_KEYS
+	tristate "MinnowBoard GPIO Keys"
+	depends on MINNOWBOARD
+	depends on KEYBOARD_GPIO_POLLED
+	default n
+	---help---
+	  Configure the four buttons tied to the E6XX GPIO lines on the
+	  MinnowBoard as keys using the gpio-keys-polled platform driver. From
+	  left to right, bind them to LEFT, DOWN, UP, RIGHT, similar to the VI
+	  directional keys.
+
+	  If you have a MinnowBoard and want to use the buttons as arrow keys,
+	  say Y or M here.
+
 endif # MINNOWBOARD
 
 
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 4dac9f0..2f55903 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -4,6 +4,7 @@
 #
 obj-$(CONFIG_MINNOWBOARD)	+= minnowboard.o
 obj-$(CONFIG_MINNOWBOARD_GPIO)	+= minnowboard-gpio.o
+obj-$(CONFIG_MINNOWBOARD_KEYS)	+= minnowboard-keys.o
 obj-$(CONFIG_ASUS_LAPTOP)	+= asus-laptop.o
 obj-$(CONFIG_ASUS_WMI)		+= asus-wmi.o
 obj-$(CONFIG_ASUS_NB_WMI)	+= asus-nb-wmi.o
diff --git a/drivers/platform/x86/minnowboard-keys.c b/drivers/platform/x86/minnowboard-keys.c
new file mode 100644
index 0000000..de96df1
--- /dev/null
+++ b/drivers/platform/x86/minnowboard-keys.c
@@ -0,0 +1,108 @@
+/*
+ * MinnowBoard Linux platform driver
+ * Copyright (c) 2013, Intel Corporation.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Author: Darren Hart <dvhart@linux.intel.com>
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/gpio.h>
+#include <linux/gpio_keys.h>
+#include <linux/input.h>
+#include <linux/minnowboard.h>
+#include "minnowboard-gpio.h"
+
+/* VI-style direction keys seem like as good as anything */
+#define GPIO_BTN0_KEY KEY_LEFT
+#define GPIO_BTN1_KEY KEY_DOWN
+#define GPIO_BTN2_KEY KEY_UP
+#define GPIO_BTN3_KEY KEY_RIGHT
+
+/* Timing in milliseconds */
+#define GPIO_DEBOUNCE 1
+#define BUTTON_POLL_INTERVAL 300
+
+/* gpio-keys platform device structures */
+static struct gpio_keys_button minnow_buttons[] = {
+	{ .code = GPIO_BTN0_KEY, .gpio = GPIO_BTN0, .active_low = 1,
+	  .desc = "minnow_btn0", .type = EV_KEY, .wakeup = 0,
+	  .debounce_interval = GPIO_DEBOUNCE, .can_disable = true },
+	{ .code = GPIO_BTN1_KEY, .gpio = GPIO_BTN1, .active_low = 1,
+	  .desc = "minnow_btn1", .type = EV_KEY, .wakeup = 0,
+	  .debounce_interval = GPIO_DEBOUNCE, .can_disable = true },
+	{ .code = GPIO_BTN2_KEY, .gpio = GPIO_BTN2, .active_low = 1,
+	  .desc = "minnow_btn2", .type = EV_KEY, .wakeup = 0,
+	  .debounce_interval = GPIO_DEBOUNCE, .can_disable = true },
+	{ .code = GPIO_BTN3_KEY, .gpio = GPIO_BTN3, .active_low = 1,
+	  .desc = "minnow_btn3", .type = EV_KEY, .wakeup = 0,
+	  .debounce_interval = GPIO_DEBOUNCE, .can_disable = true },
+};
+
+static const struct gpio_keys_platform_data minnow_buttons_platform_data = {
+	.buttons = minnow_buttons,
+	.nbuttons = ARRAY_SIZE(minnow_buttons),
+	.poll_interval = BUTTON_POLL_INTERVAL,
+	.rep = 1,
+	.enable = NULL,
+	.disable = NULL,
+	.name = "minnow_buttons",
+};
+
+static struct platform_device minnow_gpio_buttons = {
+	.name = "gpio-keys-polled",
+	.id = -1,
+	.dev = {
+		.platform_data = (void *) &minnow_buttons_platform_data,
+	},
+};
+
+static int __init minnow_keys_module_init(void)
+{
+	int err;
+
+	err = -ENODEV;
+	if (!minnow_detect())
+		goto out;
+
+#ifdef MODULE
+#ifdef CONFIG_MINNOWBOARD_MODULE
+	if (request_module("minnowboard"))
+		goto out;
+#endif
+#endif
+
+	/* Export GPIO buttons to sysfs */
+	err = platform_device_register(&minnow_gpio_buttons);
+	if (err) {
+		pr_err("Failed to register gpio-keys-polled platform device\n");
+		goto out;
+	}
+
+ out:
+	return err;
+}
+
+static void __exit minnow_keys_module_exit(void)
+{
+	platform_device_unregister(&minnow_gpio_buttons);
+}
+
+module_init(minnow_keys_module_init);
+module_exit(minnow_keys_module_exit);
+
+MODULE_LICENSE("GPL");
-- 
1.8.1.2

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Andy Shevchenko]

On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> Configure the four buttons tied to the E6XX GPIO lines on the
> MinnowBoard as keys using the gpio-keys-polled platform driver. From
> left to right, bind them to LEFT, DOWN, UP, RIGHT, similar to the VI
> directional keys.
> 
> This is separate from the minnowboard driver to provide users with the
> flexibility to write kernel drivers for their own devices using these GPIO
> lines.

I'm repeating my comment I did early to you.

I think this driver is a wrong approach, since you just use
gpio_keys_polling with custom platform data. So, it should go to the
platform code / board file under arch/x86/platform/minnow/...


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Darren Hart]

On Wed, 2013-06-26 at 11:46 +0300, Andy Shevchenko wrote:
> On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> > Configure the four buttons tied to the E6XX GPIO lines on the
> > MinnowBoard as keys using the gpio-keys-polled platform driver. From
> > left to right, bind them to LEFT, DOWN, UP, RIGHT, similar to the VI
> > directional keys.
> > 
> > This is separate from the minnowboard driver to provide users with the
> > flexibility to write kernel drivers for their own devices using these GPIO
> > lines.
> 
> I'm repeating my comment I did early to you.
> 
> I think this driver is a wrong approach, since you just use
> gpio_keys_polling with custom platform data. So, it should go to the
> platform code / board file under arch/x86/platform/minnow/...


Olof mentinoed something similar. The reason this is separate is that I
can easily see someone wanting to use these buttons in a different way
when integrating the MinnowBoard into some kind of product. The
minnowboard.c driver sets up the fixed functionality GPIO lines, such as
the LEDs (which can be easily reconfigured via triggers) while this
driver serves as an example of how the GPIO buttons could be used as
keys, but if included in the minnowboard driver, users couldn't get the
fixed functionality without also tying up these GPIO lines.

I could remedy that with driver command-line options, but I know I've
heard Greg KH discourage their use in the past.

Does that address your concern or do you still feel they should be
merged?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Greg Kroah-Hartman]

On Wed, Jun 26, 2013 at 09:28:07AM -0700, Darren Hart wrote:
> On Wed, 2013-06-26 at 11:46 +0300, Andy Shevchenko wrote:
> > On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> > > Configure the four buttons tied to the E6XX GPIO lines on the
> > > MinnowBoard as keys using the gpio-keys-polled platform driver. From
> > > left to right, bind them to LEFT, DOWN, UP, RIGHT, similar to the VI
> > > directional keys.
> > > 
> > > This is separate from the minnowboard driver to provide users with the
> > > flexibility to write kernel drivers for their own devices using these GPIO
> > > lines.
> > 
> > I'm repeating my comment I did early to you.
> > 
> > I think this driver is a wrong approach, since you just use
> > gpio_keys_polling with custom platform data. So, it should go to the
> > platform code / board file under arch/x86/platform/minnow/...
> 
> 
> Olof mentinoed something similar. The reason this is separate is that I
> can easily see someone wanting to use these buttons in a different way
> when integrating the MinnowBoard into some kind of product. The
> minnowboard.c driver sets up the fixed functionality GPIO lines, such as
> the LEDs (which can be easily reconfigured via triggers) while this
> driver serves as an example of how the GPIO buttons could be used as
> keys, but if included in the minnowboard driver, users couldn't get the
> fixed functionality without also tying up these GPIO lines.
> 
> I could remedy that with driver command-line options, but I know I've
> heard Greg KH discourage their use in the past.

Ick, yes, never do that type of thing as a command-line option, that's
what device-tree is for :)

thanks,

greg k-h

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Darren Hart]

On Wed, 2013-06-26 at 10:16 -0700, Greg Kroah-Hartman wrote:
> On Wed, Jun 26, 2013 at 09:28:07AM -0700, Darren Hart wrote:
> > On Wed, 2013-06-26 at 11:46 +0300, Andy Shevchenko wrote:
> > > On Tue, 2013-06-25 at 18:53 -0700, Darren Hart wrote: 
> > > > Configure the four buttons tied to the E6XX GPIO lines on the
> > > > MinnowBoard as keys using the gpio-keys-polled platform driver. From
> > > > left to right, bind them to LEFT, DOWN, UP, RIGHT, similar to the VI
> > > > directional keys.
> > > > 
> > > > This is separate from the minnowboard driver to provide users with the
> > > > flexibility to write kernel drivers for their own devices using these GPIO
> > > > lines.
> > > 
> > > I'm repeating my comment I did early to you.
> > > 
> > > I think this driver is a wrong approach, since you just use
> > > gpio_keys_polling with custom platform data. So, it should go to the
> > > platform code / board file under arch/x86/platform/minnow/...
> > 
> > 
> > Olof mentinoed something similar. The reason this is separate is that I
> > can easily see someone wanting to use these buttons in a different way
> > when integrating the MinnowBoard into some kind of product. The
> > minnowboard.c driver sets up the fixed functionality GPIO lines, such as
> > the LEDs (which can be easily reconfigured via triggers) while this
> > driver serves as an example of how the GPIO buttons could be used as
> > keys, but if included in the minnowboard driver, users couldn't get the
> > fixed functionality without also tying up these GPIO lines.
> > 
> > I could remedy that with driver command-line options, but I know I've
> > heard Greg KH discourage their use in the past.
> 
> Ick, yes, never do that type of thing as a command-line option, that's
> what device-tree is for :)

I suppose when I convert to ACPI drivers I could merge them and have the
ACPI table include some data that enabled or disabled things like the
minnowboard-keys.... but that seems like more work for the user than it
should be to disable the example keys driver.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Linus Walleij]

On Wed, Jun 26, 2013 at 7:23 PM, Darren Hart <dvhart@linux.intel.com> wrote:
> On Wed, 2013-06-26 at 10:16 -0700, Greg Kroah-Hartman wrote:
>> On Wed, Jun 26, 2013 at 09:28:07AM -0700, Darren Hart wrote:

>> > The reason this is separate is that I
>> > can easily see someone wanting to use these buttons in a different way
>> > when integrating the MinnowBoard into some kind of product. The
>> > minnowboard.c driver sets up the fixed functionality GPIO lines, such as
>> > the LEDs (which can be easily reconfigured via triggers) while this
>> > driver serves as an example of how the GPIO buttons could be used as
>> > keys, but if included in the minnowboard driver, users couldn't get the
>> > fixed functionality without also tying up these GPIO lines.
>> >
>> > I could remedy that with driver command-line options, but I know I've
>> > heard Greg KH discourage their use in the past.
>>
>> Ick, yes, never do that type of thing as a command-line option, that's
>> what device-tree is for :)
>
> I suppose when I convert to ACPI drivers I could merge them and have the
> ACPI table include some data that enabled or disabled things like the
> minnowboard-keys.... but that seems like more work for the user than it
> should be to disable the example keys driver.

So now you make it sound that devicetree is somehow really superior
to ACPI because it can actually be used to do some board-specific
configs, and ACPI tables are too hard to use?

Device tree was what we came up with for ARM go get *away* from
stashing custom config into the kernel, such as boardfiles and even
more horrible things like a command-line switch for every key.

Is x86 now not really presenting anything better? I would have a second
look at augmented ACPI tables, if that is what all of x86 is going to use.

FYI here is how I set up a heartbeat LED and some GPIO key in a
device tree:

        /* The user LED on the board is set up to be used for heartbeat */
        leds {
                compatible = "gpio-leds";
                user-led {
                        label = "user_led";
                        gpios = <&gpio0 2 0x1>;
                        default-state = "off";
                        linux,default-trigger = "heartbeat";
                };
        };

        /* User key mapped in as "escape" */
        gpio-keys {
                compatible = "gpio-keys";
                user-button {
                        label = "user_button";
                        gpios = <&gpio0 3 0x1>;
                        linux,code = <1>; /* KEY_ESC */
                        gpio-key,wakeup;
                };
        };

Yours,
Linus Walleij

Re: [PATCH 6/8] minnowboard-keys: Bind MinnowBoard buttons to arrow keys

[Darren Hart]

On Wed, 2013-06-26 at 21:57 +0200, Linus Walleij wrote:
> On Wed, Jun 26, 2013 at 7:23 PM, Darren Hart <dvhart@linux.intel.com> wrote:
> > On Wed, 2013-06-26 at 10:16 -0700, Greg Kroah-Hartman wrote:
> >> On Wed, Jun 26, 2013 at 09:28:07AM -0700, Darren Hart wrote:
> 
> >> > The reason this is separate is that I
> >> > can easily see someone wanting to use these buttons in a different way
> >> > when integrating the MinnowBoard into some kind of product. The
> >> > minnowboard.c driver sets up the fixed functionality GPIO lines, such as
> >> > the LEDs (which can be easily reconfigured via triggers) while this
> >> > driver serves as an example of how the GPIO buttons could be used as
> >> > keys, but if included in the minnowboard driver, users couldn't get the
> >> > fixed functionality without also tying up these GPIO lines.
> >> >
> >> > I could remedy that with driver command-line options, but I know I've
> >> > heard Greg KH discourage their use in the past.
> >>
> >> Ick, yes, never do that type of thing as a command-line option, that's
> >> what device-tree is for :)
> >
> > I suppose when I convert to ACPI drivers I could merge them and have the
> > ACPI table include some data that enabled or disabled things like the
> > minnowboard-keys.... but that seems like more work for the user than it
> > should be to disable the example keys driver.
> 
> So now you make it sound that devicetree is somehow really superior
> to ACPI because it can actually be used to do some board-specific
> configs, and ACPI tables are too hard to use?
> 
> Device tree was what we came up with for ARM go get *away* from
> stashing custom config into the kernel, such as boardfiles and even
> more horrible things like a command-line switch for every key.
> 
> Is x86 now not really presenting anything better? I would have a second
> look at augmented ACPI tables, if that is what all of x86 is going to use.
> 
> FYI here is how I set up a heartbeat LED and some GPIO key in a
> device tree:
> 
>         /* The user LED on the board is set up to be used for heartbeat */
>         leds {
>                 compatible = "gpio-leds";
>                 user-led {
>                         label = "user_led";
>                         gpios = <&gpio0 2 0x1>;
>                         default-state = "off";
>                         linux,default-trigger = "heartbeat";
>                 };
>         };

This is something we should be able to do with ACPI 5 and SSDTs.

>         /* User key mapped in as "escape" */
>         gpio-keys {
>                 compatible = "gpio-keys";
>                 user-button {
>                         label = "user_button";
>                         gpios = <&gpio0 3 0x1>;
>                         linux,code = <1>; /* KEY_ESC */
>                         gpio-key,wakeup;
>                 };
>         };

And this too actually. I was thinking along the lines of the tables in
the firmware, but using SSDTs passed in (optionally) at boot, the user
can easily decide not to use the buttons with gpio-keys simply by not
passing in that SSDT. This had been the direction we were going with the
Lures (daughter cards), but we can also apply this to the board itself
as a way to provide a proper example of how to do this.

So the answer seems to be to put any fixed functionality into the board
firmware ACPI tables and leave the optional experimenter config to SSDTs
which the user can choose to use or not.

The rest of what minnowboard.c does (LVDS/GPIO setup) can (and should)
be done in the firmware directly, also eliminating the need for the 3/8
(sch_gpio_resume_set_enable()).

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel