Date: Sun, 19 Aug 2012 09:38:52 -0600 From: Warner Losh <imp@bsdimp.com> To: Hiroki Sato <hrs@FreeBSD.org> Cc: gonzo@FreeBSD.org, freebsd-arm@FreeBSD.org Subject: Re: gpiobus_hinted_child >32 pins support, pin_getname method, and gpio-sysctl bridge patch Message-ID: <8CDAB51C-14A0-42F0-8E16-43A3EABA2703@bsdimp.com> In-Reply-To: <20120819.171723.523519054460575158.hrs@allbsd.org> References: <20120819.171723.523519054460575158.hrs@allbsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
>=20
> hint.gpioled.0.at=3D"gpiobus0"
> hint.gpioled.0.pins0=3D"0x00000000" # 41
> hint.gpioled.0.pins1=3D"0x00000200"
This is, at best, awkward. Why not '41' here?
Why not hints.gpioled.0.pins0=3D41?
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).
>=20
> 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.
>=20
> -- Hiroki
> Index: sys/dev/gpio/gpiobus.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- 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>
>=20
> #include <sys/gpio.h>
> #include <dev/gpio/gpiobusvar.h>
> @@ -47,7 +48,7 @@
> #include "gpiobus_if.h"
>=20
> 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 @@
> }
>=20
> 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 =3D GPIOBUS_IVAR(child);
> - int i, npins;
> + int i, j, pin, npins;
>=20
> npins =3D 0;
> - for (i =3D 0; i < 32; i++) {
> - if (mask & (1 << i))
> - npins++;
> + for (i =3D 0; i < masklen; i++) {
> + for (j =3D 0; j < sizeof(mask[i]) * 8; j++) {
> + if (mask[i] & (1 << j))
> + npins++;
> + }
> }
>=20
> if (npins =3D=3D 0) {
> @@ -143,27 +148,30 @@
> return (ENOMEM);
>=20
> npins =3D 0;
> - for (i =3D 0; i < 32; i++) {
> + for (i =3D 0; i < masklen; i++) {
> + for (j =3D 0; j < sizeof(mask[i]) * 8; j++) {
> + if ((mask[i] & (1 << j)) =3D=3D 0)
> + continue;
>=20
> - if ((mask & (1 << i)) =3D=3D 0)
> - continue;
> -
> - if (i >=3D sc->sc_npins) {
> - device_printf(child,
> - "invalid pin %d, max: %d\n", i, sc->sc_npins =
- 1);
> - return (EINVAL);
> + pin =3D i * sizeof(mask[i]) * 8 + j;
> + if (pin >=3D sc->sc_npins) {
> + device_printf(child,
> + "invalid pin %d, max: %d\n", pin,
> + sc->sc_npins - 1);
> + return (EINVAL);
> + }
> + devi->pins[npins++] =3D 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] =3D 1;
> }
> -
> - devi->pins[npins++] =3D 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] =3D 1;
> }
>=20
> return (0);
> @@ -192,6 +200,8 @@
> * Increase to get number of pins
> */
> sc->sc_npins++;
> + if (bootverbose)
> + device_printf(dev, "number of pins =3D %d\n", =
sc->sc_npins);
>=20
> KASSERT(sc->sc_npins !=3D 0, ("GPIO device with no pins"));
>=20
> @@ -304,19 +314,47 @@
> return (child);
> }
>=20
> +#define GPIOBUS_PINS_MAX 128
> +static int pinmask[(GPIOBUS_PINS_MAX - 1) / (sizeof(int) * 8) + 1];
> +static char pinresname[] =3D "pinsNNN";
> +
> static void
> gpiobus_hinted_child(device_t bus, const char *dname, int dunit)
> {
> struct gpiobus_softc *sc =3D GPIOBUS_SOFTC(bus);
> struct gpiobus_ivar *devi;
> device_t child;
> - int pins;
> + int i, error;
>=20
> + bzero((char *)&pinmask, sizeof(pinmask));
>=20
> child =3D BUS_ADD_CHILD(bus, 0, dname, dunit);
> devi =3D 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]) =3D=3D =
0) {
> + /*
> + * Backward compatibility for 32-pin configuration in a
> + * 32-bit hint:
> + *
> + * hint.devname.%d.pins=3D0x00000000
> + */
> + i =3D 1;
> + } else {
> + /*
> + * A 32-bit hint configures a 32-pin block:
> + *
> + * hint.devname.%d.pins0=3D0x00000000 # 0..31
> + * hint.devname.%d.pins1=3D0x00000000 # 32..63
> + * hint.devname.%d.pins2=3D0x00000000 # 64..91
> + */
> + for (i =3D 0; i < sizeof(pinmask)/sizeof(pinmask[0]); =
i++) {
> + snprintf(pinresname, sizeof(pinresname), =
"pins%d", i);
> + error =3D resource_int_value(dname, dunit, =
pinresname,
> + &pinmask[i]);
> + if (error)
> + break;
> + }
> + }
> + if (gpiobus_parse_pins(sc, child, pinmask, i))
> device_delete_child(bus, child);
> }
>=20
> @@ -409,6 +447,18 @@
> }
>=20
> static int
> +gpiobus_pin_getname(device_t dev, device_t child, uint32_t pin, char =
*name)
> +{
> + struct gpiobus_softc *sc =3D GPIOBUS_SOFTC(dev);
> + struct gpiobus_ivar *devi =3D GPIOBUS_IVAR(child);
> +
> + if (pin >=3D 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
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- sys/dev/gpio/gpiobus_if.m (revision 239381)
> +++ sys/dev/gpio/gpiobus_if.m (working copy)
> @@ -111,6 +111,16 @@
> };
>=20
> #
> +# 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
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- 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];
>=20
> sc =3D device_get_softc(dev);
> sc->sc_dev =3D dev;
> sc->sc_busdev =3D device_get_parent(dev);
> GPIOLED_LOCK_INIT(sc);
> if (resource_string_value(device_get_name(dev),
> - device_get_unit(dev), "name", &name))
> - name =3D NULL;
> -
> + device_get_unit(dev), "name", &name) !=3D 0) {
> + GPIOBUS_PIN_GETNAME(sc->sc_busdev, sc->sc_dev, 0, nbuf);
> + nbuf[sizeof(nbuf) - 1] =3D '\0';
> + if (nbuf[0] !=3D '\0')
> + name =3D nbuf;
> + else
> + name =3D device_get_nameunit(dev);
> + }
> GPIOBUS_PIN_SETFLAGS(sc->sc_busdev, sc->sc_dev, GPIOLED_PIN,
> GPIO_PIN_OUTPUT);
>=20
> - sc->sc_leddev =3D led_create(gpioled_control, sc, name ? name :
> - device_get_nameunit(dev));
> + sc->sc_leddev =3D led_create(gpioled_control, sc, name);
>=20
> return (0);
> }
> Index: sys/conf/files
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- 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
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- sys/dev/gpio/gpiosysctl.c (revision 0)
> +++ sys/dev/gpio/gpiosysctl.c (working copy)
> @@ -0,0 +1,197 @@
> +/*-
> + * Copyright (c) 2012 Hiroki Sato <hrs@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 =3D=3D 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 =3D (device_t)arg1;
> + sc =3D device_get_softc(dev);
> + if (req->newptr =3D=3D NULL) {
> + val =3D gs_gpio_in(sc);
> + ret =3D sysctl_handle_int(oidp, &val, 0, req);
> + } else {
> + ret =3D sysctl_handle_int(oidp, &val, 0, req);
> + if (ret < 0 || ret > 1) {
> + device_printf(dev, "%d: invalid value.\n", ret);
> + ret =3D 0;
> + }
> + gs_gpio_out(sc, val);
> + ret =3D 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 =3D device_get_softc(dev);
> + sc->sc_dev =3D dev;
> + sc->sc_busdev =3D device_get_parent(dev);
> + GPIO_SYSCTL_LOCK_INIT(sc);
> +
> + if (resource_string_value(device_get_name(dev), =
device_get_unit(dev),
> + "name", &name) !=3D 0) {
> + GPIOBUS_PIN_GETNAME(sc->sc_busdev, sc->sc_dev, 0, nbuf);
> + nbuf[sizeof(nbuf) - 1] =3D '\0';
> + if (nbuf[0] !=3D '\0')
> + name =3D nbuf;
> + else {
> + device_printf(dev, "no valid name.\n");
> + return (ENXIO);
> + }
> + }
> + ctx =3D device_get_sysctl_ctx(dev);
> +
> + pdev =3D device_get_parent(dev);
> + oid =3D device_get_sysctl_tree(pdev);
> + flags =3D 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 =3D device_get_softc(dev);
> + GPIO_SYSCTL_LOCK_DESTROY(sc);
> +
> + return (0);
> +}
> +
> +static devclass_t gs_devclass;
> +
> +static device_method_t gs_methods[] =3D {
> + /* Device interface */
> + DEVMETHOD(device_probe, gs_probe),
> + DEVMETHOD(device_attach, gs_attach),
> + DEVMETHOD(device_detach, gs_detach),
> + { 0, 0 },
> +};
> +
> +static driver_t gs_driver =3D {
> + GPIO_SYSCTL_DEVNAME,
> + gs_methods,
> + sizeof(struct gpiosysctl_softc),
> +};
> +
> +DRIVER_MODULE(gpiosysctl, gpiobus, gs_driver, gs_devclass, 0, 0);
> +MODULE_VERSION(gpiosysctl, 1);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8CDAB51C-14A0-42F0-8E16-43A3EABA2703>