gpio:Add ASMedia 28xx and 18xx GPIO driver

Hi Bartosz Golaszewski and Linux Walleij,
  I have modified code and created a new patch correctly.

Hi Bjorn Helgass,
  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.

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.

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.

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

czw., 28 maj 2020 o 11:50 Richard Hsu <saraon640529@gmail.com> napisał(a):
>
> Hi Bartosz Golaszewski and Linux Walleij,
>   I have modified code and created a new patch correctly.
>

Thanks, this looks much better.

Please make the title be: "gpio: asm28xx-18xx: new driver".

> Hi Bjorn Helgass,
>   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.
>

I have zero knowledge of the PCI sub-system but I'm seeing some PCI
MFD devices in drivers/mfd. Maybe this is what you're looking for?

> 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.
>
> 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.
>
> 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"
> +       depends on PCI
> +       select GPIO_GENERIC
> +       help
> +         Driver for GPIO functionality on Asmedia 28XX and 18XX PCI-E Bridge.
> +

Please remove all double newlines here and in the driver source code.

I'm not reviewing the rest of the driver on purpose: I'm 90% sure that
this device should be probed like a regular device in the linux driver
model. I'll review the code once we've sorted that out.

Bartosz

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
>