diff mbox

GPIO: Add GPIO support for the ACCES 104-IDIO-16

Message ID 20151001015832.GA11317@sophia
State New
Headers show

Commit Message

William Breathitt Gray Oct. 1, 2015, 1:58 a.m. UTC 
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

Comments

kernel test robot Oct. 2, 2015, 11:15 a.m. UTC | #1 
Hi William,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: microblaze-allmodconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=microblaze 

All error/warnings (new ones prefixed by >>):

   ERROR: "isa_io_base" [drivers/net/wan/lmc/lmc.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/wan/farsync.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/irda/vlsi_ir.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/irda/donauboe.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/hamradio/yam.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/hamradio/baycom_ser_hdx.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/hamradio/baycom_ser_fdx.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/ethernet/via/via-rhine.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/ethernet/ti/tlan.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/ethernet/sis/sis900.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/ethernet/sis/sis190.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/ethernet/intel/e1000/e1000.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/ethernet/hp/hp100.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/ethernet/dec/tulip/de4x5.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/ethernet/amd/pcnet32.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/ethernet/8390/ne2k-pci.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/ethernet/8390/8390.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/ethernet/3com/3c59x.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/can/sja1000/sja1000_isa.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/can/cc770/cc770_isa.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/arcnet/com90xx.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/arcnet/com90io.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/arcnet/com20020.ko] undefined!
   ERROR: "isa_io_base" [drivers/net/arcnet/com20020-pci.ko] undefined!
   ERROR: "isa_io_base" [drivers/misc/altera-stapl/altera-stapl.ko] undefined!
   ERROR: "isa_io_base" [drivers/message/fusion/mptbase.ko] undefined!
   ERROR: "isa_io_base" [drivers/media/radio/radio-maxiradio.ko] undefined!
   ERROR: "isa_io_base" [drivers/media/pci/dm1105/dm1105.ko] undefined!
   ERROR: "isa_io_base" [drivers/leds/leds-ot200.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hysdn/hysdn.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hisax/hisax_fcpcipnp.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hisax/hisax.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hisax/hfc4s8s_l1.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hardware/mISDN/w6692.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hardware/mISDN/speedfax.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hardware/mISDN/netjet.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hardware/mISDN/mISDNinfineon.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hardware/mISDN/hfcmulti.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hardware/mISDN/avmfritz.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hardware/eicon/divas.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hardware/avm/b1pci.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hardware/avm/b1dma.ko] undefined!
   ERROR: "isa_io_base" [drivers/isdn/hardware/avm/b1.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/touchscreen/mk712.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/serio/pcips2.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/tmdc.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/sidewinder.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/joydump.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/interact.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/guillemot.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/grip_mp.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/grip.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/gf2k.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/cobra.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/analog.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/adi.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/joystick/a3d.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/gameport/ns558.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/gameport/lightning.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/gameport/gameport.ko] undefined!
   ERROR: "isa_io_base" [drivers/input/gameport/fm801-gp.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-viapro.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-via.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-sis96x.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-sis630.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-sis5595.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-piix4.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-parport-light.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-nforce2.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-isch.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-i801.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-amd8111.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-amd756.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-ali15x3.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-ali1563.ko] undefined!
   ERROR: "isa_io_base" [drivers/i2c/busses/i2c-ali1535.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/w83627hf.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/w83627ehf.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/vt8231.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/vt1211.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/via686a.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/smsc47m1.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/smsc47b397.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/sis5595.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/sch56xx-common.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/pc87427.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/pc87360.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/nct6775.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/nct6683.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/it87.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/f71882fg.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/f71805f.ko] undefined!
   ERROR: "isa_io_base" [drivers/hwmon/dme1737.ko] undefined!
   ERROR: "isa_io_base" [drivers/gpu/drm/qxl/qxl.ko] undefined!
   ERROR: "isa_io_base" [drivers/gpu/drm/cirrus/cirrus.ko] undefined!
   ERROR: "isa_io_base" [drivers/gpu/drm/bochs/bochs-drm.ko] undefined!
   ERROR: "isa_io_base" [drivers/gpio/gpio-vx855.ko] undefined!
   ERROR: "isa_io_base" [drivers/gpio/gpio-ts5500.ko] undefined!
   ERROR: "isa_io_base" [drivers/gpio/gpio-sch311x.ko] undefined!
   ERROR: "isa_io_base" [drivers/gpio/gpio-amd8111.ko] undefined!
>> ERROR: "isa_io_base" [drivers/gpio/gpio-104-idio-16.ko] undefined!
   ERROR: "isa_io_base" [drivers/char/tpm/tpm_atmel.ko] undefined!
   ERROR: "isa_io_base" [drivers/char/ipmi/ipmi_si.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/on26.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/on20.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/ktti.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/kbic.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/frpw.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/friq.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/fit3.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/fit2.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/epia.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/epat.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/dstr.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/comm.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/bpck6.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/bpck.ko] undefined!
   ERROR: "isa_io_base" [drivers/block/paride/aten.ko] undefined!
   ERROR: "isa_io_base" [drivers/atm/zatm.ko] undefined!
   ERROR: "isa_io_base" [drivers/atm/horizon.ko] undefined!
   ERROR: "isa_io_base" [drivers/atm/ambassador.ko] undefined!
   ERROR: "isa_io_base" [drivers/ata/pata_sc1200.ko] undefined!
   ERROR: "isa_io_base" [drivers/ata/pata_pdc202xx_old.ko] undefined!
   ERROR: "isa_io_base" [drivers/ata/pata_optidma.ko] undefined!
   ERROR: "isa_io_base" [drivers/ata/pata_legacy.ko] undefined!
   ERROR: "isa_io_base" [drivers/ata/pata_hpt3x2n.ko] undefined!
   ERROR: "isa_io_base" [drivers/ata/pata_hpt37x.ko] undefined!
   ERROR: "isa_io_base" [drivers/ata/pata_cypress.ko] undefined!
   ERROR: "isa_io_base" [drivers/ata/pata_cmd64x.ko] undefined!
   ERROR: "isa_io_base" [drivers/ata/pata_artop.ko] undefined!
   ERROR: "isa_io_base" [drivers/ata/libata.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Linus Walleij Oct. 5, 2015, 8:29 a.m. UTC | #2 
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-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Oct. 5, 2015, 10:47 p.m. UTC | #3 
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-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
William Breathitt Gray Oct. 5, 2015, 11:11 p.m. UTC | #4 
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-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
William Breathitt Gray Oct. 5, 2015, 11:40 p.m. UTC | #5 
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-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 6, 2015, 7:14 a.m. UTC | #6 
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-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 6, 2015, 7:19 a.m. UTC | #7 
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-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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");