Re: GPIO revisited

[Marc Balmer <marc%msys.ch@localhost>]

Tank you for your feedback.  See a few comments inline...

> > 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).

This would not make to much sense, since I really only need the pin  
number.

> +               /* 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?

Nothing.  I took a note to fix this.

> - 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)?

netbsd# man 9 kmem
man: no entry for kmem in the manual.


> +#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.

no, that check is performed further up in the ioctl code.  only valid  
pin numbers are passed down to the actual drivers.

> +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