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

overlays: Factor out the common i2c bus selection #6636

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

pelwell
Copy link
Contributor

@pelwell pelwell commented Jan 27, 2025

This PR reduces some of the duplication in I2C overlays, factoring out the bus selection parameters into a .dtsi file. To allow for easier comparison and to improve the codebase, the first commit fixes a few minor problems with the old versions of the overlays. The second commit rewrites them to use the dtsi file, a step which is essentially cosmetic.

The README is also updated, moving the common parameters into a pseudo-overlay called i2c-bus. Overlays that use these common parameters are shown as supporting the parameter i2c-bus, whose documentation refers the user to the common documentation.

@pelwell pelwell requested a review from 6by9 January 27, 2025 22:38
@pelwell
Copy link
Contributor Author

pelwell commented Jan 28, 2025

FYI I'm most interested in feedback for the first commit, since that makes real changes to the resulting configuration. I'm happy to bear the burden for verifying the changes made by the second, but confirmation that the overall scheme looks okay would be nice.

Copy link
Contributor

@6by9 6by9 left a comment

Choose a reason for hiding this comment

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

One rogue space, but otherwise looks fine.

(The edt-ft5x06 include had my head hurting for a bit)

@6by9
Copy link
Contributor

6by9 commented Jan 28, 2025

I think the patch order will mean that some of the overlays will be functionally broken for bisection purposes if using i2c0 or 10 - they appear to drop enabling i2c0mux and i2c0if, which gets added back in with the second patch.
C'est la vie.

Otherwise those cleanups all look reasonable.

@pelwell
Copy link
Contributor Author

pelwell commented Jan 28, 2025

When using i2c0, some overlays enabled i2c0mux and i2c0if while others didn't. It looks I've chosen the wrong option (kind of obvious now looking at some of the dts files). I'll rework it the other way - not a big change.

A few small improvements, with a view to making the updated overlays
behave the same before and after the big conversion.

Signed-off-by: Phil Elwell <[email protected]>
Create an i2c-buses.dtsi to hold all of the common I2C bus selection
logic, and refactor existing overlays to use it. This patch should
have no functional change overall except to increase the range of
options for some overlays.

There is a slightly ugly mechanism for overriding the default bus, where
the mux nodes may or may not need to be enabled.

Signed-off-by: Phil Elwell <[email protected]>
@pelwell
Copy link
Contributor Author

pelwell commented Jan 28, 2025

Updated to address the feedback. With a fixed (not nobbled) ovmerge, a test script that compares the results of applying all the old parameters to each updated overlay before and after the second commit finds zero differences. Therefore I think that if you're happy with the new first commit then its safe to merge.

@6by9
Copy link
Contributor

6by9 commented Jan 28, 2025

Looks good to me.

I wonder if the original i2c0 support predated the mux, hence some i2c0 options didn't worry about it.
In theory you could use dtoverlay=i2c0 to enable BSC0 without the mux, and then this is going to cause grief. I wouldn't worry about that case though.

@pelwell
Copy link
Contributor Author

pelwell commented Jan 28, 2025

In theory you could use dtoverlay=i2c0 to enable BSC0 without the mux

For some reason, that's what I was expecting it to do.

@pelwell pelwell merged commit a320d39 into raspberrypi:rpi-6.6.y Jan 28, 2025
12 checks passed
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.

2 participants