-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update Fan Cases with mention of the Pi 4 Case Fan #3929
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement. Your additions made me think a little more critically about the section layout, so I made some suggestions. Address my nitpicky suggestions and we'll merge this in!
documentation/asciidoc/computers/raspberry-pi/frequency-management.adoc
Outdated
Show resolved
Hide resolved
documentation/asciidoc/computers/raspberry-pi/frequency-management.adoc
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much @nathan-contino! The nits are very helpful, especially early on -- I can start to see how much editing and rewriting we want to aim for in these types of PRs.
Have reviewed the style guide (and will reread it often, I'm sure). For future issues, in addition to making changes addressed in the issue, I'll think about style and structure edits at roughly the same "level" as you're suggesting here :)
@@ -78,9 +86,9 @@ Temperature decreases use the same mapping with a 5°C **hysteresis**; fan speed | |||
|
|||
At boot the fan is turned on, and the tachometer input is checked to see if the fan is spinning. If it is, then the `cooling_fan` device tree overlay is enabled. This overlay is in `bcm2712-rpi-5-b.dtb` by default, but with `status=disabled`. | |||
|
|||
==== Fan connector pinout | |||
==== Raspberry Pi 5 fan connector pinout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more nit 😁 Thoughts on this header? It's at the same level as the other "fan" headers (I didn't want to make it too small). Normally I don't think I would separate this information out using a header, but in this case, the connector info is linked in the introduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a reasonable compromise. Ideally we'd nest this within the Pi 5 section, but... that might go too deep. This seems to be the best place for this right now.
Resolves #3566.
Keeping it minimal here! My thinking (for these types of tickets in general) is that if the current/latest model is already documented, we only need to include previous models for completeness. That means that we won't have the finer details of Pi 4 fan behaviour in the doc (at least until there's a good use case to go around and dig them up!) The assembly and usage instructions for both fans can be found on their respective product pages and product briefs.
Other notes (just general tech writing thoughts):