Re: [flashrom] [patch] NetBSD support

[Carl-Daniel Hailfinger]

On 08.01.2010 17:22, Jonathan A. Kollasch wrote:
> Add support for NetBSD.
>
> Signed-off-by: Jonathan A. Kollasch

Thanks for your patch. There are a few things I'd like to see changed
before committing, though.
> Index: hwaccess.h
>
> --- hwaccess.h	(revision 837)
> +++ hwaccess.h	(working copy)
> @@ -76,6 +76,19 @@
>  #endif
>  #endif
>
> +#if defined(__NetBSD__) && (defined(__i386__) || defined(__x86_64__))
> #include <sys/types.h>
> #include <machine/sysarch.h>
> #if defined(__i386__)
> #define iopl i386_iopl
> #elif defined(__x86_64__)
> #define iopl x86_64_iopl
> #endif
> #include "pio.h"
> #define off64_t off_t
> #define lseek64 lseek
>
> +#endif
>
> #if defined(__FreeBSD__) || defined(__DragonFly__)
>  extern int io_fd;
>  #endif
> Index: Makefile
> ===================================================================
> --- Makefile	(revision 837)
> +++ Makefile	(working copy)
> @@ -40,6 +40,10 @@
>  CFLAGS += -I/usr/local/include
>  LDFLAGS += -L/usr/local/lib
>  endif
> +ifeq ($(OS_ARCH), NetBSD)
> +OS_MACHINE = $(shell uname -m)
> +LIBS += -l$(OS_MACHINE) -lpciutils

This introduces a dependency on pciutils which is not there in main
flashrom. For example, if you compile with
make CONFIG_INTERNAL=no CONFIG_SERPROG=no CONFIG_SATASII=no
CONFIG_DRKAISER=no CONFIG_NIC3COM=no CONFIG_BUSPIRATESPI=no
CONFIG_FT2232SPI=no
only dummy programmer support is available, and compilation should not
need pciutils (or any hardware access libraries).
At line 172 in Makefile there is a check if PCI support is needed. At
least the -lpciutils addition to LDFLAGS should be handled there.
> +endif
>
> CHIP_OBJS = jedec.o stm50flw0x0x.o w39v080fa.o sharplhf00l04.o w29ee011.o \
>    sst28sf040.o am29f040b.o mx29f002.o m29f400bt.o pm29f002.o w39v040c.o \
> Index: README
> ===================================================================
> --- README	(revision 837)
> +++ README	(working copy)
> @@ -65,10 +65,10 @@
>
> gmake LDFLAGS="-L$pathtolibpci" CC="gcc -I$pathtopciheaders" CFLAGS=-O2
>
> -To compile on DragonFly BSD, use:
> +To compile on NetBSD or DragonFly BSD, use:

Does this mean the additional -Wl,-rpath-link,/usr/pkg/lib is needed
under DragonFly BSD as well?
> ln -s /usr/pkg/include/pciutils pci
> gmake CFLAGS=-I. LDFLAGS="-L/usr/pkg/lib"
> gmake CFLAGS=-I. LDFLAGS="-L/usr/pkg/lib -Wl,-rpath-link,/usr/pkg/lib"
>
> To compile and run on Darwin/Mac OS X:
> Index: pio.h
>
> --- pio.h	(revision 0)
> +++ pio.h	(revision 0)

Can you merge pio.h into hwaccess.h? Having an additional header file
for hardware accesses clutters up the tree. Not sure how to handle the
"public domain" notice, though. Maybe state in the copyright notice in
hwaccess.h that the NetBSD parts are in the public domain?
> @@ -0,0 +1,52 @@
> +/*
> This file is in the public domain.
> */
>
> +#ifndef __PIO_H__
> +#define __PIO_H__
>
> +#include <stdint.h>
>
> +static inline void
> +outb(uint8_t value, uint16_t port)
> +{
> asm volatile ("outb %b0,%w1": :"a" (value), "Nd" (port));
>
> +}
>
> +static inline uint8_t
> +inb(uint16_t port)
> +{
> uint8_t value;
> asm volatile ("inb %w1,%0":"=a" (value):"Nd" (port));
> return value;
>
> +}
>
> +static inline void
> +outw(uint16_t value, uint16_t port)
> +{
> asm volatile ("outw %w0,%w1": :"a" (value), "Nd" (port));
>
> +}
>
> +static inline uint16_t
> +inw(uint16_t port)
> +{
> uint16_t value;
> asm volatile ("inw %w1,%0":"=a" (value):"Nd" (port));
> return value;
>
> +}
>
> +static inline void
> +outl(uint32_t value, uint16_t port)
> +{
> asm volatile ("outl %0,%w1": :"a" (value), "Nd" (port));
>
> +}
>
> +static inline uint32_t
> +inl(uint16_t port)
> +{
> uint32_t value;
> asm volatile ("inl %1,%0":"=a" (value):"Nd" (port));
> return value;
>
> +}
>
> +#endif /* !__PIO_H__ */

Overall, it looks good.

Regards,
Carl-Daniel