boards: raspberrypi: rpi_pico: Remove i2c1 node (again) #79461
Conversation
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
zephyrbot
added
manifest
manifest-hal_rpi_pico
DNM
soburi
force-pushed
the
pico_remove_i2c1_default
branch
from
cc53c57 to
42c1d0f
Compare
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
soburi
force-pushed
the
pico_remove_i2c1_default
branch
from
42c1d0f to
9379088
Compare
zephyrbot
removed
manifest
manifest-hal_rpi_pico
DNM
zephyrbot
requested review from
avisconti,
erwango,
jfischer-no,
kartben and
yonsch
|
Actually looking at this again, what was wrong in your opinion with having the nodes present but disabled by default? |
https://datasheets.raspberrypi.com/pico/Pico-R3-A4-Pinout.pdf In the pinout diagram, UART0, I2C0, and SPI0 are shown in dark colors, and are the general default pinmux. This is generally a shared understanding among users. However, there are no clear defaults for other peripherals, so I think it is better to leave them as they are. |
There was a problem hiding this comment.
I agree with @kartben , at most I would disable the node.
More generally, the docs are left wanting on this:
Unless explicitly recommended otherwise by this section, leave peripherals and their drivers disabled by default.
Leaving drivers disabled makes sense to me: it reduces compilation time, which is often a pain port ("Why does hello world need so many files to compile?!" is a oft-heard complaint).
I'm not sure what benefit having nodes disabled by default benefits anyone, or why the project dictates this: board authors/contributors can define a functioning combination of peripherals + pin muxing, and disabling nodes in an app-specific overlay is just as easy/hard as enabling them.
| i2c1_default: i2c1_default { | ||
| group1 { | ||
| pinmux = <I2C1_SDA_P6>, <I2C1_SCL_P7>; | ||
| input-enable; | ||
| input-schmitt-enable; | ||
| }; | ||
| }; | ||
|
|
There was a problem hiding this comment.
The pico-examples use the same pinmux for I2C1 as was defined before this PR, i.e. using GPIO6 and GPIO7. I think that the vendor's examples should be considered a strong enough precedent.
There was a problem hiding this comment.
This topic has already been discussed and agreed upon, and it violates that, so it needs to be restored.
If you would like to raise a separate issue to re-enable it, that is welcome.
There was a problem hiding this comment.
@petejohanson @carlescufi @yonsch
I'm sorry to bring up an old topic, but I would like to consult with someone who reviewed a similar issue in the past.
The board's pinctrl settings are preferred to be "minimal", and rpi_pico also follows this policy, as stated in [the past discussion https://github.com//pull/45131#discussion_r872782328.
Due to my carelessness in this review #60384 , I re-added the i2c1 settings, However as stated in the discussion above, there is already a consensus among developers to keep it minimal, so I think it would be better to revert (delete) it.
This also aims to keep the shield definition neutral.
@ajf58 seems to have a different opinion, but since it would overwrite the current consensus, I think it would be better to discuss it as a separate issue.
I would like to hear your opinion on this point.
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
soburi
force-pushed
the
pico_remove_i2c1_default
branch
from
9379088 to
d724f42
Compare
As you can see from the pinout definition at https://datasheets.raspberrypi.com/pico/Pico-R3-A4-Pinout.pdf, Pico does not have a standard location for i2c1. Therefore, defining `pico_i2c1` as a standard definition does not match the reality and would only complicate the situation. Therefore, delete the definition of `pico_i2c1`. If you use i2c1 with a shield, create a boards directory on each shield and define pinctrl for each board. For waveshare_ups, I added configure explicitly for i2c1 here Signed-off-by: TOKITA Hiroshi <[email protected]>
Zephyr's default settings usually enable only the minimum of peripherals, and rpi_pico already has `i2c0` enabled. Remove the `i2c1` node configuration. I've dealt in a82cc22, but it was added again by mistake. I'll delete it again. Signed-off-by: TOKITA Hiroshi <[email protected]>
soburi
force-pushed
the
pico_remove_i2c1_default
branch
from
d724f42 to
397b442
Compare
Zephyr's default settings usually enable only the minimum of
peripherals, and rpi_pico already has
i2c0enabled.Remove the
i2c1node configuration.I've dealt in a82cc22,
but it was added again by mistake. I'll delete it again.