Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for AD8460 #2458

Merged
merged 6 commits into from
Oct 30, 2024
Merged

Conversation

MarielTinaco
Copy link
Contributor

This adds device tree bindings compatible strings, driver support and ZedBoard device trees for AD8460 High Voltage, High Current Arbitrary Waveform Generator DAC.

For more information:
Arbitrary Waveform Generator with Integrated 14-Bit High Speed DAC

This has been tested on the ZedBoard which controls the EVAL-AD8460SDZ evaluation board

@MarielTinaco MarielTinaco force-pushed the dev/ad8460 branch 4 times, most recently from 0c03ef0 to 5110eb7 Compare March 18, 2024 11:53
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 initial feedback... Also make sure to fix compilation for arm64

Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml Outdated Show resolved Hide resolved
Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml Outdated Show resolved Hide resolved
Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml Outdated Show resolved Hide resolved
drivers/iio/dac/ad8460.c Show resolved Hide resolved
drivers/iio/dac/ad8460.c Show resolved Hide resolved
drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
@MarielTinaco
Copy link
Contributor Author

v2

  1. Moved some attributes to debugfs
  2. Applied cleanup magic
  3. Provided missing includes

@nunojsa
Copy link
Collaborator

nunojsa commented Apr 8, 2024

@MarielTinaco, sorry for the delay. I did replied to some your questions... Make sure to fix the build errors CI is reporting and push another version of the PULL.

Hint for the future:
If you push directly to ADI git tree and name your branch as staging/..., basic CI will run on your dev branch so you can catch build errors like this before opening (or force push) the PULL . If you do not have access to push into ADI tree, we can have a look.

@MarielTinaco
Copy link
Contributor Author

Hi @nunojsa unfortunately I do not have access to push into the ADI tree. But it will surely come in handy in the future. Thanks!

@MarielTinaco MarielTinaco force-pushed the dev/ad8460 branch 2 times, most recently from dd308cf to 577e6a0 Compare April 9, 2024 01:35
@MarielTinaco
Copy link
Contributor Author

V3

  • Fixed CI build errors

@MarielTinaco
Copy link
Contributor Author

V4

  1. Added buffer capability

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.

Main thing we should see now is that limits stuff. I'm not sure but maybe somehow using the hwmon subsystem would make sense. Like that we could actually control those limits over userspace. But for that, I think we would need to be capable to also read the "thing" we're putting limits on...


clock-names:
items:
- const: sync_clk
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we just have one clk, we don't really need clock-names...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I remove the usage of clock-names? how does this affect the way I call sync_clk from the driver?

in the driver code, this is the current implementation

	sync_clk = devm_clk_get_enabled(&spi->dev, "sync_clk");
	if (IS_ERR(sync_clk))
		return dev_err_probe(&spi->dev, PTR_ERR(sync_clk),
				     "Failed to get sync clk\n");

drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
drivers/iio/dac/ad8460.c Show resolved Hide resolved
drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
drivers/iio/dac/ad8460.c Show resolved Hide resolved
if (ret)
return ret;

ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), &mode);
Copy link
Collaborator

Choose a reason for hiding this comment

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

again... is a read() that bad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I don't think this should interfere with the buffer capture. I should just remove the device claim/release, is that right?

	struct iio_dev *indio_dev = arg;
	struct ad8460_state *state = iio_priv(indio_dev);
	u32 reg;
	int ret;

	ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), &reg);
	if (ret)
		return ret;

	*val = FIELD_GET(AD8460_PATTERN_DEPTH_MSK, reg);

	return 0;

Copy link
Collaborator

Choose a reason for hiding this comment

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

right, I don't think this should interfere with the buffer capture.

With the above in mind, I think so, yes...

drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
int *val,
int *val2,
long mask)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we only get samples while buffering? Or can we just read a single sample (using IIO_CHAN_INFO_RAW)?

Copy link
Collaborator

@nunojsa nunojsa Apr 12, 2024

Choose a reason for hiding this comment

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

Ups, forget this. This is a DAC... And from a quick look on DS, it looks like we don't have any register to control the word to send out (just using that pattern memory stuff)

Pattern depth and pattern memory are configurable settings for the Arbitrary Pattern Generator (APG) mode. Pattern memory is an array like property that is found on the SPI registers.

But taking your words, I'm now thinking if it would make sense to support IIO_CHAN_INFO_RAW where pattern depth is pretty much 1 and we just output a DC voltage. Would that work or does it make sense to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does make sense to support IIO_CHAN_INFO_RAW this way.

I'll include this in the next revision

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, this device has some funny stuff :)

if (ret)
return ret;

ret = ad8460_enable_apg_mode(state, val);
Copy link
Contributor

Choose a reason for hiding this comment

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

iio_device_release_direct_mode(indio_dev) here?
otherwise we leave it claimed on error;

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 take it that I should bump the iio_device_release_direct_mode(indio_dev) up before return like here?

	ret = ad8460_enable_apg_mode(state, val);
	iio_device_release_direct_mode(indio_dev);

	return ret;

Copy link
Contributor

Choose a reason for hiding this comment

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

yep

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, when upstreaming use the new iio_device_claim_direct_scoped(). Then, no need to care about releasing it :)

static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
ad8460_read_powerdown, ad8460_write_powerdown),
{},
Copy link
Contributor

Choose a reason for hiding this comment

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

general nitpick (upstream will say): null terminators, don't need a trailing comma


return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
*val = 80 * (2000 / state->rset_ohms) * (state->vref_mv / 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if it's better to rewrite this formula to prefer multiplications first and divisions later;
integer division tends to lose some precision;
then there can be a comment about the original formula, or reference it to the datasheet

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 refactored it this way

	case IIO_CHAN_INFO_SCALE:
		/* vCONVENTIONAL = 80V * (DAC_CODE / 2**14) - 40V
		 * vMAX = 80V * (2**14 / 2**14) - 40V
		 * vMIN = 80V * (0 / 2**14) - 40V
		 * vADJ = vCONVENTIONAL * (2000 / rSET) * (vREF / 1.2)
		 * vSPAN = vADJ_MAX - vADJ_MIN
		 * See datasheet page 49, section FULL-SCALE REDUCTION
		 */
		num = 80 * 2000 * state->vref_mv;
		denom = state->rset_ohms * 1200;
		*val = DIV_ROUND_CLOSEST_ULL(num, denom);

		return IIO_VAL_INT;

@MarielTinaco
Copy link
Contributor Author

V5
- remove clock-names
- refactor debugfs init
- handling of GPIO soft reset
- add raw value as channel attribute
- refactor scale calculation

- held off pm_runtime functions due to boot errors
- retained setting of alarm limits and arming device

adi,temp-lim-millicelsius:
description: Specify the overtemperature limits
minimum: 20000
maximum: 150000
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems to me that the above properties will likely not be needed. But if you stick with them, you should set a default: value for them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this attribute

if (ret)
return ret;

usleep_range(10, 50);
Copy link
Collaborator

Choose a reason for hiding this comment

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

fsleep()

drivers/iio/dac/ad8460.c Show resolved Hide resolved

reg = FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0);
return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x02),
AD8460_PATTERN_DEPTH_MSK, reg);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to review your locking... You're protecting ad8460_enable_apg_mode() internally and then ad8460_set_hvdac_byte() while the following regmap_update_bits() has no protection (apart from internal regmap lock that won't matter in this case). It seems to me that the complete sequence should be protected.

Copy link
Contributor Author

@MarielTinaco MarielTinaco Apr 24, 2024

Choose a reason for hiding this comment

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

I see, it seems like a should put locking on the higher level functions or those functions that wrap the other function calls and remove it from the lower level functions

int ret;

switch (mask) {
case IIO_CHAN_INFO_RAW:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also seems to me this should be protected with iio_device_claim_direct_scoped(). If you're buffering, you should not be able to do a write_raw(). Otherwise, buffering will break

Copy link
Contributor Author

@MarielTinaco MarielTinaco Apr 24, 2024

Choose a reason for hiding this comment

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

Will I be able to use iio_device_claim_direct_scoped() right now? is it supported in this repo yet? Or did you mean that I must have it during upstream and use iio_device_claim_direct_mode/iio_device_release_direct_mode for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, this is what write raw looks like using the IIO device claim/release

	struct ad8460_state *state = iio_priv(indio_dev);
	unsigned int reg;
	int ret;

	switch (mask) {
	case IIO_CHAN_INFO_RAW:
		ret = iio_device_claim_direct_mode(indio_dev);
		if (ret)
			return ret;

		guard(mutex)(&state->lock);

		ret = ad8460_enable_apg_mode(state, 1);
		if (ret)
			return ret;

		ret = ad8460_set_hvdac_byte(state, 0, val);
		if (ret)
			return ret;

		reg = FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0);
		ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x02),
					 AD8460_PATTERN_DEPTH_MSK, reg);
		iio_device_release_direct_mode(indio_dev);

		return ret;
	default:
		return -EINVAL;
	}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will I be able to use iio_device_claim_direct_scoped() right now? is it supported in this repo yet? Or did you mean that I must have it during upstream and use iio_device_claim_direct_mode/iio_device_release_direct_mode for now?

I think it's only upstream for now...

drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
}
DEFINE_DEBUGFS_ATTRIBUTE(ad8460_apg_pattern_depth_fops,
ad8460_show_apg_pattern_depth,
ad8460_set_apg_pattern_depth, "%llu\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still want the debugfs interface now we have it in write raw? The only justification I see would be to have more words.

Copy link
Contributor Author

@MarielTinaco MarielTinaco Apr 24, 2024

Choose a reason for hiding this comment

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

I think the pattern_depth can remain on debugfs interface. If setting two integer values at a time, (pattern index and word) is available in write raw, I think pattern_memory can also be removed from debugfs. Maybe the two values of write_raw (val and val2) can be used for this? I'll remove the apg_mode_enable from debugfs now, since it is handled on write raw and pre-enable of buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another option is to be able selection of test patterns instead such as square, ramp, default etc. this can cover both pattern_depth and pattern_memory setting. what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm not sure what you mean about the last question? Could you clarify it a bit?

drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
@MarielTinaco
Copy link
Contributor Author

V6

  • Fixed guard locking
  • Used fsleep() for delays
  • Remove apg_mode_enable from debugfs
  • Removed Fault protection in default

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.

This already looks good enough to send a first version upstream. So you can take my last review and send it out :). Couple of things to note:

  • You need to improve your commit message on the patch adding support for the driver. Don't mention via axi. You need a small description of the device (like you have in the bindings patch);
  • Don't forget to mention the hwmon capabilities and how can we better support them (if still have the intuit of adding them)
  • And be prepared for some (maybe lots) changes to be asked. This is a very funny part so I expect the upstream review to be equally "funny" :)

drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved

state->sync_clk = sync_clk;

vrefio = devm_regulator_get_optional(&spi->dev, "vrefio");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this needs to be on the bindings...

}
DEFINE_DEBUGFS_ATTRIBUTE(ad8460_apg_pattern_depth_fops,
ad8460_show_apg_pattern_depth,
ad8460_set_apg_pattern_depth, "%llu\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm not sure what you mean about the last question? Could you clarify it a bit?

static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
ad8460_read_powerdown, ad8460_write_powerdown),
{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

note you may be asked to support powerdown-mode

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 see. Although, unlike other devices where the powerdown modes are explicitly stated as part of the SPI register, in this device, I don't think there is an exact equivalent, that comes close to the selections defined in the ABI for powerdown-modes.

Copy link
Collaborator

@nunojsa nunojsa May 3, 2024

Choose a reason for hiding this comment

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

I don't think you need to support all of them... Does the datasheet sates something about what happens when you power down. Again, no special need for this now (was just warning that you might get asked for it). You can always argue this upstream and see the reply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that you mention it, it does seem to have a powerdown state when HV sleep is initiated. Device OUT is terminated to 27kohms

image

"27kohms_to_gnd" doesn't fall under the available selections in the ABI,

image

are we allowed to create our own entry?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we're allowed to update the ABI file... You can propose a new entry and support it in the drier and see how it goes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha! But since there is only possible powerdown-mode, do I just create an empty setter/getter for it? like here

static const char * const ad8460_powerdown_modes[] = {
	"27kohm_to_gnd",
};

static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
				     const struct iio_chan_spec *chan)
{
	return 0;
}

static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev,
				     const struct iio_chan_spec *chan,
				     unsigned int type)
{
	return 0;
}

static const struct iio_enum ad8460_powerdown_mode_enum = {
	.items = ad8460_powerdown_modes,
	.num_items = ARRAY_SIZE(ad8460_powerdown_modes),
	.get = ad8460_get_powerdown_mode,
	.set = ad8460_set_powerdown_mode,
};

I found one such example. stm32-dac

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, but I don't think you're mode is 27kohm_to_gnd. In the datasheet pic you pasted nothing states any ground connection. It says the output goes into high-z (with an impedance of around 27kohm). And that's effectively the three_state mode already supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. You're right

unsigned int num, denom;
int data, ret;

guard(mutex)(&state->lock);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not moving this into the IIO_CHAN_INFO_RAW case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If clean up magic is moved directly inside the "raw" case, as seen below

	switch (mask) {
	case IIO_CHAN_INFO_RAW:
		guard(mutex)(&state->lock);

		ret = ad8460_get_hvdac_byte(state, 0, &data);
		if (ret)
			return ret;

		*val = data;
		return IIO_VAL_INT;

An error is thrown because there is no declaration before it. So what I did is placed the guard(mutex).. inside the get_hvdac_byte

Copy link
Collaborator

Choose a reason for hiding this comment

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

Use:

scoped_guard(mutex, &state->lock) {
    ret = ad8460_get_hvdac_byte(state, 0, &data);
    if (ret)
        return ret;
}
...

and it should work...

int ret;

switch (mask) {
case IIO_CHAN_INFO_RAW:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will I be able to use iio_device_claim_direct_scoped() right now? is it supported in this repo yet? Or did you mean that I must have it during upstream and use iio_device_claim_direct_mode/iio_device_release_direct_mode for now?

I think it's only upstream for now...

drivers/iio/dac/ad8460.c Show resolved Hide resolved
drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
drivers/iio/dac/ad8460.c Outdated Show resolved Hide resolved
@MarielTinaco MarielTinaco force-pushed the dev/ad8460 branch 3 times, most recently from 76d6727 to e995b43 Compare May 6, 2024 10:45
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.

Small notes from me... go ahead and send it upstream (I'll also be on vacations for two weeks so no need to wait.)

}
fsleep(100);

return ret;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think ret can be used without any initialization in case gpio is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored the reset function this way. To remove the dependency to ret

static int ad8460_reset(const struct ad8460_state *state)
{
	struct device *dev = &state->spi->dev;
	struct gpio_desc *gpio;

	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
	if (IS_ERR(gpio))
		return dev_err_probe(dev, PTR_ERR(gpio),
				     "Failed to get reset gpio");
	if (gpio) {
		fsleep(100);
		gpiod_set_value_cansleep(gpio, 1);
	} else {
		return regmap_write(state->regmap, AD8460_CTRL_REG(0x03), 1);
	}
	fsleep(100);

	return 0;
}

if (ret)
return ret;

*val = (FIELD_GET(AD8460_DATA_BYTE_HIGH_MSK, high) << 8) | low;
Copy link
Collaborator

Choose a reason for hiding this comment

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

double check it but I don't think the parenthesis...

char data[16];
int ret, val;

guard(mutex)(&state->lock);
Copy link
Collaborator

Choose a reason for hiding this comment

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

use scoped_guard() and use it in the only place needed... ad8460_set_hvdac_byte()

int ret, val;
char data[16];

guard(mutex)(&state->lock);
Copy link
Collaborator

Choose a reason for hiding this comment

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

scoped_guard()

{
struct ad8460_state *state = iio_priv(indio_dev);

guard(mutex)(&state->lock);
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICT, you don't need the mutex lock here. ad8460_enable_apg_mode() only runs against your write_raw() function and the call is protected with iio_device_claim_direct_mode() which used the same internal IIO mutex as the postdisable() and preenable() callbacks. So in this case, the internal IIO lock is fine to guarantee correctness.


ret = ad8460_enable_apg_mode(state, 1);
if (ret)
return ret;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should use the scoped version of iio_device_claim_direct_mode() when upstreaming. But note that in this code you have a deadlock as you're returning without iio_device_release_direct_mode()

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 ended up with this refactor

	int ret;

	switch (mask) {
	case IIO_CHAN_INFO_RAW:
		ret = iio_device_claim_direct_mode(indio_dev);
		if (ret)
			return ret;

		ret = ad8460_enable_apg_mode(state, 1);
		iio_device_release_direct_mode(indio_dev);
		if (ret)
			return ret;

		scoped_guard(mutex, &state->lock) {
			ret = ad8460_set_hvdac_byte(state, 0, val);
			if (ret)
				return ret;

			reg = FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0);
			ret = regmap_update_bits(state->regmap,
						 AD8460_CTRL_REG(0x02),
						 AD8460_PATTERN_DEPTH_MSK, reg);
		}

		return ret;
	default:
		return -EINVAL;
	}


ret = ad8460_set_hvdac_byte(state, 0, val);
if (ret)
return ret;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@MarielTinaco
Copy link
Contributor Author

V7

Applied changes from upstream

@MarielTinaco
Copy link
Contributor Author

MarielTinaco commented Oct 23, 2024

Change logs

V7

  1. Added and enabled other voltage regulators: HVEE, HVCC, AVDD_3P3V, DVDD_3P3V, VREF_5V, VCC_5V
  2. Added overtemp, under/overvoltage and under/overcurrent fault monitoring as IIO Events
  3. Added programmable current channel
  4. Added optional temperature read, using IIO consumer channel
  5. Added entry for Shutdown and Reset GPIO's
  6. Enabled Arbitrary Pattern Generator (APG) mode by setting channel attribute "raw[0-15]". Can also be controlled by "toggle_en" and "symbol" attribute

Note: driver accepted upstream

@nunojsa
Copy link
Collaborator

nunojsa commented Oct 23, 2024

@MarielTinaco,

Could you update the PR by cherry-picking directly the commits from upstream? It then makes it obvious that this is upstream. Also, don't we have dt bindings for this?

@MarielTinaco MarielTinaco force-pushed the dev/ad8460 branch 5 times, most recently from c9f0e17 to 65d6f32 Compare October 28, 2024 23:30
MarielTinaco and others added 6 commits October 29, 2024 18:51
This adds the bindings documentation for the 14-bit
High Voltage, High Current, Waveform Generator
Digital-to-Analog converter.

Signed-off-by: Mariel Tinaco <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
The AD8460 is a “bits in, power out” high voltage, high-power,
high-speed driver optimized for large output current (up to ±1 A)
and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V)
into capacitive loads.

A digital engine implements user-configurable features: modes for
digital input, programmable supply current, and fault monitoring
and programmable protection settings for output current,
output voltage, and junction temperature. The AD8460 operates on
high voltage dual supplies up to ±55 V and a single low voltage
supply of 5 V.

Backported few API's not yet compatible with ADI linux fork

devm_iio_dmaengine_buffer_setup_ext
- Reverted to "devm_iio_dmaengine_buffer_setup"
devm_mutex_init
- Reverted to "mutex_init"
in_range
- Used explicit range checking

Signed-off-by: Mariel Tinaco <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jonathan Cameron <[email protected]>
Add entry for the AD8460 driver.

Signed-off-by: Mariel Tinaco <[email protected]>
Add devicetree for AD8460.

Signed-off-by: Mariel Tinaco <[email protected]>
Fix the DT compatible string in the of_device_id table to match the
binding documentation. There should not be a space after the comma.

Fixes: a976ef2 ("iio: dac: support the ad8460 Waveform DAC")
Signed-off-by: David Lechner <[email protected]>
Link: https://patch.msgid.link/20241018-iio-adc-ad8460-fix-dt-compatible-v1-1-058231638527@baylibre.com
Signed-off-by: Jonathan Cameron <[email protected]>
Add SPI device match table for ADI AD8460 DAC. As described in [1], this
is required for the module to automatically load, even when using DT.

[1]: https://lore.kernel.org/all/[email protected]/

Signed-off-by: David Lechner <[email protected]>
Link: https://patch.msgid.link/20241018-iio-dac-ad8460-add-spi-match-table-v1-1-84a5f903bf50@baylibre.com
Signed-off-by: Jonathan Cameron <[email protected]>
@nunojsa nunojsa merged commit e9241f4 into analogdevicesinc:main Oct 30, 2024
11 of 13 checks passed
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.

4 participants