Date: Mon, 20 Aug 2012 11:51:02 +0900 (JST) From: Hiroki Sato <hrs@FreeBSD.org> To: imp@bsdimp.com 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: <20120820.115102.2041268449045028507.hrs@allbsd.org> In-Reply-To: <8CDAB51C-14A0-42F0-8E16-43A3EABA2703@bsdimp.com> References: <20120819.171723.523519054460575158.hrs@allbsd.org> <8CDAB51C-14A0-42F0-8E16-43A3EABA2703@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
----Security_Multipart0(Mon_Aug_20_11_51_02_2012_300)--
Content-Type: Multipart/Mixed;
boundary="--Next_Part(Mon_Aug_20_11_51_02_2012_584)--"
Content-Transfer-Encoding: 7bit
----Next_Part(Mon_Aug_20_11_51_02_2012_584)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Warner Losh <imp@bsdimp.com> wrote
in <8CDAB51C-14A0-42F0-8E16-43A3EABA2703@bsdimp.com>:
im>
im> On Aug 19, 2012, at 2:17 AM, Hiroki Sato wrote:
im>
im> > Hi,
im>
im> Hi Sato-san,
im>
im> In general, I like this code in the context of the current GPIO
im> framework. I've been growing dissatisfied with the current GPIO
im> framework, however, and some of my comments reflect that more than any
im> comments about this specific code.
im>
im> > Can you review the attached patches? The first one is to add >32 pins
im> > support for hint.devname.%d.pins and pin_getname method into gpiobus
im> > similar to gpio interface. After applying this, the following hint
im> > will be supported:
im> >
im> > hint.gpioled.0.at="gpiobus0"
im> > hint.gpioled.0.pins0="0x00000000" # 41
im> > hint.gpioled.0.pins1="0x00000200"
im>
im> This is, at best, awkward. Why not '41' here?
im>
im> Why not hints.gpioled.0.pins0=41?
Ah, the reason why I used the mask was to simply reuse the old
interface. The attached patch supports the following:
hint.gpioled.0.at="gpiobus0"
hint.gpioled.0.pins.0="10"
hint.gpioled.0.pins.1="41"
hint.gpioled.0.pins.2="43"
My primary frustration was it was limited to 32 pins, so I am
wondering if someone has a strong objection or not ;)
im> In general, the gpio stuff is too mask heavy and should be more pin
im> oriented. We also need a more generic way to pass around all the GPIO
im> pins used for aux functions of embedded devices. which pin is used
im> for media status interrupts, write protect detection, vcc power, etc.
im>
im> Looks a lot like we already keep book internally in pins.
im> I was rather hoping to recast much of the GPIO stuff so we can also
im> support interrupts based on pin numbers for any device in the tree.
im> I'd like this for a gpiokey generic interface that devices can
im> register button presses. How does one configure in vs out for these
im> pins? And how does one guard against trying to use the GPIO pins for
im> things like ethernet? Again, that may be beyond the scope of what you
im> are trying to achieve.
Agreed. I was exploring more generic way to configure GPIO
connection via DTS, hints, and/or gpiobus for some time, but it was
difficult to do such things without device-specific code. Although
in/out initialization for each pin can be done via DTS (when the gpio
driver attached and read gpios property), there seems no way to
configure pin-to-interrupt relationship in the bindings. I am not
familiar with DTS syntax but the interrupt-map property can be
utilized to specify it.
-- Hiroki
----Next_Part(Mon_Aug_20_11_51_02_2012_584)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="gpiobus-part-20120820-1.diff"
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,9 @@
#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_register_pins(struct gpiobus_softc *, device_t, int *,
+ size_t);
+static int gpiobus_parse_pinmask(int, int *, size_t);
static int gpiobus_probe(device_t);
static int gpiobus_attach(device_t);
static int gpiobus_detach(device_t);
@@ -69,6 +72,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);
@@ -82,6 +86,8 @@
#define GPIOBUS_ASSERT_LOCKED(_sc) mtx_assert(&_sc->sc_mtx, MA_OWNED);
#define GPIOBUS_ASSERT_UNLOCKED(_sc) mtx_assert(&_sc->sc_mtx, MA_NOTOWNED);
+#define GPIOBUS_PINS_MAX 128
+static char pinsmib[] = "pins.NNN";
static void
gpiobus_print_pins(struct gpiobus_ivar *devi)
@@ -119,17 +125,12 @@
}
static int
-gpiobus_parse_pins(struct gpiobus_softc *sc, device_t child, int mask)
+gpiobus_register_pins(struct gpiobus_softc *sc, device_t child, int *pin,
+ size_t npins)
{
struct gpiobus_ivar *devi = GPIOBUS_IVAR(child);
- int i, npins;
+ int i;
- npins = 0;
- for (i = 0; i < 32; i++) {
- if (mask & (1 << i))
- npins++;
- }
-
if (npins == 0) {
device_printf(child, "empty pin mask");
return (EINVAL);
@@ -142,34 +143,37 @@
if (!devi->pins)
return (ENOMEM);
- npins = 0;
- for (i = 0; i < 32; i++) {
-
- 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);
- }
-
- devi->pins[npins++] = i;
+ for (i = 0; i < npins; i++) {
+ devi->pins[i] = pin[i];
/*
* Mark pin as mapped and give warning if it's already mapped
*/
- if (sc->sc_pins_mapped[i]) {
+ if (sc->sc_pins_mapped[pin[i]]) {
device_printf(child,
- "warning: pin %d is already mapped\n", i);
+ "warning: pin %d is already mapped\n", pin[i]);
return (EINVAL);
}
- sc->sc_pins_mapped[i] = 1;
}
return (0);
}
static int
+gpiobus_parse_pinmask(int mask, int *pins, size_t pinmax)
+{
+ int i, npins;
+
+ npins = 0;
+ for (i = 0; i < sizeof(mask) * 8 && i < pinmax; i++) {
+ if (mask & (1 << i)) {
+ pins[npins++] = i;
+ }
+ }
+
+ return (npins);
+}
+
+static int
gpiobus_probe(device_t dev)
{
device_set_desc(dev, "GPIO bus");
@@ -192,6 +196,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"));
@@ -310,13 +316,40 @@
struct gpiobus_softc *sc = GPIOBUS_SOFTC(bus);
struct gpiobus_ivar *devi;
device_t child;
- int pins;
+ int i, error, mask, pins[GPIOBUS_PINS_MAX];
+ bzero((char *)&pins, sizeof(pins));
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", &mask) == 0) {
+ /*
+ * Backward compatibility for 32-pinmask configuration in a
+ * 32-bit hint:
+ *
+ * hint.devname.%d.pins=0x00000000
+ */
+ i = gpiobus_parse_pinmask(mask, pins,
+ sizeof(pins)/sizeof(pins[0]));
+ error = gpiobus_register_pins(sc, child, pins, i);
+ } else {
+ /*
+ * GPIO pin spec in the following MIB:
+ *
+ * hint.devname.%d.pins.0="41"
+ * hint.devname.%d.pins.1="42"
+ * hint.devname.%d.pins.2="44"
+ */
+ for (i = 0; i < sizeof(pins)/sizeof(pins[0]); i++) {
+ snprintf(pinsmib, sizeof(pinsmib), "pins.%d", i);
+ error = resource_int_value(dname, dunit, pinsmib,
+ &pins[i]);
+ if (error)
+ break;
+ }
+ error = gpiobus_register_pins(sc, child, pins, i);
+ }
+ if (error)
device_delete_child(bus, child);
}
@@ -409,6 +442,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 +514,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),
----Next_Part(Mon_Aug_20_11_51_02_2012_584)----
----Security_Multipart0(Mon_Aug_20_11_51_02_2012_300)--
Content-Type: application/pgp-signature
Content-Transfer-Encoding: 7bit
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (FreeBSD)
iEYEABECAAYFAlAxphYACgkQTyzT2CeTzy3DUwCgt47Z67bDOkxjtP9ii4Y8o3FD
28YAn0prC1RTEaCrTrHgGELjr4m5g2IH
=0sjj
-----END PGP SIGNATURE-----
----Security_Multipart0(Mon_Aug_20_11_51_02_2012_300)----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120820.115102.2041268449045028507.hrs>