[lkml]   [2007]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [patch/rfc 1/4] GPIO implementation framework
    On Monday 12 November 2007, eric miao wrote:
    > Hi David,
    > I hope I was not late giving my humble feedback on this framework :-)
    > Can we use "per gpio based" structure instead of "per gpio_chip" based one,
    > just like what the generic IRQ layer is doing nowadays?

    We "can" do most anything. What would that improve though?

    Each irq_chip handles multiple IRQs ... just like this patch
    has each gpio_chip handling multiple GPIOs. (Not that I think
    GPIO code should closely model IRQ code; they need to address
    very different problems.)

    I can't tell what you intend to suggest as a "per-GPIO" data
    structure; since I can think of at least three different ways
    to do such a thing, you should be more concrete. I'd think it
    should be in *addition* to a gpio_chip structure though.

    > So that
    > a. you don't have to declare per gpio_chip "can_sleep", "is_out" and
    > "requested".
    > Those will be just bits of properties of a single GPIO.

    The can_sleep value is a per-controller thing. The other bits are
    indeed per-GPIO.

    So do you mean a structure with two bits, plus a pointer to a
    gpio_chip, plus likely other stuff (what?) to make it work?
    What would the hot-path costs be (for getting/setting values of
    an on-chip GPIO)?

    > b. and furthur more, one can avoid the use of ARCH_GPIOS_PER_CHIP, which
    > leads to many holes

    Why should holes (in the GPIO number sequence) be a problem? In
    this code, they don't cost much space at all. They'd cost more
    if there were a per-GPIO structure though...

    The only downside of GPIOS_PER_CHIP that I know of right now
    is that it complicates mixing gpio bank sizes; it's a ceiling,
    some controllers would allocate more than they need. The
    upside of that is efficiency, and a closer match to how
    underlying hardware works.

    Of course, GPIOS_PER_CHIP *could* be decoupled from how the
    table of gpio_chip pointers is managed. If the table were to
    group GPIOs in units of 8, a gpio_chip with 32 GPIOs could
    take four adjacent entries while an 8-bit GPIO expander could
    take just one. That'd be a very easy patch, supporting a more
    dense allocation of GPIO numbers... although it would increase
    static memory consumption by typically NR_GPIOS/4 pointers.

    > c. gpio_to_chip() will be made easy and straight forward

    I'd say "return chips[gpio / ARCH_GPIOS_PER_CHIP]" already meets
    both criteria!

    There's also "efficient" to consider; this way doesn't cost much
    memory or add levels of indirection (compared to most platforms,
    which already use a similar array).

    > d. granularity of spin_lock()/_unlock() can be made small
    > (per GPIO instead of per gpio_chip)

    Why would per-GPIO locking be needed though? Look again...

    The locking is there fundamentally because gpio_chip structures
    may need to be unregistered; that's not a per-gpio issue.
    Even when a gpio is marked in chip->requested under that lock,
    that's part of ensuring that the unregistration is prevented so
    long as the GPIO is in active use.

    Plus, fine grained locking is rarely a good idea; it normally
    increases locking overhead by involving multiple locks. Only
    add extra locks if a single lock sees too much contention; and
    even then, only if that contention can't be removed by using a
    smarter design.

    - Dave

    > What do you think?
    > - eric
    > On Nov 6, 2007 5:05 AM, David Brownell <> wrote:
    > > On Monday 29 October 2007, David Brownell wrote:
    > > >
    > > > Provides new implementation infrastructure that platforms may choose to use
    > > > when implementing the GPIO programming interface. Platforms can update their
    > > > GPIO support to use this. The downside is slower access to non-inlined GPIOs;
    > > > rarely a problem except when bitbanging some protocol.
    > >
    > > I was asked just what that overhead *is* ... and it surprised me.
    > > A summary of the results is appended to this note.
    > >
    > > Fortuntely it turns out those problems all go away if the gpiolib
    > > code uses a *raw* spinlock to guard its table lookups. With a raw
    > > spinlock, any performance impact of gpiolib seems to be well under
    > > a microsecond in this bitbang context (and not objectionable).
    > > Preempt became free; enabling debug options had only a minor cost.
    > >
    > > That's as it should be, since the only substantive changes were to
    > > grab and release a lock, do one table lookup a bit differently, and
    > > add one indirection function call ... changes which should not have
    > > any visible performance impact on per-bit codepaths, and one might
    > > expect to cost on the order of one dozen instructions.
    > >
    > >
    > > So the next version of this code will include a few minor bugfixes,
    > > and will also use a raw spinlock to protect that table. A raw lock
    > > seems appropriate there in any case, since non-sleeping GPIOs should
    > > be accessible from hardirq contexts even on RT kernels.
    > >
    > > If anyone has any strong arguments against using a raw spinlock
    > > to protect that table, it'd be nice to know them sooner rather
    > > than later.
    > >
    > > - Dave
    > >
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2007-11-13 20:13    [W:0.028 / U:11.812 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site