[PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Diego Elio Pettenò]

The driver is currently written for IT8728, but the code in it87_wdt
suggest it would work the same way on many other versions.

This is just a first step to verify the working status of the code,
and might require moving to a MFD approach to share with hwmon/it87
and it87_wdt, as they all share the same access to the Super I/O chip.
That would also allow to set a few more opitons so that you could for
instance change the function selection of a few pins from their
default to GPIO pins.

It does not yet support the polarity-inversion register that is
allowed by the chipset, which might require sysfs attributes per-gpio.

CC: Grant Likely <grant.likely@secretlab.ca>
CC: Linus Walleij <linus.walleij@stericsson.com>
Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.eu>
---
 MAINTAINERS              |    5 +
 drivers/gpio/Kconfig     |   11 ++
 drivers/gpio/Makefile    |    1 +
 drivers/gpio/gpio-it87.c |  397 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 414 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/gpio-it87.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 553ac10..a196f14 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3692,6 +3692,11 @@ S:	Maintained
 F:	Documentation/hwmon/it87
 F:	drivers/hwmon/it87.c
 
+IT87xx GPIO DRIVER
+M:	Diego Elio Pettenò <flameeyes@flameeyes.eu>
+S:	Maintained
+F:	drivers/gpio/gpio-it87.c
+
 IVTV VIDEO4LINUX DRIVER
 M:	Andy Walls <awalls@md.metrocast.net>
 L:	ivtv-devel@ivtvdriver.org (moderated for non-subscribers)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3359f1e..8dc26b7 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -91,6 +91,17 @@ config GPIO_IT8761E
 	help
 	  Say yes here to support GPIO functionality of IT8761E super I/O chip.
 
+config GPIO_IT87
+	tristate "IT87xx GPIO support"
+	depends on X86 # unconditional access to IO space.
+	help
+	  Say yes here to support GPIO functionality of IT87xx Super I/O chips.
+
+	  This driver currently supports ITE IT8728 Super I/O chips.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gpio_it87
+
 config GPIO_EP93XX
 	def_bool y
 	depends on ARCH_EP93XX
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 41fe67f..affe510 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_ARCH_DAVINCI)	+= gpio-davinci.o
 obj-$(CONFIG_GPIO_EP93XX)	+= gpio-ep93xx.o
 obj-$(CONFIG_GPIO_ICH)		+= gpio-ich.o
 obj-$(CONFIG_GPIO_IT8761E)	+= gpio-it8761e.o
+obj-$(CONFIG_GPIO_IT87)		+= gpio-it87.o
 obj-$(CONFIG_GPIO_JANZ_TTL)	+= gpio-janz-ttl.o
 obj-$(CONFIG_ARCH_KS8695)	+= gpio-ks8695.o
 obj-$(CONFIG_GPIO_LANGWELL)	+= gpio-langwell.o
