lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240907181449.1453bd41@jic23-huawei>
Date: Sat, 7 Sep 2024 18:14:49 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Mariel Tinaco <Mariel.Tinaco@...log.com>
Cc: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, Lars-Peter Clausen <lars@...afoo.de>, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Michael Hennerich <Michael.Hennerich@...log.com>, Conor Dooley
 <conor+dt@...nel.org>, Marcelo Schmitt <marcelo.schmitt1@...il.com>,
 Dimitri Fedrau <dima.fedrau@...il.com>, David Lechner
 <dlechner@...libre.com>, Nuno Sá <noname.nuno@...il.com>
Subject: Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC

On Wed, 4 Sep 2024 10:30:40 +0800
Mariel Tinaco <Mariel.Tinaco@...log.com> wrote:

> 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.
> 
> Signed-off-by: Mariel Tinaco <Mariel.Tinaco@...log.com>
A few comments inline.

Jonathan


>  obj-$(CONFIG_ADI_AXI_DAC) += adi-axi-dac.o
> diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
> new file mode 100644
> index 000000000000..6428bfaf0bd7
> --- /dev/null
> +++ b/drivers/iio/dac/ad8460.c
> @@ -0,0 +1,932 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AD8460 Waveform generator DAC Driver
> + *
> + * Copyright (C) 2024 Analog Devices, Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/buffer-dma.h>
> +#include <linux/iio/buffer-dmaengine.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>

Given there are lots of IIO specific includes, probably a case
where pulling them out as a separate block after the main
includes makes sense.

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
>
...


> +
> +	state = iio_priv(indio_dev);
> +	mutex_init(&state->lock);

Trivial but there is no a devm_mutex_init() that deals with the obscure
debug case where mutex uninit makes sense. Might as well use it here.

> +
> +	indio_dev->name = "ad8460";
> +	indio_dev->info = &ad8460_info;
> +
> +	state->spi = spi;
> +	dev = &spi->dev;
> +
> +	state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
> +	if (IS_ERR(state->regmap))
> +		return dev_err_probe(dev, PTR_ERR(state->regmap),
> +				     "Failed to initialize regmap");
> +
> +	state->sync_clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(state->sync_clk))
> +		return dev_err_probe(dev, PTR_ERR(state->sync_clk),
> +				     "Failed to get sync clk\n");
> +
> +	state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460-tmp");
> +	if (IS_ERR_OR_NULL(state->tmp_adc_channel)) {
> +		indio_dev->channels = ad8460_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> +	} else {
> +		indio_dev->channels = ad8460_channels_with_tmp_adc;
> +		indio_dev->num_channels = ARRAY_SIZE(ad8460_channels_with_tmp_adc);
> +	}
> +
Add and enable the various other supplies. They are probably always
on in which case the regulator framework will work it's magic to avoid
use having to care that they aren't in the dts.

> +	ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v");
> +	if (ret == -ENODEV)
> +		state->refio_1p2v_mv = 1200;
> +	else if (ret >= 0)
> +		state->refio_1p2v_mv = ret / 1000;
> +	else
> +		return dev_err_probe(dev, ret, "Failed to get reference voltage\n");
> +
...

> +	ret = device_property_read_u32_array(dev, "adi,range-microamp",
> +					     tmp, ARRAY_SIZE(tmp));
> +	if (!ret) {
> +		if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERCURRENT_UA))
> +			regmap_write(state->regmap, AD8460_CTRL_REG(0x08),
> +				     FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) |
> +				     AD8460_CURRENT_LIMIT_CONV(tmp[1]));

Your binding has a fixed set of values.  Yet this is anything in range.
Which is correct?  Based on datasheet I think it is flexible but
that you have listed the example values instead of the limits.


> +
> +		if (in_range(tmp[0], -AD8460_ABS_MAX_OVERCURRENT_UA, 0))
> +			regmap_write(state->regmap, AD8460_CTRL_REG(0x09),
> +				     FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) |
> +				     AD8460_CURRENT_LIMIT_CONV(abs(tmp[0])));
> +	}
> +
> +	ret = device_property_read_u32_array(dev, "adi,range-microvolt",
> +					     tmp, ARRAY_SIZE(tmp));
> +	if (!ret) {
> +		if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERVOLTAGE_UV))
> +			regmap_write(state->regmap, AD8460_CTRL_REG(0x0A),
> +				     FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) |
> +				     AD8460_VOLTAGE_LIMIT_CONV(tmp[1]));
> +
> +		if (in_range(tmp[0], -AD8460_ABS_MAX_OVERVOLTAGE_UV, 0))
> +			regmap_write(state->regmap, AD8460_CTRL_REG(0x0B),
> +				     FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) |
> +				     AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0])));
> +	}
> +
> +	ret = device_property_read_u32(dev, "adi,max-millicelsius", &temp);
> +	if (!ret) {
> +		if (in_range(temp, AD8460_MIN_OVERTEMPERATURE_MC,
> +			     AD8460_MAX_OVERTEMPERATURE_MC))
> +			regmap_write(state->regmap, AD8460_CTRL_REG(0x0C),
> +				     FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) |
> +				     AD8460_TEMP_LIMIT_CONV(abs(temp)));
> +	}
> +
> +	ret = ad8460_reset(state);
> +	if (ret)
> +		return ret;
> +
> +	/* Enables DAC by default */
> +	ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
> +				 AD8460_HVDAC_SLEEP_MSK,
> +				 FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0));
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->setup_ops = &ad8460_buffer_setup_ops;
> +
> +	ret = devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev, "tx",
> +						  IIO_BUFFER_DIRECTION_OUT);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get DMA buffer\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