Skip to content

Conversation

@mbolivar-nordic
Copy link

Pull in devicetree.h to enable HAL support for PWM
whenever the driver requests it.

Pull in devicetree.h to enable HAL support for PWM
whenever the driver requests it.

Signed-off-by: Martí Bolívar <[email protected]>
@mbolivar-nordic mbolivar-nordic requested review from anangl and galak April 20, 2020 20:55
@mbolivar-nordic
Copy link
Author

Now needed by zephyrproject-rtos/zephyr#24473

Comment on lines +149 to 157
#define NRFX_PWM_DT_ENABLED(idx) \
(DT_NODE_HAS_COMPAT(DT_NODELABEL(pwm##idx), nordic_nrf_pwm) && \
DT_HAS_NODE(DT_NODELABEL(pwm##idx)))

#if defined(CONFIG_NRFX_PWM) || DT_HAS_COMPAT(nordic_nrf_pwm)
#define NRFX_PWM_ENABLED 1
#endif
#ifdef CONFIG_NRFX_PWM0
#if defined(CONFIG_NRFX_PWM0) || NRFX_PWM_DT_ENABLED(0)
#define NRFX_PWM0_ENABLED 1
Copy link
Member

@anangl anangl Apr 21, 2020

Choose a reason for hiding this comment

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

This comment concerns also zephyrproject-rtos/zephyr#24473, which is tightly coupled with this PR.

I am not convinced that this is the right approach. What if one wants to use a different driver than nrfx_pwm.c for handling the PWM peripheral, or in general wants to use it in a custom way and only wants the information from DT (hence enables the node) but does not want the nrfx_pwm driver to be compiled?

Currently, it is possible to use a given nRF PWM peripheral in Zephyr in three ways:

  • use only its DT node properties, without automatically activating any PWM driver
  • use it through the Zephyr PWM API, after setting CONFIG_PWM=y and CONFIG_PWM_x=y (and as I understand we want to eliminate the need of setting the latter); this currently uses pwm_nrfx.c and nrfx_pwm.c underneath for for all instances, but it could be potentially extended with some other implementation of the driver
  • use it through the nrfx_pwm API, after setting CONFIG_NRFX_PWMx=y while CONFIG_PWM=n (pwm_nrfx.c will not be compiled then, only nrfx_pwm.c)

I think we should keep all these possibilities.

PWM is only an example here, same goes for other peripherals, too.

#define NRFX_PWM0_ENABLED 1
#endif
#ifdef CONFIG_NRFX_PWM1
#if defined(CONFIG_NRFX_PWM0) || NRFX_PWM_DT_ENABLED(1)
Copy link
Member

Choose a reason for hiding this comment

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

Wrong index in CONFIG_NRFX_PWMx. Here and below.

@mbolivar-nordic
Copy link
Author

Closing as discussed in zephyrproject-rtos/zephyr#24473 (comment)

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.

3 participants