gpio:asm28xx-18xx: new driver

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

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
>

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

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

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.
==================================================================================================================

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