Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1236898 > unrolled thread

[PATCH] GPIO: Add GPIO support for the ACCES 104-IDIO-16

Back to article view | Back to linux.kernel

Contents

  [PATCH] GPIO: Add GPIO support for the ACCES 104-IDIO-16 William Breathitt Gray <vilhelm.gray@gmail.com> - 2015-10-01 04:00 +0200
    Re: [PATCH] GPIO: Add GPIO support for the ACCES 104-IDIO-16 Linus Walleij <linus.walleij@linaro.org> - 2015-10-05 10:30 +0200
      Re: [PATCH] GPIO: Add GPIO support for the ACCES 104-IDIO-16 Bjorn Helgaas <bhelgaas@google.com> - 2015-10-06 00:50 +0200
        Re: [PATCH] GPIO: Add GPIO support for the ACCES 104-IDIO-16 William Breathitt Gray <vilhelm.gray@gmail.com> - 2015-10-06 01:20 +0200
          Re: [PATCH] GPIO: Add GPIO support for the ACCES 104-IDIO-16 Linus Walleij <linus.walleij@linaro.org> - 2015-10-06 09:20 +0200
      Re: [PATCH] GPIO: Add GPIO support for the ACCES 104-IDIO-16 William Breathitt Gray <vilhelm.gray@gmail.com> - 2015-10-06 01:50 +0200
        Re: [PATCH] GPIO: Add GPIO support for the ACCES 104-IDIO-16 Linus Walleij <linus.walleij@linaro.org> - 2015-10-06 09:20 +0200

#1236898 — [PATCH] GPIO: Add GPIO support for the ACCES 104-IDIO-16

The ACCES 104-IDIO-16 family of PC/104 utility boards feature 16
optically isolated inputs and 16 optically isolated FET solid state
outputs. This driver provides GPIO support for these 32 channels of
digital I/O. Change-of-State detection interrupts are not supported.

GPIO 0-15 correspond to digital outputs 0-15, while GPIO 16-31
correspond to digital inputs 0-15.

Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
 drivers/gpio/Kconfig            |  14 ++++
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-104-idio-16.c | 154 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 169 insertions(+)
 create mode 100644 drivers/gpio/gpio-104-idio-16.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8949b3f..6309071 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -684,6 +684,20 @@ config GPIO_SX150X
 
 endmenu
 
+menu "ISA GPIO expanders"
+
+menuconfig GPIO_104_IDIO_16
+	tristate "ACCES 104-IDIO-16 GPIO support"
+	help
+	  Enables GPIO support for the ACCES 104-IDIO-16 family.
+
+config 104_IDIO_16_BASE
+	hex "ACCES 104-IDIO-16 base address"
+	depends on GPIO_104_IDIO_16
+	default 0x000
+
+endmenu
+
 menu "MFD GPIO expanders"
 
 config GPIO_ADP5520
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index f79a7c4..6f2fea5 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
 # Device drivers. Generally keep list sorted alphabetically
 obj-$(CONFIG_GPIO_GENERIC)	+= gpio-generic.o
 
+obj-$(CONFIG_GPIO_104_IDIO_16)	+= gpio-104-idio-16.o
 obj-$(CONFIG_GPIO_74X164)	+= gpio-74x164.o
 obj-$(CONFIG_GPIO_74XX_MMIO)	+= gpio-74xx-mmio.o
 obj-$(CONFIG_GPIO_ADNP)		+= gpio-adnp.o
diff --git a/drivers/gpio/gpio-104-idio-16.c b/drivers/gpio/gpio-104-idio-16.c
new file mode 100644
index 0000000..5e3e89d
--- /dev/null
+++ b/drivers/gpio/gpio-104-idio-16.c
@@ -0,0 +1,154 @@
+/*
+ * GPIO driver for the ACCES 104-IDIO-16 family
+ * Copyright (C) 2015 William Breathitt Gray
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 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.
+ */
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/spinlock.h>
+
+struct a_104_idio_16_gpio {
+	struct gpio_chip chip;
+	spinlock_t lock;
+	unsigned base;
+	unsigned data;
+};
+
+#define to_a104i16gp(chip) container_of(chip, struct a_104_idio_16_gpio, chip)
+
+static void __exit a_104_idio_16_exit(void);
+static int a_104_idio_16_gpio_direction_input(struct gpio_chip *chip,
+	unsigned offset);
+static int a_104_idio_16_gpio_direction_output(struct gpio_chip *chip,
+	unsigned offset, int value);
+static int a_104_idio_16_gpio_get(struct gpio_chip *chip, unsigned offset);
+static void a_104_idio_16_gpio_set(struct gpio_chip *chip, unsigned offset,
+	int value);
+static int __init a_104_idio_16_init(void);
+
+static const unsigned A_104_IDIO_16_EXTENT = 8;
+
+static struct a_104_idio_16_gpio gp = {
+	.chip = {
+		.label = "104-IDIO-16 GPIO",
+		.owner = THIS_MODULE,
+		.base = -1,
+		.ngpio = 32,
+		.direction_input = a_104_idio_16_gpio_direction_input,
+		.direction_output = a_104_idio_16_gpio_direction_output,
+		.get = a_104_idio_16_gpio_get,
+		.set = a_104_idio_16_gpio_set
+	},
+	.base = CONFIG_104_IDIO_16_BASE
+};
+
+static void __exit a_104_idio_16_exit(void)
+{
+	pr_info("104-idio-16: Exiting 104-idio-16 module\n");
+
+	gpiochip_remove(&gp.chip);
+	release_region(CONFIG_104_IDIO_16_BASE, A_104_IDIO_16_EXTENT);
+}
+
+static int a_104_idio_16_gpio_direction_input(struct gpio_chip *chip,
+	unsigned offset)
+{
+	return 0;
+}
+
+static int a_104_idio_16_gpio_direction_output(struct gpio_chip *chip,
+	unsigned offset, int value)
+{
+	chip->set(chip, offset, value);
+	return 0;
+}
+
+static int a_104_idio_16_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct a_104_idio_16_gpio *a104i16gp = to_a104i16gp(chip);
+	const unsigned BIT_MASK = 1U << (offset-16);
+
+	if (offset < 16)
+		return 0;
+
+	if (offset < 24)
+		return !!(inb(a104i16gp->base + 1) & BIT_MASK);
+
+	return !!(inb(a104i16gp->base + 5) & (BIT_MASK>>8));
+}
+
+static void a_104_idio_16_gpio_set(struct gpio_chip *chip, unsigned offset,
+	int value)
+{
+	struct a_104_idio_16_gpio *a104i16gp = to_a104i16gp(chip);
+	const unsigned BIT_MASK = 1U << offset;
+	unsigned long flags;
+
+	if (offset > 15)
+		return;
+
+	spin_lock_irqsave(&a104i16gp->lock, flags);
+
+	if (value)
+		a104i16gp->data |= BIT_MASK;
+	else
+		a104i16gp->data &= ~BIT_MASK;
+
+	if (offset > 7)
+		outb(~a104i16gp->data >> 8, a104i16gp->base + 4);
+	else
+		outb(~a104i16gp->data, a104i16gp->base);
+
+	spin_unlock_irqrestore(&a104i16gp->lock, flags);
+}
+
+static int __init a_104_idio_16_init(void)
+{
+	int err = 0;
+
+	pr_info("104-idio-16: Initializing 104-idio-16 module\n");
+
+	if (!request_region(CONFIG_104_IDIO_16_BASE, A_104_IDIO_16_EXTENT,
+		"104-idio-16")) {
+		pr_err("104-idio-16: Unable to lock 104-idio-16 port addresses (0x%X-0x%X)\n",
+			CONFIG_104_IDIO_16_BASE,
+			CONFIG_104_IDIO_16_BASE + A_104_IDIO_16_EXTENT);
+		err = -EBUSY;
+		goto out_lock_a_104_idio_16_port;
+	}
+
+	spin_lock_init(&gp.lock);
+
+	pr_info("104-idio-16: 104-IDIO-16 GPIO detected\n");
+	err = gpiochip_add(&gp.chip);
+	if (err) {
+		pr_err("104-idio-16: GPIO registering failed (%d)\n", err);
+		goto out_gpio_register;
+	}
+
+	return 0;
+
+out_gpio_register:
+	release_region(CONFIG_104_IDIO_16_BASE, A_104_IDIO_16_EXTENT);
+out_lock_a_104_idio_16_port:
+	return err;
+}
+
+module_init(a_104_idio_16_init);
+module_exit(a_104_idio_16_exit);
+
+MODULE_AUTHOR("William Breathitt Gray <vilhelm.gray@gmail.com>");
+MODULE_DESCRIPTION("ACCES 104-IDIO-16 GPIO driver");
+MODULE_LICENSE("GPL");
-- 
2.4.9

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1239346

On Thu, Oct 1, 2015 at 3:58 AM, William Breathitt Gray
<vilhelm.gray@gmail.com> wrote:

> The ACCES 104-IDIO-16 family of PC/104 utility boards

Sounds like a PC104 keyboard :D

> feature 16
> optically isolated inputs and 16 optically isolated FET solid state
> outputs. This driver provides GPIO support for these 32 channels of
> digital I/O. Change-of-State detection interrupts are not supported.

So it has IRQ support but it's not supported yet I take it.

> GPIO 0-15 correspond to digital outputs 0-15, while GPIO 16-31
> correspond to digital inputs 0-15.
>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>

> +menu "ISA GPIO expanders"

Interesting submenu. I guess it is proper to have so OK.

Add:
depends on PCI

As they all will need that, I guess?

> +menuconfig GPIO_104_IDIO_16
> +       tristate "ACCES 104-IDIO-16 GPIO support"
> +       help
> +         Enables GPIO support for the ACCES 104-IDIO-16 family.
> +
> +config 104_IDIO_16_BASE
> +       hex "ACCES 104-IDIO-16 base address"
> +       depends on GPIO_104_IDIO_16
> +       default 0x000

This can't be right. PCI devices have their config space for a reason
I'm told. On other platforms we use device tree or ACPI to set this
up but PCI is either hotplug or wrong I think.

The driver is full of ISA style inb/outb stuff, I get all confused. Why
is this not using the PCI infrastructure?

Involving Björn Helgås for a comment on this.

> +#include <linux/gpio.h>

Only
#include <linux/gpio/driver.h>

> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/spinlock.h>
> +
> +struct a_104_idio_16_gpio {
> +       struct gpio_chip chip;
> +       spinlock_t lock;
> +       unsigned base;

Isn't this void __iomem *base?

> +       unsigned data;
> +};

kerneldoc this.

> +static void __exit a_104_idio_16_exit(void);
> +static int a_104_idio_16_gpio_direction_input(struct gpio_chip *chip,
> +       unsigned offset);
> +static int a_104_idio_16_gpio_direction_output(struct gpio_chip *chip,
> +       unsigned offset, int value);
> +static int a_104_idio_16_gpio_get(struct gpio_chip *chip, unsigned offset);
> +static void a_104_idio_16_gpio_set(struct gpio_chip *chip, unsigned offset,
> +       int value);
> +static int __init a_104_idio_16_init(void);

Re-arrange code to avoid forward declarations please.

> +static const unsigned A_104_IDIO_16_EXTENT = 8;

Looks like it could be a #define A_104_IDIO_16_EXTENT 8

> +static struct a_104_idio_16_gpio gp = {
> +       .chip = {
> +               .label = "104-IDIO-16 GPIO",
> +               .owner = THIS_MODULE,
> +               .base = -1,
> +               .ngpio = 32,
> +               .direction_input = a_104_idio_16_gpio_direction_input,
> +               .direction_output = a_104_idio_16_gpio_direction_output,
> +               .get = a_104_idio_16_gpio_get,
> +               .set = a_104_idio_16_gpio_set
> +       },
> +       .base = CONFIG_104_IDIO_16_BASE
> +};

So if you put this *below* the functions you need not forward-declare them.

> +static void __exit a_104_idio_16_exit(void)
> +{
> +       pr_info("104-idio-16: Exiting 104-idio-16 module\n");
> +
> +       gpiochip_remove(&gp.chip);

Where is that &gp.chip? Not in this file. Nor should you use any globals.

Does this driver even compile?

> +static int a_104_idio_16_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct a_104_idio_16_gpio *a104i16gp = to_a104i16gp(chip);
> +       const unsigned BIT_MASK = 1U << (offset-16);
> +
> +       if (offset < 16)
> +               return 0;

Always return 0, why? Is that really correct?

> +static int __init a_104_idio_16_init(void)
> +       spin_lock_init(&gp.lock);
> +       err = gpiochip_add(&gp.chip);

This gp global again. Read
Documentation/driver-model/design-patterns.txt
Please.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240013

Hi William and Linus,

On Mon, Oct 5, 2015 at 3:29 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Oct 1, 2015 at 3:58 AM, William Breathitt Gray
> <vilhelm.gray@gmail.com> wrote:
>
>> The ACCES 104-IDIO-16 family of PC/104 utility boards
>
> Sounds like a PC104 keyboard :D
>
>> feature 16
>> optically isolated inputs and 16 optically isolated FET solid state
>> outputs. This driver provides GPIO support for these 32 channels of
>> digital I/O. Change-of-State detection interrupts are not supported.
>
> So it has IRQ support but it's not supported yet I take it.
>
>> GPIO 0-15 correspond to digital outputs 0-15, while GPIO 16-31
>> correspond to digital inputs 0-15.
>>
>> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
>
>> +menu "ISA GPIO expanders"
>
> Interesting submenu. I guess it is proper to have so OK.
>
> Add:
> depends on PCI
>
> As they all will need that, I guess?

Why do they need PCI?  I don't see any PCI interfaces used here.

>> +menuconfig GPIO_104_IDIO_16
>> +       tristate "ACCES 104-IDIO-16 GPIO support"
>> +       help
>> +         Enables GPIO support for the ACCES 104-IDIO-16 family.
>> +
>> +config 104_IDIO_16_BASE
>> +       hex "ACCES 104-IDIO-16 base address"
>> +       depends on GPIO_104_IDIO_16
>> +       default 0x000
>
> This can't be right. PCI devices have their config space for a reason
> I'm told. On other platforms we use device tree or ACPI to set this
> up but PCI is either hotplug or wrong I think.
>
> The driver is full of ISA style inb/outb stuff, I get all confused. Why
> is this not using the PCI infrastructure?

I'm really not familiar with PC/104, but wikipedia claims it only uses
ISA.  Apparently PCI/104-Plus, PCI-104, PCI/104-Express, and PCIe/104
*do* have PCI and/or PCIe, but it looks like the base PC/104 does not.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240024

On 10/05/2015 06:47 PM, Bjorn Helgaas wrote:
> I'm really not familiar with PC/104, but wikipedia claims it only uses
> ISA.  Apparently PCI/104-Plus, PCI-104, PCI/104-Express, and PCIe/104
> *do* have PCI and/or PCIe, but it looks like the base PC/104 does not.
> 
> Bjorn
> 

This is correct: PC/104 is only ISA. The ACCES 104-IDIO-16 board is a
PC/104 stackable GPIO board, whose port address is configured via
physical jumper switches on the board; it is not "hotpluggable." For
this reason, the CONFIG_104_IDIO_16_BASE option should be set to the
same port address physically configured on the board.

- William
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240193

On Tue, Oct 6, 2015 at 1:11 AM, William Breathitt Gray
<vilhelm.gray@gmail.com> wrote:
> On 10/05/2015 06:47 PM, Bjorn Helgaas wrote:

>> I'm really not familiar with PC/104, but wikipedia claims it only uses
>> ISA.  Apparently PCI/104-Plus, PCI-104, PCI/104-Express, and PCIe/104
>> *do* have PCI and/or PCIe, but it looks like the base PC/104 does not.
>
> This is correct: PC/104 is only ISA.

OK then let's drop that confusing "PCI GPIO expanders" menu.

Maybe "ISA GPIO cards" or similar is more apropriate.

> The ACCES 104-IDIO-16 board is a
> PC/104 stackable GPIO board, whose port address is configured via
> physical jumper switches on the board; it is not "hotpluggable." For
> this reason, the CONFIG_104_IDIO_16_BASE option should be set to the
> same port address physically configured on the board.

I think the pattern in the kernel is to support such configuration for ISA
is with module parameters, not with compile-time constants. So switch
that base to use a module parameter.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240034

On 10/05/2015 04:29 AM, Linus Walleij wrote:
>> +struct a_104_idio_16_gpio {
>> +       struct gpio_chip chip;
>> +       spinlock_t lock;
>> +       unsigned base;
> 
> Isn't this void __iomem *base?

The 'base' member is used to hold the I/O port base address passed to the
inb/outb functions for port-mapped I/O operations. Since the addresses are
not dereferenced, I don't believe an __iomem pointer would be correct.

>> +static const unsigned A_104_IDIO_16_EXTENT = 8;
> 
> Looks like it could be a #define A_104_IDIO_16_EXTENT 8

I used a const variable for the benefit of type-safety; I assumed the
compiler would optimize it. What is the advantage of a #define constant?

>> +static void __exit a_104_idio_16_exit(void)
>> +{
>> +       pr_info("104-idio-16: Exiting 104-idio-16 module\n");
>> +
>> +       gpiochip_remove(&gp.chip);
> 
> Where is that &gp.chip? Not in this file. Nor should you use any globals.
> 

I agree that using a global data structure isn't good practice, but I'm not
sure how else to expose the gpio_chip structure in the respective module
_init and _exit functions since they have void parameter lists. Would it be
more appropriate to use the platform device API in this situation to avoid
the global data structure?

>> +static int a_104_idio_16_gpio_get(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       struct a_104_idio_16_gpio *a104i16gp = to_a104i16gp(chip);
>> +       const unsigned BIT_MASK = 1U << (offset-16);
>> +
>> +       if (offset < 16)
>> +               return 0;
> 
> Always return 0, why? Is that really correct?

GPIO 0-15 are output-only. The kerneldoc for 'struct gpio_chip' states that
for output signals the get function should return the value actually sensed,
or zero. Since I cannot sense the output signals, I return zero in these cases.
Is this behavior correct?

- William

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240185

On Tue, Oct 6, 2015 at 1:40 AM, William Breathitt Gray
<vilhelm.gray@gmail.com> wrote:
> On 10/05/2015 04:29 AM, Linus Walleij wrote:
>>> +struct a_104_idio_16_gpio {
>>> +       struct gpio_chip chip;
>>> +       spinlock_t lock;
>>> +       unsigned base;
>>
>> Isn't this void __iomem *base?
>
> The 'base' member is used to hold the I/O port base address passed to the
> inb/outb functions for port-mapped I/O operations. Since the addresses are
> not dereferenced, I don't believe an __iomem pointer would be correct.

You're right, sorry I was confused.

>>> +static const unsigned A_104_IDIO_16_EXTENT = 8;
>>
>> Looks like it could be a #define A_104_IDIO_16_EXTENT 8
>
> I used a const variable for the benefit of type-safety; I assumed the
> compiler would optimize it. What is the advantage of a #define constant?

Usually we use #define's for compile-time constants.

>>> +static void __exit a_104_idio_16_exit(void)
>>> +{
>>> +       pr_info("104-idio-16: Exiting 104-idio-16 module\n");
>>> +
>>> +       gpiochip_remove(&gp.chip);
>>
>> Where is that &gp.chip? Not in this file. Nor should you use any globals.
>
> I agree that using a global data structure isn't good practice, but I'm not
> sure how else to expose the gpio_chip structure in the respective module
> _init and _exit functions since they have void parameter lists. Would it be
> more appropriate to use the platform device API in this situation to avoid
> the global data structure?

It depends where your device is spawn. If there is a natural place to
instantiate the platform device like from a MFD or PCI driver or something
then yes.

>>> +static int a_104_idio_16_gpio_get(struct gpio_chip *chip, unsigned offset)
>>> +{
>>> +       struct a_104_idio_16_gpio *a104i16gp = to_a104i16gp(chip);
>>> +       const unsigned BIT_MASK = 1U << (offset-16);
>>> +
>>> +       if (offset < 16)
>>> +               return 0;
>>
>> Always return 0, why? Is that really correct?
>
> GPIO 0-15 are output-only. The kerneldoc for 'struct gpio_chip' states that
> for output signals the get function should return the value actually sensed,
> or zero. Since I cannot sense the output signals, I return zero in these cases.
> Is this behavior correct?

I guess, we recently augmented the gpiolib core so you can actually
return an error code as -ENODEV here. No strong preference though.
Maybe I should have...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web