Skip to content

Conversation

@Snap-A
Copy link
Contributor

@Snap-A Snap-A commented May 29, 2025

Summary

This PR submits the device driver code for the Texas Instrument chip family DAC N311, with N indicating the resolution (see below).
These chips provide a single output channel and support "power mode" selection by reserving the two MSB values for this.
There is a single "register" that is updated with each 16-bit write to the chip via SPI.

Code Changes

The driver was code was modeled after DAC drivers for other chip families, where different resolutions are supported.

  • The new Kconfig.tx311 enables the driver when a matching entry is added to the device tree;
  • The new driver (dac_tx311.c) processes the device tree to set the correct resolution value;
    Chip Resolution
    5311 8 bits
    6311 10 bits
    7311 12 bits
    8311 14 bits
  • The related YAML files use a "base" file to share the configuration "schema".
  • The configuration adds the property power-down-mode to set the power mode bits.

Tests

Normal Mode

This driver was developed to control the TI 6311 DAC chip that is part of an external PCB. The SPI bus was connected to a Raspberry Pico W and a sample program was created to generate a "saw tooth" signal with a period of a few seconds and the power mode set to "normal".

A oscilloscope was connected to the Vout pin of the chip to confirm the full voltage range, 0 to 3.3 V, is produced.

DS1Z_DAC_Output

Resistor to GND Mode

The power mode was set to "power-down-100k" and the test program was recompiled. The DAC output shows no saw-tooth signal.

@sonarqubecloud
Copy link

@Snap-A Snap-A marked this pull request as ready for review May 29, 2025 17:16
@github-actions github-actions bot added area: DAC Digital-to-Analog Converter platform: TI SimpleLink Texas Instruments SimpleLink MCU labels May 29, 2025
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jul 29, 2025
Copy link
Member

@martinjaeger martinjaeger left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. I missed this PR. In the future, just feel free to ping the maintainer if you don't get any feedback after a few weeks.

See my comments below.

Comment on lines 93 to 102
if ((config->resolution > DACX311_MAX_RESOLUTION) ||
(config->resolution < DACX311_MIN_RESOLUTION)) {
LOG_ERR("Unsupported resolution %d", config->resolution);
return -ENOTSUP;
}

if (config->power_down_mode > 3) {
LOG_ERR("Unsupported power mode %d", config->power_down_mode);
return -ENOTSUP;
}
Copy link
Member

Choose a reason for hiding this comment

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

This could already be checked at build-time with BUILD_ASSERT macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adjusted the code to use two BUILD_ASSERT macros. The check on the resolution value was removed, since it is "hard-coded" for each variant (see end of file).

Comment on lines 88 to 91
if (1000 * config->max_freq_khz < config->bus.config.frequency) {
LOG_ERR("SCLK frequency too high: '%d' Hz", config->bus.config.frequency);
return -ENOTSUP;
}
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can't check this at build-time. But can we put it into a driver init function, so that it's already checked before entering main?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an init function, but this check became a BUILD_ASSERT macro since it represents a chip specific max frequency that is not changing.

Comment on lines 112 to 117
/* Set bit resolution for this chip variant */
data->resolution = config->resolution;

/* Set the power mode (shifted to upper bits) */
data->power_down_mode = FIELD_PREP(BIT_MASK(2) << DACX311_MAX_RESOLUTION,
config->power_down_mode);
Copy link
Member

Choose a reason for hiding this comment

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

This should also go into a driver init function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, and changed the code.

data->configured |= BIT(channel_cfg->channel_id);

/* Set bit resolution for this chip variant */
data->resolution = config->resolution;
Copy link
Member

Choose a reason for hiding this comment

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

Should this not be like this?

Suggested change
data->resolution = config->resolution;
data->resolution = channel_cfg->resolution;

And in this case, we should also check if the desired resolution is supported by the chip.


struct dacx311_config {
struct spi_dt_spec bus;
uint8_t resolution;
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is actually the max resolution supported by the chip, but we can configure it for lower resolution, right? Should we then name it max_resolution to make the meaning more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each chip variant has a fixed resolution. The entry in this struct is present in order to use the same code logic for the different chip variants. This value is different for each variant.

@martinjaeger
Copy link
Member

Please also add the driver to tests/drivers/build_all/dac/app.overlay to make sure it's built in CI.

@Snap-A
Copy link
Contributor Author

Snap-A commented Aug 1, 2025

@martinjaeger : Thanks for the review! I will first resolve the merge conflict and then work on your comments.

@Snap-A Snap-A force-pushed the add_ti_x311_driver branch from a048710 to cb4da7e Compare August 1, 2025 15:11
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 1, 2025

@sonarqubecloud
Copy link

@kartben kartben requested a review from martinjaeger October 17, 2025 19:56
Copy link
Member

@martinjaeger martinjaeger left a comment

Choose a reason for hiding this comment

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

My previous comments were not addressed yet.

@Snap-A
Copy link
Contributor Author

Snap-A commented Nov 16, 2025

Did not have time to work in this over the last months. Hopefully, I can in the next weeks.

@Snap-A Snap-A force-pushed the add_ti_x311_driver branch from 1bb6885 to fe4d7b1 Compare November 22, 2025 16:37
@Snap-A
Copy link
Contributor Author

Snap-A commented Nov 22, 2025

Please also add the driver to tests/drivers/build_all/dac/app.overlay to make sure it's built in CI.

Working on this, next.

@Snap-A Snap-A force-pushed the add_ti_x311_driver branch from fe4d7b1 to c25cc8c Compare December 6, 2025 21:52
@zephyrbot zephyrbot added the area: Tests Issues related to a particular existing or missing test label Dec 6, 2025
@zephyrbot zephyrbot requested a review from jpanisbl December 6, 2025 21:54
@zephyrbot zephyrbot requested a review from nashif December 6, 2025 21:54
Add shared code in 'dac_tx311.c' and configuration files.
Support power mode bits via configuration.

Signed-off-by: Andreas Wolf <[email protected]>
@Snap-A Snap-A force-pushed the add_ti_x311_driver branch from c25cc8c to dc0dc90 Compare December 7, 2025 00:12
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Boards/SoCs area: DAC Digital-to-Analog Converter area: Devicetree Bindings area: Tests Issues related to a particular existing or missing test platform: TI SimpleLink Texas Instruments SimpleLink MCU

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants