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

[Linus Walleij <linus.walleij@linaro.org>]

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