> #include <linux/gpio/driver.h> instead ?
Modified in the code.
>
> > +#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.
Modified in the code.
>
> > + 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.
Modified in the code.
>
> > + 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.
Modified in the code.
>
> > +
> > + spin_unlock_irqrestore(&pt_gpio->lock, flags);
> > +
> > + return 0;
> > +}
> > +
>
> [skipped]
Sorry, will you please describe what the meaning for [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 {
> ...
> }
>
Modified in the code.
> > + 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.
Sorry, will you please describe what the meaning for Ghm.?
>
> > +
> > + dev_dbg(&pdev->dev, "pt_gpio_get_value offset=%x, value=%x\n",
> > offset,
> > + data);
> > +
> > + return data;
> > +}
> > +
>
> [skipped]
Sorry, will you please describe what the meaning for [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(...);
>
Modified in the code.
> > +
> > + 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]
>
Sorry, will you please describe what the meaning for [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.
>
Modified in the code.
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("YD Tseng <[email protected]>");
> > +MODULE_DESCRIPTION("AMD Promontory GPIO Driver");
> > +MODULE_ALIAS("platform:pt-gpio");
>
> Do you really need this alias? For what purpose?
Removed MODULE_ALIAS.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