diff --git a/drivers/gpio/gpio-it87.c b/drivers/gpio/gpio-it87.c
new file mode 100644
index 0000000..958c126
--- /dev/null
+++ b/drivers/gpio/gpio-it87.c
@@ -0,0 +1,397 @@
+/*
+ *  GPIO interface for IT87xx Super I/O chips
+ *
+ *  Author: Diego Elio Pettenò <flameeyes@flameeyes.eu>
+ *
+ *  Based on it87_wdt.c     by Oliver Schuster
+ *           gpio-it8761e.c by Denis Turischev
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License 2 as published
+ *  by the Free Software Foundation.
+ *
+ *  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.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; see the file COPYING.  If not, write to
+ *  the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/errno.h>
+#include <linux/ioport.h>
+
+#include <linux/gpio.h>
+
+#define GPIO_NAME "it87-gpio"
+#define PFX GPIO_NAME ": "
+
+/* Chip Id numbers */
+#define NO_DEV_ID	0xffff
+#define IT8728_ID	0x8728
+
+/* IO Ports */
+#define REG		0x2e
+#define VAL		0x2f
+
+/* Logical device Numbers LDN */
+#define GPIO		0x07
+
+/* Configuration Registers and Functions */
+#define LDNREG		0x07
+#define CHIPID		0x20
+#define CHIPREV		0x22
+
+/* GPIO Simple I/O Base Address registers */
+#define GPIO_BASE	0x62
+#define GPIO_IOSIZE	8
+
+/* GPIO polarity inverting registers base */
+#define GPIO_PS_BASE	0xb0 /* to 0xb4 */
+/* Simple I/O Enable registers base */
+#define GPIO_SE_BASE	0xc0 /* to 0xc4 */
+#define GPIO_SE_SIZE	5
+/* Output Enable registers base */
+#define GPIO_OE_BASE	0xc8 /* to 0xcf */
+
+/* Superio Chip */
+
+static inline int superio_enter(void)
+{
+	/*
+	 * Try to reserve REG and REG + 1 for exclusive access.
+	 */
+	if (!request_muxed_region(REG, 2, GPIO_NAME))
+		return -EBUSY;
+
+	outb(0x87, REG);
+	outb(0x01, REG);
+	outb(0x55, REG);
+	outb(0x55, REG);
+	return 0;
+}
+
+static inline void superio_exit(void)
+{
+	outb(0x02, REG);
+	outb(0x02, VAL);
+	release_region(REG, 2);
+}
+
+static inline void superio_select(int ldn)
+{
+	outb(LDNREG, REG);
+	outb(ldn, VAL);
+}
+
+static inline int superio_inb(int reg)
+{
+	outb(reg, REG);
+	return inb(VAL);
+}
+
+static inline void superio_outb(int val, int reg)
+{
+	outb(reg, REG);
+	outb(val, VAL);
+}
+
+static inline int superio_inw(int reg)
+{
+	int val;
+	outb(reg++, REG);
+	val = inb(VAL) << 8;
+	outb(reg, REG);
+	val |= inb(VAL);
+	return val;
+}
+
+static inline void superio_outw(int val, int reg)
+{
+	outb(reg++, REG);
+	outb(val >> 8, VAL);
+	outb(reg, REG);
+	outb(val, VAL);
+}
+
+static inline void superio_set_bit(int bit, int reg)
+{
+	u8 curr_val = superio_inb(reg);
+	u8 new_val = curr_val | 1 << bit;
+
+	if (curr_val != new_val)
+		superio_outb(new_val, reg);
+}
+
+static inline void superio_clear_bit(int bit, int reg)
+{
+	u8 curr_val = superio_inb(reg);
+	u8 new_val = curr_val & ~(1 << bit);
+
+	if (curr_val != new_val)
+		superio_outb(new_val, reg);
+}
+
+static u16 gpio_ba;
+
+static int it87_gpio_request(struct gpio_chip *gc, unsigned gpio_num)
+{
+	u8 bit, group;
+	int rc;
+
+	bit = gpio_num % 8;
+	group = (gpio_num / 8);
+
+	if ((rc = superio_enter()))
+		return rc;
+
+	/* enable Simple I/O on the GPIO pin, removing alternate
+	 * function; only the first five groups are programmable as
+	 * either Simple I/O or alternate function, the final free are
+	 * always set to Simple I/O.
+	 *
+	 * This might differ depending on chip type so it could have
+	 * to be made configurable.
+	 */
+	if (group >= GPIO_SE_SIZE)
+		superio_set_bit(bit, group + GPIO_SE_BASE);
+
+	/* clear output enable, setting the pin to input, as all the
+	 * newly-exported GPIO interfaces are set to input.
+	 */
+	superio_clear_bit(bit, group + GPIO_OE_BASE);
+
+	superio_exit();
+
+	return 0;
+}
+
+static int it87_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
+{
+	u16 reg;
+	u8 bit;
+
+	bit = gpio_num % 8;
+	reg = (gpio_num / 8) + gpio_ba;
+
+	return !!(inb(reg) & (1 << bit));
+}
+
+static int it87_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
+{
+	u8 bit, group;
+	int rc;
+
+	bit = gpio_num % 8;
+	group = (gpio_num / 8);
+
+	if ((rc = superio_enter()))
+		return rc;
+
+	/* clear the output enable bit */
+	superio_clear_bit(bit, group + GPIO_OE_BASE);
+
+	superio_exit();
+
+	return 0;
+}
+
+static void it87_gpio_set(struct gpio_chip *gc,
+			  unsigned gpio_num, int val)
+{
+	unsigned long flags;
+	u8 curr_vals, bit;
+	u16 reg;
+
+	bit = gpio_num % 8;
+	reg = (gpio_num / 8) + gpio_ba;
+
+	curr_vals = inb(reg);
+	if (val)
+		outb(curr_vals | (1 << bit) , reg);
+	else
+		outb(curr_vals & ~(1 << bit), reg);
+}
+
+static int it87_gpio_direction_out(struct gpio_chip *gc,
+				   unsigned gpio_num, int val)
+{
+	u8 bit, group;
+	int rc;
+
+	bit = gpio_num % 8;
+	group = (gpio_num / 8);
+
+	if ((rc = superio_enter()))
+		return rc;
+
+	/* set the output enable bit */
+	superio_set_bit(bit, group + GPIO_OE_BASE);
+
+	it87_gpio_set(gc, gpio_num, val);
+
+	superio_exit();
+
+	return 0;
+}
+
+/* ITE documentation refers to the GPIO registers as coordinates of
+ * group/bit; we alias them as otherwise it becomes hard to find a
+ * correlation between the chip's offsets and the names in the
+ * documentation.
+ */
+static const char *const it87_gpio_aliases[] = {
+	"it87_gp10",
+	"it87_gp11",
+	"it87_gp12",
+	"it87_gp13",
+	"it87_gp14",
+	"it87_gp15",
+	"it87_gp16",
+	"it87_gp17",
+	"it87_gp20",
+	"it87_gp21",
+	"it87_gp22",
+	"it87_gp23",
+	"it87_gp24",
+	"it87_gp25",
+	"it87_gp26",
+	"it87_gp27",
+	"it87_gp30",
+	"it87_gp31",
+	"it87_gp32",
+	"it87_gp33",
+	"it87_gp34",
+	"it87_gp35",
+	"it87_gp36",
+	"it87_gp37",
+	"it87_gp40",
+	"it87_gp41",
+	"it87_gp42",
+	"it87_gp43",
+	"it87_gp44",
+	"it87_gp45",
+	"it87_gp46",
+	"it87_gp47",
+	"it87_gp50",
+	"it87_gp51",
+	"it87_gp52",
+	"it87_gp53",
+	"it87_gp54",
+	"it87_gp55",
+	"it87_gp56",
+	"it87_gp57",
+	"it87_gp60",
+	"it87_gp61",
+	"it87_gp62",
+	"it87_gp63",
+	"it87_gp64",
+	"it87_gp65",
+	"it87_gp66",
+	"it87_gp67",
+	"it87_gp70",
+	"it87_gp71",
+	"it87_gp72",
+	"it87_gp73",
+	"it87_gp74",
+	"it87_gp75",
+	"it87_gp76",
+	"it87_gp77",
+	"it87_gp80",
+	"it87_gp81",
+	"it87_gp82",
+	"it87_gp83",
+	"it87_gp84",
+	"it87_gp85",
+	"it87_gp86",
+	"it87_gp87"
+};
+
+static struct gpio_chip it87_gpio_chip = {
+	.label			= GPIO_NAME,
+	.owner			= THIS_MODULE,
+	.request		= it87_gpio_request,
+	.get			= it87_gpio_get,
+	.direction_input	= it87_gpio_direction_in,
+	.set			= it87_gpio_set,
+	.direction_output	= it87_gpio_direction_out,
+	.names			= it87_gpio_aliases
+};
+
+static int __init it87_gpio_init(void)
+{
+	int rc = 0;
+	u8 chip_rev;
+	u16 chip_type;
+
+	if ((rc = superio_enter()))
+		return rc;
+
+	chip_type = superio_inw(CHIPID);
+	chip_rev  = superio_inb(CHIPREV) & 0x0f;
+	superio_exit();
+
+	switch(chip_type) {
+	case IT8728_ID:
+		break;
+	case NO_DEV_ID:
+		printk(KERN_ERR PFX "no device\n");
+		return -ENODEV;
+	default:
+		printk(KERN_ERR PFX
+		       "Unknown Chip found, Chip %04x Revision %x\n",
+		       chip_type, chip_rev);
+		return -ENODEV;
+	}
+
+	if ((rc = superio_enter()))
+		return rc;
+
+	superio_select(GPIO);
+
+	/* fetch GPIO base address */
+	gpio_ba = superio_inw(GPIO_BASE);
+
+	superio_exit();
+
+	if (!request_region(gpio_ba, GPIO_IOSIZE, GPIO_NAME))
+		return -EBUSY;
+
+	it87_gpio_chip.base = -1;
+	it87_gpio_chip.ngpio = 64;
+
+	if ((rc = gpiochip_add(&it87_gpio_chip)) < 0)
+		goto gpiochip_add_err;
+
+	return 0;
+
+gpiochip_add_err:
+	release_region(gpio_ba, GPIO_IOSIZE);
+	gpio_ba = 0;
+	return rc;
+}
+
+static void __exit it87_gpio_exit(void)
+{
+	if (gpio_ba) {
+		int ret = gpiochip_remove(&it87_gpio_chip);
+
+		WARN(ret, "%s(): gpiochip_remove() failed, ret=%d\n",
+				__func__, ret);
+
+		release_region(gpio_ba, GPIO_IOSIZE);
+		gpio_ba = 0;
+	}
+}
+module_init(it87_gpio_init);
+module_exit(it87_gpio_exit);
+
+MODULE_AUTHOR("Diego Elio Pettenò <flameeyes@flameeyes.eu>");
+MODULE_DESCRIPTION("GPIO interface for IT87xx Super I/O chips");
+MODULE_LICENSE("GPL");
-- 
1.7.8.5

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Guenter Roeck]

