Re: Another update for axe(4)

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

> My changes (for -current as of today) are attached.  (I still need to 
> update the axe(4) man page.)

Comments about only cosmetics:

> +#include <machine/bus.h>

We should use <sys/bus.h> nowadays,
but is this one actually required?

> +#ifdef __NetBSD__
> + #define letoh16 htole16
> + #define letoh32 htole32
> +#endif

I don't like these ifdefs.
We have had own le16toh(9) and le32toh(9) and
OpenBSD guys renamed them when they pulled them.

> +void axe_ax88178_init(struct axe_softc *);
> +void axe_ax88772_init(struct axe_softc *);

Why not Static?

> -     allmulti:
> +allmulti:

Not in KNF, but I like one space before labels.
(to avoid diff(1) thinks it's a function name)

> +     u_int16_t eeprom;

uintNN_t types are prefered, at least in new code.

> +void
> +axe_ax88772_init(struct axe_softc *sc)
> +{
> +     axe_cmd(sc, AXE_CMD_WRITE_GPIO, 0, 0x00b0, NULL);
> +     usbd_delay_ms(sc->axe_udev, 40);

KNF says "Insert an empty line if the function has no local variables."

> +     sc->axe_flags = axe_lookup(uaa->vendor, uaa->product)->axe_flags;

It looks sc->axe_flags should be set in axe_match()
where axe_lookup() is called first.

> +     memcpy(&sc->init_eaddr, &eaddr, sizeof(sc->init_eaddr));

Do we really have to save MAC address into softc?
Can't we refer it by "CLLADDR(ifp->if_sadl)"?

> -                     c->axe_buf = usbd_alloc_buffer(c->axe_xfer, AXE_BUFSZ);
> +                     c->axe_buf = usbd_alloc_buffer(c->axe_xfer,
> +                                                    sc->axe_bufsz);

KNF?

> +     u_char                  *buf;

This should be uint8_t?

> +                     if ((pktlen % 2) != 0)
> +                             pktlen++;

roundup2(9)?

> +struct axe_sframe_hdr {
> +     u_int16_t               len;
> +     u_int16_t               ilen;
> +} __packed;

Is __packed actually required?

---
Izumi Tsutsui