Re: GPIO revisited

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

Marc Balmer <marc%msys.ch@localhost> wrote:
> The diff itself can be found online at
> http://www.netbsd.org/~mbalmer/diffs/gpio_13.diff .  I have left older
> versions of the diff there as well for your amusement.  Apply it against
> NetBSD -current.

Few minor comments from very quick glance.

+               if (req->gp_name[0] != '\0') {
+                       pin = gpio_pinbyname(sc, req->gp_name);
+                       if (pin == -1)
+                               return EINVAL;
+               } else
+                       pin = req->gp_pin;
+               if (pin < 0 || pin >= sc->sc_npins)
+                       return EINVAL;

How about moving these checks into gpio_pinbyname() and just passing req?
Of course, in such case it would become gpio_pinbyreq() or something. It
might also be better to just return struct gpio_name or NULL, instead of
pin number (see further).

+               /* rename pin or new pin? */
+               if (set->gp_name2[0] != '\0') {
+                       found = 0;
+                       LIST_FOREACH(nm, &sc->sc_names, gp_next)
+                               if (nm->gp_pin == pin) {
+                                       strlcpy(nm->gp_name, set->gp_name2,
+                                           sizeof(nm->gp_name));
+                                       found = 1;
+                                       break;
+                               }
+                       if (!found) {
+                               nm = malloc(sizeof(struct gpio_name),
+                                   M_DEVBUF, M_WAITOK);
+                               strlcpy(nm->gp_name, set->gp_name2,
+                                   sizeof(nm->gp_name));
+                               nm->gp_pin = set->gp_pin;
+                               LIST_INSERT_HEAD(&sc->sc_names, nm, gp_next);
+                       }
+               }

- What prevents from creation of duplicates?
- It re-scans sc_names, while pointer to struct gpio_name could be re-used.
- Any reason why this (and one other use) is not using kmem(9)?

+#define        GPIOSIM_NPINS   32
+
+struct gpiosim_softc {
+       struct device           sc_dev;
+       u_int32_t               sc_state;
+       struct gpio_chipset_tag sc_gpio_gc;
+       gpio_pin_t              sc_gpio_pins[GPIOSIM_NPINS];
+};

+void
+gpiosim_pin_write(void *arg, int pin, int value)
+{
+       struct gpiosim_softc *sc = (struct gpiosim_softc *)arg;
+
+       if (value == 0)
+               sc->sc_state &= ~(1 << pin);
+       else
+               sc->sc_state |= (1 << pin);
+}

Perhaps add comment and KASSERT(pin < GPIOSIM_NPINS) here or somewhere else,
since this bitmask assumes not more than 32 pins.

+int    gpio_pinbyname(struct gpio_softc *, char *gp_name);
+
+/* Old API version */
+int    gpio_ioctl_oapi(struct gpio_softc *, u_long cmd, void *data, int flag,
+    int pinset);

KNF: no variables names in the declaration.
Also, any reason why these are not static?

+struct gpiosim_op {
+       void *cookie;
+       u_int32_t mask;
+       u_int32_t state;
+};

We use standard C99 types: uint32_t, etc.

+               bzero(&ga, sizeof(ga));
+               ga.ga_gpio = sc;

bzero/bcopy/etc are no longer used, please use memset. Same in userland.


P.S. Please create diffs with -p option. Thanks.

-- 
Mindaugas