Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

libraries: SPI: Hanlde multiple SPI instances #113

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

Merged
DhruvaG2000 merged 1 commit into zephyrproject-rtos:next from soburi:multiple_spi
Aug 30, 2024

Conversation

@soburi
Copy link
Member

@soburi soburi commented Aug 4, 2024

Create multiple SPI instance if spis array contains plural elements. Declare these as 'SPI1', 'SPI2', ..., from second element of the array. (First element is already declare as 'SPI'.)

@soburi soburi changed the title (削除) libraries: SPI: Hanlde multiple Wire instances (削除ここまで) (追記) libraries: SPI: Hanlde multiple SPI instances (追記ここまで) Aug 4, 2024
@soburi soburi marked this pull request as ready for review August 5, 2024 21:29
/* When PROP_LEN(spis) == 1, DT_FOREACH_PROP_ELEM work not correctly. */
arduino::ZephyrSPI
SPI(DEVICE_DT_GET(DT_PHANDLE_BY_IDX(DT_PATH(zephyr_user), spis, 0)));
#endif // HAS_PORP(i2cs)
Copy link
Member

@DhruvaG2000 DhruvaG2000 Aug 6, 2024

Choose a reason for hiding this comment

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

Pardon me, I didn't understand how I2C has any relevance here in SPI.cpp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I verified the code but forgot to edit the comment. I'm using the structure that was originally used for I2C.

arduino::ZephyrSPI
SPI(DEVICE_DT_GET(DT_PHANDLE_BY_IDX(DT_PATH(zephyr_user), spis, 0)));
#endif // HAS_PORP(i2cs)
/* If i2cs node is not defined, tries to use arduino_i2c */
Copy link
Member

@DhruvaG2000 DhruvaG2000 Aug 6, 2024

Choose a reason for hiding this comment

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

s/arduino_i2c/arduino_spi ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed it. Thanks.

@soburi soburi marked this pull request as draft August 6, 2024 06:32
Copy link
Member Author

soburi commented Aug 6, 2024

I'm going to put it back into draft because I have some concerns. Sorry.

DhruvaG2000 reacted with thumbs up emoji

@soburi soburi force-pushed the multiple_spi branch 2 times, most recently from ad8d4c0 to 4ce81ce Compare August 6, 2024 11:01
@soburi soburi marked this pull request as ready for review August 6, 2024 11:03
Copy link
Member

@DhruvaG2000 DhruvaG2000 left a comment

Choose a reason for hiding this comment

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

@soburi nothing against this PR, but can you add more in your commit about how this change helps us/ what feature it enables? Talk about what enhancement this is bringing in basically.

Copy link
Member

Also, s/Hanlde/handle in $subject

Copy link
Member

@Ayush1325 Ayush1325 left a comment

Choose a reason for hiding this comment

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

LGTM

SPI(DEVICE_DT_GET(DT_PHANDLE_BY_IDX(DT_PATH(zephyr_user), spis, 0)));
#endif // HAS_PORP(spis)
/* If spis node is not defined, tries to use arduino_spi */
#elif DT_NODE_EXISTS(DT_NODELABEL(arduino_spi))
Copy link
Member

@Ayush1325 Ayush1325 Aug 17, 2024

Choose a reason for hiding this comment

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

So where does the arduino-spi name come from? Are any boards already defining it using that label?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is usually define with arduino_header in zephyr/boards definitions.
This line is intended to take advantage of it if it is already defined.
And we can override with overlay in this package with defining spis property.

@soburi soburi force-pushed the multiple_spi branch 2 times, most recently from 5aa3007 to bc7cf72 Compare August 18, 2024 00:05
Copy link
Member Author

soburi commented Aug 18, 2024

@DhruvaG2000

@soburi nothing against this PR, but can you add more in your commit about how this change helps us/ what feature it enables? Talk about what enhancement this is bringing in basically.

Added description to commit message.

Also, s/Hanlde/handle in $subject

Fixed it. Thank you.

Copy link
Member

Also, s/Hanlde/handle in $subject

Fixed it. Thank you.

@soburi Nope, I still see the wrong spelling in subject. https://github.com/zephyrproject-rtos/gsoc-2022-arduino-core/pull/113/commits

#define DECL_EXTERN_SPI_N(i) extern arduino::ZephyrSPI SPI##i;
#define DECLARE_EXTERN_SPI_N(n, p, i) \
COND_CODE_1(ARDUINO_SPI_DEFINED_##i, (DECL_EXTERN_SPI_0(i)), (DECL_EXTERN_SPI_N(i)))

Copy link
Member

@DhruvaG2000 DhruvaG2000 Aug 18, 2024

Choose a reason for hiding this comment

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

Please fix above checkpatch warns @soburi .

WARNING: please, no spaces at the start of a line

Copy link
Member

@DhruvaG2000 DhruvaG2000 Aug 28, 2024

Choose a reason for hiding this comment

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

Was this fixed?
Can you please fix, rebase and force push if not done?

@soburi soburi force-pushed the multiple_spi branch 2 times, most recently from f9d18ce to 658e88c Compare August 18, 2024 11:06
#define DECL_EXTERN_SPI_N(i) extern arduino::ZephyrSPI SPI##i;
#define DECLARE_EXTERN_SPI_N(n, p, i) \
COND_CODE_1(ARDUINO_SPI_DEFINED_##i, (DECL_EXTERN_SPI_0(i)), (DECL_EXTERN_SPI_N(i)))

Copy link
Member

@DhruvaG2000 DhruvaG2000 Aug 28, 2024

Choose a reason for hiding this comment

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

Was this fixed?
Can you please fix, rebase and force push if not done?

Create multiple SPI instances if the `spis` array contains plural elements.
Declare these as 'SPI1', 'SPI2', ..., from the second element of the array.
(The first element is already declared as 'SPI'.)
If `spis` is not defined but the DTS already defines `arduino-spi` and,
use it to define the first SPI instance.
The `arduion-spi` is usually defined with arduino_header in
boards definitions.
Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
@DhruvaG2000 DhruvaG2000 merged commit 3227e1a into zephyrproject-rtos:next Aug 30, 2024
@soburi soburi deleted the multiple_spi branch September 1, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DhruvaG2000 DhruvaG2000 DhruvaG2000 approved these changes

+1 more reviewer

@Ayush1325 Ayush1325 Ayush1325 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /