Hi, On 2015/07/17 0:13, Christos Zoulas wrote: > In article <[email protected]>, > Kengo NAKAHARA <[email protected]> wrote: >> >> Thank you for your comment. I fix pci_intr_alloc() uses int *counts. >> I also fix missing about man installation and some wording. Here is >> new patch, >> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api2.patch > > +int pci_intr_alloc(const struct pci_attach_args *pa, > + pci_intr_handle_t **, int *, pci_intr_type_t); > > Parameters should not have names in declarations. The man wording could > be improved, but it is a complicated explanation and what is there now > is good enough.
I missed the wrong parameter name, thanks. Thank you for comment to man, but I should modify man a little more about you point out below. >> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-examp= >> le2.patch >> >>>> Could you comment this patch? > > + /* Allocation settings */ > + counts[PCI_INTR_TYPE_MSIX] = WM_MAX_NINTR; > > perhaps: > > + memset(counts, 0, sizeof(counts)); > > before setting the counts? > > + if (pci_intr_alloc(pa, &sc->sc_intrs, counts, PCI_INTR_TYPE_MSIX) != 0) > { > > That should probably be: > + if (pci_intr_alloc(pa, &sc->sc_intrs, counts, __arraycount(counts)) != > 0) { > or: > + if (pci_intr_alloc(pa, &sc->sc_intrs, counts, PCI_INTR_TYPE_SIZE) != 0) > { Mmm, as a side of the API implementation, below 2 code are the same behavior. [1] Current implementation ==================== counts[PCI_INTR_TYPE_MSIX] = WM_MAX_NINTR; counts[PCI_INTR_TYPE_MSI] = 1; counts[PCI_INTR_TYPE_INTX] = 1; if (pci_intr_alloc(pa, &sc->sc_intrs, counts, PCI_INTR_TYPE_MSIX) != 0) { ==================== [2] Your implementation ==================== memset(counts, 0, sizeof(counts)); counts[PCI_INTR_TYPE_MSIX] = WM_MAX_NINTR; counts[PCI_INTR_TYPE_MSI] = 1; counts[PCI_INTR_TYPE_INTX] = 1; if (pci_intr_alloc(pa, &sc->sc_intrs, counts, __arraycount(counts)) != 0) { // or if (pci_intr_alloc(pa, &sc->sc_intrs, counts, PCI_INTR_TYPE_SIZE) != 0) { ==================== I thought [1] was better specification, however I think again your [2] is better. So, I should modify man and example code as match it. > I would keep: > - if (pci_intr_type(sc->sc_intrs[0]) == PCI_INTR_TYPE_MSIX) { > + intr_type = pci_intr_type(sc->sc_intrs[0]); > + if (intr_type == PCI_INTR_TYPE_MSIX) { OK, I modify it. > So I wouldn't have to re-evaluate it. > > LGTM, ship it. Thanks! -- ////////////////////////////////////////////////////////////////////// Internet Initiative Japan Inc. Device Engineering Section, Core Product Development Department, Product Division, Technology Unit Kengo NAKAHARA <[email protected]>
