czw., 4 cze 2020 o 09:33 Richard Hsu <saraon640529@xxxxxxxxx> 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@xxxxxxxxx>
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@xxxxxxxxxxxxxx>
> + */
> +
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@xxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("ASMedia 28xx 18xx GPIO Driver");
> --
> 2.17.1
>
Re: [PATCH] gpio:asm28xx-18xx: new driver
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
- Subject: Re: [PATCH] gpio:asm28xx-18xx: new driver
- From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
- Date: Thu, 4 Jun 2020 13:54:21 +0200
- In-reply-to: <20200604073243.19280-1-saraon640529@gmail.com>
- References: <20200604073243.19280-1-saraon640529@gmail.com>
- Follow-Ups:
- Re: [PATCH] gpio:asm28xx-18xx: new driver
- From: Bjorn Helgaas
- Re: [PATCH] gpio:asm28xx-18xx: new driver
- References:
- [PATCH] gpio:asm28xx-18xx: new driver
- From: Richard Hsu
- [PATCH] gpio:asm28xx-18xx: new driver
- Prev by Date: Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
- Next by Date: Re: [PATCH v2] gpiolib: split character device into gpiolib-cdev
- Previous by thread: [PATCH] gpio:asm28xx-18xx: new driver
- Next by thread: Re: [PATCH] gpio:asm28xx-18xx: new driver
- Index(es):
 |