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

Fix incorrect bitfield name in ADC_Init #220

Open
wants to merge 1 commit into
base: release/2.6.x_k32w0
Choose a base branch
from
Open

Fix incorrect bitfield name in ADC_Init #220

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions drivers/lpc_adc/fsl_adc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,13 @@ void ADC_Init(ADC_Type *base, const adc_config_t *config)
tmp32 |= ADC_CTRL_TSAMP(config->sampleTimeNumber);
#if (defined(FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL) && FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL)
}
else
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @tcv-git, thanks for your effort. I'd like to know what ADC clock mode in your project? From the K32W0 user manual information, the device doesn't support synchronous mode, and the ADC sample time is determined by GPADC_CTRL0[GPADC_TSAMP], the CTRL[TSAMP] should stay to default during application. So, for your patch, in asynchronous mode, you have no way to set the GPADC_CTRL0[GPADC_TSAMP].
Could you please close this PR? as we need to do internal devices feature header and driver update to fix this bug.

Copy link
Author

Choose a reason for hiding this comment

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

Hi,

The device does support synchronous mode. See UM11323 Fig. 82. Perhaps you are reading the manual for another part?

I have set CLOCK_AttachClk(kNONE_to_ADC_CLK); and .clockMode = kADC_ClockSynchronousMode,. It is reading values correctly using the code fixed code I have supplied.

Have you tried this with hardware?

Copy link
Author

Choose a reason for hiding this comment

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

By the way, I have seen the part that you are referring to in section 17.1.2 of the register manual. The lack of correct punctuation and the fact that it contradicts the sentence immediately before and after it make It looks like a copy+paste error. Perhaps you could arrange for the manual to be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @tcv-git, yes, I have tried both synchronous mode and asynchronous mode on my local, ADC can convert in both modes. I checked with the internal team, it is strongly recommended you use asynchronous mode, using synchronous mode may result in reduced accuracy in some environments. The UM11323 needs to be updated to hide information about synchronous mode. We currently only describe related information in CTRL[ASYNMODE] and CTRL[CLKDIV] bitfield description field.

Copy link
Author

Choose a reason for hiding this comment

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

Hi Zhaoxiang,

Thankyou for your help. Both the pdf documentation and example projects in the SDK need to be updated because they all use synchronous mode. Can you also please confirm that this will be published in the device errata document? because many customers will have already written applications based on the pre-existing examples.

More importantly for me, if you look for the function update_ctrl0_adc_register in board_utility.c, it modifies the sample time in the GPADC_CTRL0 register. If this is required to be changed to synchronous mode then the sample time will have to be made in the CTRL register and I need to know which values to use to replace 0x1f and 0x14.

Thanks,
Tom

Copy link
Author

Choose a reason for hiding this comment

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

Hi,

By the way when you say "result in reduced accuracy in some environments", perhaps this is just caused by the driver bug writing incorrectly to the PASS field? Maybe synchronous mode does work fine once you make this fix. Are you please able to check internally what was the reason for this problem and repeat the testing? Maybe both modes should work now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Zhaoxiang,

Thankyou for your help. Both the pdf documentation and example projects in the SDK need to be updated because they all use synchronous mode. Can you also please confirm that this will be published in the device errata document? because many customers will have already written applications based on the pre-existing examples.

More importantly for me, if you look for the function update_ctrl0_adc_register in board_utility.c, it modifies the sample time in the GPADC_CTRL0 register. If this is required to be changed to synchronous mode then the sample time will have to be made in the CTRL register and I need to know which values to use to replace 0x1f and 0x14.

Thanks, Tom

Hello @tcv-git ,
Sorry for the late reply. We will update both ADC driver and driver examples, we will also push the ERRATA update.

The GPADC_CTRL0[GPADC_TSAMP] can be used in both asynchronous mode and synchronous mode. The CTRL0[TSAMP] can only be used in asynchronous mode (ADC working on 4MHz functional clock) in K32W041/061 devices.

The conclusion of ‘ADC conversion result accuracy reduces’ is based on the test results during the chip production process. I confirmed with the SoC designer, the synchronous mode is not recommended to be used. I'd like to know, the synchronous mode is must to be used on your project?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to add a new variable extendSampleTimeNumber to indicate GPADC_CTRL0[GPADC_TSAMP], add a new feature macro FSL_FEATURE_ADC_HAS_GPADC_CTRL0_GPADC_TSAMP to indicate whether the ADC in the device has the GPADC_CTRL0 register. Then the code should be:

#if (defined(FSL_FEATURE_ADC_HAS_GPADC_CTRL0_GPADC_TSAMP) && FSL_FEATURE_ADC_HAS_GPADC_CTRL0_GPADC_TSAMP)
    uint32_t tmp_ctrl0 = base->GPADC_CTRL0;
    tmp_ctrl0 &= ~ADC_GPADC_CTRL0_GPADC_TSAMP_MASK;
    tmp_ctrl0 |= ADC_GPADC_CTRL0_GPADC_TSAMP(config->extendSampleTimeNumber); 
    base->GPADC_CTRL0 = tmp_ctrl0;
#endif /* FSL_FEATURE_ADC_HAS_GPADC_CTRL0_GPADC_TSAMP */

#if defined(FSL_FEATURE_ADC_HAS_CTRL_TSAMP) & FSL_FEATURE_ADC_HAS_CTRL_TSAMP
    /* Sample time clock count. */
#if (defined(FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL) && FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL)
    if (config->clockMode == kADC_ClockAsynchronousMode)
    { 
#endif /* FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL */         
        tmp32 |= ADC_CTRL_TSAMP(config->sampleTimeNumber); 
#if (defined(FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL) && FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL)
    }
#endif /* FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL */
#endif /* FSL_FEATURE_ADC_HAS_CTRL_TSAMP */

Copy link
Author

Choose a reason for hiding this comment

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

Hi Zhaoxiang,

Yes we can use asynchronous mode with a 4MHz clock. I was only trying to save a small amount of power by using synchronous mode.

After the updated SDK is available I am happy for you to reject this pull request.

Thanks,
Tom

{
uint32_t tmp_ctrl0 = base->GPADC_CTRL0;
tmp_ctrl0 &= ~ADC_GPADC_CTRL0_GPADC_TSAMP_MASK;
tmp_ctrl0 |= ADC_GPADC_CTRL0_GPADC_TSAMP(config->sampleTimeNumber);
base->GPADC_CTRL0 = tmp_ctrl0;
}
#endif /* FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL */
#endif /* FSL_FEATURE_ADC_HAS_CTRL_TSAMP. */
#if defined(FSL_FEATURE_ADC_HAS_CTRL_LPWRMODE) & FSL_FEATURE_ADC_HAS_CTRL_LPWRMODE
Expand All @@ -109,10 +116,6 @@ void ADC_Init(ADC_Type *base, const adc_config_t *config)

#if defined(FSL_FEATURE_ADC_HAS_GPADC_CTRL0_LDO_POWER_EN) && FSL_FEATURE_ADC_HAS_GPADC_CTRL0_LDO_POWER_EN
base->GPADC_CTRL0 |= ADC_GPADC_CTRL0_LDO_POWER_EN_MASK;
if (config->clockMode == kADC_ClockSynchronousMode)
{
base->GPADC_CTRL0 |= ADC_GPADC_CTRL0_PASS_ENABLE(config->sampleTimeNumber);
}
SDK_DelayAtLeastUs(300, SDK_DEVICE_MAXIMUM_CPU_CLOCK_FREQUENCY);
#endif /* FSL_FEATURE_ADC_HAS_GPADC_CTRL0_LDO_POWER_EN */

Expand Down