gpiobus_hinted_child >32 pins support, pin_getname method,
and gpio-sysctl bridge patch
Warner Losh
imp at bsdimp.com
Sun Aug 19 15:39:00 UTC 2012
On Aug 19, 2012, at 2:17 AM, Hiroki Sato wrote:
> Hi,
Hi Sato-san,
In general, I like this code in the context of the current GPIO framework. I've been growing dissatisfied with the current GPIO framework, however, and some of my comments reflect that more than any comments about this specific code.
> Can you review the attached patches? The first one is to add >32 pins
> support for hint.devname.%d.pins and pin_getname method into gpiobus
> similar to gpio interface. After applying this, the following hint
> will be supported:
>
> hint.gpioled.0.at="gpiobus0"
> hint.gpioled.0.pins0="0x00000000" # 41
> hint.gpioled.0.pins1="0x00000200"
This is, at best, awkward. Why not '41' here?
Why not hints.gpioled.0.pins0=41?
In general, the gpio stuff is too mask heavy and should be more pin oriented. We also need a more generic way to pass around all the GPIO pins used for aux functions of embedded devices. which pin is used for media status interrupts, write protect detection, vcc power, etc.
Looks a lot like we already keep book internally in pins.
> hint.gpioled.0.pins can still be used when the pin number is lower
> than 32. The addition of pin_getname method is to reuse pin name if
> it is already defined in the parent gpio driver. The maximum of the
> pin number is limited by GPIOBUS_PINS_MAX (currently 128).
>
> Another one is a simple GPIO-sysctl bridge driver. Currently it
> supports read operation only, but it is easy to add write one.
I was rather hoping to recast much of the GPIO stuff so we can also support interrupts based on pin numbers for any device in the tree. I'd like this for a gpiokey generic interface that devices can register button presses. How does one configure in vs out for these pins? And how does one guard against trying to use the GPIO pins for things like ethernet? Again, that may be beyond the scope of what you are trying to achieve.
> I confirmed if the both work fine by using several Marvell
> 88F628[12]-based boards.
>
> -- Hiroki
> Index: sys/dev/gpio/gpiobus.c
> ===================================================================
> --- sys/dev/gpio/gpiobus.c (revision 239381)
> +++ sys/dev/gpio/gpiobus.c (working copy)
> @@ -40,6 +40,7 @@
> #include <machine/bus.h>
> #include <sys/rman.h>
> #include <machine/resource.h>
> +#include <machine/_inttypes.h>
>
> #include <sys/gpio.h>
> #include <dev/gpio/gpiobusvar.h>
> @@ -47,7 +48,7 @@
> #include "gpiobus_if.h"
>
> static void gpiobus_print_pins(struct gpiobus_ivar *);
> -static int gpiobus_parse_pins(struct gpiobus_softc *, device_t, int);
> +static int gpiobus_parse_pins(struct gpiobus_softc *, device_t, int *, size_t);
> static int gpiobus_probe(device_t);
> static int gpiobus_attach(device_t);
> static int gpiobus_detach(device_t);
> @@ -69,6 +70,7 @@
> static int gpiobus_pin_setflags(device_t, device_t, uint32_t, uint32_t);
> static int gpiobus_pin_getflags(device_t, device_t, uint32_t, uint32_t*);
> static int gpiobus_pin_getcaps(device_t, device_t, uint32_t, uint32_t*);
> +static int gpiobus_pin_getname(device_t, device_t, uint32_t, char *);
> static int gpiobus_pin_set(device_t, device_t, uint32_t, unsigned int);
> static int gpiobus_pin_get(device_t, device_t, uint32_t, unsigned int*);
> static int gpiobus_pin_toggle(device_t, device_t, uint32_t);
> @@ -119,15 +121,18 @@
> }
>
> static int
> -gpiobus_parse_pins(struct gpiobus_softc *sc, device_t child, int mask)
> +gpiobus_parse_pins(struct gpiobus_softc *sc, device_t child, int *mask,
> + size_t masklen)
I'd be tempted to adapt a different specification method for > 32bit devices. But that may be beyond the scope of what you are trying to do.
Might not be bad to document the args, but that, sadly, doesn't follow the rest of the file...
> {
> struct gpiobus_ivar *devi = GPIOBUS_IVAR(child);
> - int i, npins;
> + int i, j, pin, npins;
>
> npins = 0;
> - for (i = 0; i < 32; i++) {
> - if (mask & (1 << i))
> - npins++;
> + for (i = 0; i < masklen; i++) {
> + for (j = 0; j < sizeof(mask[i]) * 8; j++) {
> + if (mask[i] & (1 << j))
> + npins++;
> + }
> }
>
> if (npins == 0) {
> @@ -143,27 +148,30 @@
> return (ENOMEM);
>
> npins = 0;
> - for (i = 0; i < 32; i++) {
> + for (i = 0; i < masklen; i++) {
> + for (j = 0; j < sizeof(mask[i]) * 8; j++) {
> + if ((mask[i] & (1 << j)) == 0)
> + continue;
>
> - if ((mask & (1 << i)) == 0)
> - continue;
> -
> - if (i >= sc->sc_npins) {
> - device_printf(child,
> - "invalid pin %d, max: %d\n", i, sc->sc_npins - 1);
> - return (EINVAL);
> + pin = i * sizeof(mask[i]) * 8 + j;
> + if (pin >= sc->sc_npins) {
> + device_printf(child,
> + "invalid pin %d, max: %d\n", pin,
> + sc->sc_npins - 1);
> + return (EINVAL);
> + }
> + devi->pins[npins++] = pin;
> + /*
> + * Mark pin as mapped and give warning if it's
> + * already mapped
> + */
> + if (sc->sc_pins_mapped[pin]) {
> + device_printf(child,
> + "warning: pin %d is already mapped\n", pin);
> + return (EINVAL);
> + }
> + sc->sc_pins_mapped[pin] = 1;
> }
> -
> - devi->pins[npins++] = i;
> - /*
> - * Mark pin as mapped and give warning if it's already mapped
> - */
> - if (sc->sc_pins_mapped[i]) {
> - device_printf(child,
> - "warning: pin %d is already mapped\n", i);
> - return (EINVAL);
> - }
> - sc->sc_pins_mapped[i] = 1;
> }
>
> return (0);
> @@ -192,6 +200,8 @@
> * Increase to get number of pins
> */
> sc->sc_npins++;
> + if (bootverbose)
> + device_printf(dev, "number of pins = %d\n", sc->sc_npins);
>
> KASSERT(sc->sc_npins != 0, ("GPIO device with no pins"));
>
> @@ -304,19 +314,47 @@
> return (child);
> }
>
> +#define GPIOBUS_PINS_MAX 128
> +static int pinmask[(GPIOBUS_PINS_MAX - 1) / (sizeof(int) * 8) + 1];
> +static char pinresname[] = "pinsNNN";
> +
> static void
> gpiobus_hinted_child(device_t bus, const char *dname, int dunit)
> {
> struct gpiobus_softc *sc = GPIOBUS_SOFTC(bus);
> struct gpiobus_ivar *devi;
> device_t child;
> - int pins;
> + int i, error;
>
> + bzero((char *)&pinmask, sizeof(pinmask));
>
> child = BUS_ADD_CHILD(bus, 0, dname, dunit);
> devi = GPIOBUS_IVAR(child);
> - resource_int_value(dname, dunit, "pins", &pins);
> - if (gpiobus_parse_pins(sc, child, pins))
> + if (resource_int_value(dname, dunit, "pins", &pinmask[0]) == 0) {
> + /*
> + * Backward compatibility for 32-pin configuration in a
> + * 32-bit hint:
> + *
> + * hint.devname.%d.pins=0x00000000
> + */
> + i = 1;
> + } else {
> + /*
> + * A 32-bit hint configures a 32-pin block:
> + *
> + * hint.devname.%d.pins0=0x00000000 # 0..31
> + * hint.devname.%d.pins1=0x00000000 # 32..63
> + * hint.devname.%d.pins2=0x00000000 # 64..91
> + */
> + for (i = 0; i < sizeof(pinmask)/sizeof(pinmask[0]); i++) {
> + snprintf(pinresname, sizeof(pinresname), "pins%d", i);
> + error = resource_int_value(dname, dunit, pinresname,
> + &pinmask[i]);
> + if (error)
> + break;
> + }
> + }
> + if (gpiobus_parse_pins(sc, child, pinmask, i))
> device_delete_child(bus, child);
> }
>
> @@ -409,6 +447,18 @@
> }
>
> static int
> +gpiobus_pin_getname(device_t dev, device_t child, uint32_t pin, char *name)
> +{
> + struct gpiobus_softc *sc = GPIOBUS_SOFTC(dev);
> + struct gpiobus_ivar *devi = GPIOBUS_IVAR(child);
> +
> + if (pin >= devi->npins)
> + return (EINVAL);
> +
> + return GPIO_PIN_GETNAME(sc->sc_dev, devi->pins[pin], name);
> +}
> +
> +static int
> gpiobus_pin_set(device_t dev, device_t child, uint32_t pin,
> unsigned int value)
> {
> @@ -469,6 +519,7 @@
> DEVMETHOD(gpiobus_release_bus, gpiobus_release_bus),
> DEVMETHOD(gpiobus_pin_getflags, gpiobus_pin_getflags),
> DEVMETHOD(gpiobus_pin_getcaps, gpiobus_pin_getcaps),
> + DEVMETHOD(gpiobus_pin_getname, gpiobus_pin_getname),
> DEVMETHOD(gpiobus_pin_setflags, gpiobus_pin_setflags),
> DEVMETHOD(gpiobus_pin_get, gpiobus_pin_get),
> DEVMETHOD(gpiobus_pin_set, gpiobus_pin_set),
> Index: sys/dev/gpio/gpiobus_if.m
> ===================================================================
> --- sys/dev/gpio/gpiobus_if.m (revision 239381)
> +++ sys/dev/gpio/gpiobus_if.m (working copy)
> @@ -111,6 +111,16 @@
> };
>
> #
> +# Get pin name
> +#
> +METHOD int pin_getname {
> + device_t dev;
> + device_t child;
> + uint32_t pin_num;
> + char *name;
> +};
> +
> +#
> # Set current configuration and capabilities
> #
> METHOD int pin_setflags {
> Index: sys/dev/gpio/gpioled.c
> ===================================================================
> --- sys/dev/gpio/gpioled.c (revision 239381)
> +++ sys/dev/gpio/gpioled.c (working copy)
> @@ -94,20 +94,25 @@
> {
> struct gpioled_softc *sc;
> const char *name;
> + char nbuf[GPIOMAXNAME];
>
> sc = device_get_softc(dev);
> sc->sc_dev = dev;
> sc->sc_busdev = device_get_parent(dev);
> GPIOLED_LOCK_INIT(sc);
> if (resource_string_value(device_get_name(dev),
> - device_get_unit(dev), "name", &name))
> - name = NULL;
> -
> + device_get_unit(dev), "name", &name) != 0) {
> + GPIOBUS_PIN_GETNAME(sc->sc_busdev, sc->sc_dev, 0, nbuf);
> + nbuf[sizeof(nbuf) - 1] = '\0';
> + if (nbuf[0] != '\0')
> + name = nbuf;
> + else
> + name = device_get_nameunit(dev);
> + }
> GPIOBUS_PIN_SETFLAGS(sc->sc_busdev, sc->sc_dev, GPIOLED_PIN,
> GPIO_PIN_OUTPUT);
>
> - sc->sc_leddev = led_create(gpioled_control, sc, name ? name :
> - device_get_nameunit(dev));
> + sc->sc_leddev = led_create(gpioled_control, sc, name);
>
> return (0);
> }
> Index: sys/conf/files
> ===================================================================
> --- sys/conf/files (revision 239381)
> +++ sys/conf/files (working copy)
> @@ -1268,6 +1268,7 @@
> dependency "gpio_if.h"
> dev/gpio/gpioiic.c optional gpioiic
> dev/gpio/gpioled.c optional gpioled
> +dev/gpio/gpiosysctl.c optional gpiosysctl
> dev/gpio/gpio_if.m optional gpio
> dev/gpio/gpiobus_if.m optional gpio
> dev/hatm/if_hatm.c optional hatm pci
> Index: sys/dev/gpio/gpiosysctl.c
> ===================================================================
> --- sys/dev/gpio/gpiosysctl.c (revision 0)
> +++ sys/dev/gpio/gpiosysctl.c (working copy)
> @@ -0,0 +1,197 @@
> +/*-
> + * Copyright (c) 2012 Hiroki Sato <hrs at FreeBSD.org>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include <sys/cdefs.h>
> +__FBSDID("$FreeBSD$");
> +
> +#include <sys/param.h>
> +#include <sys/systm.h>
> +#include <sys/kernel.h>
> +#include <sys/module.h>
> +#include <sys/bus.h>
> +#include <sys/sysctl.h>
> +#include <sys/lock.h>
> +#include <sys/mutex.h>
> +
> +#include <machine/fdt.h>
> +#include <dev/fdt/fdt_common.h>
> +#include <dev/ofw/ofw_bus.h>
> +#include <dev/ofw/ofw_bus_subr.h>
> +
> +#include <sys/gpio.h>
> +
> +#include "gpio_if.h"
> +#include "gpiobus_if.h"
> +
> +struct gpiosysctl_softc
> +{
> + device_t sc_dev;
> + device_t sc_busdev;
> + struct mtx sc_mtx;
> +};
> +
> +#define GPIO_SYSCTL_DEVNAME "gpiosysctl"
> +#define GPIO_SYSCTL_LOCK(sc) mtx_lock(&(sc)->sc_mtx)
> +#define GPIO_SYSCTL_UNLOCK(sc) mtx_unlock(&(sc)->sc_mtx)
> +#define GPIO_SYSCTL_LOCK_INIT(sc) mtx_init(&(sc)->sc_mtx, \
> + device_get_nameunit((sc)->sc_dev), \
> + GPIO_SYSCTL_DEVNAME, MTX_DEF)
> +#define GPIO_SYSCTL_LOCK_DESTROY(sc) mtx_destroy(&(sc)->sc_mtx)
> +
> +static int gs_probe(device_t);
> +static int gs_attach(device_t);
> +static int gs_detach(device_t);
> +
> +static int
> +gs_probe(device_t dev)
> +{
> +
> + device_set_desc(dev, "GPIO-sysctl bridge driver");
> +
> + return (0);
> +}
> +
> +static int
> +gs_gpio_in(struct gpiosysctl_softc *sc)
> +{
> + int val;
> +
> + GPIO_SYSCTL_LOCK(sc);
> + GPIOBUS_LOCK_BUS(sc->sc_busdev);
> + GPIOBUS_ACQUIRE_BUS(sc->sc_busdev, sc->sc_dev);
> + GPIOBUS_PIN_GET(sc->sc_busdev, sc->sc_dev, 0, &val);
> + GPIOBUS_RELEASE_BUS(sc->sc_busdev, sc->sc_dev);
> + GPIOBUS_UNLOCK_BUS(sc->sc_busdev);
> + GPIO_SYSCTL_UNLOCK(sc);
> +
> + return (val);
> +}
> +
> +static void
> +gs_gpio_out(struct gpiosysctl_softc *sc, int val)
> +{
> + GPIO_SYSCTL_LOCK(sc);
> + GPIOBUS_LOCK_BUS(sc->sc_busdev);
> + GPIOBUS_ACQUIRE_BUS(sc->sc_busdev, sc->sc_dev);
> + GPIOBUS_PIN_SET(sc->sc_busdev, sc->sc_dev, 0,
> + (val == 0) ? GPIO_PIN_LOW : GPIO_PIN_HIGH);
> + GPIOBUS_RELEASE_BUS(sc->sc_busdev, sc->sc_dev);
> + GPIOBUS_UNLOCK_BUS(sc->sc_busdev);
> + GPIO_SYSCTL_UNLOCK(sc);
> +}
> +
> +static int
> +gs_sysctl_handler(SYSCTL_HANDLER_ARGS)
> +{
> + device_t dev;
> + struct gpiosysctl_softc *sc;
> + int val, ret;
> +
> + dev = (device_t)arg1;
> + sc = device_get_softc(dev);
> + if (req->newptr == NULL) {
> + val = gs_gpio_in(sc);
> + ret = sysctl_handle_int(oidp, &val, 0, req);
> + } else {
> + ret = sysctl_handle_int(oidp, &val, 0, req);
> + if (ret < 0 || ret > 1) {
> + device_printf(dev, "%d: invalid value.\n", ret);
> + ret = 0;
> + }
> + gs_gpio_out(sc, val);
> + ret = val;
> + }
> +
> + return (ret);
> +}
> +
> +static int
> +gs_attach(device_t dev)
> +{
> + struct gpiosysctl_softc *sc;
> + struct sysctl_ctx_list *ctx;
> + struct sysctl_oid *oid;
> + device_t pdev;
> + const char *name;
> + char nbuf[GPIOMAXNAME];
> + int flags;
> +
> + sc = device_get_softc(dev);
> + sc->sc_dev = dev;
> + sc->sc_busdev = device_get_parent(dev);
> + GPIO_SYSCTL_LOCK_INIT(sc);
> +
> + if (resource_string_value(device_get_name(dev), device_get_unit(dev),
> + "name", &name) != 0) {
> + GPIOBUS_PIN_GETNAME(sc->sc_busdev, sc->sc_dev, 0, nbuf);
> + nbuf[sizeof(nbuf) - 1] = '\0';
> + if (nbuf[0] != '\0')
> + name = nbuf;
> + else {
> + device_printf(dev, "no valid name.\n");
> + return (ENXIO);
> + }
> + }
> + ctx = device_get_sysctl_ctx(dev);
> +
> + pdev = device_get_parent(dev);
> + oid = device_get_sysctl_tree(pdev);
> + flags = CTLTYPE_INT | CTLFLAG_RD;
> + SYSCTL_ADD_PROC(ctx, SYSCTL_CHILDREN(oid), OID_AUTO, name, flags, dev,
> + 0, gs_sysctl_handler, "I", NULL);
> +
> + return (0);
> +}
> +
> +static int
> +gs_detach(device_t dev)
> +{
> + struct gpiosysctl_softc *sc;
> +
> + sc = device_get_softc(dev);
> + GPIO_SYSCTL_LOCK_DESTROY(sc);
> +
> + return (0);
> +}
> +
> +static devclass_t gs_devclass;
> +
> +static device_method_t gs_methods[] = {
> + /* Device interface */
> + DEVMETHOD(device_probe, gs_probe),
> + DEVMETHOD(device_attach, gs_attach),
> + DEVMETHOD(device_detach, gs_detach),
> + { 0, 0 },
> +};
> +
> +static driver_t gs_driver = {
> + GPIO_SYSCTL_DEVNAME,
> + gs_methods,
> + sizeof(struct gpiosysctl_softc),
> +};
> +
> +DRIVER_MODULE(gpiosysctl, gpiobus, gs_driver, gs_devclass, 0, 0);
> +MODULE_VERSION(gpiosysctl, 1);
More information about the freebsd-arm
mailing list