Skip to content

Conversation

@soburi
Copy link
Member

@soburi soburi commented Jul 7, 2022

Add support for GD32 DMA

Signed-off-by: TOKITA Hiroshi [email protected]

I verified ths PR with tests/drivers/dma/loop_transfer with GD32VF103(longan_nano board).

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep at 80 col

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, the clang-format setting is up to 100 columns(3f0e8dd).
Is this no good?

This code is formating with clang-format following instruction https://docs.zephyrproject.org/latest/contribute/guidelines.html#clang-format.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @soburi ,

There was an agreement to allow up to 100 col in code. This doesn't mean that we will follow it.
Zephyr still use Linux kernel coding style and we will keep it for GigaDevices. I point 2 exceptions that are OK to use more than 80 columns.

Please, reformat code to use 80 columns.

@nandojve nandojve mentioned this pull request Jul 9, 2022
55 tasks
@soburi soburi force-pushed the gd32_dma branch 3 times, most recently from 235757b to 1689baa Compare July 16, 2022 16:02
@soburi soburi requested a review from nandojve July 16, 2022 16:19
Copy link
Contributor

@cameled cameled left a comment

Choose a reason for hiding this comment

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

This is a big step for the tansfer performence. Have a qiuck look, will try to run this on my gd32vf103v board latter.

@cameled
Copy link
Contributor

cameled commented Jul 22, 2022

For gd32f4xx series, only dma1 support m2m, and should use multi-data mode

EDIT: m2m multi-data mode auto applied by hardware.

@soburi soburi force-pushed the gd32_dma branch 3 times, most recently from d446511 to 6a6c12b Compare July 26, 2022 22:11
Copy link
Member

Choose a reason for hiding this comment

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

long lines...

-gd32_dma_interrupt_disable(cfg->reg, i, DMA_CHXCTL_FTFIE | GD32_DMA_INTERRUPT_ERRORS);
+gd32_dma_interrupt_disable(cfg->reg, i,
+                           DMA_CHXCTL_FTFIE |
+                           GD32_DMA_INTERRUPT_ERRORS);

Copy link
Member Author

Choose a reason for hiding this comment

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

Completed all fixes.
Sorry for taking up your time, but please review.

cameled
cameled previously approved these changes Jul 29, 2022
Copy link
Contributor

@cameled cameled left a comment

Choose a reason for hiding this comment

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

LGTM, dma loop_transfer test passed on my gd32f450i_eval and gd32f350r_eval boards.

Copy link
Member

Choose a reason for hiding this comment

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

Please, keep at 80col, rework all file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rerun clang-format with ColumnLimit: 80

Copy link
Contributor

Choose a reason for hiding this comment

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

If the clang format settings aren’t the style guide this is the inevitable issue. Please file an issue or PR against the clang format settings. The docs suggest running it, and the settings define a style that should make code fit the zephyr style and avoid exactly this discussion, if not it’s something that should be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

Hi @soburi ,

There was an agreement to allow up to 100 col in code. This doesn't mean that we will follow it.
Zephyr still use Linux kernel coding style and we will keep it for GigaDevices. I point 2 exceptions that are OK to use more than 80 columns.

Please, reformat code to use 80 columns.

soburi added 3 commits July 30, 2022 21:47
Add support for GD32 DMA

Signed-off-by: TOKITA Hiroshi <[email protected]>
Add DMA support for GD32 series.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Add configuration file for GD32 boards.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Copy link
Contributor

@teburd teburd left a comment

Choose a reason for hiding this comment

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

looks like a well written driver, very nice

Copy link
Member

@nandojve nandojve left a comment

Choose a reason for hiding this comment

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

Hi @soburi ,

Thank you for the patience. You made a very good job!

@nandojve nandojve added this to the v3.2.0 milestone Aug 1, 2022
@soburi
Copy link
Member Author

soburi commented Aug 1, 2022

Hi, @nandojve , @teburd , @cameled

Thank you for your review.
It's been a long review, but it's not the last.
Please also #47504. 😃

@carlescufi carlescufi merged commit d5ab3ee into zephyrproject-rtos:main Aug 2, 2022
@soburi soburi deleted the gd32_dma branch August 2, 2022 07:17
@soburi soburi restored the gd32_dma branch August 2, 2022 07:17
@soburi soburi deleted the gd32_dma branch August 2, 2022 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Devicetree area: DMA Direct Memory Access area: RISCV RISCV Architecture (32-bit & 64-bit) platform: GD32 GigaDevice

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants