[PATCH] gpio: add GPIO support for SMSC SCH311x

[PATCH] gpio: add GPIO support for SMSC SCH311x

[Bruno Randolf]

This patch adds support for the GPIOs found on the SMSC "Super I/O" chips
SCH311x.

The chip detection and I/O functions are copied from sch311x_wdt.c

Signed-off-by: Bruno Randolf <br1@einfach.org>

---
Notes:
- All GPIOs are now supported, driver data is runtime allocated
  and I tried to address all comments as far as possible.
- Still a platform device is registered, similar to gpio-f7188x.c
---
 drivers/gpio/Kconfig        |   9 +
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-sch311x.c | 430 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 440 insertions(+)
 create mode 100644 drivers/gpio/gpio-sch311x.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0f04444..a083314 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -237,6 +237,15 @@ config GPIO_SAMSUNG
 	  Legacy GPIO support. Use only for platforms without support for
 	  pinctrl.
 
+config GPIO_SCH311X
+	tristate "SMSC SCH311x SuperI/O GPIO"
+	help
+	  Driver to enable the GPIOs found on SMSC SMSC SCH3112, SCH3114 and
+	  SCH3116 "Super I/O" chipsets.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gpio-sch311x.
+
 config GPIO_SPEAR_SPICS
 	bool "ST SPEAr13xx SPI Chip Select as GPIO support"
 	depends on PLAT_SPEAR
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 7971e36..6c53998 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_GPIO_RCAR)		+= gpio-rcar.o
 obj-$(CONFIG_GPIO_SAMSUNG)	+= gpio-samsung.o
 obj-$(CONFIG_ARCH_SA1100)	+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SCH)		+= gpio-sch.o
+obj-$(CONFIG_GPIO_SCH311X)	+= gpio-sch311x.o
 obj-$(CONFIG_GPIO_SODAVILLE)	+= gpio-sodaville.o
 obj-$(CONFIG_GPIO_SPEAR_SPICS)	+= gpio-spear-spics.o
 obj-$(CONFIG_GPIO_STA2X11)	+= gpio-sta2x11.o
diff --git a/drivers/gpio/gpio-sch311x.c b/drivers/gpio/gpio-sch311x.c
new file mode 100644
index 0000000..3cfb7f5
--- /dev/null
+++ b/drivers/gpio/gpio-sch311x.c
@@ -0,0 +1,430 @@
+/*
+ * GPIO driver for the SMSC SCH311x Super-I/O chips
+ *
+ * Copyright (C) 2013 Bruno Randolf <br1@einfach.org>
+ *
+ * SuperIO functions and chip detection:
+ * (c) Copyright 2008 Wim Van Sebroeck <wim@iguana.be>.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/bitops.h>
+
+#define DRV_NAME			"gpio-sch311x"
+
+#define SCH311X_GPIO_CONF_OUT		0x00
+#define SCH311X_GPIO_CONF_IN		0x01
+#define SCH311X_GPIO_CONF_INVERT	0x02
+#define SCH311X_GPIO_CONF_OPEN_DRAIN	0x80
+
+#define SIO_CONFIG_KEY_ENTER		0x55
+#define SIO_CONFIG_KEY_EXIT		0xaa
+
+#define GP1				0x4b
+
+static int sch311x_ioports[] = { 0x2e, 0x4e, 0x162e, 0x164e };
+
+static struct platform_device *sch311x_gpio_pdev;
+
+struct sch311x_pdev_data {		/* platform device data */
+	unsigned short runtime_reg;	/* runtime register base address */
+};
+
+struct sch311x_gpio_block {		/* one GPIO block runtime data */
+	struct gpio_chip chip;
+	unsigned short data_reg;	/* from definition below */
+	unsigned short *config_regs;	/* pointer to definition below */
+	unsigned short runtime_reg;	/* runtime register */
+	spinlock_t lock;		/* lock for this GPIO block */
+};
+
+struct sch311x_gpio_priv {		/* driver private data */
+	struct sch311x_gpio_block blocks[6];
+};
+
+struct sch311x_gpio_block_def {		/* register address definitions */
+	unsigned short data_reg;
+	unsigned short config_regs[8];
+	unsigned short base;
+};
+
+/* Note: some GPIOs are not available, these are marked with 0x00 */
+
+static struct sch311x_gpio_block_def sch311x_gpio_blocks[] = {
+	{
+		.data_reg = 0x4b,	/* GP1 */
+		.config_regs = {0x23, 0x24, 0x25, 0x26, 0x27, 0x29, 0x2a, 0x2b},
+		.base = 10,
+	},
+	{
+		.data_reg = 0x4c,	/* GP2 */
+		.config_regs = {0x00, 0x2c, 0x2d, 0x00, 0x00, 0x00, 0x00, 0x32},
+		.base = 20,
+	},
+	{
+		.data_reg = 0x4d,	/* GP3 */
+		.config_regs = {0x33, 0x34, 0x35, 0x36, 0x37, 0x00, 0x39, 0x3a},
+		.base = 30,
+	},
+	{
+		.data_reg = 0x4e,	/* GP4 */
+		.config_regs = {0x3b, 0x00, 0x3d, 0x00, 0x6e, 0x6f, 0x72, 0x73},
+		.base = 40,
+	},
+	{
+		.data_reg = 0x4f,	/* GP5 */
+		.config_regs = {0x3f, 0x40, 0x41, 0x42, 0x43, 0x44, 0x45, 0x46},
+		.base = 50,
+	},
+	{
+		.data_reg = 0x50,	/* GP6 */
+		.config_regs = {0x47, 0x48, 0x54, 0x55, 0x56, 0x57, 0x58, 0x59},
+		.base = 60,
+	},
+};
+
+static inline struct sch311x_gpio_block *
+to_sch311x_gpio_block(struct gpio_chip *chip)
+{
+	return container_of(chip, struct sch311x_gpio_block, chip);
+}
+
+
+/*
+ *	Super-IO functions
+ */
+
+static inline int sch311x_sio_enter(int sio_config_port)
+{
+	/* Don't step on other drivers' I/O space by accident. */
+	if (!request_muxed_region(sio_config_port, 2, DRV_NAME)) {
+		pr_err(DRV_NAME "I/O address 0x%04x already in use\n",
+		       sio_config_port);
+		return -EBUSY;
+	}
+
+	outb(SIO_CONFIG_KEY_ENTER, sio_config_port);
+	return 0;
+}
+
+static inline void sch311x_sio_exit(int sio_config_port)
+{
+	outb(SIO_CONFIG_KEY_EXIT, sio_config_port);
+	release_region(sio_config_port, 2);
+}
+
+static inline int sch311x_sio_inb(int sio_config_port, int reg)
+{
+	outb(reg, sio_config_port);
+	return inb(sio_config_port + 1);
+}
+
+static inline void sch311x_sio_outb(int sio_config_port, int reg, int val)
+{
+	outb(reg, sio_config_port);
+	outb(val, sio_config_port + 1);
+}
+
+
+/*
+ *	GPIO functions
+ */
+
+static int sch311x_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	struct sch311x_gpio_block *block = to_sch311x_gpio_block(chip);
+
+	if (block->config_regs[offset] == 0) /* GPIO is not available */
+		return -ENODEV;
+
+	if (!request_region(block->runtime_reg + block->config_regs[offset],
+			    1, DRV_NAME)) {
+		dev_err(chip->dev, "Failed to request region 0x%04x.\n",
+			block->runtime_reg + block->config_regs[offset]);
+		return -EBUSY;
+	}
+	return 0;
+}
+
+static void sch311x_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	struct sch311x_gpio_block *block = to_sch311x_gpio_block(chip);
+
+	if (block->config_regs[offset] == 0) /* GPIO is not available */
+		return;
+
+	release_region(block->runtime_reg + block->config_regs[offset], 1);
+}
+
+static int sch311x_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct sch311x_gpio_block *block = to_sch311x_gpio_block(chip);
+	unsigned char data;
+
+	spin_lock(&block->lock);
+	data = inb(block->runtime_reg + block->data_reg);
+	spin_unlock(&block->lock);
+
+	return !!(data & BIT(offset));
+}
+
+static void __sch311x_gpio_set(struct sch311x_gpio_block *block,
+			       unsigned offset, int value)
+{
+	unsigned char data = inb(block->runtime_reg + block->data_reg);
+	if (value)
+		data |= BIT(offset);
+	else
+		data &= ~BIT(offset);
+	outb(data, block->runtime_reg + block->data_reg);
+}
+
+static void sch311x_gpio_set(struct gpio_chip *chip, unsigned offset,
+			     int value)
+{
+	struct sch311x_gpio_block *block = to_sch311x_gpio_block(chip);
+
+	spin_lock(&block->lock);
+	 __sch311x_gpio_set(block, offset, value);
+	spin_unlock(&block->lock);
+}
+
+static int sch311x_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
+{
+	struct sch311x_gpio_block *block = to_sch311x_gpio_block(chip);
+
+	spin_lock(&block->lock);
+	outb(SCH311X_GPIO_CONF_IN, block->runtime_reg +
+	     block->config_regs[offset]);
+	spin_unlock(&block->lock);
+
+	return 0;
+}
+
+static int sch311x_gpio_direction_out(struct gpio_chip *chip, unsigned offset,
+				      int value)
+{
+	struct sch311x_gpio_block *block = to_sch311x_gpio_block(chip);
+
+	spin_lock(&block->lock);
+
+	outb(SCH311X_GPIO_CONF_OUT, block->runtime_reg +
+	     block->config_regs[offset]);
+
+	__sch311x_gpio_set(block, offset, value);
+
+	spin_unlock(&block->lock);
+	return 0;
+}
+
+static int sch311x_gpio_probe(struct platform_device *pdev)
+{
+	struct sch311x_pdev_data *pdata = pdev->dev.platform_data;
+	struct sch311x_gpio_priv *priv;
+	struct sch311x_gpio_block *block;
+	int err, i;
+
+	/* we can register all GPIO data registers at once */
+	if (!request_region(pdata->runtime_reg + GP1, 6, DRV_NAME)) {
+		dev_err(&pdev->dev, "Failed to request region 0x%04x-0x%04x.\n",
+			pdata->runtime_reg + GP1, pdata->runtime_reg + GP1 + 5);
+		return -EBUSY;
+	}
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	for (i = 0; i < ARRAY_SIZE(priv->blocks); i++) {
+		block = &priv->blocks[i];
+
+		spin_lock_init(&block->lock);
+
+		block->chip.label = DRV_NAME;
+		block->chip.owner = THIS_MODULE;
+		block->chip.request = sch311x_gpio_request;
+		block->chip.free = sch311x_gpio_free;
+		block->chip.direction_input = sch311x_gpio_direction_in;
+		block->chip.direction_output = sch311x_gpio_direction_out;
+		block->chip.get = sch311x_gpio_get;
+		block->chip.set = sch311x_gpio_set;
+		block->chip.ngpio = 8;
+		block->chip.dev = &pdev->dev;
+		block->chip.base = sch311x_gpio_blocks[i].base;
+		block->config_regs = sch311x_gpio_blocks[i].config_regs;
+		block->data_reg = sch311x_gpio_blocks[i].data_reg;
+		block->runtime_reg = pdata->runtime_reg;
+
+		err = gpiochip_add(&block->chip);
+		if (err < 0) {
+			dev_err(&pdev->dev,
+				"Could not register gpiochip, %d\n", err);
+			goto exit_err;
+		}
+		dev_info(&pdev->dev,
+			 "SMSC SCH311x GPIO block %d registered.\n", i);
+	}
+
+	return 0;
+
+exit_err:
+	release_region(pdata->runtime_reg + GP1, 6);
+	/* release already registered chips */
+	for (--i; i >= 0; i--)
+		gpiochip_remove(&priv->blocks[i].chip);
+	return err;
+}
+
+static int sch311x_gpio_remove(struct platform_device *pdev)
+{
+	struct sch311x_pdev_data *pdata = pdev->dev.platform_data;
+	struct sch311x_gpio_priv *priv = platform_get_drvdata(pdev);
+	int err, i;
+
+	release_region(pdata->runtime_reg + GP1, 6);
+
+	for (i = 0; i < ARRAY_SIZE(priv->blocks); i++) {
+		err = gpiochip_remove(&priv->blocks[i].chip);
+		if (err)
+			return err;
+		dev_info(&pdev->dev,
+			 "SMSC SCH311x GPIO block %d unregistered.\n", i);
+	}
+	return 0;
+}
+
+static struct platform_driver sch311x_gpio_driver = {
+	.driver.name	= DRV_NAME,
+	.driver.owner	= THIS_MODULE,
+	.probe		= sch311x_gpio_probe,
+	.remove		= sch311x_gpio_remove,
+};
+
+
+/*
+ *	Init & exit routines
+ */
+
+static int __init sch311x_detect(int sio_config_port, unsigned short *addr)
+{
+	int err = 0, reg;
+	unsigned short base_addr;
+	unsigned char dev_id;
+
+	err = sch311x_sio_enter(sio_config_port);
+	if (err)
+		return err;
+
+	/* Check device ID. We currently know about:
+	 * SCH3112 (0x7c), SCH3114 (0x7d), and SCH3116 (0x7f). */
+	reg = sch311x_sio_inb(sio_config_port, 0x20);
+	if (!(reg == 0x7c || reg == 0x7d || reg == 0x7f)) {
+		err = -ENODEV;
+		goto exit;
+	}
+	dev_id = reg == 0x7c ? 2 : reg == 0x7d ? 4 : 6;
+
+	/* Select logical device A (runtime registers) */
+	sch311x_sio_outb(sio_config_port, 0x07, 0x0a);
+
+	/* Check if Logical Device Register is currently active */
+	if ((sch311x_sio_inb(sio_config_port, 0x30) & 0x01) == 0)
+		pr_info("Seems that LDN 0x0a is not active...\n");
+
+	/* Get the base address of the runtime registers */
+	base_addr = (sch311x_sio_inb(sio_config_port, 0x60) << 8) |
+			   sch311x_sio_inb(sio_config_port, 0x61);
+	if (!base_addr) {
+		pr_err("Base address not set\n");
+		err = -ENODEV;
+		goto exit;
+	}
+	*addr = base_addr;
+
+	pr_info("Found an SMSC SCH311%d chip at 0x%04x\n", dev_id, base_addr);
+
+exit:
+	sch311x_sio_exit(sio_config_port);
+	return err;
+}
+
+static int __init sch311x_gpio_pdev_add(const unsigned short addr)
+{
+	struct sch311x_pdev_data pdata;
+	int err;
+
+	pdata.runtime_reg = addr;
+
+	sch311x_gpio_pdev = platform_device_alloc(DRV_NAME, -1);
+	if (!sch311x_gpio_pdev)
+		return -ENOMEM;
+
+	err = platform_device_add_data(sch311x_gpio_pdev,
+				       &pdata, sizeof(pdata));
+	if (err) {
+		pr_err(DRV_NAME "Platform data allocation failed\n");
+		goto err;
+	}
+
+	err = platform_device_add(sch311x_gpio_pdev);
+	if (err) {
+		pr_err(DRV_NAME "Device addition failed\n");
+		goto err;
+	}
+	return 0;
+
+err:
+	platform_device_put(sch311x_gpio_pdev);
+	return err;
+}
+
+static int __init sch311x_gpio_init(void)
+{
+	int err, i, found = 0;
+	unsigned short addr = 0;
+
+	for (i = 0; !found && i < ARRAY_SIZE(sch311x_ioports); i++)
+		if (sch311x_detect(sch311x_ioports[i], &addr) == 0)
+			found++;
+	if (!found)
+		return -ENODEV;
+
+	err = platform_driver_register(&sch311x_gpio_driver);
+	if (err)
+		return err;
+
+	err = sch311x_gpio_pdev_add(addr);
+	if (err)
+		goto unreg_platform_driver;
+
+	return 0;
+
+unreg_platform_driver:
+	platform_driver_unregister(&sch311x_gpio_driver);
+	return err;
+}
+
+static void __exit sch311x_gpio_exit(void)
+{
+	platform_device_unregister(sch311x_gpio_pdev);
+	platform_driver_unregister(&sch311x_gpio_driver);
+}
+
+module_init(sch311x_gpio_init);
+module_exit(sch311x_gpio_exit);
+
+MODULE_AUTHOR("Bruno Randolf <br1@einfach.org>");
+MODULE_DESCRIPTION("SMSC SCH311x GPIO Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:gpio-sch311x");
-- 
1.8.3.2

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[David Cohen]

Hi Bruno,

I've got a small suggestion below.
But regardless that, your patch looks fine.

On 12/03/2013 04:23 PM, Bruno Randolf wrote:
> This patch adds support for the GPIOs found on the SMSC "Super I/O" chips
> SCH311x.
>
> The chip detection and I/O functions are copied from sch311x_wdt.c
>
> Signed-off-by: Bruno Randolf <br1@einfach.org>
>
> ---
> Notes:
> - All GPIOs are now supported, driver data is runtime allocated
>   and I tried to address all comments as far as possible.
> - Still a platform device is registered, similar to gpio-f7188x.c
> ---
>  drivers/gpio/Kconfig        |   9 +
>  drivers/gpio/Makefile       |   1 +
>  drivers/gpio/gpio-sch311x.c | 430 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 440 insertions(+)
>  create mode 100644 drivers/gpio/gpio-sch311x.c
>

[snip]

> +static int __init sch311x_gpio_init(void)
> +{
> +	int err, i, found = 0;
> +	unsigned short addr = 0;
> +
> +	for (i = 0; !found && i < ARRAY_SIZE(sch311x_ioports); i++)
> +		if (sch311x_detect(sch311x_ioports[i], &addr) == 0)
> +			found++;

'found' variable isn't necessary here. Just turn it into a label placed
below
'return -ENODEV' then 'goto found' directly.

> +	if (!found)

This 'if' wouldn't be necessary anymore.
You'd call return unconditionally at this point.

Br, David Cohen

> +		return -ENODEV;
> +
> +	err = platform_driver_register(&sch311x_gpio_driver);
> +	if (err)
> +		return err;
> +
> +	err = sch311x_gpio_pdev_add(addr);
> +	if (err)
> +		goto unreg_platform_driver;
> +
> +	return 0;
> +
> +unreg_platform_driver:
> +	platform_driver_unregister(&sch311x_gpio_driver);
> +	return err;
> +}
> +
> +static void __exit sch311x_gpio_exit(void)
> +{
> +	platform_device_unregister(sch311x_gpio_pdev);
> +	platform_driver_unregister(&sch311x_gpio_driver);
> +}
> +
> +module_init(sch311x_gpio_init);
> +module_exit(sch311x_gpio_exit);
> +
> +MODULE_AUTHOR("Bruno Randolf <br1@einfach.org>");
> +MODULE_DESCRIPTION("SMSC SCH311x GPIO Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:gpio-sch311x");

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Linus Walleij]

On Wed, Dec 4, 2013 at 1:23 AM, Bruno Randolf <br1@einfach.org> wrote:

> This patch adds support for the GPIOs found on the SMSC "Super I/O" chips
> SCH311x.
>
> The chip detection and I/O functions are copied from sch311x_wdt.c
>
> Signed-off-by: Bruno Randolf <br1@einfach.org>
>
> ---
> Notes:
> - All GPIOs are now supported, driver data is runtime allocated
>   and I tried to address all comments as far as possible.
> - Still a platform device is registered, similar to gpio-f7188x.c

So I have a problem with this design pattern so I need to ask
Matthew Garret about this:

- Driver *always* performs some port-mapped I/O probe on
  some random ports as a compulsory initcall.

- No other hardware discovery.

- If detecting magic, registers a platform device.

- Then handles this device by also providing a device
  driver for it.

- All of this is placed in one file.

Matthew is this how discovery and registration of x86 platform
drivers for port-mapped devices should work or are we doing
something weird here?

Since earlier we have:

drivers/gpio/gpio-f7188x.c - exactly the same pattern

drivers/gpio/gpio-it8761e.c - doesn't even bother to create a platform
  device, goes on and creates a gpio_chip without any device

drivers/gpio/gpio-sch.c - port-mapped but platform device is
  created by an MFD which is probed from PCI (seems fine).

drivers/gpio/gpio-ts5500.c - aha, created from
  arch/x86/platform/ts5500/ts5500.c, a "board file".

This is a bit heterogeneous, but I can't claim to understand how
x86 want to register its devices so need some input here.

Maybe this is all the right way to do things, but I want to
be hammered down by some convincing arguments first.

Yours,
Linus Walleij

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Matthew Garrett]

On Tue, Dec 10, 2013 at 12:51:30PM +0100, Linus Walleij wrote:

> Matthew is this how discovery and registration of x86 platform
> drivers for port-mapped devices should work or are we doing
> something weird here?

This is pretty awful, but typical. LPC devices *should* be present in 
ACPI to some extent, but often you'll just find (at best) a reference to 
a superIO chip and no child devices. Do we have an example DSDT for a 
device with this part?

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

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Vivien Didelot]

Hi Linus,

You wrote:
> > This patch adds support for the GPIOs found on the SMSC "Super I/O"
> > chips
> > SCH311x.
> >
> > The chip detection and I/O functions are copied from sch311x_wdt.c
> >
> > Signed-off-by: Bruno Randolf <br1@einfach.org>
> >
> > ---
> > Notes:
> > - All GPIOs are now supported, driver data is runtime allocated
> >   and I tried to address all comments as far as possible.
> > - Still a platform device is registered, similar to gpio-f7188x.c
> 
> So I have a problem with this design pattern so I need to ask
> Matthew Garret about this:
> 
> - Driver *always* performs some port-mapped I/O probe on
>   some random ports as a compulsory initcall.
> 
> - No other hardware discovery.
> 
> - If detecting magic, registers a platform device.
> 
> - Then handles this device by also providing a device
>   driver for it.
> 
> - All of this is placed in one file.
> 
> Matthew is this how discovery and registration of x86 platform
> drivers for port-mapped devices should work or are we doing
> something weird here?
> 
> Since earlier we have:
> 
> drivers/gpio/gpio-f7188x.c - exactly the same pattern
> 
> drivers/gpio/gpio-it8761e.c - doesn't even bother to create a
> platform
>   device, goes on and creates a gpio_chip without any device
> 
> drivers/gpio/gpio-sch.c - port-mapped but platform device is
>   created by an MFD which is probed from PCI (seems fine).
> 
> drivers/gpio/gpio-ts5500.c - aha, created from
>   arch/x86/platform/ts5500/ts5500.c, a "board file".

Because of the lack of mechanism such as DMI, there's no safe way to verify the
machine at the GPIO driver level. However, the board code (which registers the
platform device) does a safe check by looking for some magic in the RAM.

Would it be safer if we make GPIO_TS5500 depends on TS5500?

> This is a bit heterogeneous, but I can't claim to understand how
> x86 want to register its devices so need some input here.
> 
> Maybe this is all the right way to do things, but I want to
> be hammered down by some convincing arguments first.

Best,
Vivien

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Linus Walleij]

On Wed, Dec 11, 2013 at 5:05 AM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
>[Me]
>> drivers/gpio/gpio-ts5500.c - aha, created from
>>   arch/x86/platform/ts5500/ts5500.c, a "board file".
>
> Because of the lack of mechanism such as DMI, there's no safe way to verify the
> machine at the GPIO driver level. However, the board code (which registers the
> platform device) does a safe check by looking for some magic in the RAM.
>
> Would it be safer if we make GPIO_TS5500 depends on TS5500?

I don't know actually. How does it work with these x86 things?
Is it so that the system comes up and then you modprobe this driver?

In that case, do you bring up a fully generic kernel or do you have
to have it compiled with CONFIG_TS5500 set to 'y' and is that
done of a say, typical distro kernel?

If we don't know if it will be modprobed or not on a certain system
we should keep it as is, but of the distro kernels all set
CONFIG_TS5500 to 'n' then we should add a dependency
like that.

Yours,
Linus Walleij

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Vivien Didelot]

Hi Linus,

You wrote:
> >> drivers/gpio/gpio-ts5500.c - aha, created from
> >>   arch/x86/platform/ts5500/ts5500.c, a "board file".
> >
> > Because of the lack of mechanism such as DMI, there's no safe way
> > to verify the
> > machine at the GPIO driver level. However, the board code (which
> > registers the
> > platform device) does a safe check by looking for some magic in the
> > RAM.
> >
> > Would it be safer if we make GPIO_TS5500 depends on TS5500?
> 
> I don't know actually. How does it work with these x86 things?
> Is it so that the system comes up and then you modprobe this driver?

With TS5500 selected, the board comes up, get probed reading the RAM
(from the board code), then registers this GPIO platform device.

> In that case, do you bring up a fully generic kernel or do you have
> to have it compiled with CONFIG_TS5500 set to 'y' and is that
> done of a say, typical distro kernel?

GPIO_TS5500 cannot probe the DIO blocks by itself. It needs a board
code to fill and register the corresponding platform device structures.

So a generic x86 kernel will work on the board, but we need TS5500 set
to 'y' in order to have the GPIOs.

> If we don't know if it will be modprobed or not on a certain system
> we should keep it as is, but of the distro kernels all set
> CONFIG_TS5500 to 'n' then we should add a dependency
> like that.

As it is an embedded platform, I don't think CONFIG_TS5500 is enabled
on distro kernels.

I hope it's a bit clearer. Let me know what is your advice on this.

Best,
Vivien

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Linus Walleij]

On Mon, Dec 16, 2013 at 3:46 AM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> [Me]
>> If we don't know if it will be modprobed or not on a certain system
>> we should keep it as is, but of the distro kernels all set
>> CONFIG_TS5500 to 'n' then we should add a dependency
>> like that.
>
> As it is an embedded platform, I don't think CONFIG_TS5500 is enabled
> on distro kernels.
>
> I hope it's a bit clearer. Let me know what is your advice on this.

Hm I think right now we should not fix what ain't broken and leave
it as it is. The module is not on any distros I've seen so they're
already avoiding to shit it it seems.

Yours,
Linus Walleij

[PATCH] gpio: add GPIO support for SMSC SCH311x

[Bruno Randolf]

This patch adds support for the first eight GPIOs found on the SMSC "Super I/O"
chips SCH311x.

This chip is used on Atom based embedded boards (e.g. "Advantec MIO-2261") and
actually has more GPIO lines, but they are not connected on the board I have
access to.

The chip detection and I/O functions are copied from sch311x_wdt.c

Signed-off-by: Bruno Randolf <br1@einfach.org>
---
 drivers/gpio/Kconfig        |  10 ++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-sch311x.c | 315 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 326 insertions(+)
 create mode 100644 drivers/gpio/gpio-sch311x.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b6ed304..3d6bc8d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -237,6 +237,16 @@ config GPIO_SAMSUNG
 	  Legacy GPIO support. Use only for platforms without support for
 	  pinctrl.
 
+config GPIO_SCH311X
+	tristate "SMSC SCH311x SuperI/O GPIO"
+	depends on X86
+	help
+	  Driver to enable the GPIOs found on SMSC SMSC SCH3112, SCH3114 and
+	  SCH3116 "Super I/O" chipsets.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gpio-sch311x.
+
 config GPIO_SPEAR_SPICS
 	bool "ST SPEAr13xx SPI Chip Select as GPIO support"
 	depends on PLAT_SPEAR
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 98e23eb..b63c11f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_GPIO_RCAR)		+= gpio-rcar.o
 obj-$(CONFIG_GPIO_SAMSUNG)	+= gpio-samsung.o
 obj-$(CONFIG_ARCH_SA1100)	+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SCH)		+= gpio-sch.o
+obj-$(CONFIG_GPIO_SCH311X)	+= gpio-sch311x.o
 obj-$(CONFIG_GPIO_SODAVILLE)	+= gpio-sodaville.o
 obj-$(CONFIG_GPIO_SPEAR_SPICS)	+= gpio-spear-spics.o
 obj-$(CONFIG_GPIO_STA2X11)	+= gpio-sta2x11.o
diff --git a/drivers/gpio/gpio-sch311x.c b/drivers/gpio/gpio-sch311x.c
new file mode 100644
index 0000000..1ca7450
--- /dev/null
+++ b/drivers/gpio/gpio-sch311x.c
@@ -0,0 +1,315 @@
+/*
+ * GPIO driver for the SMSC SCH311x Super-I/O chips
+ *
+ * Copyright (C) 2013 Bruno Randolf <br1@einfach.org>
+ *
+ * SuperIO functions and chip detection:
+ * (c) Copyright 2008 Wim Van Sebroeck <wim@iguana.be>.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/i2c-gpio.h>
+
+#define DRV_NAME	"gpio-sch311x"
+
+/*
+ * Note: This driver only supports the first 8 GPIOs of the chip.
+ */
+
+
+/* Runtime registers */
+
+#define GP1	0x4B	/* GP10-17 data register */
+
+static unsigned int sch311x_gpio_conf[] = {
+	{ 0x23,		/* GP10 config register */
+	0x24,		/* GP11 config register */
+	0x25,		/* GP12 config register */
+	0x26,		/* GP13 config register */
+	0x27,		/* GP14 config register */
+	0x29,		/* GP15 config register */
+	0x2a,		/* GP16 config register */
+	0x2b,		/* GP17 config register */
+};
+
+#define SCH311X_GPIO_CONF_IN		0x01
+#define SCH311X_GPIO_CONF_OUT		0x00
+#define SCH311X_GPIO_CONF_INV		0x02
+#define SCH311X_GPIO_CONF_OPEN_DRAIN	0x80
+
+static int sch311x_ioports[] = { 0x2e, 0x4e, 0x162e, 0x164e, 0x00 };
+
+/* internal variables */
+static struct platform_device *sch311x_gpio_pdev;
+static struct platform_device *i2c_gpio_pdev;
+
+static struct {
+	unsigned short runtime_reg;	/* Runtime Register base address */
+	spinlock_t lock;		/* lock for io operations */
+} sch311x_gpio_data;
+
+
+/*
+ *	Super-IO functions
+ */
+
+static inline void sch311x_sio_enter(int sio_config_port)
+{
+	outb(0x55, sio_config_port);
+}
+
+static inline void sch311x_sio_exit(int sio_config_port)
+{
+	outb(0xaa, sio_config_port);
+}
+
+static inline int sch311x_sio_inb(int sio_config_port, int reg)
+{
+	outb(reg, sio_config_port);
+	return inb(sio_config_port + 1);
+}
+
+static inline void sch311x_sio_outb(int sio_config_port, int reg, int val)
+{
+	outb(reg, sio_config_port);
+	outb(val, sio_config_port + 1);
+}
+
+
+/*
+ *	GPIO functions
+ */
+
+static int sch311x_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	unsigned char data = inb(sch311x_gpio_data.runtime_reg + GP1);
+	return ((data >> offset) & 1);
+}
+
+static void __sch311x_gpio_set(unsigned offset, int value)
+{
+	unsigned char data;
+	data = inb(sch311x_gpio_data.runtime_reg + GP1);
+	if (value)
+		data |= 1 << offset;
+	else
+		data &= ~(1 << offset);
+	outb(data, sch311x_gpio_data.runtime_reg + GP1);
+}
+
+static void sch311x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	spin_lock(&sch311x_gpio_data.lock);
+
+	 __sch311x_gpio_set(offset, value);
+
+	spin_unlock(&sch311x_gpio_data.lock);
+}
+
+static int sch311x_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
+{
+	spin_lock(&sch311x_gpio_data.lock);
+
+	outb(SCH311X_GPIO_CONF_IN, sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[offset]);
+
+	spin_unlock(&sch311x_gpio_data.lock);
+	return 0;
+}
+
+static int sch311x_gpio_direction_out(struct gpio_chip *chip, unsigned offset, int value)
+{
+	spin_lock(&sch311x_gpio_data.lock);
+
+#if 0
+	/* configure as "push/pull": output voltage is 3.3V */
+	outb(SCH311X_GPIO_CONF_OUT, sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[offset]);
+#else
+	/* configure as "open drain": output voltage is 5V on an unconnected PIN */
+	outb(SCH311X_GPIO_CONF_OUT | SCH311X_GPIO_CONF_OPEN_DRAIN,
+	     sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[offset]);
+#endif
+	__sch311x_gpio_set(offset, value);
+
+	spin_unlock(&sch311x_gpio_data.lock);
+	return 0;
+}
+
+static struct gpio_chip sc_gpio_chip = {
+	.label			= "sch311x_gpio",
+	.owner			= THIS_MODULE,
+	.direction_input	= sch311x_gpio_direction_in,
+	.get			= sch311x_gpio_get,
+	.direction_output	= sch311x_gpio_direction_out,
+	.set			= sch311x_gpio_set,
+	.can_sleep		= 0,
+	.base			= 0,
+	.ngpio			= 8,
+};
+
+static int sch311x_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int err, i;
+
+	spin_lock_init(&sch311x_gpio_data.lock);
+
+	if (!request_region(sch311x_gpio_data.runtime_reg + GP1, 1, DRV_NAME)) {
+		dev_err(dev, "Failed to request region 0x%04x-0x%04x.\n",
+			sch311x_gpio_data.runtime_reg + GP1,
+			sch311x_gpio_data.runtime_reg + GP1);
+		err = -EBUSY;
+		goto exit;
+	}
+
+	for (i=0; i < ARRAY_SIZE(sch311x_gpio_conf); i++) {
+		if (!request_region(sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[i], 1, DRV_NAME)) {
+			dev_err(dev, "Failed to request region 0x%04x-0x%04x.\n",
+				sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[i],
+				sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[i]);
+			err = -EBUSY;
+			goto exit_release_region;
+		}
+	}
+
+	sc_gpio_chip.dev = &pdev->dev;
+	err = gpiochip_add(&sc_gpio_chip);
+	if (err < 0) {
+		dev_err(&pdev->dev, "Could not register gpiochip, %d\n", err);
+		goto exit_release_region2;
+	}
+
+	dev_info(dev, "SMSC SCH311x GPIO registered.\n");
+
+	return 0;
+
+exit_release_region2:
+	for (i=0; i < ARRAY_SIZE(sch311x_gpio_conf); i++) {
+		release_region(sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[i], 1);
+	}
+exit_release_region:
+	release_region(sch311x_gpio_data.runtime_reg + GP1, 1);
+	sch311x_gpio_data.runtime_reg = 0;
+exit:
+	return err;
+}
+
+static int sch311x_gpio_remove(struct platform_device *pdev)
+{
+	int i;
+
+	release_region(sch311x_gpio_data.runtime_reg + GP1, 1);
+	for (i=0; i < ARRAY_SIZE(sch311x_gpio_conf); i++) {
+		release_region(sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[i], 1);
+	}
+	sch311x_gpio_data.runtime_reg = 0;
+
+	return gpiochip_remove(&sc_gpio_chip);
+}
+
+static struct platform_driver sch311x_gpio_driver = {
+	.driver.name	= DRV_NAME,
+	.driver.owner	= THIS_MODULE,
+	.probe		= sch311x_gpio_probe,
+	.remove		= sch311x_gpio_remove,
+};
+
+
+/*
+ *	Init & exit routines
+ */
+
+static int __init sch311x_detect(int sio_config_port, unsigned short *addr)
+{
+	int err = 0, reg;
+	unsigned short base_addr;
+	unsigned char dev_id;
+
+	sch311x_sio_enter(sio_config_port);
+
+	/* Check device ID. We currently know about:
+	 * SCH3112 (0x7c), SCH3114 (0x7d), and SCH3116 (0x7f). */
+	reg = sch311x_sio_inb(sio_config_port, 0x20);
+	if (!(reg == 0x7c || reg == 0x7d || reg == 0x7f)) {
+		err = -ENODEV;
+		goto exit;
+	}
+	dev_id = reg == 0x7c ? 2 : reg == 0x7d ? 4 : 6;
+
+	/* Select logical device A (runtime registers) */
+	sch311x_sio_outb(sio_config_port, 0x07, 0x0a);
+
+	/* Check if Logical Device Register is currently active */
+	if ((sch311x_sio_inb(sio_config_port, 0x30) & 0x01) == 0)
+		pr_info("Seems that LDN 0x0a is not active...\n");
+
+	/* Get the base address of the runtime registers */
+	base_addr = (sch311x_sio_inb(sio_config_port, 0x60) << 8) |
+			   sch311x_sio_inb(sio_config_port, 0x61);
+	if (!base_addr) {
+		pr_err("Base address not set\n");
+		err = -ENODEV;
+		goto exit;
+	}
+	*addr = base_addr;
+
+	pr_info("Found an SMSC SCH311%d chip at 0x%04x\n", dev_id, base_addr);
+
+exit:
+	sch311x_sio_exit(sio_config_port);
+	return err;
+}
+
+static int __init sch311x_gpio_init(void)
+{
+	int err, i, found = 0;
+	unsigned short addr = 0;
+
+	for (i = 0; !found && sch311x_ioports[i]; i++)
+		if (sch311x_detect(sch311x_ioports[i], &addr) == 0)
+			found++;
+
+	if (!found)
+		return -ENODEV;
+
+	sch311x_gpio_data.runtime_reg = addr;
+
+	err = platform_driver_register(&sch311x_gpio_driver);
+	if (err)
+		return err;
+
+	sch311x_gpio_pdev = platform_device_register_simple(DRV_NAME, addr, NULL, 0);
+
+	if (IS_ERR(sch311x_gpio_pdev)) {
+		err = PTR_ERR(sch311x_gpio_pdev);
+		goto unreg_platform_driver;
+	}
+
+	return 0;
+
+unreg_platform_driver:
+	platform_driver_unregister(&sch311x_gpio_driver);
+	return err;
+}
+
+static void __exit sch311x_gpio_exit(void)
+{
+	platform_device_unregister(sch311x_gpio_pdev);
+	platform_driver_unregister(&sch311x_gpio_driver);
+}
+
+module_init(sch311x_gpio_init);
+module_exit(sch311x_gpio_exit);
+
+MODULE_AUTHOR("Bruno Randolf <br1@einfach.org>");
+MODULE_DESCRIPTION("SMSC SCH311x GPIO Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:gpio-sch311x");
-- 
1.8.3.2

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Linus Walleij]

On Thu, Nov 28, 2013 at 1:18 AM, Bruno Randolf <br1@einfach.org> wrote:

> This patch adds support for the first eight GPIOs found on the SMSC "Super I/O"
> chips SCH311x.

Simon Guinot recently added another "Super I/O" driver, Simon can
you have a look at this driver?

> This chip is used on Atom based embedded boards (e.g. "Advantec MIO-2261") and
> actually has more GPIO lines, but they are not connected on the board I have
> access to.

But please write a driver for the chip, not for the board. Make it enabled
to handle all the GPIO lines on that chip, i.e. handle all registers, then
configure the driver to only use the first eight lines on this board.

Besides: tomorrow someone will mount this in another X86 thing
and use more of the lines.

> +config GPIO_SCH311X
> +       tristate "SMSC SCH311x SuperI/O GPIO"
> +       depends on X86

Really? What stops me from soldering this onto one of my ARM or
MIPS boards? This is not good for compile coverage either.

> +++ b/drivers/gpio/gpio-sch311x.c
(...)
> +/* Runtime registers */
> +
> +#define GP1    0x4B    /* GP10-17 data register */
> +
> +static unsigned int sch311x_gpio_conf[] = {
> +       { 0x23,         /* GP10 config register */
> +       0x24,           /* GP11 config register */
> +       0x25,           /* GP12 config register */
> +       0x26,           /* GP13 config register */
> +       0x27,           /* GP14 config register */
> +       0x29,           /* GP15 config register */
> +       0x2a,           /* GP16 config register */
> +       0x2b,           /* GP17 config register */
> +};
> +
> +#define SCH311X_GPIO_CONF_IN           0x01
> +#define SCH311X_GPIO_CONF_OUT          0x00
> +#define SCH311X_GPIO_CONF_INV          0x02
> +#define SCH311X_GPIO_CONF_OPEN_DRAIN   0x80
> +
> +static int sch311x_ioports[] = { 0x2e, 0x4e, 0x162e, 0x164e, 0x00 };

ISA-style probing, yeah I had to accept this last time ...
So no discovery on this system, no ACPI?

> +/* internal variables */
> +static struct platform_device *sch311x_gpio_pdev;
> +static struct platform_device *i2c_gpio_pdev;
> +
> +static struct {
> +       unsigned short runtime_reg;     /* Runtime Register base address */
> +       spinlock_t lock;                /* lock for io operations */
> +} sch311x_gpio_data;

This is a singleton. What happens the day someone mounts two
of these chips on a board? Please devm_kzalloc() a state container
instead, look in other GPIO drivers for examples of this.

> +static inline void sch311x_sio_enter(int sio_config_port)
> +{
> +       outb(0x55, sio_config_port);
> +}

0x55?

> +static inline void sch311x_sio_exit(int sio_config_port)
> +{
> +       outb(0xaa, sio_config_port);
> +}

0xaa?

Please #define thise magic values.

> +static int sch311x_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       unsigned char data = inb(sch311x_gpio_data.runtime_reg + GP1);
> +       return ((data >> offset) & 1);

Use this:

#include <linux/bitops.h>

return !!(data & BIT(offset));

> +static void __sch311x_gpio_set(unsigned offset, int value)
> +{
> +       unsigned char data;
> +       data = inb(sch311x_gpio_data.runtime_reg + GP1);
> +       if (value)
> +               data |= 1 << offset;

BIT(offset);

> +       else
> +               data &= ~(1 << offset);

BIT(offset);

> +       outb(data, sch311x_gpio_data.runtime_reg + GP1);
> +}

> +static int sch311x_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
> +{
> +       spin_lock(&sch311x_gpio_data.lock);

This is a sure sign that singletons are not so good.

> +static int sch311x_gpio_direction_out(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +       spin_lock(&sch311x_gpio_data.lock);
> +
> +#if 0
> +       /* configure as "push/pull": output voltage is 3.3V */
> +       outb(SCH311X_GPIO_CONF_OUT, sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[offset]);

No #if 0 code! Delete this.

> +#else
> +       /* configure as "open drain": output voltage is 5V on an unconnected PIN */
> +       outb(SCH311X_GPIO_CONF_OUT | SCH311X_GPIO_CONF_OPEN_DRAIN,
> +            sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[offset]);

So if you really only support push/pull, open drain and maybe even
open source, then it's OK to use the GPIO subsystem. If you start
doing more things, this becomes a matter of pin control.

> +#endif
> +       __sch311x_gpio_set(offset, value);
> +
> +       spin_unlock(&sch311x_gpio_data.lock);
> +       return 0;
> +}
> +
> +static struct gpio_chip sc_gpio_chip = {
> +       .label                  = "sch311x_gpio",
> +       .owner                  = THIS_MODULE,
> +       .direction_input        = sch311x_gpio_direction_in,
> +       .get                    = sch311x_gpio_get,
> +       .direction_output       = sch311x_gpio_direction_out,
> +       .set                    = sch311x_gpio_set,
> +       .can_sleep              = 0,
> +       .base                   = 0,

So what happens when there is a system with two of these
chips? How do you intend to pass a different config for another
board?

> +       .ngpio                  = 8,

This should nominally be set to however many GPIOs the
chip can actually handle, then augmented for the board.

The rest of the struct is OK.

> +static int sch311x_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       int err, i;


Here, devm_kzalloc(&pdev->dev, sizeof(container), GFP_KERNEL);

etc

> +static int __init sch311x_detect(int sio_config_port, unsigned short *addr)
> +{
> +       int err = 0, reg;
> +       unsigned short base_addr;
> +       unsigned char dev_id;
> +
> +       sch311x_sio_enter(sio_config_port);
> +
> +       /* Check device ID. We currently know about:
> +        * SCH3112 (0x7c), SCH3114 (0x7d), and SCH3116 (0x7f). */
> +       reg = sch311x_sio_inb(sio_config_port, 0x20);
> +       if (!(reg == 0x7c || reg == 0x7d || reg == 0x7f)) {
> +               err = -ENODEV;
> +               goto exit;
> +       }
> +       dev_id = reg == 0x7c ? 2 : reg == 0x7d ? 4 : 6;

This gives me the creeps but I guess I have to live with it?

> +static int __init sch311x_gpio_init(void)
> +{
> +       int err, i, found = 0;
> +       unsigned short addr = 0;
> +
> +       for (i = 0; !found && sch311x_ioports[i]; i++)
> +               if (sch311x_detect(sch311x_ioports[i], &addr) == 0)
> +                       found++;
> +
> +       if (!found)
> +               return -ENODEV;
> +
> +       sch311x_gpio_data.runtime_reg = addr;
> +
> +       err = platform_driver_register(&sch311x_gpio_driver);
> +       if (err)
> +               return err;
> +
> +       sch311x_gpio_pdev = platform_device_register_simple(DRV_NAME, addr, NULL, 0);


At the very least I want the driver split up in two files: one that
adds the GPIO driver for an arbitrary number of pins, and one
that adds this boards' configuration. You can put the latter in
drivers/platform/x86 or wherever, just not in drivers/gpio.

Maybe this leads to another interesting discussion on why
ACPI is not used for configuring the board...

Yours,
Linus Walleij

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Mika Westerberg]

On Fri, Nov 29, 2013 at 03:40:01PM +0100, Linus Walleij wrote:
> At the very least I want the driver split up in two files: one that
> adds the GPIO driver for an arbitrary number of pins, and one
> that adds this boards' configuration. You can put the latter in
> drivers/platform/x86 or wherever, just not in drivers/gpio.
> 
> Maybe this leads to another interesting discussion on why
> ACPI is not used for configuring the board...

AFAIK there are not that many systems shipping today that have ACPI 5.0
supported and you need ACPI 5.0 in order to pass GpioIo/GpioInt resources
from a device to the driver.

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Linus Walleij]

On Fri, Nov 29, 2013 at 4:02 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Nov 29, 2013 at 03:40:01PM +0100, Linus Walleij wrote:
>> At the very least I want the driver split up in two files: one that
>> adds the GPIO driver for an arbitrary number of pins, and one
>> that adds this boards' configuration. You can put the latter in
>> drivers/platform/x86 or wherever, just not in drivers/gpio.
>>
>> Maybe this leads to another interesting discussion on why
>> ACPI is not used for configuring the board...
>
> AFAIK there are not that many systems shipping today that have ACPI 5.0
> supported and you need ACPI 5.0 in order to pass GpioIo/GpioInt resources
> from a device to the driver.

Yeah, hm, hehe, maybe they should use Device Tree?

Yours,
Linus Walleij

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Bruno Randolf]

Hello Linus!

Thank you for your review. I will try to address your concerns, as my
time and knowledge permits ;)

On 11/29/2013 02:40 PM, Linus Walleij wrote:
>> +       depends on X86
> 
> Really? What stops me from soldering this onto one of my ARM or
> MIPS boards?

I suppose nothing, but I don't know. Will remove it.

> ISA-style probing, yeah I had to accept this last time ...
> So no discovery on this system, no ACPI?

The system has ACPI, but GPIOs were not working... I don't know enough
about ACPI, but if someone tells me what to look for I can try to debug it.

>> +/* internal variables */
>> +static struct platform_device *sch311x_gpio_pdev;
>> +static struct platform_device *i2c_gpio_pdev;
>> +
>> +static struct {
>> +       unsigned short runtime_reg;     /* Runtime Register base address */
>> +       spinlock_t lock;                /* lock for io operations */
>> +} sch311x_gpio_data;
> 
> This is a singleton. What happens the day someone mounts two
> of these chips on a board?

Right, this part comes from the watchdog driver for the same chip
(watchdog/sch311x_wdt.c), so I thought I can do the same. That's not a
good excuse, I know...

> Please devm_kzalloc() a state container
> instead, look in other GPIO drivers for examples of this.

Do you have a good example?

>> +static inline void sch311x_sio_enter(int sio_config_port)
>> +{
>> +       outb(0x55, sio_config_port);
>> +}
> 
> 0x55?
> 
>> +static inline void sch311x_sio_exit(int sio_config_port)
>> +{
>> +       outb(0xaa, sio_config_port);
>> +}
> 
> 0xaa?

> Please #define thise magic values.

Again, comes from watchdog/sch311x_wdt.c... will #define.

>> +static int sch311x_gpio_get(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       unsigned char data = inb(sch311x_gpio_data.runtime_reg + GP1);
>> +       return ((data >> offset) & 1);
> 
> Use this:
> 
> #include <linux/bitops.h>
> 
> return !!(data & BIT(offset));

Ah, thanks!!!

>> +#if 0
>> +       /* configure as "push/pull": output voltage is 3.3V */
>> +       outb(SCH311X_GPIO_CONF_OUT, sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[offset]);
> 
> No #if 0 code! Delete this.

OK, sorry.

>> +#else
>> +       /* configure as "open drain": output voltage is 5V on an unconnected PIN */
>> +       outb(SCH311X_GPIO_CONF_OUT | SCH311X_GPIO_CONF_OPEN_DRAIN,
>> +            sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[offset]);
> 
> So if you really only support push/pull, open drain and maybe even
> open source, then it's OK to use the GPIO subsystem. If you start
> doing more things, this becomes a matter of pin control.

It can just be configured as just push/pull or open drain. Is there any
way this can be configured within the GPIO subsystem?

> At the very least I want the driver split up in two files: one that
> adds the GPIO driver for an arbitrary number of pins, and one
> that adds this boards' configuration. You can put the latter in
> drivers/platform/x86 or wherever, just not in drivers/gpio.

OK, that makes sense.

bruno

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Linus Walleij]

On Fri, Nov 29, 2013 at 6:21 PM, Bruno Randolf <br1@einfach.org> wrote:
> [Me]

>> ISA-style probing, yeah I had to accept this last time ...
>> So no discovery on this system, no ACPI?
>
> The system has ACPI, but GPIOs were not working... I don't know enough
> about ACPI, but if someone tells me what to look for I can try to debug it.

Yeah :-/

I don't know how you do the driver/board split in x86 so
please enlighten me.

>> Please devm_kzalloc() a state container
>> instead, look in other GPIO drivers for examples of this.
>
> Do you have a good example?

Most new drivers ..
gpio-intel-mid.c
gpio-tb10x.c
gpio-em.c

>> So if you really only support push/pull, open drain and maybe even
>> open source, then it's OK to use the GPIO subsystem. If you start
>> doing more things, this becomes a matter of pin control.
>
> It can just be configured as just push/pull or open drain. Is there any
> way this can be configured within the GPIO subsystem?

Yeah that's OK. grep for GPIOF_OPEN_DRAIN,
GPIOF_OPEN_SOURCE.

Yours,
Linus Walleij

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Simon Guinot]

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

On Fri, Nov 29, 2013 at 03:40:01PM +0100, Linus Walleij wrote:
> Maybe this leads to another interesting discussion on why
> ACPI is not used for configuring the board...

Hi Linus,

I am currently working on the next Seagate NAS boards (x86-based) and
again a Super-I/O will be used to provide some GPIO lines :) The device
should be an IT8732, which may be supported by the driver gpio-it8761e.

Based on our previous experience, I am currently pushing the board
manufacturer (which also provides the BIOS) to get an get entry for
the Super-I/O device in the ACPI tables.

Let me say it is not a easy task and I am still not sure to succeed...
Clearly the guys don't understand why they have to do that because the
other OS are used to identify the device via the I/O registers.

Regards,

Simon

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

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Linus Walleij]

On Fri, Nov 29, 2013 at 9:37 PM, Simon Guinot <simon.guinot@sequanux.org> wrote:
> On Fri, Nov 29, 2013 at 03:40:01PM +0100, Linus Walleij wrote:
>> Maybe this leads to another interesting discussion on why
>> ACPI is not used for configuring the board...
>
> Hi Linus,
>
> I am currently working on the next Seagate NAS boards (x86-based) and
> again a Super-I/O will be used to provide some GPIO lines :) The device
> should be an IT8732, which may be supported by the driver gpio-it8761e.
>
> Based on our previous experience, I am currently pushing the board
> manufacturer (which also provides the BIOS) to get an get entry for
> the Super-I/O device in the ACPI tables.
>
> Let me say it is not a easy task and I am still not sure to succeed...
> Clearly the guys don't understand why they have to do that because the
> other OS are used to identify the device via the I/O registers.

OK so I report this to linux-acpi, HPA and Grant and hope that they
will get them for this "we like port I/O-probing" attitude... there is
only so much we can do.

Basically the (embedded?) x86 vendors need to get
their act together around ACPI just like the ARM vendors did with
device tree.

Only handwaving from my side, sorry... best I can do.

Yours,
Linus Walleij

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Simon Guinot]

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

On Thu, Nov 28, 2013 at 12:18:15AM +0000, Bruno Randolf wrote:
> This patch adds support for the first eight GPIOs found on the SMSC "Super I/O"
> chips SCH311x.
> 
> This chip is used on Atom based embedded boards (e.g. "Advantec MIO-2261") and
> actually has more GPIO lines, but they are not connected on the board I have
> access to.
> 
> The chip detection and I/O functions are copied from sch311x_wdt.c

Hi Bruno,

Here are few comments.

> 
> Signed-off-by: Bruno Randolf <br1@einfach.org>
> ---
>  drivers/gpio/Kconfig        |  10 ++
>  drivers/gpio/Makefile       |   1 +
>  drivers/gpio/gpio-sch311x.c | 315 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 326 insertions(+)
>  create mode 100644 drivers/gpio/gpio-sch311x.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index b6ed304..3d6bc8d 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -237,6 +237,16 @@ config GPIO_SAMSUNG
>  	  Legacy GPIO support. Use only for platforms without support for
>  	  pinctrl.
>  
> +config GPIO_SCH311X
> +	tristate "SMSC SCH311x SuperI/O GPIO"
> +	depends on X86
> +	help
> +	  Driver to enable the GPIOs found on SMSC SMSC SCH3112, SCH3114 and
> +	  SCH3116 "Super I/O" chipsets.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called gpio-sch311x.
> +
>  config GPIO_SPEAR_SPICS
>  	bool "ST SPEAr13xx SPI Chip Select as GPIO support"
>  	depends on PLAT_SPEAR
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 98e23eb..b63c11f 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_GPIO_RCAR)		+= gpio-rcar.o
>  obj-$(CONFIG_GPIO_SAMSUNG)	+= gpio-samsung.o
>  obj-$(CONFIG_ARCH_SA1100)	+= gpio-sa1100.o
>  obj-$(CONFIG_GPIO_SCH)		+= gpio-sch.o
> +obj-$(CONFIG_GPIO_SCH311X)	+= gpio-sch311x.o
>  obj-$(CONFIG_GPIO_SODAVILLE)	+= gpio-sodaville.o
>  obj-$(CONFIG_GPIO_SPEAR_SPICS)	+= gpio-spear-spics.o
>  obj-$(CONFIG_GPIO_STA2X11)	+= gpio-sta2x11.o
> diff --git a/drivers/gpio/gpio-sch311x.c b/drivers/gpio/gpio-sch311x.c
> new file mode 100644
> index 0000000..1ca7450
> --- /dev/null
> +++ b/drivers/gpio/gpio-sch311x.c
> @@ -0,0 +1,315 @@
> +/*
> + * GPIO driver for the SMSC SCH311x Super-I/O chips
> + *
> + * Copyright (C) 2013 Bruno Randolf <br1@einfach.org>
> + *
> + * SuperIO functions and chip detection:
> + * (c) Copyright 2008 Wim Van Sebroeck <wim@iguana.be>.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c-gpio.h>
> +
> +#define DRV_NAME	"gpio-sch311x"
> +
> +/*
> + * Note: This driver only supports the first 8 GPIOs of the chip.
> + */
> +
> +
> +/* Runtime registers */
> +
> +#define GP1	0x4B	/* GP10-17 data register */
> +
> +static unsigned int sch311x_gpio_conf[] = {
> +	{ 0x23,		/* GP10 config register */
        ^
Does this even compile ?

> +	0x24,		/* GP11 config register */
> +	0x25,		/* GP12 config register */
> +	0x26,		/* GP13 config register */
> +	0x27,		/* GP14 config register */
> +	0x29,		/* GP15 config register */
> +	0x2a,		/* GP16 config register */
> +	0x2b,		/* GP17 config register */
> +};
> +
> +#define SCH311X_GPIO_CONF_IN		0x01
> +#define SCH311X_GPIO_CONF_OUT		0x00
> +#define SCH311X_GPIO_CONF_INV		0x02
> +#define SCH311X_GPIO_CONF_OPEN_DRAIN	0x80
> +
> +static int sch311x_ioports[] = { 0x2e, 0x4e, 0x162e, 0x164e, 0x00 };
> +
> +/* internal variables */
> +static struct platform_device *sch311x_gpio_pdev;
> +static struct platform_device *i2c_gpio_pdev;
> +
> +static struct {
> +	unsigned short runtime_reg;	/* Runtime Register base address */
> +	spinlock_t lock;		/* lock for io operations */
> +} sch311x_gpio_data;
> +
> +
> +/*
> + *	Super-IO functions
> + */
> +
> +static inline void sch311x_sio_enter(int sio_config_port)
> +{

As a Super-I/O is a multifunction device, I think you should request
the Super-I/O port here, using request_muxed_region() for example.

This is a collaborative way to prevent against concurrent accesses.

> +	outb(0x55, sio_config_port);
> +}
> +
> +static inline void sch311x_sio_exit(int sio_config_port)
> +{
> +	outb(0xaa, sio_config_port);

Then, you may need to release the Super-I/O port here.

> +}
> +
> +static inline int sch311x_sio_inb(int sio_config_port, int reg)
> +{
> +	outb(reg, sio_config_port);
> +	return inb(sio_config_port + 1);
> +}
> +
> +static inline void sch311x_sio_outb(int sio_config_port, int reg, int val)
> +{
> +	outb(reg, sio_config_port);
> +	outb(val, sio_config_port + 1);
> +}
> +
> +
> +/*
> + *	GPIO functions
> + */
> +
> +static int sch311x_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +	unsigned char data = inb(sch311x_gpio_data.runtime_reg + GP1);
> +	return ((data >> offset) & 1);
> +}
> +
> +static void __sch311x_gpio_set(unsigned offset, int value)
> +{
> +	unsigned char data;
> +	data = inb(sch311x_gpio_data.runtime_reg + GP1);
> +	if (value)
> +		data |= 1 << offset;
> +	else
> +		data &= ~(1 << offset);
> +	outb(data, sch311x_gpio_data.runtime_reg + GP1);
> +}
> +
> +static void sch311x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	spin_lock(&sch311x_gpio_data.lock);
> +
> +	 __sch311x_gpio_set(offset, value);
> +
> +	spin_unlock(&sch311x_gpio_data.lock);
> +}
> +
> +static int sch311x_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
> +{
> +	spin_lock(&sch311x_gpio_data.lock);
> +
> +	outb(SCH311X_GPIO_CONF_IN, sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[offset]);
> +
> +	spin_unlock(&sch311x_gpio_data.lock);
> +	return 0;
> +}
> +
> +static int sch311x_gpio_direction_out(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	spin_lock(&sch311x_gpio_data.lock);
> +
> +#if 0
> +	/* configure as "push/pull": output voltage is 3.3V */
> +	outb(SCH311X_GPIO_CONF_OUT, sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[offset]);
> +#else
> +	/* configure as "open drain": output voltage is 5V on an unconnected PIN */
> +	outb(SCH311X_GPIO_CONF_OUT | SCH311X_GPIO_CONF_OPEN_DRAIN,
> +	     sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[offset]);
> +#endif
> +	__sch311x_gpio_set(offset, value);
> +
> +	spin_unlock(&sch311x_gpio_data.lock);
> +	return 0;
> +}
> +
> +static struct gpio_chip sc_gpio_chip = {
> +	.label			= "sch311x_gpio",
> +	.owner			= THIS_MODULE,
> +	.direction_input	= sch311x_gpio_direction_in,
> +	.get			= sch311x_gpio_get,
> +	.direction_output	= sch311x_gpio_direction_out,
> +	.set			= sch311x_gpio_set,
> +	.can_sleep		= 0,
> +	.base			= 0,
> +	.ngpio			= 8,
> +};
> +
> +static int sch311x_gpio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int err, i;
> +
> +	spin_lock_init(&sch311x_gpio_data.lock);
> +
> +	if (!request_region(sch311x_gpio_data.runtime_reg + GP1, 1, DRV_NAME)) {
> +		dev_err(dev, "Failed to request region 0x%04x-0x%04x.\n",
> +			sch311x_gpio_data.runtime_reg + GP1,
> +			sch311x_gpio_data.runtime_reg + GP1);
> +		err = -EBUSY;
> +		goto exit;
> +	}
> +
> +	for (i=0; i < ARRAY_SIZE(sch311x_gpio_conf); i++) {
> +		if (!request_region(sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[i], 1, DRV_NAME)) {
> +			dev_err(dev, "Failed to request region 0x%04x-0x%04x.\n",
> +				sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[i],
> +				sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[i]);
> +			err = -EBUSY;
> +			goto exit_release_region;
> +		}

It seems to me that all the sch311x_gpio_conf registers are contiguous,
except for address 0x27. Then, maybe you can transform this loop into a
single request_region() call ?

The same remark applies to the release_region() calls.

> +	}
> +
> +	sc_gpio_chip.dev = &pdev->dev;
> +	err = gpiochip_add(&sc_gpio_chip);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "Could not register gpiochip, %d\n", err);
> +		goto exit_release_region2;
> +	}
> +
> +	dev_info(dev, "SMSC SCH311x GPIO registered.\n");
> +
> +	return 0;
> +
> +exit_release_region2:
> +	for (i=0; i < ARRAY_SIZE(sch311x_gpio_conf); i++) {
> +		release_region(sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[i], 1);
> +	}
> +exit_release_region:
> +	release_region(sch311x_gpio_data.runtime_reg + GP1, 1);
> +	sch311x_gpio_data.runtime_reg = 0;
> +exit:
> +	return err;
> +}
> +
> +static int sch311x_gpio_remove(struct platform_device *pdev)
> +{
> +	int i;
> +
> +	release_region(sch311x_gpio_data.runtime_reg + GP1, 1);
> +	for (i=0; i < ARRAY_SIZE(sch311x_gpio_conf); i++) {
> +		release_region(sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[i], 1);
> +	}
> +	sch311x_gpio_data.runtime_reg = 0;
> +
> +	return gpiochip_remove(&sc_gpio_chip);
> +}
> +
> +static struct platform_driver sch311x_gpio_driver = {
> +	.driver.name	= DRV_NAME,
> +	.driver.owner	= THIS_MODULE,
> +	.probe		= sch311x_gpio_probe,
> +	.remove		= sch311x_gpio_remove,
> +};
> +
> +
> +/*
> + *	Init & exit routines
> + */
> +
> +static int __init sch311x_detect(int sio_config_port, unsigned short *addr)
> +{
> +	int err = 0, reg;
> +	unsigned short base_addr;
> +	unsigned char dev_id;
> +
> +	sch311x_sio_enter(sio_config_port);

Don't you need to check the vendor ID here ?

> +
> +	/* Check device ID. We currently know about:
> +	 * SCH3112 (0x7c), SCH3114 (0x7d), and SCH3116 (0x7f). */
> +	reg = sch311x_sio_inb(sio_config_port, 0x20);
> +	if (!(reg == 0x7c || reg == 0x7d || reg == 0x7f)) {
> +		err = -ENODEV;
> +		goto exit;
> +	}
> +	dev_id = reg == 0x7c ? 2 : reg == 0x7d ? 4 : 6;

Outch :)

> +
> +	/* Select logical device A (runtime registers) */
> +	sch311x_sio_outb(sio_config_port, 0x07, 0x0a);

How behaves the Super-I/O if an another driver selects a different
logical device later ? Is that OK ?

> +
> +	/* Check if Logical Device Register is currently active */
> +	if ((sch311x_sio_inb(sio_config_port, 0x30) & 0x01) == 0)
> +		pr_info("Seems that LDN 0x0a is not active...\n");

Maybe you can enable it ?

Regards,

Simon

> +
> +	/* Get the base address of the runtime registers */
> +	base_addr = (sch311x_sio_inb(sio_config_port, 0x60) << 8) |
> +			   sch311x_sio_inb(sio_config_port, 0x61);
> +	if (!base_addr) {
> +		pr_err("Base address not set\n");
> +		err = -ENODEV;
> +		goto exit;
> +	}
> +	*addr = base_addr;
> +
> +	pr_info("Found an SMSC SCH311%d chip at 0x%04x\n", dev_id, base_addr);
> +
> +exit:
> +	sch311x_sio_exit(sio_config_port);
> +	return err;
> +}
> +
> +static int __init sch311x_gpio_init(void)
> +{
> +	int err, i, found = 0;
> +	unsigned short addr = 0;
> +
> +	for (i = 0; !found && sch311x_ioports[i]; i++)
> +		if (sch311x_detect(sch311x_ioports[i], &addr) == 0)
> +			found++;
> +
> +	if (!found)
> +		return -ENODEV;
> +
> +	sch311x_gpio_data.runtime_reg = addr;
> +
> +	err = platform_driver_register(&sch311x_gpio_driver);
> +	if (err)
> +		return err;
> +
> +	sch311x_gpio_pdev = platform_device_register_simple(DRV_NAME, addr, NULL, 0);
> +
> +	if (IS_ERR(sch311x_gpio_pdev)) {
> +		err = PTR_ERR(sch311x_gpio_pdev);
> +		goto unreg_platform_driver;
> +	}
> +
> +	return 0;
> +
> +unreg_platform_driver:
> +	platform_driver_unregister(&sch311x_gpio_driver);
> +	return err;
> +}
> +
> +static void __exit sch311x_gpio_exit(void)
> +{
> +	platform_device_unregister(sch311x_gpio_pdev);
> +	platform_driver_unregister(&sch311x_gpio_driver);
> +}
> +
> +module_init(sch311x_gpio_init);
> +module_exit(sch311x_gpio_exit);
> +
> +MODULE_AUTHOR("Bruno Randolf <br1@einfach.org>");
> +MODULE_DESCRIPTION("SMSC SCH311x GPIO Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:gpio-sch311x");
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Bruno Randolf]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Simon and all!

Thanks for your feedback, but please keep in mind that I am not
working for any chip manufacturer or board vendor. I'm just a user who
wants to share his work, and I believe in 99% of the cases only one
single "super i/o" chip will be in a given system, and 99% of the
users will not use more than the first 8 GPIOs. However I'm going to
try to support all available GPIOs on this chip, but I have the
following problem:

The chip has the following GPIOs:

10-17			8 continous GPIOs, OK
21, 22, 27		20 and 23-26 not available
30-34, 36, 37		35 not available
40, 42, 44-47		41, 43 not available
GP50-57			8 continous GPIOs, OK
GP60-67			8 continous GPIOs, OK

The datasheet is available at:

http://www.smsc.com/Products/PC_System_and_I-O_Controllers/Embedded_I-O_Products/LPC-Based_Embedded_I-O/SCH3112_SCH3114_SCH3116

So I'd register 6 gpio_chip structures with a different base, but how
should I handle these "holes"? Via gpio_chip.request? Or return error
in direction_input and direction_output?

On 11/29/2013 06:56 PM, Simon Guinot wrote:
>> +static unsigned int sch311x_gpio_conf[] = { +	{ 0x23,		/* GP10
>> config register */
> ^ Does this even compile ?

Sorry, a mistake crept in.

>> +static inline void sch311x_sio_enter(int sio_config_port) +{
> 
> As a Super-I/O is a multifunction device, I think you should
> request the Super-I/O port here, using request_muxed_region() for
> example.
> 
> This is a collaborative way to prevent against concurrent
> accesses.

Ok, thanks for this info.

> It seems to me that all the sch311x_gpio_conf registers are
> contiguous, except for address 0x27. Then, maybe you can transform
> this loop into a single request_region() call ?

It's except 0x27, and then when I add more GPIO lines the registers
get even more non-contiguous... I could manually check which areas can
be combined, but I think it's not worth it...

>> +	sch311x_sio_enter(sio_config_port);
> 
> Don't you need to check the vendor ID here ?

How?

>> +	dev_id = reg == 0x7c ? 2 : reg == 0x7d ? 4 : 6;
> 
> Outch :)

Well, again, I have copied this part from watchdog/sch311x_wdt.c

>> + +	/* Select logical device A (runtime registers) */ +
>> sch311x_sio_outb(sio_config_port, 0x07, 0x0a);
> 
> How behaves the Super-I/O if an another driver selects a different 
> logical device later ? Is that OK ?

I'm not sure but I believe so because we just got the base address of
the logical device in the next lines.

>> + +	/* Check if Logical Device Register is currently active */ +
>> if ((sch311x_sio_inb(sio_config_port, 0x30) & 0x01) == 0) +
>> pr_info("Seems that LDN 0x0a is not active...\n");
> 
> Maybe you can enable it ?

Again, not sure, but I assume the BIOS should enable/disable logical
devices it based on what is connected and in use?

brin

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)

iQEcBAEBAgAGBQJSnIBvAAoJEE06nu8QSdWB9kAIAIgf1Js2aLHYyaZ+R0LdvQjw
Jcjbi1Ci3Q5eO69xwkl18sxCdqK4xhQOEPzSWD/DLwu0+oHL+5kXpnnNLrPXKJ9y
F/MIrtEz0RUD5siWvq97VL1OmfIaHUdhh4rz1Of93HkDMIyOPlV0fDyGYypyCk/c
0N2CVdgvt+O7KwmR+/fAAuNoi6dWtdX1Ilh3gOiF2b87SSpBgZxB/2Hi9e7GRXUD
g2A1f7mhHbhecqvu1FAxt12xEVUa7Yq5Lzt84IlL722Hk2cYJI011WRaBbaLtN5R
kVDKnLTfvtL9B4tdiDbV7LTCJ38JZMk4tWl4xKzjRcQsgbBTjP0dTZcoZrwBarE=
=9KaL
-----END PGP SIGNATURE-----

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Simon Guinot]

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

On Mon, Dec 02, 2013 at 12:43:27PM +0000, Bruno Randolf wrote:
> Hi Simon and all!

Hi Bruno,

> 
> Thanks for your feedback, but please keep in mind that I am not
> working for any chip manufacturer or board vendor. I'm just a user who
> wants to share his work, and I believe in 99% of the cases only one
> single "super i/o" chip will be in a given system, and 99% of the
> users will not use more than the first 8 GPIOs. However I'm going to
> try to support all available GPIOs on this chip, but I have the
> following problem:
> 
> The chip has the following GPIOs:
> 
> 10-17			8 continous GPIOs, OK
> 21, 22, 27		20 and 23-26 not available
> 30-34, 36, 37		35 not available
> 40, 42, 44-47		41, 43 not available
> GP50-57			8 continous GPIOs, OK
> GP60-67			8 continous GPIOs, OK
> 
> The datasheet is available at:
> 
> http://www.smsc.com/Products/PC_System_and_I-O_Controllers/Embedded_I-O_Products/LPC-Based_Embedded_I-O/SCH3112_SCH3114_SCH3116
> 
> So I'd register 6 gpio_chip structures with a different base, but how
> should I handle these "holes"? Via gpio_chip.request? Or return error
> in direction_input and direction_output?
> 
> On 11/29/2013 06:56 PM, Simon Guinot wrote:
> >> +static unsigned int sch311x_gpio_conf[] = { +	{ 0x23,		/* GP10
> >> config register */
> > ^ Does this even compile ?
> 
> Sorry, a mistake crept in.
> 
> >> +static inline void sch311x_sio_enter(int sio_config_port) +{
> > 
> > As a Super-I/O is a multifunction device, I think you should
> > request the Super-I/O port here, using request_muxed_region() for
> > example.
> > 
> > This is a collaborative way to prevent against concurrent
> > accesses.
> 
> Ok, thanks for this info.
> 
> > It seems to me that all the sch311x_gpio_conf registers are
> > contiguous, except for address 0x27. Then, maybe you can transform
> > this loop into a single request_region() call ?
> 
> It's except 0x27, and then when I add more GPIO lines the registers
> get even more non-contiguous... I could manually check which areas can
> be combined, but I think it's not worth it...
> 
> >> +	sch311x_sio_enter(sio_config_port);
> > 
> > Don't you need to check the vendor ID here ?
> 
> How?

Well, I can't find any vendor ID register in the datasheet.

> 
> >> +	dev_id = reg == 0x7c ? 2 : reg == 0x7d ? 4 : 6;
> > 
> > Outch :)
> 
> Well, again, I have copied this part from watchdog/sch311x_wdt.c
> 
> >> + +	/* Select logical device A (runtime registers) */ +
> >> sch311x_sio_outb(sio_config_port, 0x07, 0x0a);
> > 
> > How behaves the Super-I/O if an another driver selects a different 
> > logical device later ? Is that OK ?
> 
> I'm not sure but I believe so because we just got the base address of
> the logical device in the next lines.

Yes, it looks correct.

Note that this base address is shared between several drivers:
watchdog/sch311x_wdt, hwmon/dme1737 and gpio/gpio-sch311x.c.
The same logical device (0xa) is used.

Moreover some offsets against this base address are also shared. For
example, 0x47 is a "valid" offset for both the GPIO and the watchdog
drivers. I guess that the correct usage for this pin depends on how the
hardware is wired. Unfortunately, there is no way for the drivers to
guess that.

Did you had the opportunity to run all this drivers together ?

Simon

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

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Bruno Randolf]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/05/2013 04:56 PM, Simon Guinot wrote:
> Note that this base address is shared between several drivers: 
> watchdog/sch311x_wdt, hwmon/dme1737 and gpio/gpio-sch311x.c. The
> same logical device (0xa) is used.
> 
> Moreover some offsets against this base address are also shared.
> For example, 0x47 is a "valid" offset for both the GPIO and the
> watchdog drivers. I guess that the correct usage for this pin
> depends on how the hardware is wired. Unfortunately, there is no
> way for the drivers to guess that.
> 
> Did you had the opportunity to run all this drivers together ?

My access to the board is limited, but I had a chance to try it.

dme1737 doesn't load because there is another resource conflict:

# modprobe dme1737
FATAL: Error inserting dme1737
(/lib/modules/3.13.0-rc1-wl+/kernel/drivers/hwmon/dme1737.ko): Device
or resource busy

[  181.109897] ACPI Warning: 0x00000670-0x00000671 SystemIO conflicts
with Region \_SB_.PCI0.LPCB.RNTR 1 (20130927/utaddress-251)
[  181.109921] ACPI: If an ACPI driver is available for this device,
you should use it instead of the native driver

I can load and use sch311x_wdt and gpio-sch311x at the same time,
because they reserve different memory regions:

/proc/ioports:
  0647-0647 : sch311x_wdt
  064b-0650 : gpio-sch311x
  0665-0668 : sch311x_wdt

Only when I'd try to request GPIO 60 (offset 0x47) I obviously get a
conflict:

# echo 60 > /sys/class/gpio/export
- -bash: echo: write error: Device or resource busy

[  334.776859] gpio-sch311x gpio-sch311x: Failed to request region 0x0647.

I think this can be expected...

bruno

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)

iQEcBAEBAgAGBQJSpupkAAoJEE06nu8QSdWBvXYH/jE9AG5YtidekEPJS7REYLNJ
hrAgykvnthDSACSPp6jLZnA4Bh80EG5DZGJ0PwtmXmR701+cOfcqlwwFvsfvHgXk
YHxihi+niwZ8p04njwAk3sWPtODprbcboB+G4USjoVeZwDInKx8gLgiKHho7akbR
PRht6cWm6iwX7UeE1/1/BfSUHisi8L0OBCditDLBApEcw094UXxP1juqQimXz2xV
Tver8mRkOYUl7q0VT7j6WIqIaw2yq+a4QEbjoSarc3YwbYuEMYzOyevI61uczhej
Sq7sXYvkTcos2k8dQMr33oM89uJ+hgBOja9Ir2tInR1ewDjLnbzCtV+n5Q8SJLg=
=E2V6
-----END PGP SIGNATURE-----

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Simon Guinot]

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

On Tue, Dec 10, 2013 at 10:18:12AM +0000, Bruno Randolf wrote:
> On 12/05/2013 04:56 PM, Simon Guinot wrote:
> > Note that this base address is shared between several drivers: 
> > watchdog/sch311x_wdt, hwmon/dme1737 and gpio/gpio-sch311x.c. The
> > same logical device (0xa) is used.
> > 
> > Moreover some offsets against this base address are also shared.
> > For example, 0x47 is a "valid" offset for both the GPIO and the
> > watchdog drivers. I guess that the correct usage for this pin
> > depends on how the hardware is wired. Unfortunately, there is no
> > way for the drivers to guess that.
> > 
> > Did you had the opportunity to run all this drivers together ?
> 
> My access to the board is limited, but I had a chance to try it.
> 
> dme1737 doesn't load because there is another resource conflict:
> 
> # modprobe dme1737
> FATAL: Error inserting dme1737
> (/lib/modules/3.13.0-rc1-wl+/kernel/drivers/hwmon/dme1737.ko): Device
> or resource busy
> 
> [  181.109897] ACPI Warning: 0x00000670-0x00000671 SystemIO conflicts
> with Region \_SB_.PCI0.LPCB.RNTR 1 (20130927/utaddress-251)
> [  181.109921] ACPI: If an ACPI driver is available for this device,
> you should use it instead of the native driver

You can enforce the driver loading with the kernel parameter
acpi_enforce_resources={lax|no}.

> 
> I can load and use sch311x_wdt and gpio-sch311x at the same time,
> because they reserve different memory regions:
> 
> /proc/ioports:
>   0647-0647 : sch311x_wdt
>   064b-0650 : gpio-sch311x
>   0665-0668 : sch311x_wdt
> 
> Only when I'd try to request GPIO 60 (offset 0x47) I obviously get a
> conflict:
> 
> # echo 60 > /sys/class/gpio/export
> -bash: echo: write error: Device or resource busy
> 
> [  334.776859] gpio-sch311x gpio-sch311x: Failed to request region 0x0647.
> 
> I think this can be expected...

Yes indeed. I guess this kind of devices with common resources could
really make use of a mfd driver.

Regards,

Simon

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

Re: [PATCH] gpio: add GPIO support for SMSC SCH311x

[Bruno Randolf]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/10/2013 11:44 AM, Simon Guinot wrote:
>> dme1737 doesn't load because there is another resource conflict:
>> 
>> # modprobe dme1737 FATAL: Error inserting dme1737 
>> (/lib/modules/3.13.0-rc1-wl+/kernel/drivers/hwmon/dme1737.ko):
>> Device or resource busy
>> 
>> [  181.109897] ACPI Warning: 0x00000670-0x00000671 SystemIO
>> conflicts with Region \_SB_.PCI0.LPCB.RNTR 1
>> (20130927/utaddress-251) [  181.109921] ACPI: If an ACPI driver
>> is available for this device, you should use it instead of the
>> native driver
> 
> You can enforce the driver loading with the kernel parameter 
> acpi_enforce_resources={lax|no}.

Thank you! Yes, this way I can load it and get sensor data. There are
no resource conflicts:

# cat /proc/ioports |grep "sch\|dme"
  0647-0647 : sch311x_wdt
  064b-0650 : gpio-sch311x
  0665-0668 : sch311x_wdt
  0670-0671 : dme1737
    0670-0671 : dme1737

> Yes indeed. I guess this kind of devices with common resources
> could really make use of a mfd driver.

Maybe, but I'm not going to do the work...

bruno

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)

iQEcBAEBAgAGBQJSpwa2AAoJEE06nu8QSdWBpfoH/3ste3Bn4YixePdu9CMKqaFq
jQEu6MsDm7j96Iq9sk9DMyropcqeyZd0vtM9AlTVYPQCLR4fFU7oEa+HDrFa1OUz
TjphDkgSFl4hHXxirHH4GP9YadBDwCfw/6EF8B/LasdHmM4OeBA8ucFRx/LFbWcH
IfPCCgCz55DizXLvDcEvEUkdgD5nEwXSR4GKjX5djR7+dRNAvSl1Yg8rB9BXw/1p
T/CO6V5N/2US0cV1t/Hqql2+r0ZlrYRwm4e/pnieGxtdivWsQN+BBBpVgRKRhri0
oHi5MV7ZBw9vx2Y70rV48S6CdUzk0j3j6+WQDl4khtjLDjg1VH4e6QsfXf1dK8Q=
=fYLG
-----END PGP SIGNATURE-----