DACx3608 Driver Feature Support

[bbooher]

Is your enhancement proposal related to a problem? Please describe.
The broadcast channel and low-power configuration features are not supported.

Describe the solution you'd like
Add support to send DAC write values to the Broadcast Register (channel 9?). Also, modify the channel setup implementation with the ability to return channels to power-down mode after initialization.

Describe alternatives you've considered
Custom out-of-driver I2C control is the only alternative.

Additional context
Please see the description of the register map (pg 30): https://datasheet.datasheetarchive.com/originals/distributors/Datasheets_SAMA/31e6133a56d84334c2130e2ecbea856a.pdf

Possible Future Enhancements
There are also missing line controls for DAC channel synchronization which should be tied to the driver, but the existing DAC interface doesn't account for something like that. The DAC interface, in general, should be brought in line with other peripherals like ADC.

[github-actions]

Hi @bbooher! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

[martinjaeger]

Possible Future Enhancements There are also missing line controls for DAC channel synchronization which should be tied to the driver, but the existing DAC interface doesn't account for something like that. The DAC interface, in general, should be brought in line with other peripherals like ADC.

@bbooher I agree that the current DAC API is still more simple than the one for e.g. the ADC and we should think about a way to allow for channel synchronization (see also #74340). Do you already have an idea in mind how an API update for the channel sync could look like?

[bbooher]

Some thoughts on the DAC API:

• It may be useful to update the DAC channel config to include min_value / max_value output settings.
• devicetree support for DAC channel configuration (like ADC is done). This should be for individual channels.

For synchronization:

• For latch pins, I think an optional GPIO entry (phandle-array?) in the devicetree for a "latch" pin (latch-gpios) would be the way to go. (Some devices also have a reset line and/or line to clear the output - it may be helpful to add devicetree entries for those, too, but I think this is typically handled externally as too many options could clutter the definition here).

One idea is to break up the write_value API call into 2 separate functions: 1 which updates the DAC value to output and the other physically applies the output value. This is similar to the sensors ..fetch() and ...get() API (maybe ...update() and ...write() for DAC?). In this case, the update may cause an immediate output (acting like a write) if no latch pin is configured. The broadcast register support in my PR would still need to be maintained because one could write once to the broadcast register to update all of the configured channels' intended output, but still use the latch pin to synchronize the physical output. I'd have to think a bit more about backward-compatibility implications, though.

[emz229]

@bbooher I like the idea of splitting the ...update() and ...write() functions. Maybe we should enforce that the buffered flag is true for a specific channel in order for the update function to return success? I have a link to a proposed dac.h commit which would not break existing drivers but just add the update functionality. Im not sure what other changes could / should be made, please let me know your thoughts!

emz229@20f7931

[bbooher]

Maybe we should enforce that the buffered flag is true for a specific channel in order for the update function to return success?

I believe the buffered flag is a hardware configuration setting to set output impedance and timing - not for synchronization purposes (a hardware buffer, not a software buffer). See https://forum.digikey.com/t/dacs-output-buffered-and-unbuffered/1565

I think the buffered flag is a bit odd in the API since it's hardware-specific and parts generally don't support both options.

I have a link to a proposed dac.h commit which would not break existing drivers but just add the update functionality. Im not sure what other changes could / should be made, please let me know your thoughts!

This looks similar to my proposed idea - though I prefer swapping the intended use of write and update (better for backward compatibility, too). I'll use write and update in my preferred usage in the following.

My concern with the approach is that if a latch pin is not configured, then we have different output behavior of the API. With no latch pin configured, update() will result in DAC physical output. With a latch pin configured, write() will result in DAC physical output. I think there should be some care here so that the output signal always results from a call to write(), regardless of latch configuration. In other words, the application code should always call both update() and write(). I'm not sure how to enforce this concept; I guess the API would require helper functions: set_latch() and write_value().

Some pseudocode for this thought:

update(val):
  self.write_val = val;
  if latch configured:    // Note: Without this check, the following results in an immediate write
    self.api.write_value(self.write_val)

write(): // Note: Intentionally no value parameter, so that update() is necessary even with no latch
  if latch configured:
    self.api.set_latch()
  else:
    self.api.write_value(self.write_val)

[bbooher]

The additional consideration missing from my previous comment is multiple-channel storage. update(val) should have a channel input parameter and write_val should be cached for the channel. Use cases look like:

// Singular-channel update and write
for (int i = 0; i < channels; i++) {
  update(i*10, i);
  write();            // Iterates through all updated channels and writes them; in this case singular
  k_sleep(1);
}

// Multiple-channel update, then write
for (int i = 0; i < channels; i++) {
  update(i*10, i);
}
write();             // Iterates through all updated channels and writes them; in this case multiple

[emz229]

@bbooher @martinjaeger I have made a few commits and marked them WIP for this new ..update() feature. I welcome any changes and suggestions you may have being these are only meant to be experimental for now. @bbooher After reading your link above to digikey, the buffered flag does seem out of place.. really why should zephyr care at the dac api level what the chip has for a final stage? My WIP changes are here, to not interfere with the PR I currently have pending:

https://github.com/emz229/zephyr/tree/dac-api-feature

[bbooher]

I can't seem to make comments on a branch alone. I'll make a few observations here:

• look at my PR: drivers: dac: dacx3608: add broadcast register for synchronized output #74633 for broadcast register support. It's simple, but I'd like this functionality to be maintained. Additional changes to what is suggested below may be necessary.
• I think dac_write_value() should effectively always be "write all" - and the flag isn't needed.
  • this means a data structure based on the number of configured channels in the DTS (which doesn't exist today) needs to be maintained; see [1].
• please see the ADC controller yaml file as an example for adding channel support. I believe this is similar to what we want for DAC, too. Link
  • latch-gpios should exist in the dac-controller.yaml file
• My suggestions would break backward compatibility without renaming / deprecating dac_write_value(), unfortunately. New terminology may be needed.

[1] Proposed structure

struct dac_channel_write_value {
  bool is_updated;
  uint8_t channel_number;
  uint16_t value;
};

This structure would be created in the driver based on the number of channels configured in DTS. The appropriate "macrobatics" need to be implemented to iterate and initialize this data.

struct dac_channel_write_value ch_val[DTS_NUM_DAC_CHANNELS] = {
  {
    is_updated = false,
    channel_number = ITERATE_DTS_DAC_CHANNELS,
    value = 0
  },
  // ...
};

Then the dac_write_value() function looks something like (pseudocode):

// Note, no value or channel number anymore
dac_write_value(const struct device *dev) {
  for (int i = 0; i < ARRAY_SIZE(ch_val); i++) {
    if (ch_val[i].is_updated) {
      api.write_val(ch_val[i].value);
      ch_val[i].is_updated = false;
    }
  }
}

---

I understand this is a lot of code, lol, for not actually doing it. But I'd like more input before digging into things (and @emz229 seems to be making some strides in that direction already). @martinjaeger?

[martinjaeger]

Thanks! Already late here. Will comment tomorrow.

[martinjaeger]

Sorry @emz229 and @bbooher that it took me so long to react.

I've looked at different DAC implementations (both on-chip and external via I2C/SPI) regarding synchronized write mechanisms.

I think we should keep the current (simple) dac_write_value function, that writes one value immediately, as this is probably the mostly used function in on-board DACs.

For implementing the synchronized writes of many DACs in parallel (as mostly found for multi-channel external DACs) we can extend the API.

The term update makes sense to me, as it is also used by several manufacturers. However, I would suggest to call the other function trigger (more generic than latch, as this could also be done via software for integrated DAC peripherals, and more clear than write to avoid confusion with previous API).

What about something like this (with t.b.d. struct dac_sequence, similar to the ADC)?

/**
 * @brief Update a sequence of DAC channel values for synchronous triggering
 *
 * This function will only update the registers, but not actually start the conversion.
 *
 * @param dev         Pointer to the device structure for the driver instance.
 * @param seq         Pointer to the sequence of channel values.
 *
 * @retval 0        On success.
 * @retval -ENOTSUP If synchronized output is not supported by the hardware.
 * @retval -EINVAL  If a parameter with an invalid value has been provided.
 */
int dac_update_values(const struct device *dev, struct dac_sequence *seq);

/**
 * @brief Trigger DAC output based on previously updated values
 *
 * This function triggers a DAC to output of the values set using dac_update_values.
 *
 * @param dev         Pointer to the device structure for the driver instance.
 * @param channels    Flags to select which channels to trigger. For DACs with a single latch pin,
 *                    all available channels must be set.
 *
 * @retval 0        On success.
 * @retval -EINVAL  If a parameter with an invalid value has been provided.
 */
int dac_trigger(const struct device *dev, uint32_t channels);

P.S.: The current buffered flag refers to hardware (op-amp) buffering and not double-buffered writes. And yes, it does make sense to have this in the DAC API, as chips like the STM32G474 allow enabling or disabling an internal hardware buffer via software.

[bbooher]

No problem! Release time is probably pretty hectic...

I'm OK with extending the API and the struct dac_sequence is a better name than the struct dac_channel_write_value I proposed above which would serve a similar purpose. Tying into my PR which has support for a broadcast register - that's taken care of with a 1-element sequence (still need to support a unique broadcast register value, as I did with 0xFF in the PR).

The concern I have with this approach is the same as @emz229's initial take. If a trigger mechanism isn't configured or supported, then dac_update_values will behave exactly as dac_write_value - which may be confusing (at least not matching the description in the comments you posed above). With this extension, the dac_write_value function isn't needed - one could exclusively use dac_update_values instead of the simpler write function. If we can ensure dac_update_values doesn't devolve to dac_write_value when no trigger mechanism is set up (i.e. - no DAC output), then I'm more inclined to extend the API as you suggested.

With my suggestion:

• dac_update_value(const struct device *dev, struct dac_sequence *seq) (which includes a check for trigger configuration)
• dac_write_value(const struct device *dev) (which includes trigger-handling logic, if configured)

dac_write_value should always result in a DAC output, regardless of a configured trigger mechanism. And it's the only API mechanism to drive a DAC output.

This is likely more difficult to enforce as drivers are added. I'll concede that I don't have an issue if others don't share this concern.

===========================================================
P.P.S.: I still think it odd that it's missing from the devicetree definition along with the other members of struct dac_channel_cfg (as their counterparts are included in ADC devicetree definition). It's likely that these settings don't change at runtime. It's good to keep it in the API to have flexibility, but I'd guess that it's a rare use case and setting it once at initialization is adequate...but I'm not an analog guy and this is a different discussion.

[martinjaeger]

The concern I have with this approach is the same as @emz229's initial take. If a trigger mechanism isn't configured or supported, then dac_update_values will behave exactly as dac_write_value - which may be confusing (at least not matching the description in the comments you posed above). With this extension, the dac_write_value function isn't needed - one could exclusively use dac_update_values instead of the simpler write function. If we can ensure dac_update_values doesn't devolve to dac_write_value when no trigger mechanism is set up (i.e. - no DAC output), then I'm more inclined to extend the API as you suggested.

I would suggest that dac_update_value returns an error if synchronous output with a trigger is not supported by the hardware or the driver. That would be similar to other drivers, where if e.g. DMA is not supported you just use the more simple API functions.

We can also present both options to the Arch WG and see what other people think. I'm on vacation for the next 2-3 weeks, though, so we'd need to schedule this for end of August or beginning of Sept.

[bbooher]

Thanks for getting back. Returning an error sounds like the right thing to do. But since only 2-3 of us are looking at it, getting more eyes on the proposed API changes from the Arch WG is probably better.

I'm also busy, and your time frame matches my availability nicely.

[bbooher]

@martinjaeger @emz229 Bumping this discussion