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

Introduce reworked API for ADC drivers #7691

Merged
merged 21 commits into from Aug 29, 2018

Conversation

@anangl
Copy link
Collaborator

anangl commented May 21, 2018

This replaces #6176 and introduces a new API for ADC drivers basing on the feedback gathered there.
Implementations of drivers for ADC peripherals present in nRF chips are included, as well as updated and extended adc_api test to present the way the API is supposed to be used.

Marked as DNM, since existing ADC drivers (and "grove" sensors) need to be updated for this one to be merged.

List of drivers to be converted

  • nrfx
  • dw/qmsi
  • sam_afec
  • ti_adc108s102
  • mcux

Fixes #3980

@galak galak referenced this pull request May 21, 2018

Open

Driver API review/cleanup/rework #5697

18 of 33 tasks complete
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 21, 2018

Codecov Report

Merging #7691 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7691   +/-   ##
=======================================
  Coverage   52.12%   52.12%           
=======================================
  Files         212      212           
  Lines       25939    25939           
  Branches     5590     5590           
=======================================
  Hits        13521    13521           
  Misses      10177    10177           
  Partials     2241     2241

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0c3b35...47dfd81. Read the comment docs.

@anangl anangl force-pushed the anangl:adc_api_rework branch 2 times, most recently from c657298 to 7a19df3 May 22, 2018

u8_t channel_id : 5;

/** Channel type: single-ended or differential. */
bool differential : 1;

This comment has been minimized.

Copy link
@tbursztyka

tbursztyka May 24, 2018

Collaborator

Keep u8_t here, I am not entirely sure compiler will properly pack it with previous byte.

@nashif nashif added this to the v1.13.0 milestone May 29, 2018

@anangl anangl force-pushed the anangl:adc_api_rework branch 2 times, most recently from 9878e45 to 1a8b1f1 Jun 4, 2018

@anangl anangl force-pushed the anangl:adc_api_rework branch from 1a8b1f1 to 4c3a729 Jun 26, 2018

@anangl

This comment has been minimized.

Copy link
Collaborator Author

anangl commented Jun 26, 2018

  • refactored the test a bit, added one more test case for asynchronous calls
  • added an atomic variable to handle triggering of another sampling while the previous one is still in progress
  • added the possibility to perform multiple samplings with zero interval (as fast as possible)
@galak
Copy link
Contributor

galak left a comment

Use DT for IRQ info, should have DT nodes for adc controllers.

return -EBUSY;
}

IRQ_CONNECT(ADC_IRQn, CONFIG_ADC_0_IRQ_PRI,

This comment has been minimized.

Copy link
@galak

galak Jun 26, 2018

Contributor

We should get IRQ info from DT

This comment has been minimized.

Copy link
@anangl

anangl Jun 29, 2018

Author Collaborator

done

static nrfx_adc_channel_t m_channels[CONFIG_ADC_NRFX_ADC_CHANNEL_COUNT];


static int adc_nrfx_channel_setup(struct device *dev,

This comment has been minimized.

Copy link
@galak

galak Jun 26, 2018

Contributor

use nrfx_adc_ as a function prefix, its a bit confusing when seeing adc_ and thinking maybe that's an adc core function, and not a driver specific one.

This comment has been minimized.

Copy link
@anangl

anangl Jun 29, 2018

Author Collaborator

But the nrfx_ prefix is used for stuff that comes from nrfx, and in particular all API functions of the nrfx driver for ADC peripheral are prefixed with nrfx_adc_. So that would be confusing as well. On the other hand, adc_nrfx_ is consistent with the scheme we use for naming implementation files.
And as I see such approach is commonly used. See for example: spi_dw_transceive, i2c_mcux_configure, uart_stm32_poll_in. Thus, I would prefer to stay with this naming.

This comment has been minimized.

Copy link
@galak

galak Jul 3, 2018

Contributor

ok, that makes sense.

This comment has been minimized.

Copy link
@galak

galak Jul 3, 2018

Contributor

I guess my comment is about being consistent in this file, so either everything should be prefixed adc_nrfx_ or not.

nrf_saadc_int_enable(NRF_SAADC_INT_END);
NRFX_IRQ_ENABLE(SAADC_IRQn);

IRQ_CONNECT(SAADC_IRQn, CONFIG_ADC_0_IRQ_PRI,

This comment has been minimized.

Copy link
@galak

galak Jun 26, 2018

Contributor

IRQ num and priority should come from DT.

This comment has been minimized.

Copy link
@anangl

anangl Jun 29, 2018

Author Collaborator

done

@anangl anangl force-pushed the anangl:adc_api_rework branch from 4c3a729 to 77f8105 Jun 29, 2018

@anangl

This comment has been minimized.

Copy link
Collaborator Author

anangl commented Jun 29, 2018

  • rebased on the latest master to get the support for nRF52810 and update its "ADC affected" files in this PR
  • added DTS entries as requested by @galak
  • minor editorial changes: used BIT and BIT_MASK macros instead of shifting in ADC_ACQ_TIME macros, renamed ADC_DEV_NAME TO ADC_DEVICE_NAME in the "adc_api" test

@anangl anangl force-pushed the anangl:adc_api_rework branch from 77f8105 to 6cd8828 Jul 2, 2018

@anangl

This comment has been minimized.

Copy link
Collaborator Author

anangl commented Jul 2, 2018

  • removed syscalls from the API since read functions cannot use them, because of the callback that is executed in the ISR context, and there is a little sense in having syscall only for channel configuration

@anangl anangl force-pushed the anangl:adc_api_rework branch from 6cd8828 to b2cae0f Jul 3, 2018

@anangl

This comment has been minimized.

Copy link
Collaborator Author

anangl commented Jul 3, 2018

Rebased on master to resolve a conflict in "boards/arm/nrf52_pca20020/Kconfig.board".

@galak galak requested a review from fallrisk Jul 3, 2018

@galak
Copy link
Contributor

galak left a comment

Some comments on the DTS support and minor comments on code style for the NRF drivers.

@@ -6,4 +6,5 @@

config BOARD_NRF51_PCA10028
bool "nRF51 PCA10028"
select HAS_DTS_ADC

This comment has been minimized.

Copy link
@galak

galak Jul 3, 2018

Contributor

select HAS_DTS_ADC, should be with the driver, not the board.

@@ -6,5 +6,6 @@

config BOARD_NRF52810_PCA10040
bool "nRF52810 PCA10040"
select HAS_DTS_ADC

This comment has been minimized.

Copy link
@galak

galak Jul 3, 2018

Contributor

remove this, just do the select in the driver's Kconfig.

static nrfx_adc_channel_t m_channels[CONFIG_ADC_NRFX_ADC_CHANNEL_COUNT];


static int adc_nrfx_channel_setup(struct device *dev,

This comment has been minimized.

Copy link
@galak

galak Jul 3, 2018

Contributor

I guess my comment is about being consistent in this file, so either everything should be prefixed adc_nrfx_ or not.

*/
channel_id = 0;
do {
if (channels & 1) {

This comment has been minimized.

Copy link
@galak

galak Jul 3, 2018

Contributor

seems like the 1 here could be replaced w/define or some support. Is this an enable bit?

This comment has been minimized.

Copy link
@anangl

anangl Jul 10, 2018

Author Collaborator

Bits set to 1 in this bit-mask indicate that the corresponding channels are selected for the sampling sequence.
I refactored a bit this part of code (in both implementations). Hopefully, it will be clearer now.

@@ -13,10 +13,16 @@ menuconfig ADC
bool
prompt "ADC drivers"

This comment has been minimized.

Copy link
@galak

galak Jul 3, 2018

Contributor

add select HAS_DTS_ADC so that all ADC drivers require DTS support.

This comment has been minimized.

Copy link
@anangl

anangl Jul 10, 2018

Author Collaborator

But what would be the point in having HAS_DTS_ADC at all then? Right now it only excludes from Kconfig the options providing information that can be taken from DT, and these options are dependent on the same menuconfig entry that would select HAS_DTS_ADC. They should be rather removed then.
And this would be inconsistent with what we've got in other drivers. Is this a new approach that we want to take?

This comment has been minimized.

Copy link
@anangl

anangl Jul 10, 2018

Author Collaborator

As agreed on the API call, I'll add another commit with this change.

This comment has been minimized.

Copy link
@anangl

anangl Jul 11, 2018

Author Collaborator

done

@anangl anangl force-pushed the anangl:adc_api_rework branch from b2cae0f to 160d968 Jul 10, 2018

@nashif

nashif approved these changes Aug 27, 2018

@dbkinder
Copy link
Collaborator

dbkinder left a comment

Doc change LGTM. Commit message should be updated (still talks about being marked as DNM).

@MaureenHelm

This comment has been minimized.

Copy link
Member

MaureenHelm commented Aug 28, 2018

Commit message should be updated (still talks about being marked as DNM).

Which commit?

@mnkp

This comment has been minimized.

Copy link
Collaborator

mnkp commented Aug 29, 2018

I am aware that I am very late to the review, unfortunately I was not able to devote much time to the Zephyr project in last months.

The new ADC API is definitely the step in the right direction, however its design has some problems. The main one is the lack of support for triggers. ADC hardware comes in an overwhelming variety of design variants often with a distinctively different architectures. It makes it difficult to design universal API, there are however some fairly common features. One of such features is the concept of a trigger. Once the ADC module is configured - using the nomenclature of this API - the sequence is defined, the measurement can be started by a trigger whereas trigger is typically:

  • a software trigger
  • a timer trigger
  • a pin
  • a trigger of some internal trigger subsystem

of those the new API supports software trigger and makes an attempt to support timer trigger. The pin trigger and subsystem trigger is missing. The subsystem trigger is popular on many low power MCUs where it is possible to start ADC measurement on some internal, predefined condition without waking up the CPU.

Unfortunately this API can not be easily extended. The next time a project needs to trigger ADC measurement on the pin level change the ADC API would need to be completely reworked.

In Linux the ADC driver is handled by Industrial I/O driver (IIO) subsystem. Its API is build around the concept of triggers.

anangl and others added some commits May 21, 2018

drivers: adc: Introduce reworked API
This commit replaces the API for ADC drivers with a reworked one.
As requested in the issue #3980, the adc_enable/adc_disable functions
are removed. Additionaly, some new features are introduced, like:
- asynchronous calls
- configuration of channels
- multi-channel sampling

Common parts of code that are supposed to appear in each implementation
of the driver (like locking, synchronization, triggering of consecutive
samplings) are provided in the "adc_context.h" file to keep consistency
with the SPI driver. Syscalls are no longer present in the API because
the functions starting read requests cannot use them, since they can be
provided with a callback that is executed in the ISR context, and there
is no point in supporting syscalls only for the channels configuration.

"adc_api" test is updated and extended with additional test cases,
with intention to show how the API is supposed to be used.
"adc_simple" test is removed as it does not seem to add much value.

Signed-off-by: Andrzej Głąbek <andrzej.glabek@nordicsemi.no>
drivers: adc: Add shims for nrfx ADC and SAADC drivers
This commit adds translation layers to make nrfx drivers for the nRF
ADC (nRF51 series) and SAADC (nRF52 series) peripherals accessible via
the Zephyr's API. The SAADC peripheral is accessed using nrfx HAL only
as it turns out that usage of the nrfx driver in this case would be
inconvenient and would unnecessarily complicate the shim.

Signed-off-by: Andrzej Głąbek <andrzej.glabek@nordicsemi.no>
dts: nrf: Add ADC nodes and bindings for nRF SoCs
This commit adds ADC nodes to DTS files for nRF SoCs and introduces
corresponding  bindings for these nodes.

Signed-off-by: Andrzej Głąbek <andrzej.glabek@nordicsemi.no>
boards: nrf: Enable ADC nodes in DTS for nRF boards
ADC nodes are enabled in DTS for the following nRF development kits:
- nrf51_pca10028
- nrf52_pca10040
- nrf52_pca20020
- nrf52810_pca10040
- nrf52840_pca10056

Signed-off-by: Andrzej Głąbek <andrzej.glabek@nordicsemi.no>
tests: adc_api: Add configurations for nRF5 boards
This adds to the adc_api test board-specific configurations
for the following boards:
- nrf51_pca10028
- nrf52_pca10040
- nrf52840_pca10056

"adc" is indicated as supported in the yaml files for these boards
so that they are included in sanitycheck.

Signed-off-by: Andrzej Głąbek <andrzej.glabek@nordicsemi.no>
drivers: adc: Select HAS_DTS_ADC for all ADC drivers
To require relevant DTS entries on all platforms providing the ADC
driver.

Signed-off-by: Andrzej Głąbek <andrzej.glabek@nordicsemi.no>
adc: Convert mcux adc16 driver to the new adc api
Major rework of the mcux adc16 driver to convert it to the new adc api.
Currently supports a subset of the api features including synchronous
and asynchronous reads, and consecutive reads triggered by the kernel
timer.

Does not yet support some of the channel configuration options such as
gain and reference voltage because the hardware only allows these
options to be configured by peripheral instance rather than by channel.
The values are currently hardcoded in the driver, but in the future we
could introduce some flexibility per instance via device tree
attributes.

Does not yet support consecutive reads triggered by a hardware timer.

Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
tests: adc_api: Add support for kinetis boards
Adds board-specific macros to enable the adc_api test to run on kinetis
boards. This restores the test to support all the nxp boards that were
supported before the new adc api was introduced.

Signed-off-by: Maureen Helm <maureen.helm@nxp.com>
drivers: grove: Modify light sensor
Replace old ADC apis with new ones.

Signed-off-by: Punit Vara <punit.vara@intel.com>
drivers: grove: Update temperature driver
Replace old ADC driver APIs with new ones.

Signed-off-by: Punit Vara <punit.vara@intel.com>
sam: adc: Updated SAM ADC driver.
Updated to work with new ADC API.

Signed-off-by: Justin Watson <jwatson5@gmail.com>
drivers: Use Designware driver for sensor subsystem
Remove adc_qmsi_ss to use designware driver for sensor subsystem

Signed-off-by: Punit Vara <punit.vara@intel.com>
dts: arc: Use dts tree for designware driver
Get required data from dts tree.

Signed-off-by: Punit Vara <punit.vara@intel.com>
drivers: adc: Update new ADC APIs
Replace new ADC apis for designware driver

Signed-off-by: Punit Vara <punit.vara@intel.com>
tests: adc_api: Add ARC related parameters for ADC
Add ARC related parameters and modified test case to work
this test case on arc sensor subsystem.

Signed-off-by: Punit Vara <punit.vara@intel.com>
drivers: Kconfig.dw: Remove unnecessary Kconfig
Remove Kconfig which are replaced by dts and unnecessary macros which is
not useful anymore.

Signed-off-by: Punit Vara <punit.vara@intel.com>
boards: quark_se_c1000_ss_devboard: Enable ADC node
Enable ADC node in dts.

Signed-off-by: Punit Vara <punit.vara@intel.com>
samples: grove_light: Provide device name from Kconfig
check return value for sample fetch. Conditional printk
added to print adc value.

Signed-off-by: Punit Vara <punit.vara@intel.com>
samples: grove_temperature: Use device name from Kconfig
Use Kconfig to get device name. Make LIBC conditional to get
output.

Signed-off-by: Punit Vara <punit.vara@intel.com>
boards: quark_d2000_crb: Remove adc support
Remove ADC support from d2000 as it is not migrated to
new ADC apis.

Signed-off-by: Punit Vara <punit.vara@intel.com>
tests: adc_api: Remove sample width test scenario
Remove unnecessary sample width scenario.

Signed-off-by: Punit Vara <punit.vara@intel.com>

@MaureenHelm MaureenHelm force-pushed the anangl:adc_api_rework branch from ec5676f to 47dfd81 Aug 29, 2018

@nashif nashif merged commit 108b539 into zephyrproject-rtos:master Aug 29, 2018

1 check passed

Shippable Run 21470 status is SUCCESS.
Details

@anangl anangl deleted the anangl:adc_api_rework branch Dec 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.