search

flyingcys / rt-thread

RT-Thread is an open source IoT operating system.
http://www.rt-thread.io
Apache License 2.0
0 stars 1 forks source link

add board licheerv nano; add pinmux_config_gpio for gpio #30

Closed Ghazigq closed 3 months ago

Ghazigq commented 3 months ago

拉取/合并请求描述：(PR description)

[新增licheeRV nano板子支持；drv_gpio中增加将引脚设置成gpio操作（防止该引脚默认为其他复用功能）]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

代码质量 Code Quality：

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

unicornx commented 3 months ago

请确认在提交 PR 前已经使用 formating 工具对代码做过格式化检查。详情请阅读 PR 上的 "代码质量" 部分要求。

unicornx commented 3 months ago

对于 commit： [bsp/cvitek]: add board licheeRV nano

针对 commit: [bsp/cvitek]: add pinmux_config_gpio for gpio

先谈一下我们当初提供 pinmux_config 的来由：当初设计 pinmux_config 这个 API 其很大一部分初衷是为了配合 menuconfig，同时考虑到通用性，menuconfig 中 Pin 的配置直接采用了字符串形式（没有采用数字形式的原因是因为 sophgo 的 pinout 表格上没有为所有的 pin 定义数字，只能保证字符串形式的 pin name 是唯一的），而 Function 部分，因为一旦驱动类型确定了 Func 就确定了（譬如知道是 I2C，则知道是 SCL/SDA），所以 Func 我们采用了 fs_type。这也是 pinmux_config API 接口中 "const char *pin_name, fs_type func_type" 的原因。 目前为常用的驱动，I2C/SPI/... 都引入了 menuconfig 和 pinmux_config 的机制，但当时没有考虑在 menuconfig 上为 GPIO 引入该机制主要是考虑到 GPIO 涉及的对象特别多，mencuconfig 上不太好设计相应的界面，所以当时的考虑是对于 GPIO 就没有引入这套接口，如果用户需要对 GPIO 配置 pinmux 可以在代码中直接调用 pinmux_config 这个 API。

但现在想起来，让用户直接调用 pinmux_config 似乎的确不是很好的做法，因为 pinmux_config 应该是一个只在 driiver 层面的代码调用的 API。

目前看上去这个补丁中新增 pinmux_config_gpio API 的想法是想在 dwapb_pin_mode 中利用（复用）用户传入的 pin 信息来实现管脚复用设置。而我们认为 dwapb_pin_mode 实际上会被 rt_pin_mode 所调用，而 rt_pin_mode 可以认为是 RTT 暴露给应用部分调用的 API，所以感觉这个流程设计比让用户直接调用 pinmux_config 更好，虽然代码实现上还有商榷之处，具体对代码的comments 见 github 上嵌入的意见。

unicornx commented 3 months ago

PR 的 title 建议修改一下，同样加上 bsp/cvitek 的前缀，以及简短一句话说明 PR 提交的内容，不要简单复用 commit 的 title

参考其他的 PR。

unicornx commented 3 months ago

补充一下，在 commit 中请留下你的 签名（Signed-off-by: ），具体参考 72b59ab7ef542fb3c5763cd7f0c32faf9c29aa4c. 后面我们向主仓提交时也会带上你的签名，谢谢。

unicornx commented 3 months ago

@Ghazigq hi, 我看到你 force push 了开发分支，能否针对 review 的意见先逐个回复一下，没有任何回应地更新代码不是一个好习惯。

flyingcys commented 3 months ago

对于 commit： [bsp/cvitek]: add board licheeRV nano

  • 为何会包含 dtb 部分的改动，目前我们没有计划支持 dtb 吧？ @flyingcys 请帮忙确认一下？
  • pinmux 部分你的修改，主要是添加了部分驱动的 whitelist 部分，这部分改动是需要的。但我不确定的是你是否是直接 copy 了 duo 的改动，因为我发现有些注释还带有 duo 的字样（这部分希望你也一并改正）。所以希望和你确认一下你对这部分修改的理解。 我先谈一下原先的代码中设置 whitelist 的考虑，当时是根据开发板级别的管脚引出情况做了一次封装，譬如以 I2C 为例，虽然 SG2002 上有 I2C0 的输出，但是 Duo256 上没有 I2C0 的输出，所以在 whitelist 中我们标注为 "// I2C0 is not ALLOWED for Duo256"。你的设置是遵循了这个原则吗？
  • Licheerv nano 的管脚功能定义你参考的资料来源在哪里，建议在 PR 信息中给出，我希望 RTT 的 PR 信息也能够写得详细一些，向 linux 学习。参考 https://lore.kernel.org/linux-riscv/20231006121449.721-1-jszhang@kernel.org/。 如果你在 duo-v5.1.0 中 PR 时提供了这些信息，我会在后面向 RTT 主线 pr 时也复制提交上去。

针对 commit: [bsp/cvitek]: add pinmux_config_gpio for gpio

先谈一下我们当初提供 pinmux_config 的来由：当初设计 pinmux_config 这个 API 其很大一部分初衷是为了配合 menuconfig，同时考虑到通用性，menuconfig 中 Pin 的配置直接采用了字符串形式（没有采用数字形式的原因是因为 sophgo 的 pinout 表格上没有为所有的 pin 定义数字，只能保证字符串形式的 pin name 是唯一的），而 Function 部分，因为一旦驱动类型确定了 Func 就确定了（譬如知道是 I2C，则知道是 SCL/SDA），所以 Func 我们采用了 fs_type。这也是 pinmux_config API 接口中 "const char *pin_name, fs_type func_type" 的原因。 目前为常用的驱动，I2C/SPI/... 都引入了 menuconfig 和 pinmux_config 的机制，但当时没有考虑在 menuconfig 上为 GPIO 引入该机制主要是考虑到 GPIO 涉及的对象特别多，mencuconfig 上不太好设计相应的界面，所以当时的考虑是对于 GPIO 就没有引入这套接口，如果用户需要对 GPIO 配置 pinmux 可以在代码中直接调用 pinmux_config 这个 API。

但现在想起来，让用户直接调用 pinmux_config 似乎的确不是很好的做法，因为 pinmux_config 应该是一个只在 driiver 层面的代码调用的 API。

目前看上去这个补丁中新增 pinmux_config_gpio API 的想法是想在 dwapb_pin_mode 中利用（复用）用户传入的 pin 信息来实现管脚复用设置。而我们认为 dwapb_pin_mode 实际上会被 rt_pin_mode 所调用，而 rt_pin_mode 可以认为是 RTT 暴露给应用部分调用的 API，所以感觉这个流程设计比让用户直接调用 pinmux_config 更好，虽然代码实现上还有商榷之处，具体对代码的comments 见 github 上嵌入的意见。

dts在编译大核的时候是需要的，uboot在启动的时候需要检查对应的dts

flyingcys commented 3 months ago

@Ghazigq 建议同时修改 README 文件中的相关内容，增加 licheerv nano 相关内容

unicornx commented 3 months ago

@Ghazigq ”[bsp/cvitek]: add pinmux_config_gpio for gpio“ 这个补丁挺好的，能不能优先实现这个然后单独提个 pr 先 merge 掉？