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

libraries: SPI: Hanlde multiple SPI instances #113

Merged
merged 1 commit into from
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
@DhruvaG2000 DhruvaG2000 self-requested a review August 6, 2024 04:34
arduino::ZephyrSPI
SPI(DEVICE_DT_GET(DT_PHANDLE_BY_IDX(DT_PATH(zephyr_user), spis, 0)));
#endif // HAS_PORP(i2cs)
Copy link
Collaborator

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
Collaborator

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 requested a review from DhruvaG2000 August 6, 2024 06:31
@soburi soburi marked this pull request as draft August 6, 2024 06:32
@soburi
Copy link
Member Author

soburi commented Aug 6, 2024

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

@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
Collaborator

@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.

@DhruvaG2000
Copy link
Collaborator

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

arduino::ZephyrSPI
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

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
@soburi
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.

@DhruvaG2000
Copy link
Collaborator

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
Collaborator

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
Collaborator

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?

#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
Collaborator

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
3 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants