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.

Sign up for GitHub

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

Jump to bottom

drivers: ethernet: stm32: Add get_config() support #86315

Merged
merged 1 commit into from
Mar 19, 2025
Merged

drivers: ethernet: stm32: Add get_config() support #86315

merged 1 commit into from
Mar 19, 2025

Conversation

ramram885
Copy link
Contributor

Implementation of get_config() api added
Fixes: #84898

@ramram885 ramram885 force-pushed the add_get_config branch 2 times, most recently from 8d987ea to 22ed687 Compare February 26, 2025 14:52
@ramram885 ramram885 marked this pull request as ready for review February 27, 2025 01:31
@erwango erwango added this to the v4.2.0 milestone Feb 27, 2025
mathieuchopstm
mathieuchopstm reviewed Feb 27, 2025
View reviewed changes
drivers/ethernet/eth_stm32_hal.c
Comment on lines +1217 to +1391
#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_ethernet)
if (heth->Instance->MACPFR & ETH_MACPFR_PR) {
config->promisc_mode = true;
} else {
config->promisc_mode = false;
}
#else
if (heth->Instance->MACFFR & ETH_MACFFR_PM) {
config->promisc_mode = true;
} else {
config->promisc_mode = false;
}
#endif /* DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_ethernet) */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: could be split out to a dedicated function to make easier to maintain if new IPs with different registers are introduced.

Copy link
Member

Choose a reason for hiding this comment

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

+1 but could be part of a more generic driver update. See #86406

ramram885
ramram885 commented Mar 3, 2025
View reviewed changes
drivers/ethernet/eth_stm32_hal.c
Comment on lines +1217 to +1391
#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_ethernet)
if (heth->Instance->MACPFR & ETH_MACPFR_PR) {
config->promisc_mode = true;
} else {
config->promisc_mode = false;
}
#else
if (heth->Instance->MACFFR & ETH_MACFFR_PM) {
config->promisc_mode = true;
} else {
config->promisc_mode = false;
}
#endif /* DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_ethernet) */
Copy link
Contributor Author

@ramram885 ramram885 Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @mathieuchopstm and @erwango
Is it okay if I create a seperate function like this?

__STATIC_INLINE void stm32_eth_get_config_promisc_mode(Eth_HandleTypeDef *heth, struct ethernet_config *config)
{
#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_ethernet)
		if (heth->Instance->MACPFR & ETH_MACPFR_PR) {
			config->promisc_mode = true;
		} else {
			config->promisc_mode = false;
		}
#else
		if (heth->Instance->MACFFR & ETH_MACFFR_PM) {
			config->promisc_mode = true;
		} else {
			config->promisc_mode = false;
		}
#endif /* DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_ethernet) */
}

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think it would be better to address globally. See #86406

Copy link
Contributor Author

@ramram885 ramram885 Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

After reading #86406, I understood that we should use abstract function to address the issue.
Please correct me if I am wrong.
Can I use function pointers to achieve the abstraction?

or

Do you mean to solve the issue in seperate PR (Maybe in #86406. No need to address the issue in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, the problem is wider to this particular piece of code and split all over the driver. So I consider this is a work larger than fixing this new addition and hence would better be addressed in a dedicated PR.
Regarding the method, I'm opened, as long as:

  • it is readable and easy to maintain
  • it could be extended easily to new series with different register names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay @erwango. Then I will raise the pull request that solves #84898 this particular issue.

@ramram885 ramram885 force-pushed the add_get_config branch 2 times, most recently from 09d4385 to 6151ab3 Compare March 7, 2025 14:53
@ramram885 ramram885 requested a review from mathieuchopstm March 7, 2025 15:59
@maass-hamburg
Copy link
Collaborator

You have to rebase to the current main, not merge.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Mar 8, 2025 •
edited
Loading

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff

✅ All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@ramram885
Copy link
Contributor Author

Guys, I did something wrongly while rebasing. I guess it will take sometime to resolve this. Since I am going to out of station. Kindly apologize me for any inconvience caused here

@maass-hamburg
Copy link
Collaborator

Simply do a git rebase main

@ramram885
drivers: ethernet: stm32: Add get_config() support
e63e9f5 
Implementation of get_config() api added
Fixes: zephyrproject-rtos#84898

Signed-off-by: Ram Mahesh <rammaheshram1234@gmail.com>
@kartben kartben merged commit 66b2a6a into zephyrproject-rtos:main Mar 19, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erwango erwango erwango approved these changes

@maass-hamburg maass-hamburg maass-hamburg approved these changes

@decsny decsny Awaiting requested review from decsny

@djiatsaf-st djiatsaf-st Awaiting requested review from djiatsaf-st

@FRASTM FRASTM Awaiting requested review from FRASTM

@gautierg-st gautierg-st Awaiting requested review from gautierg-st

@GeorgeCGV GeorgeCGV Awaiting requested review from GeorgeCGV

@lmajewski lmajewski Awaiting requested review from lmajewski

@marwaiehm-st marwaiehm-st Awaiting requested review from marwaiehm-st

@pdgendt pdgendt Awaiting requested review from pdgendt

@mathieuchopstm mathieuchopstm Awaiting requested review from mathieuchopstm

Assignees

@erwango erwango

Labels
area: Ethernet platform: STM32 ST Micro STM32
Projects
None yet
Milestone
v4.2.0
Development

Successfully merging this pull request may close these issues.

drivers: ethernet: eth_stm32_hal: support get_config
6 participants
@ramram885 @maass-hamburg @zephyrbot @erwango @mathieuchopstm @kartben