On Thu, 12 Apr 2012 20:28:55 -0400, Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx> wrote:
> From: Jerome Oufella <jerome.oufella@xxxxxxxxxxxxxxxxxxxx>
>
> Signed-off-by: Jerome Oufella <jerome.oufella@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx>
> ---
> MAINTAINERS | 2 +
> arch/x86/include/asm/ts5500.h | 62 ++++++++
Why the separate header file? What will use these defines? I
normally expect driver-specific defines to be in the driver .c file
directly; particularly for things like gpio drivers which should be a
generic interface that doesn't need to export symbols.
> drivers/gpio/Kconfig | 7 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-ts5500.c | 347 +++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 419 insertions(+)
> create mode 100644 arch/x86/include/asm/ts5500.h
> create mode 100644 drivers/gpio/gpio-ts5500.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b126129..b7b5d95 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6624,6 +6624,8 @@ TECHNOLOGIC SYSTEMS TS-5500 MACHINE SUPPORT
> M: Savoir-faire Linux Inc. <kernel@xxxxxxxxxxxxxxxxxxxx>
> S: Maintained
> F: arch/x86/platform/ts5500/
> +F: arch/x86/include/asm/ts5500.h
> +F: drivers/gpio/gpio-ts5500.c
>
> TEGRA SUPPORT
> M: Colin Cross <ccross@xxxxxxxxxxx>
> diff --git a/arch/x86/include/asm/ts5500.h b/arch/x86/include/asm/ts5500.h
> new file mode 100644
> index 0000000..baf136c
> --- /dev/null
> +++ b/arch/x86/include/asm/ts5500.h
> @@ -0,0 +1,62 @@
> +/*
> + * Technologic Systems TS-5500 GPIO (DIO) definitions
> + *
> + * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
> + * Jerome Oufella <jerome.oufella@xxxxxxxxxxxxxxxxxxxx>
> + *
> + * 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.
> + */
> +
> +#ifndef _TS5500_H
> +#define _TS5500_H
> +
> +#define TS5500_DIO1_0 0
> +#define TS5500_DIO1_1 1
> +#define TS5500_DIO1_2 2
> +#define TS5500_DIO1_3 3
> +#define TS5500_DIO1_4 4
> +#define TS5500_DIO1_5 5
> +#define TS5500_DIO1_6 6
> +#define TS5500_DIO1_7 7
> +#define TS5500_DIO1_8 8
> +#define TS5500_DIO1_9 9
> +#define TS5500_DIO1_10 10
> +#define TS5500_DIO1_11 11
> +#define TS5500_DIO1_12 12
> +#define TS5500_DIO1_13 13
> +
> +#define TS5500_DIO2_0 14
> +#define TS5500_DIO2_1 15
> +#define TS5500_DIO2_2 16
> +#define TS5500_DIO2_3 17
> +#define TS5500_DIO2_4 18
> +#define TS5500_DIO2_5 19
> +#define TS5500_DIO2_6 20
> +#define TS5500_DIO2_7 21
> +#define TS5500_DIO2_8 22
> +#define TS5500_DIO2_9 23
> +#define TS5500_DIO2_10 24
> +#define TS5500_DIO2_11 25
> +/* #define TS5500_DIO2_12 - Keep commented out as it simply doesn't exist. */
> +#define TS5500_DIO2_13 26
> +
> +#define TS5500_LCD_0 27
> +#define TS5500_LCD_1 28
> +#define TS5500_LCD_2 29
> +#define TS5500_LCD_3 30
> +#define TS5500_LCD_4 31
> +#define TS5500_LCD_5 32
> +#define TS5500_LCD_6 33
> +#define TS5500_LCD_7 34
> +#define TS5500_LCD_EN 35
> +#define TS5500_LCD_RS 36
> +#define TS5500_LCD_WR 37
> +
> +/* Lines that can trigger IRQs */
> +#define TS5500_DIO1_13_IRQ 7
> +#define TS5500_DIO2_13_IRQ 6
> +#define TS5500_LCD_RS_IRQ 1
> +
> +#endif /* _TS5500_H */
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index edadbda..a19b0f3 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -319,6 +319,13 @@ config GPIO_TPS65912
> help
> This driver supports TPS65912 gpio chip
>
> +config GPIO_TS5500
> + tristate "TS-5500 GPIOs"
> + depends on TS5500
> + help
> + This driver supports the DIO headers for GPIO usage on the Technologic
> + Systems TS-5500 platform.
> +
> config GPIO_TWL4030
> tristate "TWL4030, TWL5030, and TPS659x0 GPIOs"
> depends on TWL4030_CORE
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 007f54b..30cbd03 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -56,6 +56,7 @@ obj-$(CONFIG_GPIO_TIMBERDALE) += gpio-timberdale.o
> obj-$(CONFIG_ARCH_DAVINCI_TNETV107X) += gpio-tnetv107x.o
> obj-$(CONFIG_GPIO_TPS65910) += gpio-tps65910.o
> obj-$(CONFIG_GPIO_TPS65912) += gpio-tps65912.o
> +obj-$(CONFIG_GPIO_TS5500) += gpio-ts5500.o
> obj-$(CONFIG_GPIO_TWL4030) += gpio-twl4030.o
> obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o
> obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o
> diff --git a/drivers/gpio/gpio-ts5500.c b/drivers/gpio/gpio-ts5500.c
> new file mode 100644
> index 0000000..01f34d8
> --- /dev/null
> +++ b/drivers/gpio/gpio-ts5500.c
> @@ -0,0 +1,347 @@
> +/*
> + * GPIO (DIO) driver for Technologic Systems TS-5500
> + *
> + * TS-5500 board has 38 GPIOs referred to as DIOs in the product's literature.
> + *
> + * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
> + * Jerome Oufella <jerome.oufella@xxxxxxxxxxxxxxxxxxxx>
> + * Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx>
> + *
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <asm/ts5500.h>
> +
> +static int dio1_irq = 1;
> +module_param(dio1_irq, int, 0644);
> +MODULE_PARM_DESC(dio1_irq, "Enable usage of IRQ7 for any DIO1 line"
> + " (default 1)");
> +
> +static int dio2_irq;
> +module_param(dio2_irq, int, 0644);
> +MODULE_PARM_DESC(dio2_irq, "Enable usage of IRQ6 for any DIO2 line"
> + " (default 0)");
> +
> +static int lcd_irq;
> +module_param(lcd_irq, int, 0644);
> +MODULE_PARM_DESC(lcd_irq, "Enable usage of IRQ1 for any LCD line"
> + " (default 0)");
> +
> +static int use_lcdio;
> +module_param(use_lcdio, int, 0644);
> +MODULE_PARM_DESC(use_lcdio, "Enable usage of the LCD header for DIO operation"
> + " (default 0)");
> +
> +static DEFINE_SPINLOCK(gpio_lock);
> +
> +static inline void port_bit_set(u8 addr, int bit)
> +{
> + u8 var;
> +
> + var = inb(addr);
> + var |= (1 << bit);
> + outb(var, addr);
> +}
> +
> +static inline void port_bit_clear(u8 addr, int bit)
> +{
> + u8 var;
> +
> + var = inb(addr);
> + var &= ~(1 << bit);
> + outb(var, addr);
> +}
> +
> +/* "DIO" line to IO port mapping table for line's value */
> +static const unsigned long line_to_port_map[] = {
> + 0x7B, 0x7B, 0x7B, 0x7B, 0x7B, 0x7B, 0x7B, 0x7B, /* DIO1_[0-7] */
> + 0x7C, 0x7C, 0x7C, 0x7C, 0x7C, 0x7C, /* DIO1_[8-13] */
> + 0x7E, 0x7E, 0x7E, 0x7E, 0x7E, 0x7E, 0x7E, 0x7E, /* DIO2_[0-7] */
> + 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, 0x7F, /* DIO2_[8-13] */
> + 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, /* LCD_[0-7] */
> + 0x73, 0x73, 0x73 /* LCD_{EN,RS,WR} */
> +};
> +
> +/* "DIO" line to IO port's bit map for line's value */
> +static const int line_to_bit_map[] = {
> + 0, 1, 2, 3, 4, 5, 6, 7, /* DIO1_[0-7] */
> + 0, 1, 2, 3, 4, 5, /* DIO1_[8-13] */
> + 0, 1, 2, 3, 4, 5, 6, 7, /* DIO2_[0-7] */
> + 0, 1, 2, 3, 4, 5, /* DIO2_[8-13] */
> + 0, 1, 2, 3, 4, 5, 6, 7, /* LCD_[0-7] */
> + 0, 7, 6 /* LCD_{EN,RS,WR} */
> +};
> +
> +/* "DIO" line's direction control mapping table */
> +static const unsigned long line_to_dir_map[] = {
> + 0x7A, 0x7A, 0x7A, 0x7A, 0x7A, 0x7A, 0x7A, 0x7A, /* DIO1_[0-7] */
> + 0x7A, 0x7A, 0x7A, 0x7A, 0, 0, /* DIO1_[8-13] */
> + 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, /* DIO2_[0-7] */
> + 0x7D, 0x7D, 0x7D, 0x7D, 0, 0, /* DIO2_[8-13] */
> + 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, 0x7D, /* LCD_[0-7] */
> + 0, 0, 0 /* LCD_{EN,RS,WR} */
> +};
> +
> +/* "DIO" line's direction control bit-mapping table */
> +static const int line_to_dir_bit_map[] = {
> + 0, 0, 0, 0, 1, 1, 1, 1, /* DIO1_[0-7] */
> + 5, 5, 5, 5, -1, -1, /* DIO1_[8-13] */
> + 0, 0, 0, 0, 1, 1, 1, 1, /* DIO2_[0-7] */
> + 5, 5, 5, 5, -1, -1, /* DIO2_[8-13] */
> + 2, 2, 2, 2, 3, 3, 3, 3, /* LCD_[0-7] */
> + -1, -1, -1 /* LCD_{EN,RS,WR} */
> +};
Splitting up this data into 4 arrays seems odd. I think it would be
better to define a single table with a tuple for each line.
> +
> +static int ts5500_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> + unsigned long dir_reg, flags;
> + int dir_bit;
> +
> + /* Some lines cannot be configured as input */
> + if (offset == TS5500_LCD_EN)
> + return -ENXIO;
> +
> + dir_reg = line_to_dir_map[offset];
> + dir_bit = line_to_dir_bit_map[offset];
> +
> + spin_lock_irqsave(&gpio_lock, flags);
> + port_bit_clear(dir_reg, dir_bit);
> + spin_unlock_irqrestore(&gpio_lock, flags);
> +
> + return 0;
> +}
> +
> +static int ts5500_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> + unsigned long ioaddr;
> + int bitno;
> +
> + ioaddr = line_to_port_map[offset];
> + bitno = line_to_bit_map[offset];
> +
> + return (inb(ioaddr) >> bitno) & 0x1;
> +}
> +
> +static int ts5500_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
> + int val)
> +{
> + unsigned long dir_reg, ioaddr, flags;
> + int dir_bit, bitno;
> +
> + /* Some lines cannot be configured as output */
> + switch (offset) {
> + case TS5500_DIO1_12:
> + case TS5500_DIO1_13:
> + case TS5500_DIO2_13:
> + case TS5500_LCD_RS:
> + case TS5500_LCD_WR:
> + return -ENXIO;
This would also do well in a 'flags' field in the per-line data table
as I described above.
> + default:
> + break;
> + }
> +
> + dir_reg = line_to_dir_map[offset];
> + dir_bit = line_to_dir_bit_map[offset];
> + ioaddr = line_to_port_map[offset];
> + bitno = line_to_bit_map[offset];
> +
> + spin_lock_irqsave(&gpio_lock, flags);
> + port_bit_set(dir_reg, dir_bit);
> + if (val)
> + port_bit_set(ioaddr, bitno);
> + else
> + port_bit_clear(ioaddr, bitno);
> + spin_unlock_irqrestore(&gpio_lock, flags);
> +
> + return 0;
> +}
> +
> +static void ts5500_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
> +{
> + int bitno;
> + unsigned long ioaddr, flags;
> +
> + ioaddr = line_to_port_map[offset];
> + bitno = line_to_bit_map[offset];
> +
> + spin_lock_irqsave(&gpio_lock, flags);
> + if (val)
> + port_bit_set(ioaddr, bitno);
> + else
> + port_bit_clear(ioaddr, bitno);
> + spin_unlock_irqrestore(&gpio_lock, flags);
> +}
> +
> +static int ts5500_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> + /* Only a few lines are IRQ-capable */
> + if (offset == TS5500_DIO1_13)
> + return TS5500_DIO1_13_IRQ;
> + if (offset == TS5500_DIO2_13)
> + return TS5500_DIO2_13_IRQ;
> + if (offset == TS5500_LCD_RS)
> + return TS5500_LCD_RS_IRQ;
> +
> + /* IRQ line may be bridged with another DIO line from the same header */
> + if (dio1_irq && offset >= TS5500_DIO1_0 && offset < TS5500_DIO1_13)
> + return TS5500_DIO1_13_IRQ;
> + if (dio2_irq && offset >= TS5500_DIO2_0 && offset < TS5500_DIO2_13)
> + return TS5500_DIO2_13_IRQ;
> + if (lcd_irq && offset >= TS5500_LCD_0 && offset <= TS5500_LCD_WR)
> + return TS5500_LCD_RS_IRQ;
Also candidate for putting in the table.
> +
> + return -ENXIO;
> +}
> +
> +static struct gpio_chip ts5500_gpio_chip = {
> + .label = "ts5500-gpio",
> + .owner = THIS_MODULE,
> + .direction_input = ts5500_gpio_direction_input,
> + .get = ts5500_gpio_get,
> + .direction_output = ts5500_gpio_direction_output,
> + .set = ts5500_gpio_set,
> + .to_irq = ts5500_gpio_to_irq,
> + .base = TS5500_DIO1_0,
Don't hard code the GPIO base. It should be dynamically assigned by
using '-1' here.
> +};
> +
> +static void ts5500_gpio_release(struct device *dev)
> +{
> + /* noop */
> +}
> +
> +static struct platform_device ts5500_gpio_pdev = {
> + .name = "ts5500-gpio",
> + .id = -1,
> + .dev = {
> + .release = ts5500_gpio_release,
> + },
> +};
> +
> +static int __devinit ts5500_gpio_probe(struct platform_device *pdev)
> +{
> + int ret;
> + unsigned long flags;
> +
> + if (pdev == NULL)
> + return -ENODEV;
> +
> + if (!request_region(0x7A, 3, "ts5500-gpio-DIO1")) {
> + dev_err(&pdev->dev, "failed to request I/O port 0x7A-7C\n");
> + return -EBUSY;
> + }
> +
> + if (!request_region(0x7D, 3, "ts5500-gpio-DIO2")) {
> + dev_err(&pdev->dev, "failed to request I/O port 0x7D-7F\n");
> + ret = -EBUSY;
> + goto release_dio1;
> + }
> +
> + if (use_lcdio && !request_region(0x72, 2, "ts5500-gpio-LCD")) {
> + dev_err(&pdev->dev, "failed to request I/O port 0x72-73\n");
> + ret = -EBUSY;
> + goto release_dio2;
> + }
> +
> + if (use_lcdio)
> + ts5500_gpio_chip.ngpio = TS5500_LCD_WR + 1;
> + else
> + ts5500_gpio_chip.ngpio = TS5500_DIO2_13 + 1;
> +
> + ret = gpiochip_add(&ts5500_gpio_chip);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register the gpio chip\n");
> + goto release_lcd;
> + }
> +
> + /* Enable IRQ generation */
> + spin_lock_irqsave(&gpio_lock, flags);
> + port_bit_set(0x7A, 7); /* DIO1_13 on IRQ7 */
> + port_bit_set(0x7D, 7); /* DIO2_13 on IRQ6 */
> + if (use_lcdio) {
> + port_bit_clear(0x7D, 4); /* LCD header usage as DIO */
> + port_bit_set(0x7D, 6); /* LCD_RS on IRQ1 */
> + }
> + spin_unlock_irqrestore(&gpio_lock, flags);
> +
> + return 0;
> +
> +release_lcd:
> + if (use_lcdio)
> + release_region(0x72, 2);
> +release_dio2:
> + release_region(0x7D, 3);
> +release_dio1:
> + release_region(0x7A, 3);
> +
> + return ret;
> +}
> +
> +static int __devexit ts5500_gpio_remove(struct platform_device *pdev)
> +{
> + int ret;
> + unsigned long flags;
> +
> + /* Disable IRQ generation */
> + spin_lock_irqsave(&gpio_lock, flags);
> + port_bit_clear(0x7A, 7);
> + port_bit_clear(0x7D, 7);
> + if (use_lcdio)
> + port_bit_clear(0x7D, 6);
> + spin_unlock_irqrestore(&gpio_lock, flags);
> +
> + release_region(0x7A, 3);
> + release_region(0x7D, 3);
> + if (use_lcdio)
> + release_region(0x72, 2);
> +
> + ret = gpiochip_remove(&ts5500_gpio_chip);
> + if (ret)
> + dev_err(&pdev->dev, "failed to remove the gpio chip\n");
> +
> + return ret;
> +}
> +
> +static struct platform_driver ts5500_gpio_driver = {
> + .driver = {
> + .name = "ts5500-gpio",
> + .owner = THIS_MODULE,
> + },
> + .probe = ts5500_gpio_probe,
> + .remove = __devexit_p(ts5500_gpio_remove),
> +};
> +
> +static int __init ts5500_gpio_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&ts5500_gpio_driver);
> + if (ret)
> + return ret;
> +
> + ret = platform_device_register(&ts5500_gpio_pdev);
> + if (ret) {
> + platform_driver_unregister(&ts5500_gpio_driver);
> + return ret;
> + }
As already discussed.... Bleah!! :-)
Please move the device registration to platform setup code.
> +
> + return 0;
> +}
> +module_init(ts5500_gpio_init);
> +
> +static void __exit ts5500_gpio_exit(void)
> +{
> + platform_driver_unregister(&ts5500_gpio_driver);
> + platform_device_unregister(&ts5500_gpio_pdev);
> +}
> +module_exit(ts5500_gpio_exit);
module_platform_driver() will replace some of the boilerplate.
g.
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
