Re: [PATCH] gpio:Add ASMedia 28xx and 18xx GPIO driver

[Bjorn Helgaas <helgaas@kernel.org>]

Details matter.

On Thu, May 28, 2020 at 05:49:55PM +0800, Richard Hsu wrote:
> Hi Bartosz Golaszewski and Linux Walleij,

s/Linux/Linus/

>   I have modified code and created a new patch correctly.
> 
> Hi Bjorn Helgass,

s/Helgass/Helgaas/

>   Thanks for your reply and suggestions.I will explain it more detail.
> 1.Why i select driver's framework based on AMD South Bridge(gpio-amd
> 8111.c)?
>  Our device is similar as it and it is not the specific gpio controller
> like others.We can't use plain pci_driver mechanism,as it is really
> multiple function,main driver that binds to the pci_device is a bus
> driver(descrbed in gpio-8111.c).This driver is almost the same with
> amd8111.The different parts is the gpio register accessing methods
> between io and pci configuration.

What are the other functions, and where is the other driver?

> 2.What's special about asm28xx that means you need PM stuff when
> almost nobody else does?
>   Main driver of asm28xx is a bus driver.And when no device be added
> on,bridge sometimes will enter power saving state.other
> sub-system or driver can access it after wake it up(ex.proc_bus_pci_
> read() in drivers/pci/Proc.c).
>
> 3.We end up with multiple drivers controlling the device without
> any coordination between them(ex. PM)?
>   This driver just wake it up before accessing specific gpio
> registers.AP(RW) also wake device up before accessing register.
> That might not be a problem for this particular case.

I don't know what "AP(RW)" means.  But this doesn't answer my question
about coordination between the drivers.

> This driver is used for particular condition and work well.And
> driver framework is the same with related AMD8111.It perhaps
> can keep in the same driver framework and i really hope so.
> 
> I am grateful for your assistance.

The text above is your response to questions.  It needs to be a commit
log instead.  The answers to the questions could be in a response to
the email that *asked* the questions.

> Signed-off-by: Richard Hsu <saraon640529@gmail.com>
> ---
>  drivers/gpio/Kconfig             |   8 +
>  drivers/gpio/Makefile            |   1 +
>  drivers/gpio/gpio-asm28xx-18xx.c | 278 +++++++++++++++++++++++++++++++
>  3 files changed, 287 insertions(+)
>  mode change 100644 => 100755 drivers/gpio/Kconfig
>  mode change 100644 => 100755 drivers/gpio/Makefile
>  create mode 100755 drivers/gpio/gpio-asm28xx-18xx.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> old mode 100644
> index 1b96169d84f7..0676082efcb7
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -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"

s|Asmedia 28XX/18XX|ASMedia 28xx/18xx|

Pick either "28xx/18xx" (as you used in the dmesg logs below) or
"28XX/18XX" and use it consistently throughout.

> +	depends on PCI
> +	select GPIO_GENERIC
> +	help
> +	  Driver for GPIO functionality on Asmedia 28XX and 18XX PCI-E Bridge.

s/Asmedia/ASMedia/
s/PCI-E/PCIe/

> +
> +
>  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
> old mode 100644
> index b2cfc21a97f3..0cee016f9d2f
> --- 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
> 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

s/Asmedia/ASMedia/

> + *
> + * 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.

s/ids/IDs/
s/id/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);

s/ASMEDIA/ASMedia/

s/gpio/GPIO/ (Or, if "gpio" is the usual spelling in this subsystem,
at least make them all consistent.  This one is lower-case; the one
below is upper-case.  They should be the same.)

> +}
> +
> +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);

s/ASMEDIA/ASMedia/

> +	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);

s/ASMEDIA/ASMedia/
s/gpio/GPIO/

> +
> +	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);

s/ASMEDIA/ASMedia/
s/gpio/GPIO/

> +
> +	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

s/Asmedia/ASMedia/

> +	 * 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");

s/ASMEDIA/ASMedia/

> +				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);

s/ASMEDIA/ASMedia/

> +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
>