Skip to content

Conversation

@FPlohl
Copy link
Contributor

@FPlohl FPlohl commented Mar 5, 2025

Add driver shim for the NXP Digital-to-Analog (DAC12) module and add DAC support for RT11xx SOCs.

Copy link
Member

@decsny decsny left a comment

Choose a reason for hiding this comment

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

it is a good contribution, so don't let this change request phase you, but I have some blocking comment about this PR that should be addressed

#include <zephyr/drivers/dac.h>
#include <zephyr/logging/log.h>

#include <fsl_dac12.h>
Copy link
Member

Choose a reason for hiding this comment

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

if this driver is depending on an MCUX SDK driver, then you need to also open a PR to the HAL_NXP zephyr module to edit the module cmake build it when appropriate. (am aware this is inconvenient, soon we are working to put this glue logic into the zephyr repo)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is depending on MCUX SDK driver and I already opened a PR on HAL_NXP module.

Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to update the zephyr/west.yaml manifest to link to your HAL_NXP PR. That will fix build issues with this PR, and keep the two repos synchronized. Setting the revision like below will link West to your HAL PR.

    - name: hal_nxp
      revision: pull/520/head

decsny
decsny previously approved these changes Mar 11, 2025
@decsny decsny added the DNM This PR should not be merged (Do Not Merge) label Mar 11, 2025
@decsny
Copy link
Member

decsny commented Mar 11, 2025

i approve generally the PR but you need to update manifest for HAL change

@DerekSnell
Copy link
Contributor

Hi @FPlohl ,
Thank you for this contribution. I do not see your update enabling the DAC at a board level. Can you enable on one of the RT11xx EVK boards and test with a DAC sample or test? Then your contribution can be automatically tested by CI on a board supported upstream.

Thank you

@FPlohl
Copy link
Contributor Author

FPlohl commented Mar 18, 2025

@DerekSnell I don't have any EVK boards, but I tested it with the DAC sample and the phyCORE-i.MX RT1170, for which a PR is currently in draft: #86756 . I added a commit to enable it for the MIMXRT1170-EVK.

Copy link
Contributor

@DerekSnell DerekSnell left a comment

Choose a reason for hiding this comment

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

Hi @FPlohl ,

Thank you for this contribution. I tested the DAC sample on the mimxrt1170_evk@A board, and the DAC outputs as it should. It works with both the cm7 and cm4 targets.

I see the dac_api test fails to build, and that appears to be causing CI failures for this PR. Since you will be updating, can you also update the MIMXRT1170-EVK board page and add a DAC row to the Supported Features table?

#include <zephyr/drivers/dac.h>
#include <zephyr/logging/log.h>

#include <fsl_dac12.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to update the zephyr/west.yaml manifest to link to your HAL_NXP PR. That will fix build issues with this PR, and keep the two repos synchronized. Setting the revision like below will link West to your HAL PR.

    - name: hal_nxp
      revision: pull/520/head

@decsny decsny removed the DNM This PR should not be merged (Do Not Merge) label Mar 20, 2025
@decsny
Copy link
Member

decsny commented Mar 20, 2025

i removed the DNM since CI fails due to manifest not being updated anyways

@FPlohl FPlohl force-pushed the WIP/FPlohl/rt11xx_dac branch from 1561e35 to e8c1ea9 Compare March 21, 2025 08:04
@zephyrbot
Copy link

zephyrbot commented Mar 21, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff

All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_nxp DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Mar 21, 2025
@FPlohl FPlohl force-pushed the WIP/FPlohl/rt11xx_dac branch 2 times, most recently from 1f86310 to f51b552 Compare March 31, 2025 07:48
@github-actions github-actions bot removed manifest manifest-hal_nxp DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Mar 31, 2025
FPlohl added 4 commits April 23, 2025 08:54
Include DAC12 HAL driver in case CONFIG_DAC_MCUX_DAC12 is enabled.

Signed-off-by: Florijan Plohl <florijan.plohl@norik.com>
Add driver shim for the NXP Digital-to-Analog (DAC12) module.

Signed-off-by: Florijan Plohl <florijan.plohl@norik.com>
Add DAC support for RT11xx SOCs.

Signed-off-by: Florijan Plohl <florijan.plohl@norik.com>
Enable DAC on MIMXRT1170-EVK for both Cortex M7 and M4.

Signed-off-by: Florijan Plohl <florijan.plohl@norik.com>
@FPlohl FPlohl force-pushed the WIP/FPlohl/rt11xx_dac branch from f51b552 to b7c3df7 Compare April 23, 2025 07:03
@FPlohl FPlohl requested review from DerekSnell and decsny April 23, 2025 08:55
@kartben kartben merged commit f72f1eb into zephyrproject-rtos:main May 13, 2025
28 checks passed
@FPlohl FPlohl deleted the WIP/FPlohl/rt11xx_dac branch May 13, 2025 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: DAC Digital-to-Analog Converter platform: NXP Drivers NXP Semiconductors, drivers platform: NXP NXP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants