Re: [PATCH v2] drivers: field for new GPIO driver

[Dmitry Eremin-Solenikov <dbaryshkov@xxxxxxxxx>]

2015-10-13 12:04 GMT+03:00 YD Tseng <ltyu101@xxxxxxxxx>:
> Dear all:
>
> This patch adds a new GPIO driver for AMD Promontory chip.  This GPIO controller is enumerated by ACPI and the ACPI compliant hardware ID is AMDF030.
>
> Signed-off-by: YD Tseng <Yd_Tseng@xxxxxxxxxxxxxx>
>
> ---
> v2: 1. fixed the coding style
>     2. registers renaming
>
> A new file is added and 2 files are modified.
> drivers/gpio/gpio-amdpt.c            New file
> drivers/gpio/Kconfig                       Modified file
> drivers/gpio/Makefile                    Modified file
>
> diff -uprN -X linux-4.2.vanilla/Documentation/dontdiff linux-4.2.vanilla/drivers/gpio/gpio-amdpt.c linux-4.2/drivers/gpio/gpio-amdpt.c
> --- linux-4.2.vanilla/drivers/gpio/gpio-amdpt.c 1969-12-31 19:00:00.000000000 -0500
> +++ linux-4.2/drivers/gpio/gpio-amdpt.c 2015-05-21 07:49:24.736020310 -0400
> @@ -0,0 +1,289 @@
> +/*
> + * AMD Promontory GPIO driver
> + *
> + * Copyright (C) 2015 ASMedia Technology Inc.
> + * Author: YD Tseng <yd_tseng@xxxxxxxxxxxxxx>
> + *
> + * 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/gpio.h>

#include <linux/gpio/driver.h> instead ?

> +#include <linux/spinlock.h>
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +
> +#define TOTAL_GPIO_PINS 8
> +
> +/* PCI-E MMIO register offsets */
> +#define PT_DIRECTION_REG   0x00
> +#define PT_INPUTDATA_REG   0x04
> +#define PT_OUTPUTDATA_REG  0x08
> +#define PT_CLOCKRATE_REG   0x0C
> +#define PT_SYNC_REG        0x28
> +
> +struct pt_gpio_chip {
> +       struct gpio_chip         gc;
> +       struct platform_device   *pdev;

You don't really need pdev. You can use gc->dev for logging information instead.

> +       void __iomem             *reg_base;
> +       spinlock_t               lock;
> +};
> +
> +#define to_pt_gpio(c)  container_of(c, struct pt_gpio_chip, gc)
> +
> +static int pt_gpio_request(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct pt_gpio_chip *pt_gpio = to_pt_gpio(gc);
> +       struct platform_device *pdev;
> +       unsigned long flags;
> +       u32 using_pins;
> +
> +       pdev = pt_gpio->pdev;
> +
> +       dev_dbg(&pdev->dev, "pt_gpio_request offset=%x\n", offset);
> +
> +       spin_lock_irqsave(&pt_gpio->lock, flags);
> +
> +       using_pins = readl(pt_gpio->reg_base + PT_SYNC_REG);
> +       if (using_pins&(1<<offset)) {

spaces? Also using_pins & BIT(offset) would be more readable.

> +               dev_warn(&pdev->dev, "PT GPIO pin %x reconfigured\n", offset);
> +               spin_unlock_irqrestore(&pt_gpio->lock, flags);
> +               return -EINVAL;
> +       }
> +       else
> +               writel(using_pins|(1<<offset), pt_gpio->reg_base + PT_SYNC_REG);

Please refer to CodingStyle document.

Also you don't need the "else" word at all here.

> +
> +       spin_unlock_irqrestore(&pt_gpio->lock, flags);
> +
> +       return 0;
> +}
> +

[skipped]

> +static int pt_gpio_get_value(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct pt_gpio_chip *pt_gpio = to_pt_gpio(gc);
> +       struct platform_device *pdev;
> +       unsigned long flags;
> +       u32 data;
> +
> +       pdev = pt_gpio->pdev;
> +
> +       spin_lock_irqsave(&pt_gpio->lock, flags);
> +
> +       /* configure as output */
> +       if ((readl(pt_gpio->reg_base + PT_DIRECTION_REG)>>offset)&0x1)

data = readl(...);
if (data & BIT(offset)) {
   ...
} else {
  ...
}

> +               data = readl(pt_gpio->reg_base + PT_OUTPUTDATA_REG);
> +       else    /* configure as input */
> +               data = readl(pt_gpio->reg_base + PT_INPUTDATA_REG);
> +
> +       spin_unlock_irqrestore(&pt_gpio->lock, flags);
> +
> +       data >>= offset;
> +       data &= 1;

Ghm.

> +
> +       dev_dbg(&pdev->dev, "pt_gpio_get_value offset=%x, value=%x\n", offset,
> +               data);
> +
> +       return data;
> +}
> +

[skipped]

> +
> +static int pt_gpio_direction_output(struct gpio_chip *gc,
> +                                       unsigned offset, int value)
> +{
> +       struct pt_gpio_chip *pt_gpio = to_pt_gpio(gc);
> +       struct platform_device *pdev;
> +       unsigned long flags;
> +       u32 data;
> +
> +       pdev = pt_gpio->pdev;
> +
> +       dev_dbg(&pdev->dev, "pt_gpio_direction_output offset=%x, value=%x\n",
> +               offset, value);
> +
> +       spin_lock_irqsave(&pt_gpio->lock, flags);
> +
> +       data = readl( pt_gpio->reg_base + PT_OUTPUTDATA_REG);
> +       if (value)
> +               writel(data |= BIT(offset),  pt_gpio->reg_base + PT_OUTPUTDATA_REG);
> +       else
> +               writel(data &= ~BIT(offset),  pt_gpio->reg_base + PT_OUTPUTDATA_REG);

data = readl(..);
if (value)
  data |= BIT(offset);
else
  data &= ~BIT(offset);
writel(...);

> +
> +       data = readl(pt_gpio->reg_base + PT_DIRECTION_REG);
> +       data |= BIT(offset);
> +       writel(data, pt_gpio->reg_base + PT_DIRECTION_REG);
> +
> +       spin_unlock_irqrestore(&pt_gpio->lock, flags);
> +
> +       return 0;
> +}

[skipped]

> +
> +static int __init pt_gpio_init(void)
> +{
> +       return platform_driver_register(&pt_gpio_driver);
> +}
> +
> +static void __exit pt_gpio_exit(void)
> +{
> +       platform_driver_unregister(&pt_gpio_driver);
> +}
> +
> +module_init(pt_gpio_init);
> +module_exit(pt_gpio_exit);

I'd suggest to use module_platform_driver() macro here.

> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("YD Tseng <yd_tseng@xxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("AMD Promontory GPIO Driver");
> +MODULE_ALIAS("platform:pt-gpio");

Do you really need this alias? For what purpose?


-- 
With best wishes
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html