Skip to content

Conversation

@npal-cy
Copy link
Contributor

@npal-cy npal-cy commented Nov 15, 2024

Infineon: board: Add CONFIG_GPIO to defconfigs

Add CONFIG_GPIO from defconfigs for Infineon boards.

Revert pull/81377, which affect some ble samples which
used GPIO.

Signed-off-by: Nazar Palamar [email protected]

test: arm: irq: Add overlays files for Infineon boards

Changed interrupt priority for GPIO, default 6 is not suitable for
for the ZERO_LATENCY_IRQS function used in this test.
used in this test.

FIX CI: https://github.com/zephyrproject-rtos/zephyr/actions/runs/11524969451/job/32695832470?pr=80025
Signed-off-by: Nazar Palamar [email protected]

Add CONFIG_GPIO from defconfigs for Infineon boards.

Revert pull/81377, which affect some ble samples which
used GPIO.

Signed-off-by: Nazar Palamar <[email protected]>
Changed interrupt priority for GPIO, default 6 is not suitable for
for the ZERO_LATENCY_IRQS function used in this test.
used in this test.

Signed-off-by: Nazar Palamar <[email protected]>
@ifyall ifyall added this to the v4.0.0 milestone Nov 15, 2024
@mmahadevan108
Copy link
Contributor

I had expressed my concerns about deleting GPIO from the defconfig #81377 (comment)
However you had mentioned that change was needed for the zero latency use-case.
Can you please explain what you are changing here?

Copy link
Contributor

@thedjnK thedjnK left a comment

Choose a reason for hiding this comment

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

This was literally removed not even 1 day ago 7e1b00d why was this removed only to be re-added?

@npal-cy
Copy link
Contributor Author

npal-cy commented Nov 15, 2024

I had expressed my concerns about deleting GPIO from the defconfig #81377 (comment)
However you had mentioned that change was needed for the zero latency use-case.
Can you please explain what you are changing here?

Yes this changes was fixed zero latency use-case, but i found it affected some sample(#81435). So before 4.0 release it safe to fix locally only zero latency by adding overlay files rather then add overlays to a lot set of samples...(see my closed PR #81435)

@npal-cy
Copy link
Contributor Author

npal-cy commented Nov 15, 2024

This was literally removed not even 1 day ago 7e1b00d why was this removed only to be re-added?

see my comment above.

@npal-cy npal-cy requested a review from thedjnK November 15, 2024 22:38
@dkalowsk
Copy link
Contributor

Looking at this right now we are marking this for backport to 4.0.1. We're not comfortable dismissing the change request comment here based upon the answer provided.

@thedjnK
Copy link
Contributor

thedjnK commented Nov 16, 2024

Revert pull/81377, which affect some ble samples which used GPIO.

If a sample needs GPIO then the sample should enable it

@npal-cy
Copy link
Contributor Author

npal-cy commented Nov 18, 2024

If a sample needs GPIO then the sample should enable it

Hi thedjnK, need some clarification here.

  1. If board has some LED/buttons defined in board DTS, should we mandatory enable CONFIG_GPIO or not?

  2. "If a sample needs GPIO then the sample should enable..." I came to the same conclusion when I disabled GPIO from the board defconfig (7e1b00d). but in samples: bluetooth: update prj.conf (added CONFIG_GPIO) #81435 was explained that GPIO should be enabled by board-specific conf files (samples: bluetooth: update prj.conf (added CONFIG_GPIO) #81435 (comment)).
    I have decided to revert 7e1b00d. Otherwise, I will have to go over all the samples and check if they use GPIO. If so, I will need to add the board-specific configuration files to enable board.

@thedjnK
Copy link
Contributor

thedjnK commented Nov 18, 2024

If board has some LED/buttons defined in board DTS, should we mandatory enable CONFIG_GPIO or not?

It should be in the defconfig yes

"If a sample needs GPIO then the sample should enable..." I came to the same conclusion when I disabled GPIO from the board defconfig (7e1b00d). but in #81435 was explained that GPIO should be enabled by board-specific conf files (#81435 (comment)).

@Thalley seems like there is a problem with the samples, there should be #ifdef's for the GPIOs, one has it correct: https://github.com/npal-cy/zephyr-1/blob/35cd59ce4cee79aec8cb4956bb01d417c731d87e/samples/bluetooth/peripheral_hr/src/main.c#L122 but this is wrong because it assumes GPIO support is always enabled: https://github.com/npal-cy/zephyr-1/blob/35cd59ce4cee79aec8cb4956bb01d417c731d87e/samples/bluetooth/mesh/src/board.c#L14 a board might have buttons defined but if CONFIG_GPIO=n then the buttons shouldn't even be looked at

Copy link
Contributor

@thedjnK thedjnK left a comment

Choose a reason for hiding this comment

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

Changes here are OK but (outside scope of this PR) Bluetooth samples should be fixed so that having GPIO disabled does not cause build errors

@Thalley
Copy link
Contributor

Thalley commented Nov 18, 2024

If board has some LED/buttons defined in board DTS, should we mandatory enable CONFIG_GPIO or not?

It should be in the defconfig yes

"If a sample needs GPIO then the sample should enable..." I came to the same conclusion when I disabled GPIO from the board defconfig (7e1b00d). but in #81435 was explained that GPIO should be enabled by board-specific conf files (#81435 (comment)).

@Thalley seems like there is a problem with the samples, there should be #ifdef's for the GPIOs, one has it correct: https://github.com/npal-cy/zephyr-1/blob/35cd59ce4cee79aec8cb4956bb01d417c731d87e/samples/bluetooth/peripheral_hr/src/main.c#L122 but this is wrong because it assumes GPIO support is always enabled: https://github.com/npal-cy/zephyr-1/blob/35cd59ce4cee79aec8cb4956bb01d417c731d87e/samples/bluetooth/mesh/src/board.c#L14 a board might have buttons defined but if CONFIG_GPIO=n then the buttons shouldn't even be looked at

Yeah, I got aware of this issue. I haven't touched any of those samples, nor am I maintainer of them, so I would suggest to create GH issue for them and assign to @PavelVPV and @alwa-nordic (unless they want to do themselves). None of the ISO/LE audio samples have this issue as far as I am aware

@nashif nashif merged commit 6172092 into zephyrproject-rtos:main Nov 19, 2024
26 checks passed
SandraArrow added a commit to SandraArrow/zephyr that referenced this pull request Dec 13, 2024
Changed interrupt priority for GPIO.
See zephyrproject-rtos#81464

Signed-off-by: Sandra Schmidt <[email protected]>
kartben pushed a commit that referenced this pull request Dec 17, 2024
Changed interrupt priority for GPIO.
See #81464

Signed-off-by: Sandra Schmidt <[email protected]>
coreboot-bot pushed a commit to coreboot/zephyr-cros that referenced this pull request Dec 19, 2024
Changed interrupt priority for GPIO.
See zephyrproject-rtos/zephyr#81464

(cherry picked from commit ad6ef7d)

Original-Signed-off-by: Sandra Schmidt <[email protected]>
GitOrigin-RevId: ad6ef7d
Cr-Build-Id: 8728310571250663457
Cr-Build-Url: https://cr-buildbucket.appspot.com/build/8728310571250663457
Copybot-Job-Name: zephyr-main-copybot-downstream
Change-Id: I05ecbe95a7b69d0083a7fc83c272356d60aa45a6
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/6101992
Tested-by: Dawid Niedźwiecki <[email protected]>
Tested-by: ChromeOS Prod (Robot) <[email protected]>
Reviewed-by: Dawid Niedźwiecki <[email protected]>
Commit-Queue: Dawid Niedźwiecki <[email protected]>
Devansh0210 pushed a commit to Devansh0210/zephyr that referenced this pull request Jan 7, 2025
Changed interrupt priority for GPIO.
See zephyrproject-rtos#81464

Signed-off-by: Sandra Schmidt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ARM ARM (32-bit) Architecture platform: Infineon Infineon Technologies AG

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants