Skip to content

Conversation

@pabigot
Copy link
Contributor

@pabigot pabigot commented Oct 16, 2019

We need to be able to specify GPIO flags in devicetree without that preventing translation from the Arduino specifier to the host GPIO specifier. Set up to ignore the low 6 bits of the flags field when matching the child specifier, and to copy those bits to the parent specifier.

This is a blocking capability for topic-gpio where we need to be able to specify the active level of signals from sensors connected through Arduino shields.

To preemptively answer a question: the low 6 bits are defined for devicetree in Linux although Zephyr currently does not use bit 3. Bit 3 is used to signal suspend/resume/reset persistance, so may be needed in the future.

ulfalizer
ulfalizer previously approved these changes Oct 16, 2019
@ulfalizer
Copy link
Contributor

ulfalizer commented Oct 16, 2019

The old devicetree scripts do not handle *-map-mask and *-map-pass-thru by the way, but it might not matter as long as they don't crash on them, since they're only used to generate some deprecated #defines.

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Should we add some test in order this is not omitted when new connector type is added?
(#19751 for instance)

@pabigot
Copy link
Contributor Author

pabigot commented Oct 16, 2019

Should we add some test in order this is not omitted when new connector type is added?
(#19751 for instance)

That could perhaps be done in the devicetree scripting if it were possible to determine that the nexus map is associated with GPIOs. IMO its lack shouldn't slow incorporation of this change.

@ulfalizer
Copy link
Contributor

ulfalizer commented Oct 16, 2019

That could perhaps be done in the devicetree scripting if it were possible to determine that the nexus map is associated with GPIOs. IMO its lack shouldn't slow incorporation of this change.

_check_dt() in edtlib.py is meant for random devicetree checks like that btw, so could add something there if you want.

If it doesn't need to look at low-level devicetree stuff, it could be added to gen_defines.py instead. Might be cleaner if it's very Zephyr-specific too.

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

I'm fine with the change, but it is a tricky one, so I'd like to see some documentation explaining the "trick" with some details.
Motivations:

  • Doc is always welcome
  • This is only described in V0.3 device tree spec, and it hasn't been released yet
  • Even though v0.3 would be the current release, device tree spec explanations are abstruse

@pabigot
Copy link
Contributor Author

pabigot commented Oct 17, 2019

I'm fine with the change, but it is a tricky one, so I'd like to see some documentation explaining the "trick" with some details.

Wonderful.

@ulfalizer since you maintain the devicetree documentation could you provide a patch with this that I can add to this PR? If you can point me to it in a branch I can cherry-pick it into this.

@pabigot pabigot requested a review from dbkinder as a code owner October 17, 2019 21:47
@pabigot
Copy link
Contributor Author

pabigot commented Oct 17, 2019

@erwango @ulfalizer @dbkinder please review the newly added documentation on this feature.

@zephyrbot
Copy link

zephyrbot commented Oct 17, 2019

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@pabigot pabigot dismissed ulfalizer’s stale review October 17, 2019 22:10

Need approval of added documentation

Comment on lines 56 to 58
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if there is a kind of bitmap drawing available we can point to but it would be nice.
(To do later since it is a different branch)

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Much clearer than dts specification!

Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Sorry, didn't notice the update until now.

Looks good to me.

@pabigot
Copy link
Contributor Author

pabigot commented Oct 22, 2019

@dbkinder Could you please check the documentation addition to this for acceptability? This PR blocks work on driver conversion to the new gpio API. Thanks.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

While I'd prefer the gpio_nexus.inc content to be put in the rst file directly, it's OK as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason to use an include file would be if this content were placed in more than one part of our documentation. If you don't see that as a possibility (there isn't a second reference today) then I'd recommend putting this addition into the index.rst file instead of a separate .inc file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll integrate it into the file. I need to rebase and add a shippable override to get CI to pass anyway.

We need to be able to specify GPIO flags in devicetree without that
preventing translation from the Arduino specifier to the host GPIO
specifier.  Set up to ignore the low 6 bits of the flags field when
matching the child specifier, and to copy those bits to the parent
specifier.

Signed-off-by: Peter Bigot <[email protected]>
Document why and how we use the devicetree nexus map properties to
preserve devicetree flag specifications.

Signed-off-by: Peter Bigot <[email protected]>
@pabigot pabigot force-pushed the pr/20191016a branch 2 times, most recently from 77ea63c to 505d154 Compare October 22, 2019 19:37
@pabigot
Copy link
Contributor Author

pabigot commented Oct 22, 2019

@galak all tests pass via https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/54062/summary/console

I've now removed the shippable override. This should be good for merge.

@galak galak merged commit 4d97252 into zephyrproject-rtos:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants