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
boards: shields: Adding support for the adafruit can picowbell shield #59820
boards: shields: Adding support for the adafruit can picowbell shield #59820
Conversation
bf94c14
to
c14e630
Compare
Is there anything else I need to do for this merge request to be approved? :) |
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
&spi0{ |
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.
spi0 is not a well-known connector, see https://docs.zephyrproject.org/latest/hardware/porting/shields.html#board-compatibility. Not sure what it would be for rpi_pico. Is there a common pattern what spi instance is used by the rpi_pico shields?
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.
I couldn't find any other "spi"'s or aliases for a nodelabel for spi0 for rpi_pico. I can add one to the "rpi_pico.dts" file and name it "pico_spi". Would also need to edit the "rpi_pico_w.dts" to ensure it works for both boards
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.
Is there a common pattern of what SPI is used by the rpi_pico shields? If so, then a nodelabel like pico_spi is the right approach, otherwise I have no idea how to handle it generically, perhaps add nodelabels spi0, spi1 ....
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.
I couldn't find any pico shields so I have edited "boards/arm/rpi_pico/rpi_pico-common.dtsi" and added a nodelabel. I have also changed the shield overlay to comply with the node label :)
c14e630
to
db9a9a8
Compare
mcp2515_adafruit_can_picowbell: can@0 { | ||
compatible = "microchip,mcp2515"; | ||
spi-max-frequency = <1000000>; | ||
int-gpios = <&gpio0 21 GPIO_ACTIVE_LOW>; |
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.
For arduino header there would be something like int-gpios = <&arduino_header 12 (GPIO_ACTIVE_LOW)>;
. You probably need to introduce pico_header. @erwango what is your opinion?
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.
Added "pico_header" GPIO nexus node & updated shield ".overlay" to use the "pico_header" now :)
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.
Two nitpicks but generally looks good (I'm unfamiliar with the shield)
boards/shields/mcp2515/doc/index.rst
Outdated
|
||
The Adafruit PiCowbell CAN Bus Shield uses the Microchip MCP2515 controller | ||
with an TJA1051/3 transceiver. This shield is built for the raspberry pi pico | ||
and uses the SPI interface. It also contains a qwiic connector to add support |
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.
nit: capitalize Raspberry Pi Pico and Qwiic
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.
Just updated now :)
62afb71
to
1afe735
Compare
boards/shields/mcp2515/doc/index.rst
Outdated
|
||
.. figure:: adafruit_can_picowbell.jpg | ||
:align: center | ||
:alt: ADAFRUIT_CAN_PICOWBELL_SHIELD |
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.
:alt: ADAFRUIT_CAN_PICOWBELL_SHIELD | |
:alt: Adafruit PiCowbell CAN Bus Shield | |
Adafruit PiCowbell CAN Bus Shield |
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.
@joeker64 the "double" occurence of "Adafruit PiCowbell CAN Bus Shield" in my suggested edit was actually intentional: the first one is for the alt attribute, while the second is the caption for the figure. Thanks!
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.
I'll make the changes now :)
1afe735
to
541c31b
Compare
541c31b
to
2f53285
Compare
Adding support for the adafruit can picowbell shield for the raspberry pi picoi. Also added nodelable for spi0 called 'pico_spi' as well as an GPIO nexus node 'pico_header' Signed-off-by: Joseph Yates <joeyatessecond@gmail.com>
2f53285
to
444bed2
Compare
@erwango can you please review this one? |
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.
Hi @joeker64!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!
To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.
Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁
Adding support for the adafruit can picowbell shield designed for the raspberry pi pico