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

[drivers/spi]修复spi configure会执行两次的问题 #9972

Merged
merged 3 commits into from
Feb 11, 2025

Conversation

Rbb666
Copy link
Member

@Rbb666 Rbb666 commented Feb 8, 2025

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

发现 device->bus->owner == RT_NULL || device->bus->owner == device 会导致设置两次SPI配置
[

为什么提交这份PR (why to submit this PR)

你的解决方案是什么 (what is your solution)

请提供验证的bsp和config (provide the config and bsp)

  • BSP:
  • .config:
  • action:

]

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

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

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality:

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

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确,包括缩进空格,命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
  • 没有垃圾代码,代码尽量精简,不包含#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的,并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 代码是高质量的 Code in this PR is of high quality
  • 已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification
  • 如果是新增bsp, 已经添加ci检查到.github/workflows/bsp_buildings.yml 详细请参考链接BSP自查

@Rbb666
Copy link
Member Author

Rbb666 commented Feb 8, 2025

@CYFS3 麻烦看下这个问题,我在st和瑞萨开发板上复现的,发现进入了两次配置

image

image

@CYFS3
Copy link
Contributor

CYFS3 commented Feb 9, 2025

@CYFS3 麻烦看下这个问题,我在st和瑞萨开发板上复现的,发现进入了两次配置

image

image

当时我这么改是因为发现当想配置spi的参数的时候,会返回非RT_EOK,而spi总线现在是没有被哪个spi的设备拥有的。所以我当时觉得需要加当等于总线拥有者等于null的时候也配置成功比较合理点,而且在soft spi验证的时候也没有出现问题。
上面这个问题也许可以改成下面这样。
PixPin_2025-02-09_10-31-11
看大佬什么别的看法,因为改成原来的也没有问题。

@Rbb666
Copy link
Member Author

Rbb666 commented Feb 10, 2025

@CYFS3 麻烦看下这个问题,我在st和瑞萨开发板上复现的,发现进入了两次配置
image
image

当时我这么改是因为发现当想配置spi的参数的时候,会返回非RT_EOK,而spi总线现在是没有被哪个spi的设备拥有的。所以我当时觉得需要加当等于总线拥有者等于null的时候也配置成功比较合理点,而且在soft spi验证的时候也没有出现问题。 上面这个问题也许可以改成下面这样。 PixPin_2025-02-09_10-31-11 看大佬什么别的看法,因为改成原来的也没有问题。

d17fafb 修改了下rt_spi_bus_configure的判忙条件,这次不会报错了

@CYFS3
Copy link
Contributor

CYFS3 commented Feb 10, 2025

@CYFS3 麻烦看下这个问题,我在st和瑞萨开发板上复现的,发现进入了两次配置
image
image

当时我这么改是因为发现当想配置spi的参数的时候,会返回非RT_EOK,而spi总线现在是没有被哪个spi的设备拥有的。所以我当时觉得需要加当等于总线拥有者等于null的时候也配置成功比较合理点,而且在soft spi验证的时候也没有出现问题。 上面这个问题也许可以改成下面这样。 PixPin_2025-02-09_10-31-11 看大佬什么别的看法,因为改成原来的也没有问题。

d17fafb 修改了下rt_spi_bus_configure的判忙条件,这次不会报错了

accept

@Rbb666
Copy link
Member Author

Rbb666 commented Feb 10, 2025

@wdfk-prog 麻烦看下,针对PR:#9503 的判忙条件进行了修改,这样改是否合理?

/* RT_EBUSY is not an error condition and
* the configuration will take effect once the device has the bus
*/
result = -RT_EBUSY;
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 这边的修改,强行把错误返回值改成了-RT_EBUSY了,但是rt_mutex_take也有可能有实际的错误返回值发生,这样错误情况就不对了;
  • 这一块应该还是得还原为原来的逻辑更合理.

/* RT_EBUSY is not an error condition and
* the configuration will take effect once the device has the bus
*/
result = -RT_EBUSY;
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 这里的判定是没问题的.
  • 不是当前使用的设备尝试配置总线,返回RT_EBUSY既可

Copy link
Member Author

@Rbb666 Rbb666 Feb 11, 2025

Choose a reason for hiding this comment

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

发现当前使用的设备尝试配置总线也会返回busy的,比如下面的测试代码:

/* 挂载到指定SPI总线 */
rt_hw_spi_device_attach("spi1", "wspi", -1);
spi_device = (struct rt_spi_device *)rt_device_find("wspi");
/* 返回的res会是-RT_EBUSY  */
rt_err_t res = rt_spi_configure(rw007_spi.spi_device, &cfg);

因为rt_spi_configure里面的 device->bus->owner 是 RT_NULL,在

加上device->bus->owner = device; 给owner设备赋值可以解决问题。

Copy link
Contributor

Choose a reason for hiding this comment

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

发现当前使用的设备尝试配置总线也会返回busy的,比如下面的测试代码:

/* 挂载到指定SPI总线 */
rt_hw_spi_device_attach(”spi1“, "wspi", -1);
spi_device = (struct rt_spi_device *)rt_device_find("wspi");
/* 返回的res会是-RT_EBUSY  */
rt_err_t res = rt_spi_configure(rw007_spi.spi_device, &cfg);

因为rt_spi_configure里面的 device->bus->owner 是 RT_NULL,在

加上device->bus->owner = device; 给owner设备赋值可以解决问题。

  • 这个位置添加这个逻辑,我觉得合理.
  • 应该加一个判断,避免每个attach device都修改owner
        if(device->bus->owner == RT_NULL)
        {
            device->bus->owner = device;
        }

Copy link
Member Author

Choose a reason for hiding this comment

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

发现当前使用的设备尝试配置总线也会返回busy的,比如下面的测试代码:

/* 挂载到指定SPI总线 */
rt_hw_spi_device_attach(”spi1“, "wspi", -1);
spi_device = (struct rt_spi_device *)rt_device_find("wspi");
/* 返回的res会是-RT_EBUSY  */
rt_err_t res = rt_spi_configure(rw007_spi.spi_device, &cfg);

因为rt_spi_configure里面的 device->bus->owner 是 RT_NULL,在

加上device->bus->owner = device; 给owner设备赋值可以解决问题。

  • 这个位置添加这个逻辑,我觉得合理.
  • 应该加一个判断,避免每个attach device都修改owner
        if(device->bus->owner == RT_NULL)
        {
            device->bus->owner = device;
        }

已修改

@Rbb666 Rbb666 merged commit f820c19 into RT-Thread:master Feb 11, 2025
45 checks passed
@Rbb666 Rbb666 deleted the spi branch February 11, 2025 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants