pon., 25 maj 2020 o 07:48 Richard Hsu <saraon640529@xxxxxxxxx> napisał(a):
>
> This driver provide GPIO functionality on Asmedia 28XX and 18XX PCI-E
> Bridge
Hi Richard,
please make the commit subject more explicit: it should be "gpio:
<name of the driver>: new driver/add support" or something similar.
>
> Signed-off-by: Richard Hsu <saraon640529@xxxxxxxxx>
> ---
> patch | 312 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 312 insertions(+)
> create mode 100644 patch
>
> diff --git a/patch b/patch
> index 0000000..a713f91
> --- /dev/null
> +++ b/patch
> @@ -0,0 +1,312 @@
> +diff -uprN -X linux-5.7.0-rc6/Documentation/dontdiff linux-5.7.0-rc6/drivers/gpio/gpio-asm28xx-18xx.c linux-5.7.0-rc6-patch/drivers/gpio/gpio-asm28xx-18xx.c
> +--- linux-5.7.0-rc6/drivers/gpio/gpio-asm28xx-18xx.c 1970-01-01 08:00:00.000000000 +0800
> ++++ linux-5.7.0-rc6-patch/drivers/gpio/gpio-asm28xx-18xx.c 2020-05-22 11:55:13.736272177 +0800
> +@@ -0,0 +1,282 @@
> ++// SPDX-License-Identifier: GPL-2.0-only
> ++/*
> ++ * Asmedia 28xx/18xx GPIO driver
> ++ *
> ++ * Copyright (C) 2020 ASMedia Technology Inc.
> ++ * Author: Richard Hsu <Richard_Hsu@xxxxxxxxxxxxxx>
> ++ */
> ++
> ++
> ++#include <linux/module.h>
> ++#include <linux/kernel.h>
> ++#include <linux/gpio/driver.h>
> ++#include <linux/pci.h>
> ++#include <linux/spinlock.h>
> ++#include <linux/pm_runtime.h>
> ++
> ++
> ++/* GPIO registers offsets */
> ++#define ASM_GPIO_CTRL 0x920
> ++#define ASM_GPIO_OUTPUT 0x928
> ++#define ASM_GPIO_INPUT 0x930
> ++#define ASM_REG_SWITCH 0xFFF
> ++
> ++#define ASM_REG_SWITCH_CTL 0x01
> ++
> ++#define ASM_GPIO_PIN5 5
> ++#define ASM_GPIO_DEFAULT 0
> ++
> ++
> ++#define PCI_DEVICE_ID_ASM_28XX_PID1 0x2824
> ++#define PCI_DEVICE_ID_ASM_28XX_PID2 0x2812
> ++#define PCI_DEVICE_ID_ASM_28XX_PID3 0x2806
> ++#define PCI_DEVICE_ID_ASM_18XX_PID1 0x1824
> ++#define PCI_DEVICE_ID_ASM_18XX_PID2 0x1812
> ++#define PCI_DEVICE_ID_ASM_18XX_PID3 0x1806
> ++#define PCI_DEVICE_ID_ASM_81XX_PID1 0x812a
> ++#define PCI_DEVICE_ID_ASM_81XX_PID2 0x812b
> ++#define PCI_DEVICE_ID_ASM_80XX_PID1 0x8061
> ++
> ++/*
> ++ * Data for PCI driver interface
> ++ *
> ++ * This data only exists for exporting the supported
> ++ * PCI ids via MODULE_DEVICE_TABLE. We do not actually
> ++ * register a pci_driver, because someone else might one day
> ++ * want to register another driver on the same PCI id.
> ++ */
> ++static const struct pci_device_id pci_tbl[] = {
> ++ { PCI_DEVICE(PCI_VENDOR_ID_ASMEDIA, PCI_DEVICE_ID_ASM_28XX_PID1), 0 },
> ++ { PCI_DEVICE(PCI_VENDOR_ID_ASMEDIA, PCI_DEVICE_ID_ASM_28XX_PID2), 0 },
> ++ { PCI_DEVICE(PCI_VENDOR_ID_ASMEDIA, PCI_DEVICE_ID_ASM_28XX_PID3), 0 },
> ++ { PCI_DEVICE(PCI_VENDOR_ID_ASMEDIA, PCI_DEVICE_ID_ASM_18XX_PID1), 0 },
> ++ { PCI_DEVICE(PCI_VENDOR_ID_ASMEDIA, PCI_DEVICE_ID_ASM_18XX_PID2), 0 },
> ++ { PCI_DEVICE(PCI_VENDOR_ID_ASMEDIA, PCI_DEVICE_ID_ASM_18XX_PID3), 0 },
> ++ { PCI_DEVICE(PCI_VENDOR_ID_ASMEDIA, PCI_DEVICE_ID_ASM_81XX_PID1), 0 },
> ++ { PCI_DEVICE(PCI_VENDOR_ID_ASMEDIA, PCI_DEVICE_ID_ASM_81XX_PID2), 0 },
> ++ { PCI_DEVICE(PCI_VENDOR_ID_ASMEDIA, PCI_DEVICE_ID_ASM_80XX_PID1), 0 },
> ++ { 0, }, /* terminate list */
> ++};
> ++MODULE_DEVICE_TABLE(pci, pci_tbl);
> ++
Stray newline.
> ++
> ++struct asm28xx_gpio {
> ++ struct gpio_chip chip;
> ++ struct pci_dev *pdev;
> ++ spinlock_t lock;
> ++};
> ++
> ++void pci_config_pm_runtime_get(struct pci_dev *pdev)
> ++{
> ++ struct device *dev = &pdev->dev;
> ++ struct device *parent = dev->parent;
> ++
> ++ if (parent)
> ++ pm_runtime_get_sync(parent);
> ++ pm_runtime_get_noresume(dev);
> ++ /*
> ++ * pdev->current_state is set to PCI_D3cold during suspending,
> ++ * so wait until suspending completes
> ++ */
> ++ pm_runtime_barrier(dev);
> ++ /*
> ++ * Only need to resume devices in D3cold, because config
> ++ * registers are still accessible for devices suspended but
> ++ * not in D3cold.
> ++ */
> ++ if (pdev->current_state == PCI_D3cold)
> ++ pm_runtime_resume(dev);
> ++}
> ++
> ++void pci_config_pm_runtime_put(struct pci_dev *pdev)
> ++{
> ++ struct device *dev = &pdev->dev;
> ++ struct device *parent = dev->parent;
> ++
> ++ pm_runtime_put(dev);
> ++ if (parent)
> ++ pm_runtime_put_sync(parent);
> ++}
> ++
> ++static int asm28xx_gpio_request(struct gpio_chip *chip, unsigned offset)
> ++{
> ++
Stray new line.
> ++ dev_dbg(chip->parent, "ASMEDIA-28xx/18xx gpio %d request\n", offset);
> ++
Please don't do this, we have enough debug messages and trace points
in gpiolib core.
> ++ if (offset == ASM_GPIO_PIN5)
> ++ return -ENODEV;
> ++
> ++ return 0;
> ++}
> ++
> ++static void asm28xx_gpio_free(struct gpio_chip *chip, unsigned offset)
> ++{
> ++ dev_dbg(chip->parent, "ASMEDIA-28xx/18xx gpio %d free\n", offset);
And *especially* don't do this. Just don't implement this callback.
> ++}
> ++
> ++static void asm28xx_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> ++{
> ++ struct asm28xx_gpio *agp = gpiochip_get_data(chip);
> ++ unsigned char temp;
> ++ unsigned long flags;
> ++
> ++ pci_config_pm_runtime_get(agp->pdev);
> ++ spin_lock_irqsave(&agp->lock, flags);
> ++ pci_read_config_byte(agp->pdev, ASM_GPIO_OUTPUT, &temp);
> ++ if (value)
> ++ temp |= BIT(offset);
> ++ else
> ++ temp &= ~BIT(offset);
> ++
> ++ pci_write_config_byte(agp->pdev, ASM_GPIO_OUTPUT, temp);
> ++ spin_unlock_irqrestore(&agp->lock, flags);
> ++ pci_config_pm_runtime_put(agp->pdev);
> ++ dev_dbg(chip->parent, "ASMEDIA-28xx/18xx gpio %d set %d reg=%02x\n", offset, value, temp);
> ++}
> ++
> ++static int asm28xx_gpio_get(struct gpio_chip *chip, unsigned offset)
> ++{
> ++ struct asm28xx_gpio *agp = gpiochip_get_data(chip);
> ++ unsigned char temp;
> ++ unsigned long flags;
> ++
> ++ pci_config_pm_runtime_get(agp->pdev);
> ++ spin_lock_irqsave(&agp->lock, flags);
> ++ pci_read_config_byte(agp->pdev, ASM_GPIO_INPUT, &temp);
> ++ spin_unlock_irqrestore(&agp->lock, flags);
> ++ pci_config_pm_runtime_put(agp->pdev);
> ++
> ++ dev_dbg(chip->parent, "ASMEDIA-28xx/18xx GPIO Pin %d get reg=%02x\n", offset, temp);
> ++ return (temp & BIT(offset)) ? 1 : 0;
> ++}
> ++
> ++static int asm28xx_gpio_dirout(struct gpio_chip *chip, unsigned offset, int value)
> ++{
> ++ struct asm28xx_gpio *agp = gpiochip_get_data(chip);
> ++ unsigned char temp;
> ++ unsigned long flags;
> ++
> ++ pci_config_pm_runtime_get(agp->pdev);
> ++ spin_lock_irqsave(&agp->lock, flags);
> ++ pci_read_config_byte(agp->pdev, ASM_GPIO_CTRL, &temp);
> ++ temp |= BIT(offset);
> ++ pci_write_config_byte(agp->pdev, ASM_GPIO_CTRL, temp);
> ++ spin_unlock_irqrestore(&agp->lock, flags);
> ++ pci_config_pm_runtime_put(agp->pdev);
> ++ dev_dbg(chip->parent, "ASMEDIA-28xx/18xx dirout gpio %d reg=%02x\n", offset, temp);
> ++
> ++ return 0;
> ++}
> ++
> ++static int asm28xx_gpio_dirin(struct gpio_chip *chip, unsigned offset)
> ++{
> ++ struct asm28xx_gpio *agp = gpiochip_get_data(chip);
> ++ unsigned char temp;
> ++ unsigned long flags;
> ++
> ++ pci_config_pm_runtime_get(agp->pdev);
> ++ spin_lock_irqsave(&agp->lock, flags);
> ++ pci_read_config_byte(agp->pdev, ASM_GPIO_CTRL, &temp);
> ++ temp &= ~BIT(offset);
> ++ pci_write_config_byte(agp->pdev, ASM_GPIO_CTRL, temp);
> ++ spin_unlock_irqrestore(&agp->lock, flags);
> ++ pci_config_pm_runtime_put(agp->pdev);
> ++ dev_dbg(chip->parent, "ASMEDIA-28xx/18xx dirin gpio %d reg=%02x\n", offset, temp);
> ++
> ++ return 0;
> ++}
> ++
> ++static struct asm28xx_gpio gp = {
> ++ .chip = {
> ++ .label = "ASM28XX-18XX GPIO",
> ++ .owner = THIS_MODULE,
> ++ .ngpio = 8,
> ++ .request = asm28xx_gpio_request,
> ++ .free = asm28xx_gpio_free,
> ++ .set = asm28xx_gpio_set,
> ++ .get = asm28xx_gpio_get,
> ++ .direction_output = asm28xx_gpio_dirout,
> ++ .direction_input = asm28xx_gpio_dirin,
> ++ },
> ++};
> ++
> ++static int __init asm28xx_gpio_init(void)
> ++{
> ++ int err = -ENODEV;
> ++ struct pci_dev *pdev = NULL;
> ++ const struct pci_device_id *ent;
> ++ u8 temp;
> ++ unsigned long flags;
> ++ int type;
> ++
> ++ for_each_pci_dev(pdev) {
> ++ ent = pci_match_id(pci_tbl, pdev);
> ++ if (ent) {
> ++ /* Because GPIO Registers only work on Upstream port. */
> ++ type = pci_pcie_type(pdev);
> ++ if (type == PCI_EXP_TYPE_UPSTREAM) {
> ++ dev_info(&pdev->dev, "ASMEDIA-28xx/18xx Init Upstream detected\n");
> ++ goto found;
> ++ }
> ++ }
> ++ }
> ++
> ++ /* ASMEDIA-28xx/18xx GPIO not found. */
> ++ dev_info(&pdev->dev, "ASMEDIA-28xx/18xx Init GPIO not detected\n");
> ++ goto out;
> ++
> ++found:
> ++ gp.pdev = pdev;
> ++ gp.chip.parent = &pdev->dev;
> ++
> ++ spin_lock_init(&gp.lock);
> ++
> ++ err = gpiochip_add_data(&gp.chip, &gp);
> ++ if (err) {
> ++ dev_err(&pdev->dev, "GPIO registering failed (%d)\n", err);
> ++ goto out;
> ++ }
> ++
> ++ pci_config_pm_runtime_get(pdev);
> ++
> ++ /* Set PCI_CFG_Switch bit = 1,then we can access GPIO Registers. */
> ++ spin_lock_irqsave(&gp.lock, flags);
> ++ pci_read_config_byte(pdev, ASM_REG_SWITCH, &temp);
> ++ temp |= ASM_REG_SWITCH_CTL;
> ++ pci_write_config_byte(pdev, ASM_REG_SWITCH, temp);
> ++ pci_read_config_byte(pdev, ASM_REG_SWITCH, &temp);
> ++ spin_unlock_irqrestore(&gp.lock, flags);
> ++
> ++ pci_config_pm_runtime_put(pdev);
> ++ dev_err(&pdev->dev, "ASMEDIA-28xx/18xx Init SWITCH = 0x%x\n", temp);
> ++out:
> ++ return err;
> ++}
> ++
> ++static void __exit asm28xx_gpio_exit(void)
> ++{
> ++ unsigned long flags;
> ++
> ++ pci_config_pm_runtime_get(gp.pdev);
> ++
> ++ spin_lock_irqsave(&gp.lock, flags);
> ++ /* Set GPIO Registers to default value. */
> ++ pci_write_config_byte(gp.pdev, ASM_GPIO_OUTPUT, ASM_GPIO_DEFAULT);
> ++ pci_write_config_byte(gp.pdev, ASM_GPIO_INPUT, ASM_GPIO_DEFAULT);
> ++ pci_write_config_byte(gp.pdev, ASM_GPIO_CTRL, ASM_GPIO_DEFAULT);
> ++ /* Clear PCI_CFG_Switch bit = 0,then we can't access GPIO Registers. */
> ++ pci_write_config_byte(gp.pdev, ASM_REG_SWITCH, ASM_GPIO_DEFAULT);
> ++ spin_unlock_irqrestore(&gp.lock, flags);
> ++ pci_config_pm_runtime_put(gp.pdev);
> ++
> ++ gpiochip_remove(&gp.chip);
> ++}
While I admit I'm not familiar with the PCI subsystem, this is surely
not the way you want to do this. What you want to do here is define a
struct pci_driver with appropriate probe callback and register it
using module_pci_driver().
Bartosz
> ++
> ++module_init(asm28xx_gpio_init);
> ++module_exit(asm28xx_gpio_exit);
> ++
> ++MODULE_LICENSE("GPL");
> ++MODULE_AUTHOR("Richard Hsu <Richard_Hsu@xxxxxxxxxxxxxx>");
> ++MODULE_DESCRIPTION("Asmedia 28xx 18xx GPIO Driver");
> ++
> ++
Stray newline.
> +diff -uprN -X linux-5.7.0-rc6/Documentation/dontdiff linux-5.7.0-rc6/drivers/gpio/Kconfig linux-5.7.0-rc6-patch/drivers/gpio/Kconfig
> +--- linux-5.7.0-rc6/drivers/gpio/Kconfig 2020-05-22 11:54:00.862644198 +0800
> ++++ linux-5.7.0-rc6-patch/drivers/gpio/Kconfig 2020-05-22 11:55:13.680270929 +0800
> +@@ -113,6 +113,14 @@ config GPIO_AMDPT
> + driver for GPIO functionality on Promontory IOHub
> + Require ACPI ASL code to enumerate as a platform device.
> +
> ++config GPIO_ASM28XX
> ++ tristate "Asmedia 28XX/18XX GPIO support"
> ++ depends on PCI
> ++ select GPIO_GENERIC
> ++ help
> ++ driver for GPIO functionality on Asmedia 28XX and 18XX PCI-E Bridge.
Capitalize the first word of help text.
> ++
> ++
> + config GPIO_ASPEED
> + tristate "Aspeed GPIO support"
> + depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
> +diff -uprN -X linux-5.7.0-rc6/Documentation/dontdiff linux-5.7.0-rc6/drivers/gpio/Makefile linux-5.7.0-rc6-patch/drivers/gpio/Makefile
> +--- linux-5.7.0-rc6/drivers/gpio/Makefile 2020-05-22 11:54:00.862644198 +0800
> ++++ linux-5.7.0-rc6-patch/drivers/gpio/Makefile 2020-05-22 11:55:13.680270929 +0800
> +@@ -176,3 +176,4 @@ obj-$(CONFIG_GPIO_XTENSA) += gpio-xtens
> + obj-$(CONFIG_GPIO_ZEVIO) += gpio-zevio.o
> + obj-$(CONFIG_GPIO_ZX) += gpio-zx.o
> + obj-$(CONFIG_GPIO_ZYNQ) += gpio-zynq.o
> ++obj-$(CONFIG_GPIO_ASM28XX) += gpio-asm28xx-18xx.o
We use alphabetical order in the Makefile.
> --
> 2.17.1
>
Re: [PATCH] GPIO: Submit a new GPIO driver
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
- Subject: Re: [PATCH] GPIO: Submit a new GPIO driver
- From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
- Date: Mon, 25 May 2020 11:35:13 +0200
- In-reply-to: <20200525054833.28995-1-saraon640529@gmail.com>
- References: <20200525054833.28995-1-saraon640529@gmail.com>
- References:
- [PATCH] GPIO: Submit a new GPIO driver
- From: Richard Hsu
- [PATCH] GPIO: Submit a new GPIO driver
- Prev by Date: Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2
- Next by Date: Re: [PATCH v7 0/4] Introduce the for_each_set_clump macro
- Previous by thread: [PATCH] GPIO: Submit a new GPIO driver
- Next by thread: Re: [PATCH] GPIO: Submit a new GPIO driver
- Index(es):
 |