Re: SMSC LAN9118 Family driver

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

> I will commit the support for 'SMSC LAN9118 Family driver'(slan) at
> next week.

I'm still afraid some people might complain that the name 'slan'
is too generic and ambiguous. (it could imply something like vlan(4))

How about smh(4)?
- we already have sm(4) for SMC 91c100/91c96 (FEAST)
- SMSC calls 9115/9116/9117/9118  "High Performance" or "Highly Efficient"
  http://www.smsc.com/index.php?tid=145
  (I guess that's because many people claim the FEAST is
   the "worst" controller, as src/sys/dev/ic/rtl81x9.c says ;-p)

> Please reveiw.  Moreover, review the included man page.  ;-)

With a quick glance, the driver itself looks good
except some minor comments:

>+      val = bus_space_read_4(sc->sc_iot, sc->sc_ioh, LAN9118_BYTE_TEST);
>+      if (sc->sc_flags & LAN9118_FLAGS_SWAP)
>+              val = bswap32(val);
>+      if (val != LAN9118_BYTE_TEST_VALUE) {
>+              aprint_error_dev(sc->sc_dev, "failed to detect chip\n");
>+              return EINVAL;
>+      }

I wonder if LAN9118_FLAGS_SWAP should be handled by MD bus_space(9) or not.

>+              DPRINTFN(1, ("lan9118_attach: need byte swap\n"));

It's better to use __func__ for debug printfs.

> +     while (bus_space_read_4(sc->sc_iot, sc->sc_ioh, LAN9118_MAC_CSR_CMD) &
> +         LAN9118_MAC_CSR_CMD_BUSY);

It might be better to put some timeout check in possible infinite loops.
(and KNF says "continue;" is better for an explicit no-op loop)

>+      if (myea == NULL) {
 :
>+      } else
>+              /* We set MACaddr to MAC CSR, after Soft reset. */
>+              memcpy(sc->sc_enaddr, myea, ETHER_ADDR_LEN);

I'm not sure how/when backends pass the myea values, but if it
could be board specific, it might be better to pass it via proplib
like wm(4) for iyonix.

>+                      * We are assuming that the size of mbus always align
>+                      * in 4 bytes.

This assumption seems problematic. Strictly speaking, we should
copy data to safe temporary buffer to avoid possible data leak.
(but I think it's acceptable for initial commit)

>+      timo = 5 * hz;  /* XXXX 5sec */

mstohz(9)?

>+              bus_space_read_multi_4(sc->sc_iot, sc->sc_ioh, LAN9118_RXDFIFOP,
>+                  mtod(m, uint32_t *),
>+                  roundup(pad + pktlen, sizeof(uint32_t)) >> 2);
>+              m->m_data += 2;

Inconsistent use of "pad" vs "2"?

>+              m->m_flags |= M_HASFCS;

I think M_HASFCS is deprecated and we should adjust ETHER_CRC_LEN
for m_len etc. directly as other drivers:
http://mail-index.NetBSD.org/port-arm/2004/03/31/0001.html
bpf(4) could also be confused, as noted in PR kern/28339.

BTW, do you have a backend driver for ISA variants?
If not, it should not be mentioned in man page yet.
---
Izumi Tsutsui