Max22007 dev

[jansunil]

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

[gastmaier]

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]

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

[nunojsa]

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]

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

[machschmitt]

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.

[machschmitt]

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.

[jansunil]

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?

[machschmitt]

It's not about hardcoding, it's about hardware description. Devicetree is useful because it follows a specification that makes it easier to describe hardware such that the description is both readable to humans and compilable by machines. Some info clearly relates to the hardware, e.g. pins that have device-specific functionality, specific types of inputs/outputs to/from device pins (clocks, voltage/current supplies, GPIOs, ...), non-conventional hardware connections (e.g. controller CS connected to peripheral SDI), characteristics intrinsic to a design (e.g. max transfer clock frequency, chip address, etc.). Other info might not need to be encoded into the hardware description (note: in some platforms, the only way to update the hardware device tree is with a system reboot).
IMHO, the ideal for MAX22007 CRC would be to have some sort of CRC toggle device property. That way, the user would be able to enable/disable CRC at runtime, and there would be no need to have a CRC property in dt. Though I didn't find any documented ABI for that so it would have to be proposed. An alternative might be to always have CRC checking enabled on initial MAX22007 support.

That said, there is indeed some precedent for CRC dt properties.
regulator/nxp,pf0900.yaml (nxp,i2c-crc-enable)
net/adi,adin1110.yaml (adi,spi-crc)
mtd/qcom,nandc.yaml (qcom,cmd-crci and qcom,data-crci) (looks like CRC but not really sure)
qcom/qcom,gsbi.yaml (qcom,crci) (not sure it's about CRC either)
If you are confident that you do need a CRC dt prop for MAX22007, then please use adi,spi-crc (so we may at least try to keep only a few property names).

[nunojsa]

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.

[gastmaier]

Adding here to also note that to avoid the merge conflicts due to the mirror_ci head, base your work on the common stable commit, in this case

Linux 6.19-rc1 8f0b4cc

[gastmaier]

Suggested change

	#define MAX22007_CH_PWRON_CH_MASK(ch) BIT(8 + (ch))
	#define MAX22007_DAC_LATCH_MODE_MASK(ch) BIT(12 + (ch))
	#define MAX22007_LDAC_UPDATE_MASK(ch) BIT(12 + (ch))
	#define MAX22007_CH_PWRON_CH_MASK(ch) BIT(8 + (ch))
	#define MAX22007_DAC_LATCH_MODE_MASK(ch) BIT(12 + (ch))
	#define MAX22007_LDAC_UPDATE_MASK(ch) BIT(12 + (ch))

note: github gui sets tabsize to 4 instead of the 8, MAX22007_DAC_LATCH_MODE_MASK is missing a tab

[gastmaier]

Some parts of the code are using

struct max22007_state *state

instead of st, use always the same name, in my preference, st

[gastmaier]

The values are defined at

#include <dt-bindings/iio/addac/adi,ad74413r.h>

as mentioned in the dt-binding
I would use the defines from the header, and drop the magic numbers 1 and 2, as well as the translation to mode (store CH_FUNC_VOLTAGE_OUTPUT, CH_FUNC_CURRENT_OUTPUT directly) attention to the 1 value increment.

Suggested change

	if (ch_func == 1) {
	mode = MAX22007_VOLTAGE_MODE;
	chan_type = IIO_VOLTAGE;
	} else if (ch_func == 2) {
	mode = MAX22007_CURRENT_MODE;
	chan_type = IIO_CURRENT;
	} else {
	return dev_err_probe(dev, -EINVAL,
	"invalid adi,ch-func %u for %pfwP\n",
	ch_func, child);
	}
	switch (ch_func) {
	case CH_FUNC_VOLTAGE_OUTPUT:
	chan_type = IIO_VOLTAGE;
	break;
	case CH_FUNC_CURRENT_OUTPUT:
	chan_type = IIO_CURRENT;
	break;
	default:
	return dev_err_probe(dev, -EINVAL,
	"invalid adi,ch-func %u for %pfwP\n",
	ch_func, child);
	}

the add the

regmap_update_bits(st->regmap, MAX22007_CHANNEL_MODE_REG

you could just subtract 1

		ret = regmap_update_bits(st->regmap, MAX22007_CHANNEL_MODE_REG,
					 MAX22007_CH_MODE_CH_MASK(reg),
					 MAX22007_CH_MODE_VAL(reg, ch_func - 1));

[gastmaier]

dt-binding uses

#include <dt-bindings/iio/addac/adi,ad74413r.h>

to define similar value (1-incremented)

[gastmaier]

Suggested change

	#define MAX22007_CH_PWR_VAL(ch, val) (((val) & 0x1) << (8 + (ch)))
	#define MAX22007_CH_MODE_VAL(ch, val) (((val) & 0x1) << (12 + (ch)))
	#define MAX22007_DAC_LATCH_MODE_VAL(ch, val) (((val) & 0x1) << (12 + (ch)))
	#define MAX22007_CH_PWR_VAL(ch, val) (((val) & 0x1) << (8 + (ch)))
	#define MAX22007_CH_MODE_VAL(ch, val) (((val) & 0x1) << (12 + (ch)))
	#define MAX22007_DAC_LATCH_MODE_VAL(ch, val) (((val) & 0x1) << (12 + (ch)))

also just indentation

[gastmaier]

As jonathan suggested

I think you can chain CRCs as follows and avoid the need for a local array
just to marshal the data.

	calculated_crc = crc8(max22007_crc8_table, &reg_byte, 1, 0x00);
	calculated_crc = crc8(max22007_crc8_table, rx_buf, 2, calculated_crc);

then you can remove the intermediary variable crc_data
The last value is

lib/crc/crc8.c 
 * @crc: previous returned crc8 value.

[gastmaier]

can be simplified into a single statement, as jonathan suggested

Something like the following reduces duplication:

	ret = regmap_update_bits(st->regmap, MAX22007_CHANNEL_MODE_REG,
				 MAX2207_CH_PWRON_CH_MASK(chan->channel),
				 MAX2207_CH_PWR_VAL(chan->channel, powerdown ? 1 : 0);

[gastmaier]

Reorder for inverted tree

-----------
-------
---