linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Anton Vorontsov <avorontsov@ru.mvista.com>
To: Matt Sealey <matt@genesi-usa.com>
Cc: Mitch Bradley <wmb@firmworks.com>,
	linuxppc-dev list <linuxppc-dev@ozlabs.org>,
	devicetree-discuss list <devicetree-discuss@ozlabs.org>
Subject: Re: GPIO - marking individual pins (not) available in device tree
Date: Tue, 28 Oct 2008 05:37:22 +0300	[thread overview]
Message-ID: <20081028023722.GA30057@oksana.dev.rtsoft.ru> (raw)
In-Reply-To: <490666B0.2010500@genesi-usa.com>

On Mon, Oct 27, 2008 at 08:11:12PM -0500, Matt Sealey wrote:
> Anton Vorontsov wrote:
>> On Mon, Oct 27, 2008 at 04:56:57PM -0500, Matt Sealey wrote:
>>>
>>> A bunch of things spring to mind:
>>>
>>> *   How do you define a GPIO bank in a device tree, not a controller
>>
>> Not a controller? What do you mean by "bank"? There is no such
>> thing. There are GPIO controllers, and _maybe_ _their_ banks of
>> GPIOs.
>
> I don't know what you want to call it, I don't know what the official
> Linux term might be for a grouped bunch of GPIO used for a peripheral.

You don't know this term because there isn't any. There is no term
for "bunch of GPIO used for a peripheral".

But there are:

- gpio controllers;
- devices that use some gpios;
- gpios = <>; property that is used to denote which gpios the device
  is using.

What's so hard about that?

>>>     but a grouping of pins which fit that pattern, of which there
>>>     may be multiple groupings for multiple peripheral functions
>>
>> Why do you need this, _exactly_? What problem this would solve? But
>> see below, this is still possible to implement.
>
> I made a pretty good example with the lcd controller, I thought..

Yes, thanks for the example. And I don't see any problems with
describing the lcd controller.

>> But I still wonder what problem exactly you're trying to solve
>> here? Prevent requesting reserved gpios? I don't see any point
>> in this, really.
>
> There's plenty of reason for it, it's totally wrong to be able
> to request a GPIO which has been multiplexed to another device
> in the SoC, and probably completely undefined behaviour if you
> start toggling a pin that's been assigned to an internal
> controller.

Then just don't specify the wrong GPIO in the gpios = <>! It is
that simple.

You don't specify the SOC's peripheral memory in the /memory node,
do you? I bet you don't. Can you? Yes. Bad things will happen? Sure.

You may bring the gpio sysfs example. But I can answer back with the
/dev/mem example. There are plenty of things you can do bad things
with, even being in the userspace.

> Since you have no way of knowing except intimate board design
> knowledge in the driver.. well, that defeats the purpose of the
> device tree in general and is a step back into hardcoded information
> in board support from arch/ppc days etc..
> 
> This information SHOULD be in the device tree specifically because
> there is one very popular chip here which has GPIO multiplexed with
> other very-often-used peripherals, and I can think of at least 5
> other chips which also have device trees and GPIO potential (4 of
> which we have designed boards around) which fit the same model and
> would have the same requirement.
>
> There are two ways you can do it; implicit and explicit. If a
> pin is not defined for a peripheral in the device tree, it is
> not requestable from GPIOLIB. Or, you make sure that you specify

This is doable and _maybe_ a good idea. Though I don't see much
value in this.

>> No problem. Define bindings for "8 GPIOs data group".
>>
>> /* 8bit Parallel I/O port using arbitrary gpios */
>> byte_pio: byte-pio {
>> 	compatible = "byte-pio";
>> 	gpios = <&controller1 0 1   /* data line 0 */
>> 		 &controller2 12 0  /* data line 1 */
>> 		 ...>;
>> };
>>
>> lcd-controller {
>> 	/* the lcd controller can simply use the pio_out_8(),
>> 	 * pio_in_8() API. The API is easily implemented. */
>> 	lcd-data-port = <&byte_pio>;
>> }
>>
>>> and 1 for data/control selection, it would be nice for the
>>> software to know which pin is which and, slightly unrelated,
>>> which way around the data pins went - MSB first or LSB first
>>> - from the device tree, as this is a BOARD LEVEL DESIGN DECISION
>>> which is what the device tree is meant to abstract - in the same
>>> way we define i2c controllers and clients?)
>
> And this? What about the other two control lines? Do they just get
> set in the gpios property of the lcd-controller? How do you determine
> which is which?

This depends on how will you write the bindings.

> Comments don't get compiled..

If you _really_ want to complicate things, you can write gpios for
devices like this:

device {
	data0-gpio = <&controller1 1 1>;
	...
	rw-gpio = <&controller1 2 1>;
}

This _will_ get compiled in. This is insane, but this is more friendly
to a device tree reader, if you like.

> and a driver will have
> to be written FROM the comments, hardcoding the pins into it, again.

What do you mean by "hardcoding"? Let's see: interrupts = <33 32>;
and then extracting them via
of_irq_to_resource(node, 0, &tx_irq),
of_irq_to_resource(node, 1, &rx_irq).

Does this mean "hard-coding"?

