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

imxrt: ADC driver #1868

Merged
merged 2 commits into from Sep 23, 2020
Merged

imxrt: ADC driver #1868

merged 2 commits into from Sep 23, 2020

Conversation

thomasactia
Copy link
Contributor

Based on LPC17xx_40xx and STM32 drivers.

Summary

ADC driver for the i.MX RT family.

Impact

Option to enable ADC driver for IMX RT chips. Default configured for the IMXRT1060 EVK.
ADC configuration is not tuned, but we have tested that it works.

Testing

Tested multiple channels on both ADCs with IMXRT1060 on custom board.

NOTE

ADSTS in ADC_CFG differs in IMXRT1060/1050 compared to 1020.
This PR changes the define names to correspond to the 1060/1050 names.
This needs to be split / done in a better way.

@davids5
Copy link
Contributor

davids5 commented Sep 22, 2020

@thomasactia -

Thank you for the PR!

There are some coding style issues causing this to fail CI. Have a look at

$ ./tools/checkpatch.sh -h

To fix them.

I will review this is the AM when I am fresh.

@thomasactia
Copy link
Contributor Author

@davids5

There are some coding style issues causing this to fail CI.

I did some modifications in existing headers and was hoping it could be acceptable. I'm not sure I'm up for the task at reorganizing those headers, if there isn't a tool that does the bulk job.

Based on LPC17xx_40xx and STM32 drivers.
@acassis
Copy link
Contributor

acassis commented Sep 23, 2020

Hi Thomas,
There still some issues:
../nuttx/tools/checkpatch.sh -g 3df8f79..HEAD
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:77:93: error: Block comments have different lengths
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:1:85: error: Block comments have different lengths
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:1:85: error: Long line found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:35:85: error: Long line found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:40:85: error: Long line found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:42:85: error: Long line found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:47:85: error: Long line found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:49:85: error: Long line found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:51:85: error: Long line found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:77:93: error: Long line found
##[warning]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:82:86: warning: Wrong column position of comment right of code
##[warning]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:83:86: warning: Wrong column position of comment right of code
##[warning]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:84:86: warning: Wrong column position of comment right of code
##[warning]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:85:86: warning: Wrong column position of comment right of code
##[warning]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:86:86: warning: Wrong column position of comment right of code
##[warning]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:87:86: warning: Wrong column position of comment right of code
##[warning]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:88:86: warning: Wrong column position of comment right of code
##[warning]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:89:86: warning: Wrong column position of comment right of code
##[warning]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:90:86: warning: Wrong column position of comment right of code
##[warning]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:91:86: warning: Wrong column position of comment right of code
##[warning]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/arch/arm/src/imxrt/hardware/imxrt_adc.h:92:86: warning: Wrong column position of comment right of code

@thomasactia
Copy link
Contributor Author

Hi @acassis ,

Please see my earlier reply to David:

davids5: There are some coding style issues causing this to fail CI.
thomasactia: > I did some modifications in existing headers and was hoping it could be acceptable. I'm not sure I'm up for the task at reorganizing those headers, if there isn't a tool that does the bulk job.

I did minor modifications in those files and my hope was that I would not have to fix the whole of them. Otherwise, is there any recommended tool or at least recommended layout?

@acassis
Copy link
Contributor

acassis commented Sep 23, 2020

Hi @acassis ,

Please see my earlier reply to David:

davids5: There are some coding style issues causing this to fail CI.
thomasactia: > I did some modifications in existing headers and was hoping it could be acceptable. I'm not sure I'm up for the task at reorganizing those headers, if there isn't a tool that does the bulk job.

I did minor modifications in those files and my hope was that I would not have to fix the whole of them. Otherwise, is there any recommended tool or at least recommended layout?

Thomas, there is not a tool to do the job for you, but I use nxstyle to spot the issues:
cd nuttx
cd tools
make nxstyle
cd ..
./tools nxstyle arch/blabla/blablabla/file.[ch]

@protobits
Copy link
Contributor

@davids5

There are some coding style issues causing this to fail CI.

I did some modifications in existing headers and was hoping it could be acceptable. I'm not sure I'm up for the task at reorganizing those headers, if there isn't a tool that does the bulk job.

Hi @thomasactia
as per our contribution guidlines we need your help in fixing style issues, even if you aren't responsible for them. If you don't know how to fix one, please ask and we'll help you.

@thomasactia
Copy link
Contributor Author

All right, I took a stab at the style problems. nxstyle produces no more errors.

@acassis acassis merged commit f193f0f into apache:master Sep 23, 2020
@davids5
Copy link
Contributor

davids5 commented Sep 23, 2020

@thomasactia - Thank you for fixing the the CS violations.

On the topic of: ADSTS in ADC_CFG differs in IMXRT1060/1050 to the 1020

The RT got a case of #ifdef rash when the 1020 came in. We have a very clean chip version separation on the STM32F7 and it should be used as template for how to keep the code clean. We will have to clean this up, before the RT turns into a fix-it-break-it party. It is early enough to catch it now before it gets out of hand as the STM32 did over the years.

Duplication is preferred some times, also using logical #if defing based on the IP features as apposed to chips. Kinetis uses this approach.

In the former case there would be a hardware/rt1020_adc.h hardware/rt1050-01060_adc.h selected in the rt_adc.h file. There may even be a common c file that includes the chip specific code.

In the latter case. The code get's #if defined(RT_HAS_ADSTS), then the hardware file just lites define that for the the specific chip.

The header file looks like:
https://github.com/apache/incubator-nuttx/blob/15f28896a0b2a9f7fe3fbae207a07b868b1e9fd3/arch/arm/src/kinetis/hardware/kinetis_mcg.h#L332-L341

and the versioning comes from
https://github.com/apache/incubator-nuttx/blob/15f28896a0b2a9f7fe3fbae207a07b868b1e9fd3/arch/arm/include/kinetis/kinetis_mcg.h#L286-L290

It is explained here
https://github.com/apache/incubator-nuttx/blob/a32912040f04b36d601e4748175c02ea89b1d0f6/arch/arm/include/kinetis/kinetis_sim.h#L50-L309

It takes a lot of work, but it makes for really clean code. The best part is that every time a chip gets added all the files do not change.

@btashton btashton added this to To-Add in Release Notes - 10.0.0 Oct 14, 2020
@Ouss4 Ouss4 moved this from To-Add to In Progress in Release Notes - 10.0.0 Oct 17, 2020
@Ouss4 Ouss4 moved this from In Progress to Added in Release Notes - 10.0.0 Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants