Re: [PATCH] gpio:asm28xx-18xx: new driver

[Bartosz Golaszewski <bgolaszewski@baylibre.com>]

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
>