Re: [PATCH v4] serial: 8250: add gpio support to exar

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jan 18, 2016 at 2:10 PM, Sudip Mukherjee
<sudipm.mukherjee@xxxxxxxxx> wrote:
> Exar XR17V352/354/358 chips have 16 multi-purpose inputs/outputs which
> can be controlled using gpio interface.
> Add support to use these pins.

Looks better, but…

>  drivers/gpio/Kconfig                |   7 ++
>  drivers/gpio/Makefile               |   1 +

Why is it here?

>  drivers/tty/serial/8250/8250_gpio.c | 243 ++++++++++++++++++++++++++++++++++++



>  drivers/tty/serial/8250/8250_pci.c  |  41 +++++-
>  4 files changed, 291 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/tty/serial/8250/8250_gpio.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index b18bea0..1294d8d 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -983,6 +983,13 @@ config GPIO_SODAVILLE
>         help
>           Say Y here to support Intel Sodaville GPIO.
>
> +config GPIO_EXAR
> +       tristate "Support for GPIO pins on XR17V352/354/358"
> +       depends on SERIAL_8250_PCI
> +       help
> +         Selecting this option will enable handling of GPIO pins present
> +         on Exar XR17V352/354/358 chips.
> +
>  endmenu
>
>  menu "SPI GPIO expanders"
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 986dbd8..68efdb5 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_GPIO_DWAPB)      += gpio-dwapb.o
>  obj-$(CONFIG_GPIO_EM)          += gpio-em.o
>  obj-$(CONFIG_GPIO_EP93XX)      += gpio-ep93xx.o
>  obj-$(CONFIG_GPIO_ETRAXFS)     += gpio-etraxfs.o

> +obj-$(CONFIG_GPIO_EXAR)                += gpio-exar.o

And this file is exactly where?

>  obj-$(CONFIG_GPIO_F7188X)      += gpio-f7188x.o
>  obj-$(CONFIG_GPIO_GE_FPGA)     += gpio-ge.o
>  obj-$(CONFIG_GPIO_GRGPIO)      += gpio-grgpio.o
> diff --git a/drivers/tty/serial/8250/8250_gpio.c b/drivers/tty/serial/8250/8250_gpio.c
> new file mode 100644
> index 0000000..44511de
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_gpio.c

Name is too generic for the more specific case.

> @@ -0,0 +1,243 @@
> +/*
> + * GPIO driver for Exar XR17V35X chip
> + *
> + * Copyright (C) 2015 Sudip Mukherjee <sudipm.mukherjee@xxxxxxxxx>
> + *
> + * 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.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/pci.h>
> +#include <linux/gpio.h>
> +#include "8250.h"
> +
> +#define EXAR_OFFSET_MPIOLVL_LO 0x90
> +#define EXAR_OFFSET_MPIOSEL_LO 0x93
> +#define EXAR_OFFSET_MPIOLVL_HI 0x96
> +#define EXAR_OFFSET_MPIOSEL_HI 0x99
> +
> +static LIST_HEAD(exar_list);
> +static DEFINE_MUTEX(exar_mtx); /* lock while manipulating the list */
> +
> +struct exar_gpio_chip {
> +       struct gpio_chip gpio_chip;
> +       struct mutex lock;
> +       struct uart_8250_port *port;
> +       struct list_head list;
> +       int index;
> +       void __iomem *regs;
> +       char name[16];
> +};
> +
> +#define to_exar_chip(n) container_of(n, struct exar_gpio_chip, gpio_chip)
> +
> +static inline unsigned int read_exar_reg(struct exar_gpio_chip *chip,
> +                                        int offset)
> +{
> +       dev_dbg(chip->gpio_chip.dev, "%s regs=%p offset=%x\n", __func__,
> +               chip->regs, offset);
> +       return readb(chip->regs + offset);
> +}
> +
> +static inline void write_exar_reg(struct exar_gpio_chip *chip, int offset,
> +                                 int value)
> +{
> +       dev_dbg(chip->gpio_chip.dev,
> +               "%s regs=%p value=%x offset=%x\n", __func__, chip->regs,
> +               value, offset);
> +       writeb(value, chip->regs + offset);
> +}
> +
> +void xr17v35x_gpio_exit(struct uart_8250_port *port)
> +{
> +       struct exar_gpio_chip *exar_gpio, *exar_pos_e, *exar_temp_e;
> +
> +       if (!port || !port->port.private_data)
> +               return;
> +
> +       exar_gpio = port->port.private_data;
> +       mutex_lock(&exar_mtx);
> +       list_for_each_entry_safe(exar_pos_e, exar_temp_e, &exar_list, list) {
> +               if (exar_pos_e->index == exar_gpio->index) {
> +                       list_del(&exar_pos_e->list);
> +                       break;
> +               }
> +       }
> +       mutex_unlock(&exar_mtx);
> +
> +       gpiochip_remove(&exar_gpio->gpio_chip);
> +       mutex_destroy(&exar_gpio->lock);
> +       iounmap(exar_gpio->regs);
> +}
> +EXPORT_SYMBOL(xr17v35x_gpio_exit);
> +
> +static void exar_set(struct gpio_chip *chip, unsigned int reg,
> +                    unsigned int offset, int val)
> +{
> +       struct exar_gpio_chip *exar_gpio = to_exar_chip(chip);
> +       int temp;
> +
> +       mutex_lock(&exar_gpio->lock);
> +       temp = read_exar_reg(exar_gpio, reg);
> +       temp &= ~(1 << offset);
> +       temp |= val << offset;
> +       write_exar_reg(exar_gpio, reg, temp);
> +       mutex_unlock(&exar_gpio->lock);
> +}
> +
> +static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
> +                                int value)
> +{
> +       if (offset < 8)
> +               exar_set(chip, EXAR_OFFSET_MPIOSEL_LO, 0, offset);
> +       else
> +               exar_set(chip, EXAR_OFFSET_MPIOSEL_HI, 0, offset - 8);
> +       return 0;
> +}
> +
> +static int exar_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> +       if (offset < 8)
> +               exar_set(chip, EXAR_OFFSET_MPIOSEL_LO, 1, offset);
> +       else
> +               exar_set(chip, EXAR_OFFSET_MPIOSEL_HI, 1, offset - 8);
> +       return 0;
> +}
> +
> +static int exar_get(struct gpio_chip *chip, unsigned int reg)
> +{
> +       struct exar_gpio_chip *exar_gpio = to_exar_chip(chip);
> +       int value;
> +
> +       if (!exar_gpio)
> +               return -ENODEV;
> +
> +       mutex_lock(&exar_gpio->lock);
> +       value = read_exar_reg(exar_gpio, reg);
> +       mutex_unlock(&exar_gpio->lock);
> +
> +       return value;
> +}
> +
> +static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +       int value;
> +
> +       if (offset < 8) {
> +               value = exar_get(chip, EXAR_OFFSET_MPIOSEL_LO);
> +       } else {
> +               value = exar_get(chip, EXAR_OFFSET_MPIOSEL_HI);
> +               offset -= 8;
> +       }
> +
> +       if (value > 0) {
> +               value >>= offset;
> +               value &= 0x01;
> +       }
> +
> +       return value;
> +}
> +
> +static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
> +{
> +       int value;
> +
> +       if (offset < 8) {
> +               value = exar_get(chip, EXAR_OFFSET_MPIOLVL_LO);
> +       } else {
> +               value = exar_get(chip, EXAR_OFFSET_MPIOLVL_HI);
> +               offset -= 8;
> +       }
> +
> +       if (value > 0) {
> +               value >>= offset;
> +               value &= 0x01;
> +       }
> +
> +       return value;
> +}
> +
> +static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
> +                          int value)
> +{
> +       if (offset < 8)
> +               exar_set(chip, EXAR_OFFSET_MPIOLVL_LO, value, offset);
> +       else
> +               exar_set(chip, EXAR_OFFSET_MPIOLVL_HI, value, offset - 8);
> +}
> +
> +int xr17v35x_gpio_init(struct pci_dev *dev, struct uart_8250_port *port,
> +                      int idx)
> +{
> +       struct exar_gpio_chip *exar_gpio, *exar_temp;
> +       void __iomem *p;
> +       int index = 1;
> +       int ret;
> +
> +       if (idx != 0)
> +               return 0;
> +
> +       if (dev->vendor != PCI_VENDOR_ID_EXAR)
> +               return 0;
> +
> +       exar_gpio = devm_kzalloc(&dev->dev, sizeof(*exar_gpio), GFP_KERNEL);
> +       if (!exar_gpio)
> +               return -ENOMEM;
> +
> +       p = pci_ioremap_bar(dev, 0);
> +       if (!p)
> +               return -ENOMEM;
> +
> +       mutex_init(&exar_gpio->lock);
> +       INIT_LIST_HEAD(&exar_gpio->list);
> +
> +       mutex_lock(&exar_mtx);
> +       /* find the first unused index */
> +       list_for_each_entry(exar_temp, &exar_list, list) {
> +               if (exar_temp->index == index)
> +                       index++;
> +       }
> +
> +       sprintf(exar_gpio->name, "exar_gpio%d", index);
> +       exar_gpio->gpio_chip.label = exar_gpio->name;
> +       exar_gpio->gpio_chip.dev = &dev->dev;
> +       exar_gpio->gpio_chip.direction_output = exar_direction_output;
> +       exar_gpio->gpio_chip.direction_input = exar_direction_input;
> +       exar_gpio->gpio_chip.get_direction = exar_get_direction;
> +       exar_gpio->gpio_chip.get = exar_get_value;
> +       exar_gpio->gpio_chip.set = exar_set_value;
> +       exar_gpio->gpio_chip.base = -1;
> +       exar_gpio->gpio_chip.ngpio = 16;

> +       exar_gpio->gpio_chip.owner = THIS_MODULE;

Is it still needed?

> +       exar_gpio->regs = p;
> +       exar_gpio->index = index;
> +
> +       ret = gpiochip_add(&exar_gpio->gpio_chip);
> +       if (ret)
> +               goto err_destroy;
> +
> +       exar_gpio->port = port;
> +       port->port.private_data = exar_gpio;
> +
> +       list_add_tail(&exar_gpio->list, &exar_list);
> +       mutex_unlock(&exar_mtx);
> +
> +       return 0;
> +
> +err_destroy:
> +       mutex_unlock(&exar_mtx);
> +       mutex_destroy(&exar_gpio->lock);
> +       pci_iounmap(dev, p);
> +       return ret;
> +}
> +EXPORT_SYMBOL(xr17v35x_gpio_init);
> +
> +MODULE_DESCRIPTION("Exar GPIO driver");
> +MODULE_AUTHOR("Sudip Mukherjee <sudipm.mukherjee@xxxxxxxxx>");
> +MODULE_LICENSE("GPL");

MODULE_ALIAS("platform:" DRIVER_NAME);
Where DRIVER_NAME defined above as something sane.

> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 4097f3f..5c03d5c 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -1764,12 +1764,25 @@ xr17v35x_has_slave(struct serial_private *priv)
>                 (dev_id == PCI_DEVICE_ID_EXAR_XR17V8358));
>  }
>
> +static void pci_xr17v35x_exit(struct pci_dev *dev)
> +{
> +       struct serial_private *priv = pci_get_drvdata(dev);
> +       struct uart_8250_port *port = serial8250_get_port(priv->line[0]);
> +       struct platform_device *pdev = port->port.private_data;
> +
> +       if (pdev) {
> +               platform_device_unregister(pdev);
> +               port->port.private_data = NULL;
> +       }
> +}
> +
>  static int
>  pci_xr17v35x_setup(struct serial_private *priv,
>                   const struct pciserial_board *board,
>                   struct uart_8250_port *port, int idx)
>  {
>         u8 __iomem *p;
> +       int ret;
>
>         p = pci_ioremap_bar(priv->dev, 0);
>         if (p == NULL)
> @@ -1807,7 +1820,28 @@ pci_xr17v35x_setup(struct serial_private *priv,
>         writeb(128, p + UART_EXAR_RXTRG);
>         iounmap(p);
>
> -       return pci_default_setup(priv, board, port, idx);
> +       ret = pci_default_setup(priv, board, port, idx);
> +       if (ret)
> +               return ret;
> +
> +       if (idx == 0) {
> +               struct platform_device *device;
> +
> +               device = platform_device_alloc("gpio_exar",
> +                                              PLATFORM_DEVID_AUTO);
> +               if (!device)
> +                       return -ENOMEM;
> +
> +               if (platform_device_add(device) < 0) {
> +                       platform_device_put(device);
> +                       return -ENODEV;
> +               }
> +
> +               port->port.private_data = device;
> +               platform_set_drvdata(device, priv->dev);
> +       }
> +
> +       return 0;
>  }
>
>  #define PCI_DEVICE_ID_COMMTECH_4222PCI335 0x0004
> @@ -2407,6 +2441,7 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
>                 .subvendor      = PCI_ANY_ID,
>                 .subdevice      = PCI_ANY_ID,
>                 .setup          = pci_xr17v35x_setup,
> +               .exit           = pci_xr17v35x_exit,
>         },
>         {
>                 .vendor = PCI_VENDOR_ID_EXAR,
> @@ -2414,6 +2449,7 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
>                 .subvendor      = PCI_ANY_ID,
>                 .subdevice      = PCI_ANY_ID,
>                 .setup          = pci_xr17v35x_setup,
> +               .exit           = pci_xr17v35x_exit,
>         },
>         {
>                 .vendor = PCI_VENDOR_ID_EXAR,
> @@ -2421,6 +2457,7 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
>                 .subvendor      = PCI_ANY_ID,
>                 .subdevice      = PCI_ANY_ID,
>                 .setup          = pci_xr17v35x_setup,
> +               .exit           = pci_xr17v35x_exit,
>         },
>         {
>                 .vendor = PCI_VENDOR_ID_EXAR,
> @@ -2428,6 +2465,7 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
>                 .subvendor      = PCI_ANY_ID,
>                 .subdevice      = PCI_ANY_ID,
>                 .setup          = pci_xr17v35x_setup,
> +               .exit           = pci_xr17v35x_exit,
>         },
>         {
>                 .vendor = PCI_VENDOR_ID_EXAR,
> @@ -2435,6 +2473,7 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
>                 .subvendor      = PCI_ANY_ID,
>                 .subdevice      = PCI_ANY_ID,
>                 .setup          = pci_xr17v35x_setup,
> +               .exit           = pci_xr17v35x_exit,
>         },

Okay, why not to:
1) split out exar bits of 8250_pci.c (see 8250_mid.c case);
2) add gpio code either in place or separate file with sane and more
specific name?

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux