Skip to content

Mec172x hal 001 #13

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

Closed

Conversation

scottwcpg
Copy link
Collaborator

Added MEC172x HAL headers. MCHP team generating headers generates complete headers for each chip instead of creating common files;<(

Origin: MCHP
https://github.com/MicrochipTech/hal_microchip

Status:
version 1.3.0

Add Microchip MEC172x HAL header files.
Update HAL common headers.

Signed-off-by: Scott Worley <[email protected]>
Origin: MCHP
https://github.com/MicrochipTech/hal_microchip

Status:
version 1.3.0

Add Microchip MEC172x HAL header files.
Update HAL common headers.

Signed-off-by: Scott Worley <[email protected]>
MEC152x keyscan header has a misspelled define being
used in a driver. Add misspelled define to MEC172x
keyscan header.

Signed-off-by: Scott Worley <[email protected]>
Add more defines for the new 32K clock configuration
and monitor in the PCR and VBAT register blocks.

Signed-off-by: Scott Worley <[email protected]>
Add missing MEC172x ADC register defines.

Signed-off-by: Scott Worley <[email protected]>
Update MEC172x basic timer register structure instance
names using new naming convention with backwards
compatibility where required.

Signed-off-by: Scott Worley <[email protected]>
Fix reserved space between MEC172x VBAT registers in
register structure.

Signed-off-by: Scott Worley <[email protected]>
@scottwcpg scottwcpg requested review from nashif and albertofloyd May 28, 2021 15:34
@sjg20
Copy link
Collaborator

sjg20 commented Jun 1, 2021

Can we put these files into the main Zephyr tree? Why is a module needed?

@henrikbrixandersen
Copy link
Member

Having those 12k LoC in a module allows downstream users to exclude them if not using this particular platform.

@sjg20
Copy link
Collaborator

sjg20 commented Jun 1, 2021

Having those 12k LoC in a module allows downstream users to exclude them if not using this particular platform.

Yes I understand that, but why? As soon as we add tests for this code we will need it. 12k LoC is not a lot compared to the >2 million in Zephyr. The disadvantage is that the core code is somewhere else and must be updated with a separate PR, so is harder to keep in step

Copy link
Collaborator

@sjg20 sjg20 left a comment

Choose a reason for hiding this comment

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

I think this should be in the main repo

@keith-zephyr
Copy link
Collaborator

Having those 12k LoC in a module allows downstream users to exclude them if not using this particular platform.

Yes I understand that, but why? As soon as we add tests for this code we will need it. 12k LoC is not a lot compared to the >2 million in Zephyr. The disadvantage is that the core code is somewhere else and must be updated with a separate PR, so is harder to keep in step

I agree, it would be preferred to add these files to the main Zephyr repo.

@keith-zephyr keith-zephyr self-requested a review June 1, 2021 20:33
@albertofloyd
Copy link
Collaborator

albertofloyd commented Jun 1, 2021

ode is somewhere else and must be updated with a separate PR, so is harder to keep in step

This is not MCHP-specific but it's there for all Zephyr HAL and modules https://docs.zephyrproject.org/latest/guides/modules.html

To keep track of specific HALs, and other module/components west is used.
https://docs.zephyrproject.org/latest/guides/west/manifest.html

@sjg20
Copy link
Collaborator

sjg20 commented Jun 1, 2021

ode is somewhere else and must be updated with a separate PR, so is harder to keep in step

This is not MCHP-specific but it's there for all Zephyr HAL and modules https://docs.zephyrproject.org/latest/guides/modules.html

To keep track of specific HALs, and other module/components west is used.
https://docs.zephyrproject.org/latest/guides/west/manifest.html

Does MHCP mean Microchip? If so, it seems that the code in this PR is actually Microchip-specific. If it is not, can you please explain what other microcontrollers use it?

@albertofloyd
Copy link
Collaborator

ode is somewhere else and must be updated with a separate PR, so is harder to keep in step

This is not MCHP-specific but it's there for all Zephyr HAL and modules https://docs.zephyrproject.org/latest/guides/modules.html
To keep track of specific HALs, and other module/components west is used.
https://docs.zephyrproject.org/latest/guides/west/manifest.html

Does MHCP mean Microchip? If so, it seems that the code in this PR is actually Microchip-specific. If it is not, can you please explain what other microcontrollers use it?

What I mean is that the module approach/usage is not Microchip-specific. It's the way all HAL and other Zephyr external modules are structured. Check the links given

@nashif
Copy link
Member

nashif commented Jun 1, 2021

@sjg20 is suggesting we move all those new headers into Zephyr and skip the module part completely. In the case of microchip and this new "HAL" it is actually just header files and no code that need to be shimmed, so all of the driver code (.c files) is in the tree and we depend on external headers to build this code. Maybe it is something to consider if the license allows that and if those files are all being used, ie, we should not import files into zephyr that are not used.

@sjg20
Copy link
Collaborator

sjg20 commented Jun 2, 2021

at I mean is that the module approach/usage is not Microchip-specific. It's the way all HAL and other Zephyr external modules are structured. Check the links given

I certainly understand that this is done. I even understand that several vendors do things this way (e.g. STM32), perhaps for historical reasons and for a desire to maintain their code somewhat separate from Zephyr. But I really don't see the point. It just makes things harder IMO, for the reasons I mention above.

@scottwcpg
Copy link
Collaborator Author

Abandoning. Working on MEC172x downsized header for Zephyr SoC folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants