Date:	Wed, 31 Oct 2012 19:59:04 +0100
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Roland Stigge <stigge@...com.de>
Cc:	gregkh@...uxfoundation.org, linus.walleij@...aro.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	w.sang@...gutronix.de, jbe@...gutronix.de, plagnioj@...osoft.com,
	highguy@...il.com, broonie@...nsource.wolfsonmicro.com,
	daniel-gl@....net, rmallon@...il.com
Subject: Re: [PATCH RESEND 1/5 v6] gpio: Add a block GPIO API to gpiolib

Hi Roland

On Wed, Oct 31, 2012 at 6:19 PM, Roland Stigge <stigge@...com.de> wrote:
> On 10/31/2012 04:00 PM, Grant Likely wrote:
>> For the API, I don't think it is a good idea at all to try and
>> abstract away gpios on multiple controllers. I understand that it
>> makes life a lot easier for userspace to abstract those details away,
>> but the problem is that it hides very important information about how
>> the system is actually constructed that is important to actually get
>> things to work. For example, say you have a gpio-connected device with
>> the constraint that GPIOA must change either before or at the same
>> time as GPIOB, but never after. If those GPIOs are on separate
>> controllers, then the order is completely undefined
>
> It is correct that it's not (yet) well documented and the API is also
> not very explicit about it, but the actual approach of the manipulation
> order is to let drivers handle gpios "as simultaneous as possible" and
> when not possible, do it in the _order of bits specified_ (either
> defined at the device tree level, or when created via
> block_gpio_create() directly).

The documentation is actually fine. I do understand that the intent is
"as simultaneous as possible", but I accept the point that the order
of specification affects the behaviour*. However, it still remains
that the method used by the ABI abstracts at the wrong level and that
blocking arbitrary GPIO pins into a single virtual GPIO register is a
bad idea.

*note that the current code doesn't implement that intended behaviour
either since the gpios are processed in the order of the controllers,
not the order of the bits.

> I'm not sure how far you tested the API in depth: You can already define
> a block that maps onto a subset of gpios on a controller and internally
> of course maps onto those set and clear operations. Whenever you need to
> manipulate a different subset (whether disjoint or overlapping), you can
> easily define _additional_ blocks. From my experience, this solves most
> of the real world problems when n-bit busses are bit banged over GPIOs.
> Doesn't this already solve this (in a different way, though)?

Blech! Requiring a new block for each possible combination of
write-at-once bits is a horrible ABI. That just strengthens my opinion
that the abstraction isn't right yet.

> Pin direction currently needs to be set up separately, analogous to
> requesting gpios. Need to document this better, right. The assumption is
> that I/O needs to be efficient primarily, before bloating the API with
> direction functions. Or should I add functions for this?

Since this is a userspace facing ABI, once it is merged it cannot be
changed in an incompatible way. I cannot merge it until there is at
least a plan for how to handle all of the reasonable use cases. That
means it must support set/clear or mask operations. Also, if it sticks
with the design of grouping pins from multiple controllers, then it
needs to handle explicitly constraining what order operations are
performed in at the time of the operation. At the time of setup
doesn't work since constraints between pins may not always be in the
same order.

I really think you should consider implementing a command stream type
of interface.

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

Powered by blists - more mailing lists