Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Grant Likely]

Oops, forgot to cc tglx...

On Tue, May 3, 2011 at 3:09 PM, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
> On Mon, Apr 11, 2011 at 12:21:47PM +0100, Jamie Iles wrote:
>> Updated from v2 with change to use the set/dat registers for input/output
>> register pair devices and removed some rebasing breakage.
>>
>> Jamie Iles (7):
>>   basic_mmio_gpio: remove runtime width/endianness evaluation
>>   basic_mmio_gpio: convert to platform_{get,set}_drvdata()
>>   basic_mmio_gpio: allow overriding number of gpio
>>   basic_mmio_gpio: request register regions
>>   basic_mmio_gpio: detect output method at probe time
>>   basic_mmio_gpio: support different input/output registers
>>   basic_mmio_gpio: support direction registers
>>
>>  drivers/gpio/basic_mmio_gpio.c  |  390 +++++++++++++++++++++++++++++++--------
>>  include/linux/basic_mmio_gpio.h |    1 +
>>  2 files changed, 311 insertions(+), 80 deletions(-)
>
> Hey Jamie,
>
> Thanks a lot for putting this series together.
>
> While on the topic of a generic mmio gpio driver, I've been thinking a
> lot about things that Alan, Anton, and others have been doing, and I
> took a good look at the irq_chip_generic work[1] that tglx (cc'd) put
> together.
>
> There are two things that stood out.  Alan pointed out (IIRC) that a
> generic gpio driver should not require each bank to be encapsulated in
> a separate struct platform_device, and after mulling over it a while I
> agree.  It was also pointed out by Anton that often GPIO controllers
> are embedded into other devices register addresses intertwined with
> other gpio banks, or even other functions.
>
> In parallel, tglx posted the irq_chip_generic patch[1] which has to
> deal with pretty much the same set of issues.  I took a close look at
> how he handled it for interrupt controllers, and I think it is
> entirely appropriate to use the same pattern for creating a
> gpio_mmio_generic library.
>
> [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/113697
>
> So, the direction I would like to go is to split the basic_mmio_gpio
> drivers into two parts;
>  - a platform_driver, and
>  - a gpio_mmio_generic library.
>
> The platform driver would be responsible for parsing pdata and/or
> device tree node data, but would call into the gpio_mmio_generic
> library for actually registering the gpio banks.
>
> I envision the gpio_mmio_generic library would look something like
> this:
>
> First, a structure for storing the register offsets froma  base
> address:
>
> struct gpio_mmio_regs {
> };
>
> Next, a structure for each generic mmio gpio instance:
>
> struct gpio_mmio_generic {
>        spinlock_t      lock;
>
>        /* initialized by user */
>        unsigned long   reg_data;
>        unsigned long   reg_set;
>        unsigned long   reg_clr;
>        unsigned long   reg_dir;
>
>        void __iomem    *reg_base;
>
>        /* Runtime register value caches; may be initialized by user */
>        u32             data_cache;
>        u32             dir_cache;
>
>        /* Embedded gpio_chip.  Helpers functions set up accessors, but user
>         * can override before calling gpio_mmio_generic_add() */
>        struct gpio_chip chip;
> };
>
> And then some helpers for initializing/adding/removing the embedded gpio_chip:
>
> void gpio_mmio_generic_setup(struct gpio_mmio_generic *gmg, int register_width);
> int gpio_mmio_generic_add(struct gpio_mmio_generic *gmg);
> void gpio_mmio_generic_remove(struct gpio_mmio_generic *gmg);
>
> gpio_mmio_generic_setup() sets up the common callback ops in the
> embedded gpio_chip, but the decisions it makes could be overridden by
> the user before calling gpio_mmio_generic_add().
>
> I've not had time to prototype this yet, but I wanted to get it
> written down and out onto the list for feedback since I probably won't
> have any chance to get to it until after UDS.  Bonus points to anyone
> who wants to take the initiative to hack it together before I get to
> it.  Extra bonus points to anyone who also converts some of the other
> gpio controllers to use it.  :-D

And triple bonus points to anyone who thinks this is stupid and comes
up with a better approach.  :-)

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/