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

[Bjorn Helgaas <helgaas@kernel.org>]

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