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

sysfs: (fixes #26) Add GPIO support for the Jetson Orin AGX board #27

Closed
wants to merge 8 commits into from

Conversation

penguinland
Copy link

This is a fix for #26. It's ugly, and I kinda suspect I should have put the data elsewhere, but it works. I'm totally open to suggestions of moving things elsewhere, though!

I am relatively new to both Go and sysfs. I've probably written something unidiomatic, and would be happy to change whatever it is.

The implementation:

  • At initialization, store the board name (the contents of /proc/device-tree/model).
  • When you need to generate the root directory of a GPIO pin, check the board name: if it's a Jetson Orin AGX, use a giant lookup table to go from pin number to their custom name, and for literally everything else ever, use the standard name like usual.

Tried on a Jetson Orin AGX on my desk: I'm able to turn the GPIO pins on and off! I have not tried it on other devices, but I believe this repo has a CI system that will test it on various other hardware, and no changes on other hardware are intended.

  1. Once integrated, send a PR to https://github.com/periph/cmd to leverage the new functionality (if relevant).

I don't know if that's relevant or what would go in the PR. Any advice?

@google-cla
Copy link

google-cla bot commented Jan 9, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@google-cla
Copy link

google-cla bot commented Jan 9, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Member

@maruel maruel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution.

sysfs/gpio.go Outdated Show resolved Hide resolved
sysfs/gpio.go Show resolved Hide resolved
sysfs/gpio.go Outdated
// The NVidia Jetson Orin AGX uses nonstandard names within /sys/class/gpio. This is a mapping
// from pin numbers to their names on that machine.
jetsonOrinAgxPins := map[int]string{
316: "AA.00", 317: "AA.01", 318: "AA.02", 319: "AA.03", 320: "AA.04", 321: "AA.05",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code allocates the whole map at every single function call. This is inefficient. A slightly better version would look like this (simplified form):

var mapping[...]string{"AA.00", "AA.01", ...}

p -= 316
return mapping[i]

I have concerns though;

https://docs.nvidia.com/jetson/archives/r35.1/DeveloperGuide/text/HR/JetsonModuleAdaptationAndBringUp/JetsonAgxOrinSeries.html#changing-the-pinmux

and

https://docs.nvidia.com/jetson/archives/r35.1/DeveloperGuide/text/HR/JetsonModuleAdaptationAndBringUp/JetsonAgxOrinSeries.html#identifying-the-gpio-number

has two interesting bits:

  • In the JetPack 5.0.2-GA and later, you can use the GPIO sysfs tool to update the pinmux register
  • Because the Jetson module dynamically registers GPIOs, search the kernel messages to check the GPIO allocation ranges for each GPIO group

so basically, they say the pin mapping name can change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the pin mapping name can change

Ugh... you're right, but fixing that so it Just Works for everyone seems very hard. To search the kernel messages, I ran dmesg | grep gpiochip on my Orin (per your second link), and discovered that I need to use sudo to use dmesg. but I really don't want to require root access to use this library, so the approach of "parse sudo dmesg | grep gpio to get the base numbers" isn't going to work in a straightforward way.

How would you feel about just putting a warning in the readme, that if you change the pinmux on an Orin, this library probably won't work right? My gut instinct is that almost no one will change the pinmux, and supporting that feature isn't worth the hassle, though I could be wrong. and getting something that works most of the time beats something that doesn't work at all, even if it's not perfect.

(I'm still working on the rest of your feedback, but should be done soon)

@maruel
Copy link
Member

maruel commented Jan 10, 2023

I was wondering, could we have the code look at gpiochipXX symlinks, then look at /base and /label and try to calculate from there?

@maruel
Copy link
Member

maruel commented Jan 10, 2023

https://www.kernel.org/doc/html/latest/driver-api/gpio/index.html?highlight=names#c.gpio_chip tells me names is probably filled up with the right data.

@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2023

Codecov Report

Merging #27 (0716beb) into main (339d73c) will decrease coverage by 0.0%.
The diff coverage is 0.0%.

@@           Coverage Diff           @@
##            main     #27     +/-   ##
=======================================
- Coverage   34.3%   34.3%   -0.0%     
=======================================
  Files         51      51             
  Lines       7992    8002     +10     
=======================================
  Hits        2744    2744             
- Misses      5117    5127     +10     
  Partials     131     131             
Impacted Files Coverage Δ
sysfs/gpio.go 53.6% <0.0%> (-1.8%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@penguinland
Copy link
Author

You might be right that there's a way to find the right names even for dynamically renumbered pins by examining that data structure. but I feel out of my depth (where would I even get that data structure from?), and don't have the time right now to research this further. I've gotten far enough that my main work is no longer blocked, so I'm going to go back to that instead. You're welcome to modify this PR further to support those extra cases (or close the PR, or something else).

Thank you for the advice and feedback you've provided!

@penguinland
Copy link
Author

Closing because no one (including myself) has looked at this for 1.5 years.

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

Successfully merging this pull request may close these issues.

None yet

3 participants