linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Grant Likely <grant.likely@secretlab.ca>
To: Roland Stigge <stigge@antcom.de>,
	gregkh@linuxfoundation.org, linus.walleij@linaro.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, w.sang@pengutronix.de,
	jbe@pengutronix.de, plagnioj@jcrosoft.com, highguy@gmail.com,
	broonie@opensource.wolfsonmicro.com, daniel-gl@gmx.net,
	rmallon@gmail.com, sr@denx.de, wg@grandegger.com,
	tru@work-microwave.de, mark.rutland@arm.com
Cc: Roland Stigge <stigge@antcom.de>
Subject: Re: [PATCH RESEND 1/6 v13] gpio: Add a block GPIO API to gpiolib
Date: Fri, 15 Feb 2013 14:47:00 +0000	[thread overview]
Message-ID: <20130215144700.35A1F3E044B@localhost> (raw)
In-Reply-To: <1358250716-21986-2-git-send-email-stigge@antcom.de>

On Tue, 15 Jan 2013 12:51:51 +0100, Roland Stigge <stigge@antcom.de> wrote:
> The recurring task of providing simultaneous access to GPIO lines (especially
> for bit banging protocols) needs an appropriate API.
> 
> This patch adds a kernel internal "Block GPIO" API that enables simultaneous
> access to several GPIOs. This is done by abstracting GPIOs to an n-bit word:
> Once requested, it provides access to a group of GPIOs which can range over
> multiple GPIO chips.
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>
> ---
> 
>  Documentation/gpio.txt     |   58 +++++++++++
>  drivers/gpio/gpiolib.c     |  227 +++++++++++++++++++++++++++++++++++++++++++++
>  include/asm-generic/gpio.h |   17 +++
>  include/linux/gpio.h       |   97 +++++++++++++++++++
>  4 files changed, 399 insertions(+)
> 
> --- linux-2.6.orig/Documentation/gpio.txt
> +++ linux-2.6/Documentation/gpio.txt
> @@ -481,6 +481,64 @@ exact name string of pinctrl device has
>  argument to this routine.

Hi Roland,

The first thing that jumps out at me on this is that it really is two
separate concepts implemented in one patch.
1) allow individual chips to expose a block API for that single controller.
2) creating a global block gpio interface for multiple arbitrary
groupings of chips

The first is relatively noncontroversial. It is easy to implement and
there are there are real performance issues that it addresses. If you
split that out as a separate patch it is something I think I can merge.
I do have some minor comments on this feature, but I'll put the details
below.

The second I'm not that thrilled with, or at least I think the
implementation is more complex than it needs to be. The big problem is
that it tries to abstract the fact that GPIOs may or may not be on the
same controller, and in doing so it has to do a bunch of housekeeping to
map 'virtual' gpio numbers to real gpios. I recognized that the feature
is needed to take advantage of gpiochip block access, but I'd like to
suggest a different implementation.

Instead of keeping track of separate block_gpio chip references, how
about an API that consolidates GPIO operations. For example (rough
sketch, and using the new gpiodesc infrastructure):

struct gpiocmd {
	struct gpio_desc *gpio;
	int data;
}

int gpio_set_sequence(struct gpiocmd *gpiocmd, int count)
{
	struct gpio_chip *gc = GPIO_DESC_TO_GPIOCHIP(gpiocmd->gpio);
	int bit = GPIO_DESC_TO_BIT(gpiocmd->gpio);
	unsigned long data, mask;

	/*
	 * Consolidate and execute the gpio commands. A little naive,
	 * but you get the idea.
	 *
	 * GPIO_DESC_TO_GPIOCHIP() and GPIO_DESC_TO_BIT() are fictions
	 * at the moment; something will need to be implemented here.
	 */
	mask = 1 << bit;
	data = gpiocmd->data << bit;
	for (i = 1; i < count; i++, gpiocmd++) {
		struct gpio_chip *nextgc = GPIO_DESC_TO_GPIOCHIP(gpiocmd->gpio):
		bit = GPIO_DESC_TO_BIT(gpiocmd->gpio);

		/* Consolidate if same gpio_chip and go to next iteration */
		if (gc == nextgc) {
			mask &= 1 << bit
			data &= gpiocmd->data << bit
			continue;
		}

		gc->set_block(gc, mask, data);

		gc = nextgc;
		mask = 1 << bit;
		data = gpiocmd->data << bit;
	}

	/* Last one because the loop alwasy exits with at least one more
	 * thing to do */
	gc->set_block(gc, mask, data);
}

And that's it. It maintains the abstraction that GPIOs are separate and
that assumptions cannot be made about how there are wired together, but
still allows any driver to take advantage of speedups with consolidating
operations. Drivers also get to use the same handles they are currently
using instead of a separate gpio_block namespace which means it
interworks nicely with the existing API.

It also should be lighter weight when it comes to speed of processing.
Normally I'm not too worried about that, but when it comes to GPIOs and
bitbanging it can end up having a really big cost.

Naturally the thing I hacked together above is just a draft. It can
certainly be refined. It might make sense for it to be a bitbang
statemachine that can handle both set, get and barrier/delay operations.
That might actually be simpler and better than having separate set & get
sequences with callers handing the bitbanging.

For performance, it might make sense to separate the consolidation pass
from the execution pass.

g.


  parent reply	other threads:[~2013-02-15 14:47 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-01-15 11:51 [PATCH RESEND 0/6 v13] gpio: Add block GPIO Roland Stigge
2013-01-15 11:51 ` [PATCH RESEND 1/6 v13] gpio: Add a block GPIO API to gpiolib Roland Stigge
2013-01-18 12:13   ` Stijn Devriendt
2013-01-18 13:18     ` Roland Stigge
2013-02-15 13:16     ` Grant Likely
2013-02-15 14:47   ` Grant Likely [this message]
2013-02-15 16:34   ` Grant Likely
2013-01-15 11:51 ` [PATCH RESEND 2/6 v13] gpio: Add sysfs support to block GPIO API Roland Stigge
2013-02-15 21:50   ` Grant Likely
2013-01-15 11:51 ` [PATCH RESEND 3/6 v13] gpio: Add userland device interface to block GPIO Roland Stigge
2013-01-15 11:51 ` [PATCH RESEND 4/6 v13] gpiolib: Fix default attributes for class Roland Stigge
2013-02-15 22:55   ` Grant Likely
2013-01-15 11:51 ` [PATCH RESEND 5/6 v13] gpio: Add device tree support to block GPIO API Roland Stigge
2013-01-15 11:51 ` [PATCH RESEND 6/6 v13] gpio: Add block gpio to several gpio drivers Roland Stigge
2013-01-15 13:18   ` Nicolas Ferre
2013-01-15 13:30     ` Roland Stigge

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130215144700.35A1F3E044B@localhost \
    --to=grant.likely@secretlab.ca \
    --cc=broonie@opensource.wolfsonmicro.com \
    --cc=daniel-gl@gmx.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=highguy@gmail.com \
    --cc=jbe@pengutronix.de \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=plagnioj@jcrosoft.com \
    --cc=rmallon@gmail.com \
    --cc=sr@denx.de \
    --cc=stigge@antcom.de \
    --cc=tru@work-microwave.de \
    --cc=w.sang@pengutronix.de \
    --cc=wg@grandegger.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).