Enable lpdac for lpcxpresso55s36 #61459

Albort12138 · 2023-08-14T06:44:58Z

Created mcux lpdac driver;
Enabled dac example on lpcxpresso55s36;
Verify dac test suit on lpcxpresso55s36.

Albort12138 · 2023-08-14T07:29:37Z

Blocked by approve of zephyrproject-rtos/hal_nxp#265

mmahadevan108 · 2023-08-15T15:51:32Z

@Albort12138 thank you for the PR, can you help address the other CI failures.

dts/arm/nxp/nxp_lpc55S3x_common.dtsi

soc/arm/nxp_lpc/lpc55xxx/soc.c

dts/bindings/dac/nxp, lpdac.yaml

Albort12138 · 2023-08-16T14:40:36Z

@Albort12138 thank you for the PR, can you help address the other CI failures.

Thanks your comment, I have updated the PR.

mmahadevan108 · 2023-08-16T17:32:33Z

@Albort12138 Reopening the PR, I am assuming it was accidentally closed.

mmahadevan108 · 2023-08-16T17:33:02Z

HAL side has merged, please update west.yml

boards/arm/lpcxpresso55s36/lpcxpresso55s36-pinctrl.dtsi

mmahadevan108 · 2023-08-16T20:29:49Z

@Albort12138 , please take a look at the CI failures.

drivers/dac/dac_mcux_lpdac.c

decsny · 2023-08-21T19:53:40Z

Previous driver implementation uses the mcux prefix for the .c implementation, but kinetis in the bindings/compatibles (e.g. nxp,kinetis-dac). Are all NXP MCUs named MCUX and is kinetis the series? If yes, this driver should probably be prefixed with lpc, as LPC is the series of this one, right?

LPDAC is on more than just LPC, same as the one called kinetis is on more than just kinetis. Whoever implemented the "kinetis" one originally just called it that because they were probably doing it for a kinetis part. We are now trying to avoid using SOC series names in binding names for this reason, because the IP blocks are not specific to certain series, over time, they get re-used. If anything, the name of the kinetis one should be changed (there are a lot of nxp bindings in the tree that are not ideally named but for backward compatibility we don't change them willy-nilly)

And where does the name lpdac come from? In the LPC553x Reference Manual it is just named DAC, as far as I can see.

The chapter is called, DAC, section 47.2 says the full name (Low Power Digital-to-Analog Converter), and introduces an abbreviation DAC to conveniently refer to within the RM and chapter (since there is only one type of DAC in the SOC, it's not confusing within that context). The NXP HAL calls it LPDAC in the device files from this full name to distinguish it from other dacs. I believe the driver folder cmake module however is called dac_1.

martinjaeger · 2023-08-21T20:03:54Z

Ok, thanks @decsny for the explanation. Final question: What does MCUX stand for? Does this cover all NXP MCUs?

@Albort12138 can you please add a comment to the binding stating the MCU series this compatible covers (so far)? This will be helpful for people adding support for future MCUs and for people like me who are not so familiar with all these names and IPs. Also please explain what the abbreviation lpdac stands for (see above).

martinjaeger · 2023-08-21T20:07:36Z

drivers/dac/Kconfig.mcux

@@ -19,6 +19,13 @@ config DAC_MCUX_DAC32
 	help
 	  Enable the driver for the NXP Kinetis MCUX DAC32.

+config DAC_MCUX_LPDAC
+	bool "NXP Kinetis MCUX LPDAC driver"


Given the discussion about naming we had before, this is probably wrong?

I agree that we should remove "kinetis" at least from any terminology. "mcux" I think is loosely used too but comes from the name of the NXP MCU SDK which is called MCUX SDK.

MCUX should not be in binding names, but if a driver is using MCUX SDK to implement, and calls some functions in the code or kconfigs as MCUX, I think it's probably fine, but it doesn't matter much because the user of zephyr ideally doesn't see this

Would CONFIG_DAC_NXP_LPDAC not be the most self-explanatory then? MCUX seems to be used also by other vendors.

That would be an acceptable Kconfig name, yes, we don't need to have mcux in the name

Ok. It would also match the compatible.

@Albort12138 can you please change to CONFIG_DAC_NXP_LPDAC? After that we should be ready to go.

decsny · 2023-08-21T20:44:43Z

@Albort12138 can you please add a comment to the binding stating the MCU series this compatible covers (so far)?

I don't think this should be done. The IP block is agnostic to what SOC series it is used in, and therefore should not be part of the binding at all, including the description. Personally I just use grep in the dts folder when I want to quickly check what platforms a binding is used on.

Also please explain what the abbreviation lpdac stands for (see above).

agree

What does MCUX stand for? Does this cover all NXP MCUs?

It is short for "MCUXpresso" which is the branding of all the NXP MCU software development tools.

martinjaeger · 2023-08-21T20:51:06Z

@Albort12138 can you please add a comment to the binding stating the MCU series this compatible covers (so far)?

I don't think this should be done. The IP block is agnostic to what SOC series it is used in, and therefore should not be part of the binding at all, including the description. Personally I just use grep in the dts folder when I want to quickly check what platforms a binding is used on.

Ok, fine with that. Just want to have as much clarity as possible. Explaining the abbreviation would do it then.

drivers/dac/dac_mcux_lpdac.c

mmahadevan108 · 2023-08-29T16:20:57Z

drivers/dac/dac_mcux_lpdac.c

+
+static int mcux_lpdac_init(const struct device *dev)
+{
+	return 0;


Can we call DAC_Init() from this function instead of channel_setup? The init should be done only once.

DAC_Init is the only function that the SDK provides to configure the channels, so I don't think so.

Albort12138 · 2023-09-06T01:55:50Z

Hi team, any other comments about this PR?

danieldegrasse · 2023-09-06T12:49:52Z

dts/bindings/dac/nxp,lpdac.yaml

+    type: int
+    required: true
+    description: |
+      DAC voltage reference select. The meaning of the value maybe


Suggested change

DAC voltage reference select. The meaning of the value maybe

DAC voltage reference select. The meaning of the value may be

danieldegrasse · 2023-09-06T12:51:17Z

dts/arm/nxp/nxp_lpc55S3x_common.dtsi

@@ -294,6 +294,33 @@
 		dma-names = "adc0-dma0", "adc0-dma1";
 	};

+	dac0: dac@b2000 {
+		compatible = "nxp,lpdac";
+		reg = < 0xB2000 0x1000>;


please align casing for these reg properties with remainder of file: IE:

Suggested change

reg = < 0xB2000 0x1000>;

reg = < 0xb2000 0x1000>;

Create dac_mcxu_lpdac.c file to implement mcux lpdac, add binding for the mcux lpdac, update Kconfig.mcux and CMakeLists.txt file to support mcux lpdac. Signed-off-by: Albort Xue <yao.xue@nxp.com>

Added dac support for the LPC55S36 board, updated lpc55xxx/soc.c to enable clock and power for dac0. Signed-off-by: Albort Xue <yao.xue@nxp.com>

Added overlay for lpcxpresso55s36, updated README.rst Signed-off-by: Albort Xue <yao.xue@nxp.com>

Added overlay for lpcxpresso55s36 to verify dac driver. Signed-off-by: Albort Xue <yao.xue@nxp.com>

Albort12138 · 2023-09-09T10:07:32Z

Seems I am not authorized to merge this PR, Could someone help to merge this PR?

github-actions

Hi @Albort12138!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Albort12138 requested review from nashif, mmahadevan108, dleach02 and martinjaeger as code owners August 14, 2023 06:44

zephyrbot added area: DAC Digital-to-Analog Converter area: Devicetree Binding PR modifies or adds a Device Tree binding platform: NXP NXP labels Aug 14, 2023

zephyrbot requested review from danieldegrasse, dbaluta, decsny, DerekSnell, EmilioCBen, galak, iuliana-prodan, manuargue and yvanderv August 14, 2023 06:45

zephyrbot assigned martinjaeger Aug 14, 2023

danieldegrasse reviewed Aug 16, 2023

View reviewed changes

dts/arm/nxp/nxp_lpc55S3x_common.dtsi Outdated Show resolved Hide resolved

soc/arm/nxp_lpc/lpc55xxx/soc.c Show resolved Hide resolved

dts/bindings/dac/nxp, lpdac.yaml Outdated Show resolved Hide resolved

Albort12138 force-pushed the enable-lpdac-for-lpcxpresso55s36 branch from 81b6d4b to 0108eda Compare August 16, 2023 14:38

Albort12138 closed this Aug 16, 2023

mmahadevan108 reopened this Aug 16, 2023

mmahadevan108 reviewed Aug 16, 2023

View reviewed changes

boards/arm/lpcxpresso55s36/lpcxpresso55s36-pinctrl.dtsi Show resolved Hide resolved

martinjaeger reviewed Aug 16, 2023

View reviewed changes

drivers/dac/dac_mcux_lpdac.c Outdated Show resolved Hide resolved

Albort12138 force-pushed the enable-lpdac-for-lpcxpresso55s36 branch from 0108eda to 995d542 Compare August 17, 2023 03:30

martinjaeger requested changes Aug 21, 2023

View reviewed changes

Albort12138 force-pushed the enable-lpdac-for-lpcxpresso55s36 branch from feccdcb to fef3dba Compare August 22, 2023 01:59

zephyrbot requested a review from bperseghetti August 22, 2023 02:00

Albort12138 force-pushed the enable-lpdac-for-lpcxpresso55s36 branch 2 times, most recently from b32e82a to 9b18736 Compare August 22, 2023 06:25

mmahadevan108 reviewed Aug 29, 2023

View reviewed changes

drivers/dac/dac_mcux_lpdac.c Show resolved Hide resolved

mmahadevan108 reviewed Aug 29, 2023

View reviewed changes

Albort12138 force-pushed the enable-lpdac-for-lpcxpresso55s36 branch from 9b18736 to b11acca Compare August 30, 2023 02:40

zephyrbot requested a review from PetervdPerk-NXP August 30, 2023 02:41

Albort12138 force-pushed the enable-lpdac-for-lpcxpresso55s36 branch from b11acca to 0272c62 Compare August 31, 2023 09:04

martinjaeger previously approved these changes Sep 6, 2023

View reviewed changes

danieldegrasse reviewed Sep 6, 2023

View reviewed changes

Albort12138 dismissed martinjaeger’s stale review via 9dca99e September 7, 2023 08:53

Albort12138 force-pushed the enable-lpdac-for-lpcxpresso55s36 branch from 0272c62 to 9dca99e Compare September 7, 2023 08:53

Albort12138 added 4 commits September 8, 2023 16:23

drivers: dac: Add driver for mcux lpdac

49eb0cc

Create dac_mcxu_lpdac.c file to implement mcux lpdac, add binding for the mcux lpdac, update Kconfig.mcux and CMakeLists.txt file to support mcux lpdac. Signed-off-by: Albort Xue <yao.xue@nxp.com>

boards: arm: lpcxpresso55s36: Added dac support for LPC55S36

f037515

Added dac support for the LPC55S36 board, updated lpc55xxx/soc.c to enable clock and power for dac0. Signed-off-by: Albort Xue <yao.xue@nxp.com>

samples: drivers: dac: Enable dac example on lpcxpresso55s36 board

c907628

Added overlay for lpcxpresso55s36, updated README.rst Signed-off-by: Albort Xue <yao.xue@nxp.com>

tests: drivers: dac: Verify dac on lpcxpresso55s36 board

31ae1fc

Added overlay for lpcxpresso55s36 to verify dac driver. Signed-off-by: Albort Xue <yao.xue@nxp.com>

Albort12138 force-pushed the enable-lpdac-for-lpcxpresso55s36 branch from 9dca99e to 31ae1fc Compare September 8, 2023 08:24

danieldegrasse approved these changes Sep 8, 2023

View reviewed changes

martinjaeger approved these changes Sep 8, 2023

View reviewed changes

carlescufi merged commit 4d9fb55 into zephyrproject-rtos:main Sep 11, 2023
18 checks passed

github-actions bot reviewed Sep 11, 2023

View reviewed changes

Albort12138 deleted the enable-lpdac-for-lpcxpresso55s36 branch September 12, 2023 01:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable lpdac for lpcxpresso55s36 #61459

Enable lpdac for lpcxpresso55s36 #61459

Albort12138 commented Aug 14, 2023

Albort12138 commented Aug 14, 2023 •

edited

mmahadevan108 commented Aug 15, 2023

Albort12138 commented Aug 16, 2023 •

edited

mmahadevan108 commented Aug 16, 2023

mmahadevan108 commented Aug 16, 2023

mmahadevan108 commented Aug 16, 2023

decsny commented Aug 21, 2023 •

edited

martinjaeger commented Aug 21, 2023

martinjaeger Aug 21, 2023

decsny Aug 21, 2023 •

edited

martinjaeger Aug 21, 2023

decsny Aug 22, 2023

martinjaeger Aug 22, 2023

decsny commented Aug 21, 2023 •

edited

martinjaeger commented Aug 21, 2023

mmahadevan108 Aug 29, 2023 •

edited

decsny Aug 29, 2023

Albort12138 commented Sep 6, 2023

danieldegrasse Sep 6, 2023

danieldegrasse Sep 6, 2023

Albort12138 commented Sep 9, 2023

github-actions bot left a comment

	DAC voltage reference select. The meaning of the value maybe
	DAC voltage reference select. The meaning of the value may be

Enable lpdac for lpcxpresso55s36 #61459

Enable lpdac for lpcxpresso55s36 #61459

Conversation

Albort12138 commented Aug 14, 2023

Albort12138 commented Aug 14, 2023 • edited

mmahadevan108 commented Aug 15, 2023

Albort12138 commented Aug 16, 2023 • edited

mmahadevan108 commented Aug 16, 2023

mmahadevan108 commented Aug 16, 2023

mmahadevan108 commented Aug 16, 2023

decsny commented Aug 21, 2023 • edited

martinjaeger commented Aug 21, 2023

martinjaeger Aug 21, 2023

Choose a reason for hiding this comment

decsny Aug 21, 2023 • edited

Choose a reason for hiding this comment

martinjaeger Aug 21, 2023

Choose a reason for hiding this comment

decsny Aug 22, 2023

Choose a reason for hiding this comment

martinjaeger Aug 22, 2023

Choose a reason for hiding this comment

decsny commented Aug 21, 2023 • edited

martinjaeger commented Aug 21, 2023

mmahadevan108 Aug 29, 2023 • edited

Choose a reason for hiding this comment

decsny Aug 29, 2023

Choose a reason for hiding this comment

Albort12138 commented Sep 6, 2023

danieldegrasse Sep 6, 2023

Choose a reason for hiding this comment

danieldegrasse Sep 6, 2023

Choose a reason for hiding this comment

Albort12138 commented Sep 9, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

Albort12138 commented Aug 14, 2023 •

edited

Albort12138 commented Aug 16, 2023 •

edited

decsny commented Aug 21, 2023 •

edited

decsny Aug 21, 2023 •

edited

decsny commented Aug 21, 2023 •

edited

mmahadevan108 Aug 29, 2023 •

edited