On Fri, Mar 16, 2012 at 08:48:13PM +0100, Diego Elio Pettenò wrote:
> The driver is currently written for IT8728, but the code in it87_wdt
> suggest it would work the same way on many other versions.
> 

The it87 hwmon driver accesses superio registers only during initialization,
so the risk for conflicts is quite low. Seems to me that is not the case here.

Maybe about time to convert the drivers to use the mfd infrastructure.
Hope that won't require datasheets for their recent chips, as close-lipped
as ITD is about those lately.

Guenter

> This is just a first step to verify the working status of the code,
> and might require moving to a MFD approach to share with hwmon/it87
> and it87_wdt, as they all share the same access to the Super I/O chip.
> That would also allow to set a few more opitons so that you could for
> instance change the function selection of a few pins from their
> default to GPIO pins.
> 
> It does not yet support the polarity-inversion register that is
> allowed by the chipset, which might require sysfs attributes per-gpio.
> 
> CC: Grant Likely <grant.likely@secretlab.ca>
> CC: Linus Walleij <linus.walleij@stericsson.com>
> Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.eu>
> ---
>  MAINTAINERS              |    5 +
>  drivers/gpio/Kconfig     |   11 ++
>  drivers/gpio/Makefile    |    1 +
>  drivers/gpio/gpio-it87.c |  397 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 414 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/gpio/gpio-it87.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 553ac10..a196f14 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3692,6 +3692,11 @@ S:	Maintained
>  F:	Documentation/hwmon/it87
>  F:	drivers/hwmon/it87.c
>  
> +IT87xx GPIO DRIVER
> +M:	Diego Elio Pettenò <flameeyes@flameeyes.eu>
> +S:	Maintained
> +F:	drivers/gpio/gpio-it87.c
> +
>  IVTV VIDEO4LINUX DRIVER
>  M:	Andy Walls <awalls@md.metrocast.net>
>  L:	ivtv-devel@ivtvdriver.org (moderated for non-subscribers)
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 3359f1e..8dc26b7 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -91,6 +91,17 @@ config GPIO_IT8761E
>  	help
>  	  Say yes here to support GPIO functionality of IT8761E super I/O chip.
>  
> +config GPIO_IT87
> +	tristate "IT87xx GPIO support"
> +	depends on X86 # unconditional access to IO space.
> +	help
> +	  Say yes here to support GPIO functionality of IT87xx Super I/O chips.
> +
> +	  This driver currently supports ITE IT8728 Super I/O chips.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called gpio_it87
> +
>  config GPIO_EP93XX
>  	def_bool y
>  	depends on ARCH_EP93XX
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 41fe67f..affe510 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_ARCH_DAVINCI)	+= gpio-davinci.o
>  obj-$(CONFIG_GPIO_EP93XX)	+= gpio-ep93xx.o
>  obj-$(CONFIG_GPIO_ICH)		+= gpio-ich.o
>  obj-$(CONFIG_GPIO_IT8761E)	+= gpio-it8761e.o
> +obj-$(CONFIG_GPIO_IT87)		+= gpio-it87.o
>  obj-$(CONFIG_GPIO_JANZ_TTL)	+= gpio-janz-ttl.o
>  obj-$(CONFIG_ARCH_KS8695)	+= gpio-ks8695.o
>  obj-$(CONFIG_GPIO_LANGWELL)	+= gpio-langwell.o
> diff --git a/drivers/gpio/gpio-it87.c b/drivers/gpio/gpio-it87.c
> new file mode 100644
> index 0000000..958c126
> --- /dev/null
> +++ b/drivers/gpio/gpio-it87.c
> @@ -0,0 +1,397 @@
> +/*
> + *  GPIO interface for IT87xx Super I/O chips
> + *
> + *  Author: Diego Elio Pettenò <flameeyes@flameeyes.eu>
> + *
> + *  Based on it87_wdt.c     by Oliver Schuster
> + *           gpio-it8761e.c by Denis Turischev
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License 2 as published
> + *  by the Free Software Foundation.
> + *
> + *  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.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; see the file COPYING.  If not, write to
> + *  the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/errno.h>
> +#include <linux/ioport.h>
> +
> +#include <linux/gpio.h>
> +
> +#define GPIO_NAME "it87-gpio"
> +#define PFX GPIO_NAME ": "
> +
> +/* Chip Id numbers */
> +#define NO_DEV_ID	0xffff
> +#define IT8728_ID	0x8728
> +
> +/* IO Ports */
> +#define REG		0x2e
> +#define VAL		0x2f
> +
> +/* Logical device Numbers LDN */
> +#define GPIO		0x07
> +
> +/* Configuration Registers and Functions */
> +#define LDNREG		0x07
> +#define CHIPID		0x20
> +#define CHIPREV		0x22
> +
> +/* GPIO Simple I/O Base Address registers */
> +#define GPIO_BASE	0x62
> +#define GPIO_IOSIZE	8
> +
> +/* GPIO polarity inverting registers base */
> +#define GPIO_PS_BASE	0xb0 /* to 0xb4 */
> +/* Simple I/O Enable registers base */
> +#define GPIO_SE_BASE	0xc0 /* to 0xc4 */
> +#define GPIO_SE_SIZE	5
> +/* Output Enable registers base */
> +#define GPIO_OE_BASE	0xc8 /* to 0xcf */
> +
> +/* Superio Chip */
> +
> +static inline int superio_enter(void)
> +{
> +	/*
> +	 * Try to reserve REG and REG + 1 for exclusive access.
> +	 */
> +	if (!request_muxed_region(REG, 2, GPIO_NAME))
> +		return -EBUSY;
> +
> +	outb(0x87, REG);
> +	outb(0x01, REG);
> +	outb(0x55, REG);
> +	outb(0x55, REG);
> +	return 0;
> +}
> +
> +static inline void superio_exit(void)
> +{
> +	outb(0x02, REG);
> +	outb(0x02, VAL);
> +	release_region(REG, 2);
> +}
> +
> +static inline void superio_select(int ldn)
> +{
> +	outb(LDNREG, REG);
> +	outb(ldn, VAL);
> +}
> +
> +static inline int superio_inb(int reg)
> +{
> +	outb(reg, REG);
> +	return inb(VAL);
> +}
> +
> +static inline void superio_outb(int val, int reg)
> +{
> +	outb(reg, REG);
> +	outb(val, VAL);
> +}
> +
> +static inline int superio_inw(int reg)
> +{
> +	int val;
> +	outb(reg++, REG);
> +	val = inb(VAL) << 8;
> +	outb(reg, REG);
> +	val |= inb(VAL);
> +	return val;
> +}
> +
> +static inline void superio_outw(int val, int reg)
> +{
> +	outb(reg++, REG);
> +	outb(val >> 8, VAL);
> +	outb(reg, REG);
> +	outb(val, VAL);
> +}
> +
> +static inline void superio_set_bit(int bit, int reg)
> +{
> +	u8 curr_val = superio_inb(reg);
> +	u8 new_val = curr_val | 1 << bit;
> +
> +	if (curr_val != new_val)
> +		superio_outb(new_val, reg);
> +}
> +
> +static inline void superio_clear_bit(int bit, int reg)
> +{
> +	u8 curr_val = superio_inb(reg);
> +	u8 new_val = curr_val & ~(1 << bit);
> +
> +	if (curr_val != new_val)
> +		superio_outb(new_val, reg);
> +}
> +
> +static u16 gpio_ba;
> +
> +static int it87_gpio_request(struct gpio_chip *gc, unsigned gpio_num)
> +{
> +	u8 bit, group;
> +	int rc;
> +
> +	bit = gpio_num % 8;
> +	group = (gpio_num / 8);
> +
> +	if ((rc = superio_enter()))
> +		return rc;
> +
> +	/* enable Simple I/O on the GPIO pin, removing alternate
> +	 * function; only the first five groups are programmable as
> +	 * either Simple I/O or alternate function, the final free are
> +	 * always set to Simple I/O.
> +	 *
> +	 * This might differ depending on chip type so it could have
> +	 * to be made configurable.
> +	 */
> +	if (group >= GPIO_SE_SIZE)
> +		superio_set_bit(bit, group + GPIO_SE_BASE);
> +
> +	/* clear output enable, setting the pin to input, as all the
> +	 * newly-exported GPIO interfaces are set to input.
> +	 */
> +	superio_clear_bit(bit, group + GPIO_OE_BASE);
> +
> +	superio_exit();
> +
> +	return 0;
> +}
> +
> +static int it87_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
> +{
> +	u16 reg;
> +	u8 bit;
> +
> +	bit = gpio_num % 8;
> +	reg = (gpio_num / 8) + gpio_ba;
> +
> +	return !!(inb(reg) & (1 << bit));
> +}
> +
> +static int it87_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
> +{
> +	u8 bit, group;
> +	int rc;
> +
> +	bit = gpio_num % 8;
> +	group = (gpio_num / 8);
> +
> +	if ((rc = superio_enter()))
> +		return rc;
> +
> +	/* clear the output enable bit */
> +	superio_clear_bit(bit, group + GPIO_OE_BASE);
> +
> +	superio_exit();
> +
> +	return 0;
> +}
> +
> +static void it87_gpio_set(struct gpio_chip *gc,
> +			  unsigned gpio_num, int val)
> +{
> +	unsigned long flags;
> +	u8 curr_vals, bit;
> +	u16 reg;
> +
> +	bit = gpio_num % 8;
> +	reg = (gpio_num / 8) + gpio_ba;
> +
> +	curr_vals = inb(reg);
> +	if (val)
> +		outb(curr_vals | (1 << bit) , reg);
> +	else
> +		outb(curr_vals & ~(1 << bit), reg);
> +}
> +
> +static int it87_gpio_direction_out(struct gpio_chip *gc,
> +				   unsigned gpio_num, int val)
> +{
> +	u8 bit, group;
> +	int rc;
> +
> +	bit = gpio_num % 8;
> +	group = (gpio_num / 8);
> +
> +	if ((rc = superio_enter()))
> +		return rc;
> +
> +	/* set the output enable bit */
> +	superio_set_bit(bit, group + GPIO_OE_BASE);
> +
> +	it87_gpio_set(gc, gpio_num, val);
> +
> +	superio_exit();
> +
> +	return 0;
> +}
> +
> +/* ITE documentation refers to the GPIO registers as coordinates of
> + * group/bit; we alias them as otherwise it becomes hard to find a
> + * correlation between the chip's offsets and the names in the
> + * documentation.
> + */
> +static const char *const it87_gpio_aliases[] = {
> +	"it87_gp10",
> +	"it87_gp11",
> +	"it87_gp12",
> +	"it87_gp13",
> +	"it87_gp14",
> +	"it87_gp15",
> +	"it87_gp16",
> +	"it87_gp17",
> +	"it87_gp20",
> +	"it87_gp21",
> +	"it87_gp22",
> +	"it87_gp23",
> +	"it87_gp24",
> +	"it87_gp25",
> +	"it87_gp26",
> +	"it87_gp27",
> +	"it87_gp30",
> +	"it87_gp31",
> +	"it87_gp32",
> +	"it87_gp33",
> +	"it87_gp34",
> +	"it87_gp35",
> +	"it87_gp36",
> +	"it87_gp37",
> +	"it87_gp40",
> +	"it87_gp41",
> +	"it87_gp42",
> +	"it87_gp43",
> +	"it87_gp44",
> +	"it87_gp45",
> +	"it87_gp46",
> +	"it87_gp47",
> +	"it87_gp50",
> +	"it87_gp51",
> +	"it87_gp52",
> +	"it87_gp53",
> +	"it87_gp54",
> +	"it87_gp55",
> +	"it87_gp56",
> +	"it87_gp57",
> +	"it87_gp60",
> +	"it87_gp61",
> +	"it87_gp62",
> +	"it87_gp63",
> +	"it87_gp64",
> +	"it87_gp65",
> +	"it87_gp66",
> +	"it87_gp67",
> +	"it87_gp70",
> +	"it87_gp71",
> +	"it87_gp72",
> +	"it87_gp73",
> +	"it87_gp74",
> +	"it87_gp75",
> +	"it87_gp76",
> +	"it87_gp77",
> +	"it87_gp80",
> +	"it87_gp81",
> +	"it87_gp82",
> +	"it87_gp83",
> +	"it87_gp84",
> +	"it87_gp85",
> +	"it87_gp86",
> +	"it87_gp87"
> +};
> +
> +static struct gpio_chip it87_gpio_chip = {
> +	.label			= GPIO_NAME,
> +	.owner			= THIS_MODULE,
> +	.request		= it87_gpio_request,
> +	.get			= it87_gpio_get,
> +	.direction_input	= it87_gpio_direction_in,
> +	.set			= it87_gpio_set,
> +	.direction_output	= it87_gpio_direction_out,
> +	.names			= it87_gpio_aliases
> +};
> +
> +static int __init it87_gpio_init(void)
> +{
> +	int rc = 0;
> +	u8 chip_rev;
> +	u16 chip_type;
> +
> +	if ((rc = superio_enter()))
> +		return rc;
> +
> +	chip_type = superio_inw(CHIPID);
> +	chip_rev  = superio_inb(CHIPREV) & 0x0f;
> +	superio_exit();
> +
> +	switch(chip_type) {
> +	case IT8728_ID:
> +		break;
> +	case NO_DEV_ID:
> +		printk(KERN_ERR PFX "no device\n");
> +		return -ENODEV;
> +	default:
> +		printk(KERN_ERR PFX
> +		       "Unknown Chip found, Chip %04x Revision %x\n",
> +		       chip_type, chip_rev);
> +		return -ENODEV;
> +	}
> +
> +	if ((rc = superio_enter()))
> +		return rc;
> +
> +	superio_select(GPIO);
> +
> +	/* fetch GPIO base address */
> +	gpio_ba = superio_inw(GPIO_BASE);
> +
> +	superio_exit();
> +
> +	if (!request_region(gpio_ba, GPIO_IOSIZE, GPIO_NAME))
> +		return -EBUSY;
> +
> +	it87_gpio_chip.base = -1;
> +	it87_gpio_chip.ngpio = 64;
> +
> +	if ((rc = gpiochip_add(&it87_gpio_chip)) < 0)
> +		goto gpiochip_add_err;
> +
> +	return 0;
> +
> +gpiochip_add_err:
> +	release_region(gpio_ba, GPIO_IOSIZE);
> +	gpio_ba = 0;
> +	return rc;
> +}
> +
> +static void __exit it87_gpio_exit(void)
> +{
> +	if (gpio_ba) {
> +		int ret = gpiochip_remove(&it87_gpio_chip);
> +
> +		WARN(ret, "%s(): gpiochip_remove() failed, ret=%d\n",
> +				__func__, ret);
> +
> +		release_region(gpio_ba, GPIO_IOSIZE);
> +		gpio_ba = 0;
> +	}
> +}
> +module_init(it87_gpio_init);
> +module_exit(it87_gpio_exit);
> +
> +MODULE_AUTHOR("Diego Elio Pettenò <flameeyes@flameeyes.eu>");
> +MODULE_DESCRIPTION("GPIO interface for IT87xx Super I/O chips");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.8.5
> 
> 

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Diego Elio Pettenò]

Il giorno sab, 17/03/2012 alle 12.31 +0000, Grant Likely ha scritto:
> Really?  The config line above claims IT8761E support.  How similar are
> these devices?  Either this line needs to be more specifc, or the drivers
> need to be merged.

I'm not sure about IT8761E, and it seems it's only partially the case
from the code I read; it probably would be possible — Denis would you
mind taking a look?

It could possibly work to merge them. For what it's worth I've more or
less followed on the steps of the two watchdog timer drivers for IT87
series.

> use #define pr_fmt and pr_*() macros.  Better yet, use dev_{info,err,debug}()
> macros where possible.

Will do.

> Why "superio_" prefix here?  Are these functions that are cut-and-paste
> from another driver, or are they it87xx specific?  If they are it87xx specific
> then they should have the it87_ prefix.

They seem to be shared across a number of different Super I/O chips; in
particular though they come out of wdt_it87 (which interfaces in part
through the GPIO). As I said, and as Guenter already confirmed, it might
be the case to have an it87_sio driver for the Super I/O chip that
provides the basic interface for gpio, hwmon and wdt at the same time.

I'm just not sure if I can get through it alone, especially since
reading through the mfd drivers there seem to be those that simply
"earmark" the ports for other drivers, which can then be loaded, and
those that simply create everything in their own code, and I wouldn't
know which one of the two to use ...

Another reason to have a MFD driver would be to allow tweaking the few
more knobs that (at least IT8728F) expose, such as the ability to switch
a few pre-defined inputs and outputs to GPIO instead, and so on...

> No spinlock around the critical regions?  This is racy.

Okay I'll reintroduce that. I wonder though if it should be a ngpio/8
array of spinlocks or just one, but I guess it's fast enough it
shouldn't matter.

> If you're going to do a lookup table like this, then be explicit for
> the offsets:

Will do that.

-- 
Diego Elio Pettenò — Flameeyes
http://blog.flameeyes.eu/

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Guenter Roeck]

On Sat, Mar 17, 2012 at 07:53:42PM +0100, Diego Elio Pettenò wrote:
> Il giorno sab, 17/03/2012 alle 12.31 +0000, Grant Likely ha scritto:
> > Really?  The config line above claims IT8761E support.  How similar are
> > these devices?  Either this line needs to be more specifc, or the drivers
> > need to be merged.
> 
> I'm not sure about IT8761E, and it seems it's only partially the case
> from the code I read; it probably would be possible — Denis would you
> mind taking a look?
> 
> It could possibly work to merge them. For what it's worth I've more or
> less followed on the steps of the two watchdog timer drivers for IT87
> series.
> 
I don't think that would make sense. IT8761E has a completely different set
of registers. It is a "Legacy Low Pin Count I/O" only. The other chips are
environmental control chips. I didn't follow the context of this discussion,
but IT8761E does not support a watchdog. It also only supports 16 GPIO pins.

Guenter

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Linus Walleij]

On Sat, Mar 17, 2012 at 7:53 PM, Diego Elio Pettenò
<flameeyes@flameeyes.eu> wrote:

> Another reason to have a MFD driver would be to allow tweaking the few
> more knobs that (at least IT8728F) expose, such as the ability to switch
> a few pre-defined inputs and outputs to GPIO instead, and so on...

Hm that sounds an awful lot like pin multiplexing. Is that the case?

Yours,
Linus Walleij

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Guenter Roeck]

On Sun, Mar 18, 2012 at 07:14:45PM -0400, Linus Walleij wrote:
> On Sat, Mar 17, 2012 at 7:53 PM, Diego Elio Pettenò
> <flameeyes@flameeyes.eu> wrote:
> 
> > Another reason to have a MFD driver would be to allow tweaking the few
> > more knobs that (at least IT8728F) expose, such as the ability to switch
> > a few pre-defined inputs and outputs to GPIO instead, and so on...
> 
> Hm that sounds an awful lot like pin multiplexing. Is that the case?
> 
Depending on the chip configuration, a physical pin may be a GPIO pin, or a UART pin,
or a fan control pin, or a FD pin, or something else. This does not apply to all pins,
but to many of the GPIO pins.

On IT8721F (which is supposedly mostly compatible to IT8728F), only four of the 87 GPIO pins
are GPIO-only (unless I miscounted ;). All others have at least two different functions.

Not sure if it is a good idea to re-configure any of those pins. I would assume
there is a reason if a given set of pins is configured as UART, for example.

Guenter

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Linus Walleij]

On Mon, Mar 19, 2012 at 1:54 AM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
> On Sun, Mar 18, 2012 at 07:14:45PM -0400, Linus Walleij wrote:
>> On Sat, Mar 17, 2012 at 7:53 PM, Diego Elio Pettenņ
>> <flameeyes@flameeyes.eu> wrote:
>>
>> > Another reason to have a MFD driver would be to allow tweaking the few
>> > more knobs that (at least IT8728F) expose, such as the ability to switch
>> > a few pre-defined inputs and outputs to GPIO instead, and so on...
>>
>> Hm that sounds an awful lot like pin multiplexing. Is that the case?
>>
> Depending on the chip configuration, a physical pin may be a GPIO pin, or a UART pin,
> or a fan control pin, or a FD pin, or something else. This does not apply to all pins,
> but to many of the GPIO pins.

OK that is pinmuxing, if that is to be software-controlled the place to do
it is in drivers/pinctrl. (Documentation/pinctrl.txt gives the backrgound.)

The rule is in that case to put the driver below drivers/pinctrl/* and also
expose a gpiolib interface from that driver.

As we convert drivers to the new pinctrl interfaces we tend to move them
from drivers/gpio to drivers/pinctrl. But no biggie, if pinctrl is added later,
it can be moved by then. I'm mainly concerned with blocking any
extra custom interfaces to the gpio drivers.

Yours,
Linus Walleij

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Guenter Roeck]

On Mon, Mar 19, 2012 at 04:26:44AM -0400, Linus Walleij wrote:
> On Mon, Mar 19, 2012 at 1:54 AM, Guenter Roeck
> <guenter.roeck@ericsson.com> wrote:
> > On Sun, Mar 18, 2012 at 07:14:45PM -0400, Linus Walleij wrote:
> >> On Sat, Mar 17, 2012 at 7:53 PM, Diego Elio Pettenņ
> >> <flameeyes@flameeyes.eu> wrote:
> >>
> >> > Another reason to have a MFD driver would be to allow tweaking the few
> >> > more knobs that (at least IT8728F) expose, such as the ability to switch
> >> > a few pre-defined inputs and outputs to GPIO instead, and so on...
> >>
> >> Hm that sounds an awful lot like pin multiplexing. Is that the case?
> >>
> > Depending on the chip configuration, a physical pin may be a GPIO pin, or a UART pin,
> > or a fan control pin, or a FD pin, or something else. This does not apply to all pins,
> > but to many of the GPIO pins.
> 
> OK that is pinmuxing, if that is to be software-controlled the place to do
> it is in drivers/pinctrl. (Documentation/pinctrl.txt gives the backrgound.)
> 
> The rule is in that case to put the driver below drivers/pinctrl/* and also
> expose a gpiolib interface from that driver.
> 
> As we convert drivers to the new pinctrl interfaces we tend to move them
> from drivers/gpio to drivers/pinctrl. But no biggie, if pinctrl is added later,
> it can be moved by then. I'm mainly concerned with blocking any
> extra custom interfaces to the gpio drivers.
> 
Personally I would not dare touch those pins. If a pin is configured as UART pin
(or whatever), it is quite unlikely that it is exposed as GPIO pin. Worst case,
changing the configuration may result in broken hardware. Not sure what is supposed
to be gained by enabling the user to do that.

Guenter

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Linus Walleij]

On Mon, Mar 19, 2012 at 2:56 PM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
> On Mon, Mar 19, 2012 at 04:26:44AM -0400, Linus Walleij wrote:

>> OK that is pinmuxing, if that is to be software-controlled the place to do
>> it is in drivers/pinctrl. (Documentation/pinctrl.txt gives the backrgound.)
(...)
>>
> Personally I would not dare touch those pins. If a pin is configured as UART pin
> (or whatever), it is quite unlikely that it is exposed as GPIO pin. Worst case,
> changing the configuration may result in broken hardware. Not sure what is supposed
> to be gained by enabling the user to do that.

Define "user", pinctrl does not have userspace interfaces if that's
what you mean.

If you mean "kernel programmer", these creatures seem to not respect what
they should and shouldn't poke at ;-)

The usecase is typically if the hardware is used in two different systems
where the ROM/bootcode/etc does not set up muxing correctly, or are setup
in a sub-optimal way (e.g. working but consuming too much power) and the
kernel needs to get involved. Unless you have any scenario like that, no
need to worry about it.

Yours,
Linus Walleij

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Diego Elio Pettenò]

Il giorno dom, 18/03/2012 alle 09.07 -0700, Guenter Roeck ha scritto:
> I don't think that would make sense. IT8761E has a completely
> different set
> of registers. It is a "Legacy Low Pin Count I/O" only. The other chips
> are
> environmental control chips. I didn't follow the context of this
> discussion,
> but IT8761E does not support a watchdog. It also only supports 16 GPIO
> pins.

Not for watchdog, but the GPIO does require just a few modifications to
support both chips as far as I can tell, I have some testable code here
I think, but I wanted to see if I could implement MFD as well.

-- 
Diego Elio Pettenò — Flameeyes
http://blog.flameeyes.eu/

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Diego Elio Pettenò]

Il giorno dom, 18/03/2012 alle 17.54 -0700, Guenter Roeck ha scritto:
> On IT8721F (which is supposedly mostly compatible to IT8728F), only four of the 87 GPIO pins
> are GPIO-only (unless I miscounted ;). All others have at least two different functions.

On the IT8728F the 64 GPIO (11 to 87, they use an X-Y notation) are
divided in eight banks, and only the latest three banks are GPIO-only,
the others all have alternate functions.

> Not sure if it is a good idea to re-configure any of those pins. I would assume
> there is a reason if a given set of pins is configured as UART, for example.

That's true, on the other hand there are a few pins that should be
reused without other issues: for instance the LED ones. That's what I
was thinking of with an MFD SuperIO driver.

At any rate I'll look at the pinctrl subsystem since the gpio_request
function in the gpio driver is indeed changing the mapping like pinmux
should. And there I thought it would have been an easy driver to write
^^;;

-- 
Diego Elio Pettenò — Flameeyes
http://blog.flameeyes.eu/

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Guenter Roeck]

On Mon, 2012-03-19 at 10:05 -0400, Linus Walleij wrote:
> On Mon, Mar 19, 2012 at 2:56 PM, Guenter Roeck
> <guenter.roeck@ericsson.com> wrote:
> > On Mon, Mar 19, 2012 at 04:26:44AM -0400, Linus Walleij wrote:
> 
> >> OK that is pinmuxing, if that is to be software-controlled the place to do
> >> it is in drivers/pinctrl. (Documentation/pinctrl.txt gives the backrgound.)
> (...)
> >>
> > Personally I would not dare touch those pins. If a pin is configured as UART pin
> > (or whatever), it is quite unlikely that it is exposed as GPIO pin. Worst case,
> > changing the configuration may result in broken hardware. Not sure what is supposed
> > to be gained by enabling the user to do that.
> 
> Define "user", pinctrl does not have userspace interfaces if that's
> what you mean.
> 
> If you mean "kernel programmer", these creatures seem to not respect what
> they should and shouldn't poke at ;-)
> 
> The usecase is typically if the hardware is used in two different systems
> where the ROM/bootcode/etc does not set up muxing correctly, or are setup
> in a sub-optimal way (e.g. working but consuming too much power) and the
> kernel needs to get involved. Unless you have any scenario like that, no
> need to worry about it.
> 
Ok, guess that makes sense.

Thanks,
Guenter

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Guenter Roeck]

On Mon, 2012-03-19 at 12:01 -0400, Diego Elio Pettenò wrote:
> Il giorno dom, 18/03/2012 alle 17.54 -0700, Guenter Roeck ha scritto:
> > On IT8721F (which is supposedly mostly compatible to IT8728F), only four of the 87 GPIO pins
> > are GPIO-only (unless I miscounted ;). All others have at least two different functions.
> 
> On the IT8728F the 64 GPIO (11 to 87, they use an X-Y notation) are
> divided in eight banks, and only the latest three banks are GPIO-only,
> the others all have alternate functions.

Hmm - at least the GPIO part seems to be quite different to IT8721F
after all. Too bad that ITE is so restrictive about their datasheets. I
hope the differences don't affect hwmon functionality - so far we assume
that it is compatible to IT8721F.

Guenter

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Diego Elio Pettenò]

Il giorno lun, 19/03/2012 alle 09.43 -0700, Guenter Roeck ha scritto:
> 
> 
> Hmm - at least the GPIO part seems to be quite different to IT8721F
> after all. Too bad that ITE is so restrictive about their datasheets.
> I
> hope the differences don't affect hwmon functionality - so far we
> assume
> that it is compatible to IT8721F. 

I plan on taking a look at this at some point; it's not critical for the
job I'm working on right now, as we're not keeping the temperatures
under control, but I have the datasheet and I can compare.

For what it's worth, `sensors` works fine.

-- 
Diego Elio Pettenò — Flameeyes
http://blog.flameeyes.eu/

Re: [PATCH] gpio: add support for ITE IT87xx Super I/O GPIO.

[Guenter Roeck]

On Mon, 2012-03-19 at 12:54 -0400, Diego Elio Pettenò wrote:
> Il giorno lun, 19/03/2012 alle 09.43 -0700, Guenter Roeck ha scritto:
> > 
> > 
> > Hmm - at least the GPIO part seems to be quite different to IT8721F
> > after all. Too bad that ITE is so restrictive about their datasheets.
> > I
> > hope the differences don't affect hwmon functionality - so far we
> > assume
> > that it is compatible to IT8721F. 
> 
> I plan on taking a look at this at some point; it's not critical for the
> job I'm working on right now, as we're not keeping the temperatures
> under control, but I have the datasheet and I can compare.
> 
> For what it's worth, `sensors` works fine.
> 
Good to know. I am more concerned about fan control, though. Some of the
fan control pins are multiplexed with GPIO pins, and from your feedback
it is possible if not likely that the code used to detect the
configuration is wrong.

Thanks,
Guenter