[...]
>>> I don't feel the current spec actually takes this into account
>>> and the patch submission the other day from Wolfgang made me
>>> think that if a "base" register is the obvious solution to some
>>> problem, then it either can't be that clear to others (i.e. it
>>> is not just me being stupid) or it is simply not possible under
>>> the current binding.
>>
>> Wolfgang's patch proved to be unnecessary, you haven't seen
>> the solution?
>
> Wolfgang's patch would never have been considered by Wolfgang
> if the solution wasn't more obvious... this is why I started
> the thread :D

Oh, device tree/correct bindings/proper ways to solve things
aren't always obvious? It's news to me. ;-)

[...]
>> I don't know how did you come to these conclusions. Pretty much
>> people were involved into the gpio bindings discussion.
>
> I was watching the threads and I expected a little more than 20
> lines of documentation and a couple LED drivers would come out
> of it.
>
> GPIOLIB excited me when I talked to the original author about
> it, now that there has to be a device tree behind it that is
> an absolutely undefined, gpio-controller-specific implementation
> for every chip

You seem to disagree with the whole device tree idea and the OF
in general. Interrupts are so controller-specific stuff that
we should reconsider using it, right?

> or even every board with significant information
> hardcoded into drivers, I am considerably less enthused.

What's so bad about board-specific drivers for board-specific
devices? If a gpio-header is board-specific, then we have to
write a board-specific driver for it. To make things better
we can write the driver in a such way that the driver could be
easily adopted/extended for similar boards/setups.

OR we can write bindings that could fully describe the gpio
header, and then just use the universal driver for it.

Both solutions are doable.

>>> Yes, your idea, Mitch's discussion was great, I just don't think
>>> anything will get done about it (emotional moment) as usual.
>>
>> Hm. If idea looks ok, then it is matter of implementation. And
>> the gpio-header case is quite easy to implement, really.
>
> Okay. I think I am understanding this enough that I could write
> a bit more comprehensive documentation for it that would
> encompass the result of this discussion and some other things
> going on right now...
>
> As for implementation, since the only hardware I have here to
> test is a) broken b) only has 3 GPIO pins or b) working but
> such a horrid design I wouldn't use it as a doorstop, I don't
> think I am really qualified to put it into action..

Yeah, there are always excuses when it comes to send patches... ;-)

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

  reply	other threads:[~2008-10-28  2:37 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-10-23 21:32 GPIO - marking individual pins (not) available in device tree Matt Sealey
2008-10-23 22:22 ` Mitch Bradley
2008-10-23 23:05   ` Matt Sealey
2008-10-24  0:52     ` Mitch Bradley
2008-10-24  3:29       ` David Gibson
2008-10-24  4:17         ` Mitch Bradley
2008-10-24  4:45           ` David Gibson
2008-10-24 22:14             ` Matt Sealey
2008-10-26 23:47               ` David Gibson
2008-10-27 15:40                 ` Matt Sealey
2008-10-27 18:34                   ` Anton Vorontsov
2008-10-27 18:56                     ` Matt Sealey
2008-10-27 20:10                       ` Anton Vorontsov
2008-10-27 21:56                         ` Matt Sealey
2008-10-27 23:12                           ` Anton Vorontsov
2008-10-27 23:40                             ` Anton Vorontsov
2008-10-28  0:47                               ` Matt Sealey
2008-10-28  1:11                             ` Matt Sealey
2008-10-28  2:37                               ` Anton Vorontsov [this message]
2008-10-28 16:53                                 ` Matt Sealey
2008-10-28 17:39                                   ` Grant Likely
2008-10-28 19:46                                     ` Matt Sealey
2008-10-28  0:15                   ` David Gibson
2008-10-28  0:51                     ` Matt Sealey
2008-10-28  1:50                       ` David Gibson
2008-10-28  5:20                       ` Grant Likely
2008-10-24 22:03       ` Matt Sealey
2008-10-24 22:20         ` Stephen Neuendorffer
2008-10-26 21:39           ` Matt Sealey
2008-10-24 23:44         ` Mitch Bradley
2008-10-26 21:13           ` Matt Sealey
2008-10-26 23:53             ` David Gibson
2008-10-27 16:12               ` Matt Sealey
2008-10-27 16:35                 ` Scott Wood
2008-10-27 17:05                   ` Matt Sealey
2008-10-27 17:25                     ` Scott Wood
2008-10-27 17:49                       ` Matt Sealey
2008-10-27 17:54                         ` Scott Wood
2008-10-28  0:38                           ` David Gibson
2008-10-28  0:34                 ` David Gibson
2008-10-24  4:58     ` David Gibson
2008-10-24  3:27   ` David Gibson
2008-10-24 16:41 ` Anton Vorontsov
2008-10-24 17:01   ` Anton Vorontsov
2008-10-24 22:17     ` Matt Sealey
2008-10-24 22:37       ` Anton Vorontsov
  -- strict thread matches above, loose matches on Subject: below --
2008-10-28 13:31 Konstantinos Margaritis
2008-10-28 14:11 ` Anton Vorontsov
2008-10-28 14:15 ` Grant Likely
2008-10-28 17:06   ` Matt Sealey
2008-10-28 17:32     ` Grant Likely
2008-10-28 23:37 ` David Gibson

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=20081028023722.GA30057@oksana.dev.rtsoft.ru \
    --to=avorontsov@ru.mvista.com \
    --cc=devicetree-discuss@ozlabs.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=matt@genesi-usa.com \
    --cc=wmb@firmworks.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).