Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add overlay that provides GPIO line names #27

Closed
wants to merge 1 commit into from

Conversation

dberlin
Copy link

@dberlin dberlin commented Sep 22, 2022

This enables gpioinfo and other tools to display the GPIO name for a given line offset/etc.

This enables gpioinfo and other tools to display the GPIO name for a given line offset/etc.
@dberlin
Copy link
Author

dberlin commented Sep 22, 2022

Without this patch, gpioinfo/etc gives no info about what each line offset maps to. It has no info at all in fact.
With this overlay, gpioinfo looks like this, which is IMHO, much more helpful and let's you directly see which offsets match to which GPIO's.

$ gpioinfo
gpiochip0 - 32 lines:
        line   0:   "GPIO0_A0"       unused   input  active-high
        line   1:   "GPIO0_A1"       unused   input  active-high
        line   2:   "GPIO0_A2"       unused   input  active-high
        line   3:   "GPIO0_A3"       unused   input  active-high
        line   4:   "GPIO0_A4"       unused   input  active-high
        line   5:   "GPIO0_A5"       unused   input  active-high
        line   6:   "GPIO0_A6"       unused   input  active-high
        line   7:   "GPIO0_A7"       unused   input  active-high
        line   8:   "GPIO0_B0"       unused   input  active-high
        line   9:   "GPIO0_B1"       unused   input  active-high
        line  10:   "GPIO0_B2"       unused   input  active-high
        line  11:   "GPIO0_B3"       unused   input  active-high
        line  12:   "GPIO0_B4"       unused   input  active-high
        line  13:   "GPIO0_B5"       unused   input  active-high
        line  14:   "GPIO0_B6"       unused   input  active-high
        line  15:   "GPIO0_B7"       unused   input  active-high
        line  16:   "GPIO0_C0"       unused   input  active-high
        line  17:   "GPIO0_C1"       unused   input  active-high
        line  18:   "GPIO0_C2"       unused   input  active-high
        line  19:   "GPIO0_C3"       unused   input  active-high
        line  20:   "GPIO0_C4"       unused   input  active-high
        line  21:   "GPIO0_C5"       unused   input  active-high
        line  22:   "GPIO0_C6"       unused   input  active-high
        line  23:   "GPIO0_C7"       unused   input  active-high
        line  24:   "GPIO0_D0"       unused   input  active-high
        line  25:   "GPIO0_D1"       unused   input  active-high
        line  26:   "GPIO0_D2"       unused   input  active-high
        line  27:   "GPIO0_D3" "snps,reset"  output   active-low [used]
        line  28:   "GPIO0_D4"       unused   input  active-high
        line  29:   "GPIO0_D5"       unused  output  active-high
        line  30:   "GPIO0_D6"       unused   input  active-high
        line  31:   "GPIO0_D7"       unused   input  active-high
...
<other gpiochips are similar>

This was tested on a firefly ROC-RK3588S-PC because i don't have a rock pi 5 board, but assuming radxa hasn't reordered the GPIO chip numbers from the rk3588 base, it should work.
This could be integrated directly into the base rk3588 dts as well, i did it as an overlay so that it was more easily portable to all rk3588* based boards, since they all share the same gpio controller layout but all have different kernel trees.

@jack-ma jack-ma assigned jack-ma and RadxaStephen and unassigned jack-ma Sep 24, 2022
@jack-ma
Copy link
Member

jack-ma commented Sep 24, 2022

This looks good, @RadxaStephen can you test and merge it?

@RadxaStephen
Copy link
Member

I will test that.

@RadxaYuntian
Copy link
Member

RadxaYuntian commented Oct 10, 2022

There is no point for this to be an overlay. This should be added to the base dts like what I did with Zero 2, and customized to have names that make sense. There is no point to label an unexported GPIO pin.

@dberlin
Copy link
Author

dberlin commented Oct 10, 2022

There is no point for this to be an overlay. This should be added to the base dts like what I did with Zero 2, and customized to have names that make sense. There is no point to label an unexported GPIO pin.

  1. Of course there is a point to label non-exported GPIO pins. If you don't label them at all, nobody knows if they are inaccessible or if someone forgot to label it. Knowing they are inaccessible tells you that whatever offset or whatever you calculated is wrong, because you are trying to set/unset an unusable GPIO and when you look at gpioinfo it will tell you that.
    When that isn't possible, having the name at least lets you do roughly the same thing - tell you whether you are trying to access the GPIO name you thought you are.
    It's not as nice as knowing whether the pin is even user-accessible in the first place, but it gives you some idea.
    But, when left blank, it's not simply possible to tell if someone just forgot to label it, or whether it's inaccessible. There are lots of boards where ~70% of things are labeled. Some of the rest are accessible but blank. Some of them are blank and inaccessible. That's nowhere near as helpful as it could be.

  2. As I mentioned, I think having it built-in, and having everything properly labeled. I think that's a great idea.
    The halfway state is the bad one, where half the things are labeled and half not.

@RadxaYuntian
Copy link
Member

With gpiod you should run command like sudo gpioget $(sudo gpiofind PIN_8) or sudo gpioget $(sudo gpiofind PIN_8)sudo gpioset $(sudo gpiofind PIN_8)=1. So you are never calculating the gpio number yourself. Out of hundreds possible GPIOs only a selected few is available to the user, so not labeling unused ones should be the default choice, otherwise there will be too much noise.

If boards don't label all the available pins, that's an inconsistency and should be fixed. We should not use a known broken state as an argument but should compare your proposal with how thing is supposed to work.

@dberlin
Copy link
Author

dberlin commented Oct 10, 2022

So, again, no disagreement on whether to calculate it yourself, but your wiki, and every embedded wiki, has the user calculate it themselves.

See https://wiki.radxa.com/Rock5/hardware/5b/gpio for example

This is part of the larger point i'll make a little - you are suggesting an answer that is clearly a really good technical end state.
Which i don't disagree with. But that doesn't make it good for users, at all, and that is the target of a feature like this.

Again, users literally can't tell what unlabeled state means. Ever. That isn't just about your boards. If the gpio line name was a required property, or they could be marked so that gpioinfo could display more than just a blank string (IE had a user-available flag" or something), i think you might have a good argument. But right now you are suggesting that lots of things should change, most of which you do not control, before a user could ever rely on "blank = unavailable" in their projects. That is true even in your boards.

As for what to do, of course you should use the current broken state to decide what to do. The goal is not abstract perfection, but to help the users the most. It is a common, but not-great-for-users idea to chase the best possible technical end state. I do it myself sometimes. Nor do I actually disagree on what the technical best end state is. I only disagree that it makes sense to try to get there in one step, and put it on everyone else to follow. You should chase the best-reasonable-user-end-state first, then figure out if you can make a better technical state if you want.
More to the point - as a user, i'm telling you i actually don't care about a perfect technical end state where only usable things are labeled.
I instead care about a reasonable user end state where i can tell which things are usable or not. Those are not the same.
I 100% cannot rely on you, and everyone else being perfect right now, for the reason i mentioned.

So by leaving them blank, you will only make that harder. If you go to the forums, and ask the same question of users, i would bet you would get the same answer.

As a result, I can't see any reason, given the current broken state, to use "" rather than "unavailable" (or the GPIO names) to label thing that are not exported.
You argue this is somehow too much noise, but i'm really unsure what this is based on other than your personal preference. The output of gpioinfo is really clean easy to follow either way.
All you will do by following down this path is make things less useful for users, both now, and in the future.

Meanwhile, if we ever get to the point where everything is labeled, it's trivial to later replace the strings to be blank again (or whatever).

In any case, do what you want, it's your project. I will not bother you about it further.
If you choose the path of trying to be perfect, I'll keep my overlay so i can actually tell what is going on for real, and i'll make it available to others to do the same. and I would bet, years from now, the available GPIO's still won't be 100% labeled :)

@RadxaYuntian
Copy link
Member

Since we labeled GPIO on the 40-pin only, close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants