drivers: Add Atmel SAM DAC (DACC) driver #37656
Conversation
mnkp
added
area: Drivers
platform: Microchip SAM
github-actions
bot
added
area: Boards
area: Devicetree
area: Devicetree Binding
Add Digital-to-Analog Converter driver (based on DACC module) for Atmel SAM MCU family. Only SAME70, SAMV71 series devices are supported in this version. Tested on Atmel SMART SAM E70 Xplained board. Origin: Original Signed-off-by: Piotr Mienkowski <[email protected]>
mnkp
force-pushed
the
add_dac_sam_driver
branch
from
ff0cb23 to
b3d2c77
Compare
martinjaeger
left a comment
martinjaeger
left a comment
There was a problem hiding this comment.
LGTM from DAC driver perspective.
I'm not very familiar with Atmel SAM platform specifics and it looks like we don't have an official maintainer. Any suggestions for someone with Atmel SAM experience for a second review?
There was a problem hiding this comment.
For other boards this is defined in samples/drivers/dac/boards/board_xyz.overlay. This might be the better location so that the original board dts file does not get too cluttered and also because those nodes are only used in that sample code.
There was a problem hiding this comment.
When preparing the PR I have noticed that putting the settings directly in the board file breaks the pattern. Never a good thing. However, using overlays does not seem to be the right approach. Overlays make the settings application specific, that's not the case. If we add new sample applications (or if the user was to use the board in a few different firmware projects) we would need to duplicate overlay files. Their maintenance will quickly become troublesome. In a way boards/ directory was introduced to unclutter application directories. But of course, if I'm requested to use .overlay files I'll update the PR.
There was a problem hiding this comment.
Well, the setting is kind-of application-specific, as the application needs to define which of the many available DAC pins it wants to choose. In contrast to LEDs, we don't have a fixed pin for the DAC.
If we decide it's the wrong approach to use overlays for this sample, then let's do it for all boards, but not just for one board. So I think it's better to not break the pattern here.
There was a problem hiding this comment.
How are these settings being used across different applications? Is this something that needs to be pointed at from the DAC node itself in the devicetree instead?
There was a problem hiding this comment.
At the moment we have only one sample application samples/drivers/dac/ that is using the properties.
What we try to achieve here if very similar to what we do to describe GPIO pin that is driving an LED. In principle we should do it the same way as GPIO. That is, by passing extra cells when referring to DAC phandle, as in
foo: device@0 { };
device@1 {
sibling = <&foo 1 2>;
};
All the DAC compatibles define
"#io-channel-cells":
const: 1
io-channel-cells:
- output
in DTS we would have
some_consumer {
compatible = "some-consumer";
io-channels = <&dac 10>, <&dac 11>;
};
That comes from Linux. Unfortunately, we can't do it currently since there is no way to get dac-resolution which application also requires. I believe the best course of action would be to extend DAC API to return resolution (or the default resolution) and write proper DT.
What I did is based on existing examples
For other boards this is defined in samples/drivers/dac/boards/board_xyz.overlay.
It's just that I placed it directly in the boards/ folder instead of using overlays. I'm going to move the settings to overlays as requested by @martinjaeger. We can fix all boards at the same time.
There was a problem hiding this comment.
I agree this would be the right approach for a custom application. But for the in-tree sample it means we need a generic some-consumer dts binding for DAC outputs. That would end up in a similar discussion as we had for GPIOs, where the agreement was that there should be no generic GPIO pin binding as each GPIO has an application-specific purpose that should be expressed in the binding.
Removing the dac-resolution from the overlays by creating API call to get the max or default resolution sounds like a good idea to me.
mbolivar-nordic
left a comment
mbolivar-nordic
left a comment
There was a problem hiding this comment.
zephyr,user needs more discussion. This is explicitly for application use.
This node is meant for sample code and applications.
https://docs.zephyrproject.org/latest/guides/dts/bindings.html#inferred-bindings
Enable support for Atmel SAM DAC driver on the following boards: - sam_e70_xplained - sam_e70b_xplained - sam_v71_xult - sam_v71b_xult Signed-off-by: Piotr Mienkowski <[email protected]>
Enable support for Atmel SAM DAC driver on the following boards: - sam_e70_xplained - sam_e70b_xplained - sam_v71_xult - sam_v71b_xult Signed-off-by: Piotr Mienkowski <[email protected]>
mnkp
force-pushed
the
add_dac_sam_driver
branch
from
b3d2c77 to
5c933fb
Compare
6 participants
Add Digital-to-Analog Converter driver (based on DACC module) for Atmel
SAM MCU family. Only SAME70, SAMV71 series devices are supported in
this version.
In case of remaining SoC series (SAM3X, SAM4E, SAM4L, SAM4S) the DACC module has one important difference, namely the layout of the Mode Register (DACC_MR) is not compatible. Probably the best course of action would be to extend the driver in the future to support the whole family using conditional compilation. For now only SAME70, SAMV71 series is supported.
Tested on Atmel SMART SAM E70 Xplained board.