Skip to content

Conversation

@jansunil
Copy link
Collaborator

@jansunil jansunil commented Nov 7, 2025

PR Description

  • Please replace this comment with a summary of your changes, and add any context
    necessary to understand them. List any dependencies required for this change.
  • To check the checkboxes below, insert a 'x' between square brackets (without
    any space), or simply check them after publishing the PR.
  • If you changes include a breaking change, please specify dependent PRs in the
    description and try to push all related PRs simultaneously.

PR Type

  • Bug fix (a change that fixes an issue)
  • New feature (a change that adds new functionality)
  • Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

  • I have conducted a self-review of my own code changes
  • I have compiled my changes, including the documentation
  • I have tested the changes on the relevant hardware
  • I have updated the documentation outside this repo accordingly
  • I have provided links for the relevant upstream lore

Copy link
Contributor

@gastmaier gastmaier left a comment

Choose a reason for hiding this comment

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

Hi @jansunil overall looks good, just some comments from my side.
I believe we can drop the crc feature and really streamline the driver reg access as a consequence.

Best regards,

@nunojsa
Copy link
Collaborator

nunojsa commented Nov 12, 2025

I'll wait this getting out of draft to jump in :)

Copy link
Collaborator

@nunojsa nunojsa left a comment

Choose a reason for hiding this comment

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

Here it goes my first round. Couple more points:

  • In the bindings patch do not day "...Add MAX22007 DAC bindings". I mean, dont mention "bindings". Its already obvious from the prefix. So instead say something like "Document MAX22007 DAC". Also refactor a bit the message and add a small description of the device.
  • Also refactor the driver patch commit message. See git log for other examples. Give a small description of the device. It is also a fairly simple driver so I do not think there is any meaningful to state about the implementation in the commit message.

Regarding process, note that I expect this PR to be opened against main (without the overlay). Then after merged with main, you can open a PR against the PI branch only with the overlay.

@jansunil jansunil force-pushed the max22007-dev branch 2 times, most recently from ae84b9d to bb72180 Compare November 14, 2025 10:42
@jansunil
Copy link
Collaborator Author

Changelog V2:

    1) Remove description for spi max frequency in the yaml
2) Remove "|" character in the yaml
3) Add a default field in the properties wherever required
4) Add regmap in the Kconfig
5) Correct copyright year in the driver
6) Rearrange includes in alphabetical order and add  missing includes
7) Pass SPI buffers into state structure
8) Reformat code to meet 80 column limit
9) Remove redundant masking of buffer elements in max22007_spi_reg_write()
10) Remove locking within APIs
11) Remove explicit function for softreset and use regmap directly
12) Add macros for MAX value check for DAC Raw data
13) Implement custom regmap bus
14) Removed caching of channel configuration as it is not used again
15) Add validation for values passed from devicetree
16) Remove spi_setup
17) Add extra spaces at necessary lines of code
18) Parse CRC value from devicetree inside the max22007_configure_crc()
19) Return error code from max22007_parse_channel_cfg() and add an argument to capture the number of channels
20) Add RESET pin gpio support
21) Switch the CRC functionality to use adi,crc-disable instead of adi,crc-enable
22) Remove CRC from the Tx buffer during SPI read operations
    23) Add ABI documentation for per channel LDAC update

Copy link
Contributor

@machschmitt machschmitt left a comment

Choose a reason for hiding this comment

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

Hi @jansunil ,

Commenting mostly about the dt docs and proposed ABI. Only superficially looked into the driver code as that might change significantly depending on the discussion about the dt properties and IIO ABI.

Comment on lines +48 to +52
adi,crc-disable:
type: boolean
description:
Disable CRC8 error checking for SPI communications. By default, CRC8 is
enabled for data integrity verification. Set this property to disable it.
Copy link
Contributor

Choose a reason for hiding this comment

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

I see in the data sheet that SPI transfers can run with CRC disabled (set CRC_EN field to 0x0 within the configuration register (0x03) ). For max22007, CRC is a runtime configuration and has nothing to do with how hardware is set up. Drop this property.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The property here was added so that it enables the user to take a decision on the state of the CRC and not hardcoding it in the driver. Do you still suggest removing the CRC property and hardcoding it in the driver?

Copy link
Collaborator

@nunojsa nunojsa left a comment

Choose a reason for hiding this comment

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

Some more comments from me. An additional note regarding commits. Your git subject for the bindings is nok. Please run git log --oneline Documentation/devicetree/bindings/ and follow the sytle.

@jansunil jansunil force-pushed the max22007-dev branch 3 times, most recently from b39e6d8 to 7d8c82d Compare December 10, 2025 12:50
Devicetree bindings for MAX22007 4-channel
12-bit DAC that drives a voltage or current
output on each channel

Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Add support for the MAX22007 4 channel DAC
that drives a voltage or current output on each channel.

Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Add documentation for MAX22007 driver which describes how the user
can access the driver using dtoverlays

Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants