All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] gpio:asm28xx-18xx: new driver
@ 2020-06-01  7:36 Richard Hsu
  2020-06-01 15:36   ` kbuild test robot
  2020-06-05 17:12 ` Bjorn Helgaas
  0 siblings, 2 replies; 14+ messages in thread
From: Richard Hsu @ 2020-06-01  7:36 UTC (permalink / raw)
  To: Richard_Hsu, Yd_Tseng, Jesse1_Chang, linus.walleij, bgolaszewski,
	bhelgaas
  Cc: linux-gpio, linux-pci, Richard Hsu

Hi Bjorn Helgaas,
 1. What are the other functions and where is the other driver?
 >PCI bus and GPIO can be considered as two functions independently.
 And the driver is located at drivers/gpio/gpio-amd8111.c

 2.We end up with multiple drivers controlling the device without
any coordination between them?
 >Yes,because two functions are independently in the device,and
the main driver for PCI bus function is more important.We wish
they can't be affected and coordinated between two drivers
as much as possible.If main driver is affected,it is more
serious.
 In our case,we have gpio registers on pci configuration space
of asm28xx pci-e bridge(with main pci bus driver).If we want
to use it by another driver that use gpio subsystem /sys/class/
gpio/(sysfs interface).I find the driver(gpio-amd8111.c) that
meet our request.Sorry! i am not best friend with git,and
reply mail in the same way. 


Hi Bartosz Golaszewski,
 Thank you.And i have studied PCI MFD device in drivers/mfd.
Maybe,it is not what i am looking for.This type of device
include core and miscellaneous function drivers.And function
drivers export resources(io/mem/dma) to sysfs.Fist,we can't
implement another pci bus driver as core driver,and secondly,
it don't use gpio subsystem with /sys/class/gpio/(sysfs
interface).
 So,you will review this driver and upstream to mainline
kernel?

 BR,
 Richard

Signed-off-by: Richard Hsu <saraon640529@gmail.com>
---
 drivers/gpio/Kconfig             |   7 +
 drivers/gpio/Makefile            |   1 +
 drivers/gpio/gpio-asm28xx-18xx.c | 278 +++++++++++++++++++++++++++++++
 3 files changed, 286 insertions(+)
 create mode 100644 drivers/gpio/gpio-asm28xx-18xx.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 1b96169d84f7..932f128f18c9 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -113,6 +113,13 @@ 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.
+
 config GPIO_ASPEED
 	tristate "Aspeed GPIO support"
 	depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index b2cfc21a97f3..0cee016f9d2f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_GPIO_AMD8111)		+= gpio-amd8111.o
 obj-$(CONFIG_GPIO_AMD_FCH)		+= gpio-amd-fch.o
 obj-$(CONFIG_GPIO_AMDPT)		+= gpio-amdpt.o
 obj-$(CONFIG_GPIO_ARIZONA)		+= gpio-arizona.o
+obj-$(CONFIG_GPIO_ASM28XX)		+= gpio-asm28xx-18xx.o
 obj-$(CONFIG_GPIO_ASPEED)		+= gpio-aspeed.o
 obj-$(CONFIG_GPIO_ASPEED_SGPIO)		+= gpio-aspeed-sgpio.o
 obj-$(CONFIG_GPIO_ATH79)		+= gpio-ath79.o
diff --git a/drivers/gpio/gpio-asm28xx-18xx.c b/drivers/gpio/gpio-asm28xx-18xx.c
new file mode 100644
index 000000000000..8c1972044c80
--- /dev/null
+++ b/drivers/gpio/gpio-asm28xx-18xx.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Asmedia 28xx/18xx GPIO driver
+ *
+ * Copyright (C) 2020 ASMedia Technology Inc.
+ * Author: Richard Hsu <Richard_Hsu@asmedia.com.tw>
+ */
+
+
+#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>
+#include <linux/bits.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);
+
+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)
+{
+	if (offset == ASM_GPIO_PIN5)
+		return -ENODEV;
+
+	return 0;
+}
+
+static void asm28xx_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct asm28xx_gpio *agp = gpiochip_get_data(chip);
+	u8 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);
+	u8 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);
+	u8 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);
+	u8 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,
+		.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;
+
+	/* We look for our device - Asmedia 28XX and 18XX Bridge
+	 * I don't know about a system with two such bridges,
+	 * so we can assume that there is max. one device.
+	 *
+	 * We can't use plain pci_driver mechanism,
+	 * as the device is really a multiple function device,
+	 * main driver that binds to the pci_device is an bus
+	 * driver and have to find & bind to the device this way.
+	 */
+
+	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;
+			}
+		}
+	}
+	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);
+}
+
+module_init(asm28xx_gpio_init);
+module_exit(asm28xx_gpio_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Richard Hsu <Richard_Hsu@asmedia.com.tw>");
+MODULE_DESCRIPTION("ASMedia 28xx 18xx GPIO Driver");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio:asm28xx-18xx: new driver
  2020-06-01  7:36 [PATCH] gpio:asm28xx-18xx: new driver Richard Hsu
@ 2020-06-01 15:36   ` kbuild test robot
  2020-06-05 17:12 ` Bjorn Helgaas
  1 sibling, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2020-06-01 15:36 UTC (permalink / raw)
  To: Richard Hsu, Richard_Hsu, Yd_Tseng, Jesse1_Chang, linus.walleij,
	bgolaszewski, bhelgaas
  Cc: kbuild-all, linux-gpio, linux-pci, Richard Hsu

[-- Attachment #1: Type: text/plain, Size: 2583 bytes --]

Hi Richard,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on gpio/for-next]
[also build test WARNING on v5.7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Richard-Hsu/gpio-asm28xx-18xx-new-driver/20200601-160444
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/gpio/gpio-asm28xx-18xx.c:69:6: warning: no previous prototype for 'pci_config_pm_runtime_get' [-Wmissing-prototypes]
69 | void pci_config_pm_runtime_get(struct pci_dev *pdev)
|      ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpio/gpio-asm28xx-18xx.c:91:6: warning: no previous prototype for 'pci_config_pm_runtime_put' [-Wmissing-prototypes]
91 | void pci_config_pm_runtime_put(struct pci_dev *pdev)
|      ^~~~~~~~~~~~~~~~~~~~~~~~~

vim +/pci_config_pm_runtime_get +69 drivers/gpio/gpio-asm28xx-18xx.c

    68	
  > 69	void pci_config_pm_runtime_get(struct pci_dev *pdev)
    70	{
    71		struct device *dev = &pdev->dev;
    72		struct device *parent = dev->parent;
    73	
    74		if (parent)
    75			pm_runtime_get_sync(parent);
    76		pm_runtime_get_noresume(dev);
    77		/*
    78		 * pdev->current_state is set to PCI_D3cold during suspending,
    79		 * so wait until suspending completes
    80		 */
    81		pm_runtime_barrier(dev);
    82		/*
    83		 * Only need to resume devices in D3cold, because config
    84		 * registers are still accessible for devices suspended but
    85		 * not in D3cold.
    86		 */
    87		if (pdev->current_state == PCI_D3cold)
    88			pm_runtime_resume(dev);
    89	}
    90	
  > 91	void pci_config_pm_runtime_put(struct pci_dev *pdev)
    92	{
    93		struct device *dev = &pdev->dev;
    94		struct device *parent = dev->parent;
    95	
    96		pm_runtime_put(dev);
    97		if (parent)
    98			pm_runtime_put_sync(parent);
    99	}
   100	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72272 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio:asm28xx-18xx: new driver
@ 2020-06-01 15:36   ` kbuild test robot
  0 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2020-06-01 15:36 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 2654 bytes --]

Hi Richard,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on gpio/for-next]
[also build test WARNING on v5.7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Richard-Hsu/gpio-asm28xx-18xx-new-driver/20200601-160444
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/gpio/gpio-asm28xx-18xx.c:69:6: warning: no previous prototype for 'pci_config_pm_runtime_get' [-Wmissing-prototypes]
69 | void pci_config_pm_runtime_get(struct pci_dev *pdev)
|      ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpio/gpio-asm28xx-18xx.c:91:6: warning: no previous prototype for 'pci_config_pm_runtime_put' [-Wmissing-prototypes]
91 | void pci_config_pm_runtime_put(struct pci_dev *pdev)
|      ^~~~~~~~~~~~~~~~~~~~~~~~~

vim +/pci_config_pm_runtime_get +69 drivers/gpio/gpio-asm28xx-18xx.c

    68	
  > 69	void pci_config_pm_runtime_get(struct pci_dev *pdev)
    70	{
    71		struct device *dev = &pdev->dev;
    72		struct device *parent = dev->parent;
    73	
    74		if (parent)
    75			pm_runtime_get_sync(parent);
    76		pm_runtime_get_noresume(dev);
    77		/*
    78		 * pdev->current_state is set to PCI_D3cold during suspending,
    79		 * so wait until suspending completes
    80		 */
    81		pm_runtime_barrier(dev);
    82		/*
    83		 * Only need to resume devices in D3cold, because config
    84		 * registers are still accessible for devices suspended but
    85		 * not in D3cold.
    86		 */
    87		if (pdev->current_state == PCI_D3cold)
    88			pm_runtime_resume(dev);
    89	}
    90	
  > 91	void pci_config_pm_runtime_put(struct pci_dev *pdev)
    92	{
    93		struct device *dev = &pdev->dev;
    94		struct device *parent = dev->parent;
    95	
    96		pm_runtime_put(dev);
    97		if (parent)
    98			pm_runtime_put_sync(parent);
    99	}
   100	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 72272 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio:asm28xx-18xx: new driver
  2020-06-01  7:36 [PATCH] gpio:asm28xx-18xx: new driver Richard Hsu
  2020-06-01 15:36   ` kbuild test robot
@ 2020-06-05 17:12 ` Bjorn Helgaas
  2020-06-08  8:57   ` Richard Hsu(許育彰)
  1 sibling, 1 reply; 14+ messages in thread
From: Bjorn Helgaas @ 2020-06-05 17:12 UTC (permalink / raw)
  To: Richard Hsu
  Cc: Richard_Hsu, Yd_Tseng, Jesse1_Chang, linus.walleij, bgolaszewski,
	bhelgaas, linux-gpio, linux-pci, Lee Jones

[+cc Lee in case he can shed light on the MFD question below]

On Mon, Jun 01, 2020 at 03:36:04PM +0800, Richard Hsu wrote:
> Hi Bjorn Helgaas,
>  1. What are the other functions and where is the other driver?
>  >PCI bus and GPIO can be considered as two functions independently.
>  And the driver is located at drivers/gpio/gpio-amd8111.c

I'm obviously missing the point here; sorry for being slow.

drivers/gpio/gpio-amd8111.c uses for_each_pci_dev() to look for
PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8111_SMBUS [1022:746b] devices.
drivers/i2c/busses/i2c-amd756.c claims that same device using the
normal PCI probe mechanism.

In this case both i2c-amd756 and gpio-amd8111 want to use the same
device, so there's at least a reason why gpio-amd8111 uses the
non-standard mechanism.

Your driver looks for PCI_VENDOR_ID_ASMEDIA devices: [1b21:2824],
[1b21:2812], [1b21:2806], [1b21:1824], etc).  But I haven't found a
second driver that needs to claim these devices.

I can't tell what any of these devices are (other than that they seem
to have some GPIO).  You might want to add them to the Linux PCI
database at https://pci-ids.ucw.cz/read/PC/1b21 .  If you do, then
"lspci" will show the correct names for them.

You mention below that these devices are PCIe bridges.  If that's the
case, they would be claimed by the "pcieport" driver in the PCI core
(drivers/pci/pcie/portdrv_pci.c).  If you collect the output of "sudo
lspci -vvxxxx", it would tell us whether the pcieport driver will
claim it.

If it does, then we have a problem because the PCIe port services
(AER, PME, DPC, etc) currently require pcieport.  If you want the AER,
PME, etc functionality in addition to GPIO, then we have to figure out
how to coordinate things.

>  2.We end up with multiple drivers controlling the device without
> any coordination between them?
>  >Yes,because two functions are independently in the device,and
> the main driver for PCI bus function is more important.We wish
> they can't be affected and coordinated between two drivers
> as much as possible.If main driver is affected,it is more
> serious.
>  In our case,we have gpio registers on pci configuration space
> of asm28xx pci-e bridge(with main pci bus driver).If we want
> to use it by another driver that use gpio subsystem /sys/class/
> gpio/(sysfs interface).I find the driver(gpio-amd8111.c) that
> meet our request.Sorry! i am not best friend with git,and
> reply mail in the same way. 
> 
> 
> Hi Bartosz Golaszewski,
>  Thank you.And i have studied PCI MFD device in drivers/mfd.

I'm not familiar with drivers/mfd.  It looks like it might be useful
for cases where a single PCI function implements multiple sorts of
functionality.

But if the problem is that you have a single function that is a PCIe
switch port and also implements some GPIOs, I doubt drivers/mfd will
help.  In that case, both the existing pcieport driver and your new
gpio-asm28xx-18xx driver would need to operate the same function, and
we'd have to make some significant changes to both of them to fit into
the MFD framework.

Long-term, I think it would be good to move the pcieport things
directly into the PCI core instead of being a separate driver.  We've
tripped over this problem before with things like performance counters
in PCIe ports.

> Maybe,it is not what i am looking for.This type of device
> include core and miscellaneous function drivers.And function
> drivers export resources(io/mem/dma) to sysfs.Fist,we can't
> implement another pci bus driver as core driver,and secondly,
> it don't use gpio subsystem with /sys/class/gpio/(sysfs
> interface).
>  So,you will review this driver and upstream to mainline
> kernel?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] gpio:asm28xx-18xx: new driver
  2020-06-05 17:12 ` Bjorn Helgaas
@ 2020-06-08  8:57   ` Richard Hsu(許育彰)
  0 siblings, 0 replies; 14+ messages in thread
From: Richard Hsu(許育彰) @ 2020-06-08  8:57 UTC (permalink / raw)
  To: Bjorn Helgaas, Richard Hsu, Richard Hsu(許育彰)
  Cc: Yd Tseng(曾裕達),
	Jesse1 Chang(張國器),
	linus.walleij, bgolaszewski, bhelgaas, linux-gpio, linux-pci,
	Lee Jones

Hi Bjorn Helgaas,
    Thank for your detailed explanation and benefited me a lot.
  I am sorry for your confusion. As you mentioned, i have a single PCIe switch port
and also implements some GPIOs.

>Your driver looks for PCI_VENDOR_ID_ASMEDIA devices: [1b21:2824], [1b21:2812], [1b21:2806], [1b21:1824], etc).
>But I haven't found a second driver that needs to claim these devices. 
   these devices are PCIe switch port and use "pcieport" as main driver.

Hi Bartosz Golaszewski and linus Walleij,
   Thanks for your help. I almost know the problem of this driver. Sorry! This is my mistake to use driver's framework incorrectly. 

BR,
   Richard 

-----Original Message-----
From: Bjorn Helgaas <helgaas@kernel.org> 
Sent: Saturday, June 6, 2020 1:13 AM
To: Richard Hsu <saraon640529@gmail.com>
Cc: Richard Hsu(許育彰) <Richard_Hsu@asmedia.com.tw>; Yd Tseng(曾裕達) <Yd_Tseng@asmedia.com.tw>; Jesse1 Chang(張國器) <Jesse1_Chang@asmedia.com.tw>; linus.walleij@linaro.org; bgolaszewski@baylibre.com; bhelgaas@google.com; linux-gpio@vger.kernel.org; linux-pci@vger.kernel.org; Lee Jones <lee.jones@linaro.org>
Subject: Re: [PATCH] gpio:asm28xx-18xx: new driver

[+cc Lee in case he can shed light on the MFD question below]

On Mon, Jun 01, 2020 at 03:36:04PM +0800, Richard Hsu wrote:
> Hi Bjorn Helgaas,
>  1. What are the other functions and where is the other driver?
>  >PCI bus and GPIO can be considered as two functions independently.
>  And the driver is located at drivers/gpio/gpio-amd8111.c

I'm obviously missing the point here; sorry for being slow.

drivers/gpio/gpio-amd8111.c uses for_each_pci_dev() to look for PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8111_SMBUS [1022:746b] devices.
drivers/i2c/busses/i2c-amd756.c claims that same device using the normal PCI probe mechanism.

In this case both i2c-amd756 and gpio-amd8111 want to use the same device, so there's at least a reason why gpio-amd8111 uses the non-standard mechanism.

Your driver looks for PCI_VENDOR_ID_ASMEDIA devices: [1b21:2824], [1b21:2812], [1b21:2806], [1b21:1824], etc).  But I haven't found a second driver that needs to claim these devices.

I can't tell what any of these devices are (other than that they seem to have some GPIO).  You might want to add them to the Linux PCI database at https://pci-ids.ucw.cz/read/PC/1b21 .  If you do, then "lspci" will show the correct names for them.

You mention below that these devices are PCIe bridges.  If that's the case, they would be claimed by the "pcieport" driver in the PCI core (drivers/pci/pcie/portdrv_pci.c).  If you collect the output of "sudo lspci -vvxxxx", it would tell us whether the pcieport driver will claim it.

If it does, then we have a problem because the PCIe port services (AER, PME, DPC, etc) currently require pcieport.  If you want the AER, PME, etc functionality in addition to GPIO, then we have to figure out how to coordinate things.

>  2.We end up with multiple drivers controlling the device without any 
> coordination between them?
>  >Yes,because two functions are independently in the device,and the 
> main driver for PCI bus function is more important.We wish they can't 
> be affected and coordinated between two drivers as much as possible.If 
> main driver is affected,it is more serious.
>  In our case,we have gpio registers on pci configuration space of 
> asm28xx pci-e bridge(with main pci bus driver).If we want to use it by 
> another driver that use gpio subsystem /sys/class/ gpio/(sysfs 
> interface).I find the driver(gpio-amd8111.c) that meet our 
> request.Sorry! i am not best friend with git,and reply mail in the 
> same way.
> 
> 
> Hi Bartosz Golaszewski,
>  Thank you.And i have studied PCI MFD device in drivers/mfd.

I'm not familiar with drivers/mfd.  It looks like it might be useful for cases where a single PCI function implements multiple sorts of functionality.

But if the problem is that you have a single function that is a PCIe switch port and also implements some GPIOs, I doubt drivers/mfd will help.  In that case, both the existing pcieport driver and your new gpio-asm28xx-18xx driver would need to operate the same function, and we'd have to make some significant changes to both of them to fit into the MFD framework.

Long-term, I think it would be good to move the pcieport things directly into the PCI core instead of being a separate driver.  We've tripped over this problem before with things like performance counters in PCIe ports.

> Maybe,it is not what i am looking for.This type of device include core 
> and miscellaneous function drivers.And function drivers export 
> resources(io/mem/dma) to sysfs.Fist,we can't implement another pci bus 
> driver as core driver,and secondly, it don't use gpio subsystem with 
> /sys/class/gpio/(sysfs interface).
>  So,you will review this driver and upstream to mainline kernel?
==================================================================================================================
This email and any attachments to it contain confidential information and are intended solely for the use of the individual to whom it 
is addressed.If you are not the intended recipient or receive it accidentally, please immediately notify the sender by e-mail and delete 
the message and any attachments from your computer system, and destroy all hard copies. If any, please be advised that any unauthorized 
disclosure, copying, distribution or any action taken or omitted in reliance on this, is illegal and prohibited. Furthermore, any views 
or opinions expressed are solely those of the author and do not represent those of ASMedia Technology Inc. Thank you for your cooperation.
==================================================================================================================

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio:asm28xx-18xx: new driver
  2020-06-05 10:02       ` Richard Hsu(許育彰)
@ 2020-06-05 10:13           ` Bartosz Golaszewski
  0 siblings, 0 replies; 14+ messages in thread
From: Bartosz Golaszewski @ 2020-06-05 10:13 UTC (permalink / raw)
  To: Richard Hsu(許育彰)
  Cc: Bartosz Golaszewski, Bjorn Helgaas, Richard Hsu,
	Yd Tseng(曾裕達),
	Jesse1 Chang(張國器),
	Linus Walleij, Bjorn Helgaas, kbuild test robot, kbuild-all,
	linux-gpio, linux-pci

pt., 5 cze 2020 o 12:02 Richard Hsu(許育彰) <Richard_Hsu@asmedia.com.tw>
napisał(a):
>
> Hi Bjorn Helgass,
>     Thanks for your detailed explanation.

Richard, please format your e-mails correctly for the mailing list. I
already directed you to the documentation on patch submission in the
linux kernel source. Also: don't top-post.

> Hi Bartosz Golaszewski,
>     I am grateful for your suggestion. The driver of bridge is a pci bus driver. It isn't written by us and is more complex. I have zero knowledge of
>  the PCI sub-system, and perhaps it(pci bus driver) don't use gpio subsystem with /sys/class/gpio/(sysfs interface) that I need. I just follow the other driver(gpio-amd8111.c)'s
> framework and maybe it can be used again. Thank you
>

The gpio-amd8111 driver does in fact do what you try to copy here but
it doesn't mean it's correct. It's quite possible that 8 years ago we
didn't know any better and merged it as it is. It doesn't justify
doing the same now with a new driver, especially since Bjorn explained
in detail why it's wrong. Rather, we should try to fix gpio-amd8111
instead.

Bartosz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio:asm28xx-18xx: new driver
@ 2020-06-05 10:13           ` Bartosz Golaszewski
  0 siblings, 0 replies; 14+ messages in thread
From: Bartosz Golaszewski @ 2020-06-05 10:13 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]

pt., 5 cze 2020 o 12:02 Richard Hsu(許育彰) <Richard_Hsu@asmedia.com.tw>
napisał(a):
>
> Hi Bjorn Helgass,
>     Thanks for your detailed explanation.

Richard, please format your e-mails correctly for the mailing list. I
already directed you to the documentation on patch submission in the
linux kernel source. Also: don't top-post.

> Hi Bartosz Golaszewski,
>     I am grateful for your suggestion. The driver of bridge is a pci bus driver. It isn't written by us and is more complex. I have zero knowledge of
>  the PCI sub-system, and perhaps it(pci bus driver) don't use gpio subsystem with /sys/class/gpio/(sysfs interface) that I need. I just follow the other driver(gpio-amd8111.c)'s
> framework and maybe it can be used again. Thank you
>

The gpio-amd8111 driver does in fact do what you try to copy here but
it doesn't mean it's correct. It's quite possible that 8 years ago we
didn't know any better and merged it as it is. It doesn't justify
doing the same now with a new driver, especially since Bjorn explained
in detail why it's wrong. Rather, we should try to fix gpio-amd8111
instead.

Bartosz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] gpio:asm28xx-18xx: new driver
  2020-06-05  7:55     ` Bartosz Golaszewski
@ 2020-06-05 10:02       ` Richard Hsu(許育彰)
  2020-06-05 10:13           ` Bartosz Golaszewski
  0 siblings, 1 reply; 14+ messages in thread
From: Richard Hsu(許育彰) @ 2020-06-05 10:02 UTC (permalink / raw)
  To: Bartosz Golaszewski, Bjorn Helgaas,
	Richard Hsu(許育彰)
  Cc: Bartosz Golaszewski, Richard Hsu,
	Yd Tseng(曾裕達),
	Jesse1 Chang(張國器),
	Linus Walleij, Bjorn Helgaas, kbuild test robot, kbuild-all,
	linux-gpio, linux-pci

Hi Bjorn Helgass,
    Thanks for your detailed explanation.
Hi Bartosz Golaszewski,
    I am grateful for your suggestion. The driver of bridge is a pci bus driver. It isn't written by us and is more complex. I have zero knowledge of
 the PCI sub-system, and perhaps it(pci bus driver) don't use gpio subsystem with /sys/class/gpio/(sysfs interface) that I need. I just follow the other driver(gpio-amd8111.c)'s
framework and maybe it can be used again. Thank you 

BR,
  Richard


       
    

-----Original Message-----
From: Bartosz Golaszewski <brgl@bgdev.pl> 
Sent: Friday, June 5, 2020 3:56 PM
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>; Richard Hsu <saraon640529@gmail.com>; Richard Hsu(許育彰) <Richard_Hsu@asmedia.com.tw>; Yd Tseng(曾裕達) <Yd_Tseng@asmedia.com.tw>; Jesse1 Chang(張國器) <Jesse1_Chang@asmedia.com.tw>; Linus Walleij <linus.walleij@linaro.org>; Bjorn Helgaas <bhelgaas@google.com>; kbuild test robot <lkp@intel.com>; kbuild-all@lists.01.org; linux-gpio <linux-gpio@vger.kernel.org>; linux-pci@vger.kernel.org
Subject: Re: [PATCH] gpio:asm28xx-18xx: new driver

czw., 4 cze 2020 o 19:55 Bjorn Helgaas <helgaas@kernel.org> napisał(a):
>
> > > +       /* We look for our device - Asmedia 28XX and 18XX Bridge
> > > +        * I don't know about a system with two such bridges,
> > > +        * so we can assume that there is max. one device.
> > > +        *
> > > +        * We can't use plain pci_driver mechanism,
> > > +        * as the device is really a multiple function device,
> > > +        * main driver that binds to the pci_device is an bus
> > > +        * driver and have to find & bind to the device this way.
> > > +        */
> > > +
> > > +       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;
> > > +                       }
> > > +               }
> > > +       }
> > > +       goto out;
> > > +
> >
> > Bjorn: is this approach really correct? It looks very strange to me 
> > and even if we were to do this kind of lookup I'd expect there to be 
> > a real pci device registered as child of pdev here so that we can 
> > have a proper driver in place with probe() et al.
>
> No, this is pretty broken.  The model is that one PCI device goes with 
> one driver.  If there are two bits of functionality associated with a 
> single PCI device, it's up to the single PCI driver to sort that out.
>
> The comment above mentions "multiple function device," which may lead 
> to some confusion about the terminology.  In the PCI specs, the 
> smallest addressable unit of PCI hardware is the "Function."  A 
> "Device" may consist of one or more Functions.  A Device with more 
> than one Function is referred to in the spec as a "Multi-Function 
> Device".
>
> These PCI Functions are addressed by a (domain, bus, device, function) 
> tuple.  For example, my system has these:
>
>   0000:00:14.0 Intel USB 3.0 xHCI Controller
>   0000:00:14.2 Intel Thermal Subsystem
>
> These two Functions are parts of the 0000:00:14 Multi-Function Device.
>
> In Linux, a "struct pci_dev" refers to a single Function, so there's a 
> pci_dev for 0000:00:14.0 and another for 0000:00:14.2.  These are 
> pretty much independent, and can be claimed by two separate drivers.
>
> But I think the "multiple function device" comment in *this* patch 
> probably doesn't refer to a "Multi-Function Device" as used in the PCI 
> specs.  It probably means a single PCI Function that has two kinds of 
> functionality.
>
> In the Linux model, that means the Function should be claimed by a 
> single driver, and that driver is responsible for coordinating the two 
> pieces of functionality.
>

Thanks for the detailed explanation!

Richard: in this case I think it's pretty clear now that whatever driver supports the "bridge" mentioned in the comment - needs to be extended with GPIO functionality.

Bart
==================================================================================================================
This email and any attachments to it contain confidential information and are intended solely for the use of the individual to whom it 
is addressed.If you are not the intended recipient or receive it accidentally, please immediately notify the sender by e-mail and delete 
the message and any attachments from your computer system, and destroy all hard copies. If any, please be advised that any unauthorized 
disclosure, copying, distribution or any action taken or omitted in reliance on this, is illegal and prohibited. Furthermore, any views 
or opinions expressed are solely those of the author and do not represent those of ASMedia Technology Inc. Thank you for your cooperation.
==================================================================================================================

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio:asm28xx-18xx: new driver
  2020-06-04 17:55     ` Bjorn Helgaas
  (?)
@ 2020-06-05  7:55     ` Bartosz Golaszewski
  2020-06-05 10:02       ` Richard Hsu(許育彰)
  -1 siblings, 1 reply; 14+ messages in thread
From: Bartosz Golaszewski @ 2020-06-05  7:55 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Bartosz Golaszewski, Richard Hsu, Richard_Hsu, Yd_Tseng,
	Jesse1_Chang, Linus Walleij, Bjorn Helgaas, kbuild test robot,
	kbuild-all, linux-gpio, linux-pci

czw., 4 cze 2020 o 19:55 Bjorn Helgaas <helgaas@kernel.org> napisał(a):
>
> > > +       /* We look for our device - Asmedia 28XX and 18XX Bridge
> > > +        * I don't know about a system with two such bridges,
> > > +        * so we can assume that there is max. one device.
> > > +        *
> > > +        * We can't use plain pci_driver mechanism,
> > > +        * as the device is really a multiple function device,
> > > +        * main driver that binds to the pci_device is an bus
> > > +        * driver and have to find & bind to the device this way.
> > > +        */
> > > +
> > > +       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;
> > > +                       }
> > > +               }
> > > +       }
> > > +       goto out;
> > > +
> >
> > Bjorn: is this approach really correct? It looks very strange to me
> > and even if we were to do this kind of lookup I'd expect there to be a
> > real pci device registered as child of pdev here so that we can have a
> > proper driver in place with probe() et al.
>
> No, this is pretty broken.  The model is that one PCI device goes with
> one driver.  If there are two bits of functionality associated with a
> single PCI device, it's up to the single PCI driver to sort that out.
>
> The comment above mentions "multiple function device," which may lead
> to some confusion about the terminology.  In the PCI specs, the
> smallest addressable unit of PCI hardware is the "Function."  A
> "Device" may consist of one or more Functions.  A Device with more
> than one Function is referred to in the spec as a "Multi-Function
> Device".
>
> These PCI Functions are addressed by a (domain, bus, device, function)
> tuple.  For example, my system has these:
>
>   0000:00:14.0 Intel USB 3.0 xHCI Controller
>   0000:00:14.2 Intel Thermal Subsystem
>
> These two Functions are parts of the 0000:00:14 Multi-Function Device.
>
> In Linux, a "struct pci_dev" refers to a single Function, so there's
> a pci_dev for 0000:00:14.0 and another for 0000:00:14.2.  These are
> pretty much independent, and can be claimed by two separate drivers.
>
> But I think the "multiple function device" comment in *this* patch
> probably doesn't refer to a "Multi-Function Device" as used in the PCI
> specs.  It probably means a single PCI Function that has two kinds of
> functionality.
>
> In the Linux model, that means the Function should be claimed by a
> single driver, and that driver is responsible for coordinating the two
> pieces of functionality.
>

Thanks for the detailed explanation!

Richard: in this case I think it's pretty clear now that whatever
driver supports the "bridge" mentioned in the comment - needs to be
extended with GPIO functionality.

Bart

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio:asm28xx-18xx: new driver
  2020-06-04 11:54   ` Bartosz Golaszewski
@ 2020-06-04 17:55     ` Bjorn Helgaas
  -1 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-06-04 17:55 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Richard Hsu, Richard_Hsu, Yd_Tseng, Jesse1_Chang, Linus Walleij,
	Bjorn Helgaas, kbuild test robot, kbuild-all, linux-gpio,
	linux-pci

On Thu, Jun 04, 2020 at 01:54:21PM +0200, Bartosz Golaszewski wrote:
> czw., 4 cze 2020 o 09:33 Richard Hsu <saraon640529@gmail.com> napisał(a):
> >
> > Hi Linus Walleij,Bartosz Golaszewski and kbuild test robot,
> >    I have fixed the warnings(make W=1 ARCH=i386) by replace two functions
> > with static type,and resend the patch.
> > line 69:
> > <<void pci_config_pm_runtime_get(struct pci_dev *pdev)
> > >>static void pci_config_pm_runtime_get(struct pci_dev *pdev)
> > line 91:
> > <<void pci_config_pm_runtime_put(struct pci_dev *pdev)
> > >>static void pci_config_pm_runtime_put(struct pci_dev *pdev)
> >
> > Thanks
> >
> > BR,
> >   Richard
> > Signed-off-by: Richard Hsu <saraon640529@gmail.com>

> > +       /* We look for our device - Asmedia 28XX and 18XX Bridge
> > +        * I don't know about a system with two such bridges,
> > +        * so we can assume that there is max. one device.
> > +        *
> > +        * We can't use plain pci_driver mechanism,
> > +        * as the device is really a multiple function device,
> > +        * main driver that binds to the pci_device is an bus
> > +        * driver and have to find & bind to the device this way.
> > +        */
> > +
> > +       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;
> > +                       }
> > +               }
> > +       }
> > +       goto out;
> > +
> 
> Bjorn: is this approach really correct? It looks very strange to me
> and even if we were to do this kind of lookup I'd expect there to be a
> real pci device registered as child of pdev here so that we can have a
> proper driver in place with probe() et al.

No, this is pretty broken.  The model is that one PCI device goes with
one driver.  If there are two bits of functionality associated with a
single PCI device, it's up to the single PCI driver to sort that out.

The comment above mentions "multiple function device," which may lead
to some confusion about the terminology.  In the PCI specs, the
smallest addressable unit of PCI hardware is the "Function."  A
"Device" may consist of one or more Functions.  A Device with more
than one Function is referred to in the spec as a "Multi-Function
Device".

These PCI Functions are addressed by a (domain, bus, device, function)
tuple.  For example, my system has these:

  0000:00:14.0 Intel USB 3.0 xHCI Controller
  0000:00:14.2 Intel Thermal Subsystem

These two Functions are parts of the 0000:00:14 Multi-Function Device.

In Linux, a "struct pci_dev" refers to a single Function, so there's
a pci_dev for 0000:00:14.0 and another for 0000:00:14.2.  These are
pretty much independent, and can be claimed by two separate drivers.

But I think the "multiple function device" comment in *this* patch
probably doesn't refer to a "Multi-Function Device" as used in the PCI
specs.  It probably means a single PCI Function that has two kinds of
functionality.

In the Linux model, that means the Function should be claimed by a
single driver, and that driver is responsible for coordinating the two
pieces of functionality.

Bjorn

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio:asm28xx-18xx: new driver
@ 2020-06-04 17:55     ` Bjorn Helgaas
  0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2020-06-04 17:55 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3547 bytes --]

On Thu, Jun 04, 2020 at 01:54:21PM +0200, Bartosz Golaszewski wrote:
> czw., 4 cze 2020 o 09:33 Richard Hsu <saraon640529@gmail.com> napisał(a):
> >
> > Hi Linus Walleij,Bartosz Golaszewski and kbuild test robot,
> >    I have fixed the warnings(make W=1 ARCH=i386) by replace two functions
> > with static type,and resend the patch.
> > line 69:
> > <<void pci_config_pm_runtime_get(struct pci_dev *pdev)
> > >>static void pci_config_pm_runtime_get(struct pci_dev *pdev)
> > line 91:
> > <<void pci_config_pm_runtime_put(struct pci_dev *pdev)
> > >>static void pci_config_pm_runtime_put(struct pci_dev *pdev)
> >
> > Thanks
> >
> > BR,
> >   Richard
> > Signed-off-by: Richard Hsu <saraon640529@gmail.com>

> > +       /* We look for our device - Asmedia 28XX and 18XX Bridge
> > +        * I don't know about a system with two such bridges,
> > +        * so we can assume that there is max. one device.
> > +        *
> > +        * We can't use plain pci_driver mechanism,
> > +        * as the device is really a multiple function device,
> > +        * main driver that binds to the pci_device is an bus
> > +        * driver and have to find & bind to the device this way.
> > +        */
> > +
> > +       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;
> > +                       }
> > +               }
> > +       }
> > +       goto out;
> > +
> 
> Bjorn: is this approach really correct? It looks very strange to me
> and even if we were to do this kind of lookup I'd expect there to be a
> real pci device registered as child of pdev here so that we can have a
> proper driver in place with probe() et al.

No, this is pretty broken.  The model is that one PCI device goes with
one driver.  If there are two bits of functionality associated with a
single PCI device, it's up to the single PCI driver to sort that out.

The comment above mentions "multiple function device," which may lead
to some confusion about the terminology.  In the PCI specs, the
smallest addressable unit of PCI hardware is the "Function."  A
"Device" may consist of one or more Functions.  A Device with more
than one Function is referred to in the spec as a "Multi-Function
Device".

These PCI Functions are addressed by a (domain, bus, device, function)
tuple.  For example, my system has these:

  0000:00:14.0 Intel USB 3.0 xHCI Controller
  0000:00:14.2 Intel Thermal Subsystem

These two Functions are parts of the 0000:00:14 Multi-Function Device.

In Linux, a "struct pci_dev" refers to a single Function, so there's
a pci_dev for 0000:00:14.0 and another for 0000:00:14.2.  These are
pretty much independent, and can be claimed by two separate drivers.

But I think the "multiple function device" comment in *this* patch
probably doesn't refer to a "Multi-Function Device" as used in the PCI
specs.  It probably means a single PCI Function that has two kinds of
functionality.

In the Linux model, that means the Function should be claimed by a
single driver, and that driver is responsible for coordinating the two
pieces of functionality.

Bjorn

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio:asm28xx-18xx: new driver
  2020-06-04  7:32 Richard Hsu
@ 2020-06-04 11:54   ` Bartosz Golaszewski
  0 siblings, 0 replies; 14+ messages in thread
From: Bartosz Golaszewski @ 2020-06-04 11:54 UTC (permalink / raw)
  To: Richard Hsu
  Cc: Richard_Hsu, Yd_Tseng, Jesse1_Chang, Linus Walleij,
	Bjorn Helgaas, kbuild test robot, kbuild-all, linux-gpio,
	linux-pci

czw., 4 cze 2020 o 09:33 Richard Hsu <saraon640529@gmail.com> napisał(a):
>
> Hi Linus Walleij,Bartosz Golaszewski and kbuild test robot,
>    I have fixed the warnings(make W=1 ARCH=i386) by replace two functions
> with static type,and resend the patch.
> line 69:
> <<void pci_config_pm_runtime_get(struct pci_dev *pdev)
> >>static void pci_config_pm_runtime_get(struct pci_dev *pdev)
> line 91:
> <<void pci_config_pm_runtime_put(struct pci_dev *pdev)
> >>static void pci_config_pm_runtime_put(struct pci_dev *pdev)
>
> Thanks
>
> BR,
>   Richard
> Signed-off-by: Richard Hsu <saraon640529@gmail.com>

Richar: please add a proper commit message to your patch and fix your
subject line: there should be a space after "gpio:". Use numbering for
subsequent patch versions ("[PATCH v2]" etc.).

> ---

Any additional comments as well as changes between versions should go here.

>  drivers/gpio/gpio-asm28xx-18xx.c | 278 +++++++++++++++++++++++++++++++
>  1 file changed, 278 insertions(+)
>  create mode 100644 drivers/gpio/gpio-asm28xx-18xx.c
>
> diff --git a/drivers/gpio/gpio-asm28xx-18xx.c b/drivers/gpio/gpio-asm28xx-18xx.c
> index 000000000000..0cf8d0df5407
> --- /dev/null
> +++ b/drivers/gpio/gpio-asm28xx-18xx.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Asmedia 28xx/18xx GPIO driver
> + *
> + * Copyright (C) 2020 ASMedia Technology Inc.
> + * Author: Richard Hsu <Richard_Hsu@asmedia.com.tw>
> + */
> +

Please remove all stray newlines from the driver.

> +
> +#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>
> +#include <linux/bits.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);
> +
> +struct asm28xx_gpio {
> +       struct gpio_chip        chip;
> +       struct pci_dev          *pdev;
> +       spinlock_t              lock;
> +};
> +
> +static 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);
> +}
> +
> +static 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)
> +{
> +       if (offset == ASM_GPIO_PIN5)
> +               return -ENODEV;
> +
> +       return 0;
> +}
> +
> +static void asm28xx_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +       struct asm28xx_gpio *agp = gpiochip_get_data(chip);
> +       u8 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);
> +       u8 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);
> +       u8 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);
> +       u8 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,
> +               .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;
> +
> +       /* We look for our device - Asmedia 28XX and 18XX Bridge
> +        * I don't know about a system with two such bridges,
> +        * so we can assume that there is max. one device.
> +        *
> +        * We can't use plain pci_driver mechanism,
> +        * as the device is really a multiple function device,
> +        * main driver that binds to the pci_device is an bus
> +        * driver and have to find & bind to the device this way.
> +        */
> +
> +       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;
> +                       }
> +               }
> +       }
> +       goto out;
> +

Bjorn: is this approach really correct? It looks very strange to me
and even if we were to do this kind of lookup I'd expect there to be a
real pci device registered as child of pdev here so that we can have a
proper driver in place with probe() et al.

Bart

> +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);
> +}
> +
> +module_init(asm28xx_gpio_init);
> +module_exit(asm28xx_gpio_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Richard Hsu <Richard_Hsu@asmedia.com.tw>");
> +MODULE_DESCRIPTION("ASMedia 28xx 18xx GPIO Driver");
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] gpio:asm28xx-18xx: new driver
@ 2020-06-04 11:54   ` Bartosz Golaszewski
  0 siblings, 0 replies; 14+ messages in thread
From: Bartosz Golaszewski @ 2020-06-04 11:54 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 12036 bytes --]

czw., 4 cze 2020 o 09:33 Richard Hsu <saraon640529@gmail.com> napisał(a):
>
> Hi Linus Walleij,Bartosz Golaszewski and kbuild test robot,
>    I have fixed the warnings(make W=1 ARCH=i386) by replace two functions
> with static type,and resend the patch.
> line 69:
> <<void pci_config_pm_runtime_get(struct pci_dev *pdev)
> >>static void pci_config_pm_runtime_get(struct pci_dev *pdev)
> line 91:
> <<void pci_config_pm_runtime_put(struct pci_dev *pdev)
> >>static void pci_config_pm_runtime_put(struct pci_dev *pdev)
>
> Thanks
>
> BR,
>   Richard
> Signed-off-by: Richard Hsu <saraon640529@gmail.com>

Richar: please add a proper commit message to your patch and fix your
subject line: there should be a space after "gpio:". Use numbering for
subsequent patch versions ("[PATCH v2]" etc.).

> ---

Any additional comments as well as changes between versions should go here.

>  drivers/gpio/gpio-asm28xx-18xx.c | 278 +++++++++++++++++++++++++++++++
>  1 file changed, 278 insertions(+)
>  create mode 100644 drivers/gpio/gpio-asm28xx-18xx.c
>
> diff --git a/drivers/gpio/gpio-asm28xx-18xx.c b/drivers/gpio/gpio-asm28xx-18xx.c
> index 000000000000..0cf8d0df5407
> --- /dev/null
> +++ b/drivers/gpio/gpio-asm28xx-18xx.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Asmedia 28xx/18xx GPIO driver
> + *
> + * Copyright (C) 2020 ASMedia Technology Inc.
> + * Author: Richard Hsu <Richard_Hsu@asmedia.com.tw>
> + */
> +

Please remove all stray newlines from the driver.

> +
> +#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>
> +#include <linux/bits.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);
> +
> +struct asm28xx_gpio {
> +       struct gpio_chip        chip;
> +       struct pci_dev          *pdev;
> +       spinlock_t              lock;
> +};
> +
> +static 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);
> +}
> +
> +static 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)
> +{
> +       if (offset == ASM_GPIO_PIN5)
> +               return -ENODEV;
> +
> +       return 0;
> +}
> +
> +static void asm28xx_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +       struct asm28xx_gpio *agp = gpiochip_get_data(chip);
> +       u8 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);
> +       u8 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);
> +       u8 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);
> +       u8 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,
> +               .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;
> +
> +       /* We look for our device - Asmedia 28XX and 18XX Bridge
> +        * I don't know about a system with two such bridges,
> +        * so we can assume that there is max. one device.
> +        *
> +        * We can't use plain pci_driver mechanism,
> +        * as the device is really a multiple function device,
> +        * main driver that binds to the pci_device is an bus
> +        * driver and have to find & bind to the device this way.
> +        */
> +
> +       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;
> +                       }
> +               }
> +       }
> +       goto out;
> +

Bjorn: is this approach really correct? It looks very strange to me
and even if we were to do this kind of lookup I'd expect there to be a
real pci device registered as child of pdev here so that we can have a
proper driver in place with probe() et al.

Bart

> +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);
> +}
> +
> +module_init(asm28xx_gpio_init);
> +module_exit(asm28xx_gpio_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Richard Hsu <Richard_Hsu@asmedia.com.tw>");
> +MODULE_DESCRIPTION("ASMedia 28xx 18xx GPIO Driver");
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH] gpio:asm28xx-18xx: new driver
@ 2020-06-04  7:32 Richard Hsu
  2020-06-04 11:54   ` Bartosz Golaszewski
  0 siblings, 1 reply; 14+ messages in thread
From: Richard Hsu @ 2020-06-04  7:32 UTC (permalink / raw)
  To: Richard_Hsu, Yd_Tseng, Jesse1_Chang, linus.walleij, bgolaszewski,
	bhelgaas, lkp
  Cc: kbuild-all, linux-gpio, linux-pci, Richard Hsu

Hi Linus Walleij,Bartosz Golaszewski and kbuild test robot,
   I have fixed the warnings(make W=1 ARCH=i386) by replace two functions
with static type,and resend the patch.
line 69:
<<void pci_config_pm_runtime_get(struct pci_dev *pdev)
>>static void pci_config_pm_runtime_get(struct pci_dev *pdev)
line 91:
<<void pci_config_pm_runtime_put(struct pci_dev *pdev)
>>static void pci_config_pm_runtime_put(struct pci_dev *pdev)

Thanks

BR,
  Richard
Signed-off-by: Richard Hsu <saraon640529@gmail.com>
---
 drivers/gpio/gpio-asm28xx-18xx.c | 278 +++++++++++++++++++++++++++++++
 1 file changed, 278 insertions(+)
 create mode 100644 drivers/gpio/gpio-asm28xx-18xx.c

diff --git a/drivers/gpio/gpio-asm28xx-18xx.c b/drivers/gpio/gpio-asm28xx-18xx.c
index 000000000000..0cf8d0df5407
--- /dev/null
+++ b/drivers/gpio/gpio-asm28xx-18xx.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Asmedia 28xx/18xx GPIO driver
+ *
+ * Copyright (C) 2020 ASMedia Technology Inc.
+ * Author: Richard Hsu <Richard_Hsu@asmedia.com.tw>
+ */
+
+
+#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>
+#include <linux/bits.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);
+
+struct asm28xx_gpio {
+	struct gpio_chip	chip;
+	struct pci_dev		*pdev;
+	spinlock_t		lock;
+};
+
+static 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);
+}
+
+static 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)
+{
+	if (offset == ASM_GPIO_PIN5)
+		return -ENODEV;
+
+	return 0;
+}
+
+static void asm28xx_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct asm28xx_gpio *agp = gpiochip_get_data(chip);
+	u8 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);
+	u8 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);
+	u8 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);
+	u8 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,
+		.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;
+
+	/* We look for our device - Asmedia 28XX and 18XX Bridge
+	 * I don't know about a system with two such bridges,
+	 * so we can assume that there is max. one device.
+	 *
+	 * We can't use plain pci_driver mechanism,
+	 * as the device is really a multiple function device,
+	 * main driver that binds to the pci_device is an bus
+	 * driver and have to find & bind to the device this way.
+	 */
+
+	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;
+			}
+		}
+	}
+	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);
+}
+
+module_init(asm28xx_gpio_init);
+module_exit(asm28xx_gpio_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Richard Hsu <Richard_Hsu@asmedia.com.tw>");
+MODULE_DESCRIPTION("ASMedia 28xx 18xx GPIO Driver");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-06-08  9:04 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-01  7:36 [PATCH] gpio:asm28xx-18xx: new driver Richard Hsu
2020-06-01 15:36 ` kbuild test robot
2020-06-01 15:36   ` kbuild test robot
2020-06-05 17:12 ` Bjorn Helgaas
2020-06-08  8:57   ` Richard Hsu(許育彰)
2020-06-04  7:32 Richard Hsu
2020-06-04 11:54 ` Bartosz Golaszewski
2020-06-04 11:54   ` Bartosz Golaszewski
2020-06-04 17:55   ` Bjorn Helgaas
2020-06-04 17:55     ` Bjorn Helgaas
2020-06-05  7:55     ` Bartosz Golaszewski
2020-06-05 10:02       ` Richard Hsu(許育彰)
2020-06-05 10:13         ` Bartosz Golaszewski
2020-06-05 10:13           ` Bartosz Golaszewski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.