Re: [PATCH -mm 00/12] adds include/asm-generic/pci-dma-common.h

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 09 February 2010, e@lab.ntt.co.jp wrote:
> This patchset adds include/asm-generic/pci-dma-common.h that defines
> the pci_unmap state API:
> 
> DECLARE_PCI_UNMAP_ADDR(ADDR_NAME)
> DECLARE_PCI_UNMAP_LEN(LEN_NAME)
> pci_unmap_addr(PTR, ADDR_NAME)
> pci_unmap_addr_set(PTR, ADDR_NAME, VAL)
> pci_unmap_len(PTR, LEN_NAME)
> pci_unmap_len_set(PTR, LEN_NAME, VAL)
> 
> This enables us to remove lots of the duplication in architecture
> implementations since there are only two ways to define the API.

Good stuff, moving this code to a common location is certainly
a step in the right direction.

A few general comments though:

* It seems you left out arch/cris for no good reason.
* The general tendency nowadays is to avoid new HAVE_ARCH_*
  macros and instead using either a Kconfig symbol or
  checking the presence of the macros themselves. In this case,
  the macro would always be present, so CONFIG_PCI_UNMAP
  would be reasonable.
* AFAICT, there are no architectures left doing the
  non-generic version, so the code could be moved to
  a global file in include/linux instead of include/asm-generic.
  While include/linux/pci.h is already a bit overloaded, it's
  probably the right place for this. Alternatively,
  include/linux/pci-dma.h, included from include/linux/pci.h.

> In the long term, it's better to replace the API with the generic
> device model API such as DECLARE_DMA_UNMAP_ADDR. We can map the API to
> the generic one (like dma-mapping-compat.h does). This patch also
> makes the migration process easier.

Agreed.
 
	Arnd