[PATCH v5 1/3] iio: frequency: admv1013: add support for ADMV1013

[PATCH v5 1/3] iio: frequency: admv1013: add support for ADMV1013

[Antoniu Miclaus]

The ADMV1013 is a wideband, microwave upconverter optimized
for point to point microwave radio designs operating in the
24 GHz to 44 GHz radio frequency (RF) range.

Datasheet:
https://www.analog.com/media/en/technical-documentation/data-sheets/ADMV1013.pdf

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
NOTE: This patch depends on
https://patchwork.kernel.org/project/linux-iio/patch/20211119085627.6348-1-antoniu.miclaus@analog.com/
changes in v5:
 - use FIELD_GET/FIELD_PREP where possible
 - fix very long lines
 drivers/iio/frequency/Kconfig    |  11 +
 drivers/iio/frequency/Makefile   |   1 +
 drivers/iio/frequency/admv1013.c | 648 +++++++++++++++++++++++++++++++
 3 files changed, 660 insertions(+)
 create mode 100644 drivers/iio/frequency/admv1013.c

diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
index 240b81502512..411b3b961e46 100644
--- a/drivers/iio/frequency/Kconfig
+++ b/drivers/iio/frequency/Kconfig
@@ -49,5 +49,16 @@ config ADF4371
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called adf4371.
+
+config ADMV1013
+	tristate "Analog Devices ADMV1013 Microwave Upconverter"
+	depends on SPI && COMMON_CLK
+	help
+	  Say yes here to build support for Analog Devices ADMV1013
+	  24 GHz to 44 GHz, Wideband, Microwave Upconverter.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called admv1013.
+
 endmenu
 endmenu
diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
index 518b1e50caef..559922a8196e 100644
--- a/drivers/iio/frequency/Makefile
+++ b/drivers/iio/frequency/Makefile
@@ -7,3 +7,4 @@
 obj-$(CONFIG_AD9523) += ad9523.o
 obj-$(CONFIG_ADF4350) += adf4350.o
 obj-$(CONFIG_ADF4371) += adf4371.o
+obj-$(CONFIG_ADMV1013) += admv1013.o
diff --git a/drivers/iio/frequency/admv1013.c b/drivers/iio/frequency/admv1013.c
new file mode 100644
index 000000000000..6cb92fb1ad8d
--- /dev/null
+++ b/drivers/iio/frequency/admv1013.c
@@ -0,0 +1,648 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ADMV1013 driver
+ *
+ * Copyright 2021 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/notifier.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/units.h>
+
+#include <asm/unaligned.h>
+
+/* ADMV1013 Register Map */
+#define ADMV1013_REG_SPI_CONTROL		0x00
+#define ADMV1013_REG_ALARM			0x01
+#define ADMV1013_REG_ALARM_MASKS		0x02
+#define ADMV1013_REG_ENABLE			0x03
+#define ADMV1013_REG_LO_AMP_I			0x05
+#define ADMV1013_REG_LO_AMP_Q			0x06
+#define ADMV1013_REG_OFFSET_ADJUST_I		0x07
+#define ADMV1013_REG_OFFSET_ADJUST_Q		0x08
+#define ADMV1013_REG_QUAD			0x09
+#define ADMV1013_REG_VVA_TEMP_COMP		0x0A
+
+/* ADMV1013_REG_SPI_CONTROL Map */
+#define ADMV1013_PARITY_EN_MSK			BIT(15)
+#define ADMV1013_SPI_SOFT_RESET_MSK		BIT(14)
+#define ADMV1013_CHIP_ID_MSK			GENMASK(11, 4)
+#define ADMV1013_CHIP_ID			0xA
+#define ADMV1013_REVISION_ID_MSK		GENMASK(3, 0)
+
+/* ADMV1013_REG_ALARM Map */
+#define ADMV1013_PARITY_ERROR_MSK		BIT(15)
+#define ADMV1013_TOO_FEW_ERRORS_MSK		BIT(14)
+#define ADMV1013_TOO_MANY_ERRORS_MSK		BIT(13)
+#define ADMV1013_ADDRESS_RANGE_ERROR_MSK	BIT(12)
+
+/* ADMV1013_REG_ENABLE Map */
+#define ADMV1013_VGA_PD_MSK			BIT(15)
+#define ADMV1013_MIXER_PD_MSK			BIT(14)
+#define ADMV1013_QUAD_PD_MSK			GENMASK(13, 11)
+#define ADMV1013_BG_PD_MSK			BIT(10)
+#define ADMV1013_MIXER_IF_EN_MSK		BIT(7)
+#define ADMV1013_DET_EN_MSK			BIT(5)
+
+/* ADMV1013_REG_LO_AMP Map */
+#define ADMV1013_LOAMP_PH_ADJ_FINE_MSK		GENMASK(13, 7)
+#define ADMV1013_MIXER_VGATE_MSK		GENMASK(6, 0)
+
+/* ADMV1013_REG_OFFSET_ADJUST Map */
+#define ADMV1013_MIXER_OFF_ADJ_P_MSK		GENMASK(15, 9)
+#define ADMV1013_MIXER_OFF_ADJ_N_MSK		GENMASK(8, 2)
+
+/* ADMV1013_REG_QUAD Map */
+#define ADMV1013_QUAD_SE_MODE_MSK		GENMASK(9, 6)
+#define ADMV1013_QUAD_FILTERS_MSK		GENMASK(3, 0)
+
+/* ADMV1013_REG_VVA_TEMP_COMP Map */
+#define ADMV1013_VVA_TEMP_COMP_MSK		GENMASK(15, 0)
+
+/* ADMV1013 Miscellaneous Defines */
+#define ADMV1013_READ				BIT(7)
+#define ADMV1013_REG_ADDR_READ_MSK		GENMASK(6, 1)
+#define ADMV1013_REG_ADDR_WRITE_MSK		GENMASK(22, 17)
+#define ADMV1013_REG_DATA_MSK			GENMASK(16, 1)
+
+enum {
+	ADMV1013_RFMOD_I,
+	ADMV1013_RFMOD_Q
+};
+
+enum {
+	ADMV1013_SE_MODE_POS = 6,
+	ADMV1013_SE_MODE_NEG = 9,
+	ADMV1013_SE_MODE_DIFF = 12
+};
+
+static const char * const admv1013_modes[] = {
+	[0] = "iq",
+	[1] = "if"
+};
+
+struct admv1013_state {
+	struct spi_device	*spi;
+	struct clk		*clkin;
+	/* Protect against concurrent accesses to the device and to data */
+	struct mutex		lock;
+	struct regulator	*reg;
+	struct notifier_block	nb;
+	unsigned int		quad_se_mode;
+	bool			det_en;
+	u8			data[3] ____cacheline_aligned;
+};
+
+static int __admv1013_spi_read(struct admv1013_state *st, unsigned int reg,
+			       unsigned int *val)
+{
+	int ret;
+	struct spi_transfer t = {0};
+
+	st->data[0] = ADMV1013_READ | FIELD_PREP(ADMV1013_REG_ADDR_READ_MSK, reg);
+	st->data[1] = 0x0;
+	st->data[2] = 0x0;
+
+	t.rx_buf = &st->data[0];
+	t.tx_buf = &st->data[0];
+	t.len = 3;
+
+	ret = spi_sync_transfer(st->spi, &t, 1);
+	if (ret)
+		return ret;
+
+	*val = FIELD_GET(ADMV1013_REG_DATA_MSK, get_unaligned_be24(&st->data[0]));
+
+	return ret;
+}
+
+static int admv1013_spi_read(struct admv1013_state *st, unsigned int reg,
+			     unsigned int *val)
+{
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = __admv1013_spi_read(st, reg, val);
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int __admv1013_spi_write(struct admv1013_state *st,
+				unsigned int reg,
+				unsigned int val)
+{
+	put_unaligned_be24(FIELD_PREP(ADMV1013_REG_DATA_MSK, val) |
+			   FIELD_PREP(ADMV1013_REG_ADDR_WRITE_MSK, reg), &st->data[0]);
+
+	return spi_write(st->spi, &st->data[0], 3);
+}
+
+static int admv1013_spi_write(struct admv1013_state *st, unsigned int reg,
+			      unsigned int val)
+{
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = __admv1013_spi_write(st, reg, val);
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int __admv1013_spi_update_bits(struct admv1013_state *st, unsigned int reg,
+				      unsigned int mask, unsigned int val)
+{
+	int ret;
+	unsigned int data, temp;
+
+	ret = __admv1013_spi_read(st, reg, &data);
+	if (ret)
+		return ret;
+
+	temp = (data & ~mask) | (val & mask);
+
+	return __admv1013_spi_write(st, reg, temp);
+}
+
+static int admv1013_spi_update_bits(struct admv1013_state *st, unsigned int reg,
+				    unsigned int mask, unsigned int val)
+{
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = __admv1013_spi_update_bits(st, reg, mask, val);
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int admv1013_get_mode(struct iio_dev *indio_dev,
+			     const struct iio_chan_spec *chan)
+{
+	struct admv1013_state *st = iio_priv(indio_dev);
+	unsigned int data;
+	int ret;
+
+	ret = admv1013_spi_read(st, ADMV1013_REG_ENABLE, &data);
+	if (ret)
+		return ret;
+
+	data = FIELD_GET(ADMV1013_MIXER_IF_EN_MSK, data);
+
+	return data;
+}
+
+static int admv1013_set_mode(struct iio_dev *indio_dev,
+			     const struct iio_chan_spec *chan,
+			     unsigned int mode)
+{
+	struct admv1013_state *st = iio_priv(indio_dev);
+
+	return admv1013_spi_update_bits(st, ADMV1013_REG_ENABLE,
+					ADMV1013_MIXER_IF_EN_MSK,
+					FIELD_PREP(ADMV1013_MIXER_IF_EN_MSK, mode));
+}
+
+static ssize_t admv1013_read(struct iio_dev *indio_dev,
+			     uintptr_t private,
+			     const struct iio_chan_spec *chan,
+			     char *buf)
+{
+	struct admv1013_state *st = iio_priv(indio_dev);
+	unsigned int data, addr;
+	int ret;
+
+	if (chan->differential) {
+		switch ((u32)private) {
+		case ADMV1013_RFMOD_I:
+			addr = ADMV1013_REG_LO_AMP_I;
+			break;
+		case ADMV1013_RFMOD_Q:
+			addr = ADMV1013_REG_LO_AMP_Q;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		ret = admv1013_spi_read(st, addr, &data);
+		if (ret)
+			return ret;
+
+		data = FIELD_GET(ADMV1013_LOAMP_PH_ADJ_FINE_MSK, data);
+	} else {
+		switch ((u32)private) {
+		case ADMV1013_RFMOD_I:
+			addr = ADMV1013_REG_OFFSET_ADJUST_I;
+			break;
+		case ADMV1013_RFMOD_Q:
+			addr = ADMV1013_REG_OFFSET_ADJUST_Q;
+			break;
+		default:
+			return -EINVAL;
+		}
+		ret = admv1013_spi_read(st, addr, &data);
+
+		if (!chan->channel)
+			data = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_P_MSK, data);
+		else
+			data = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_N_MSK, data);
+	}
+
+	return sysfs_emit(buf, "%u\n", data);
+}
+
+static ssize_t admv1013_write(struct iio_dev *indio_dev,
+			      uintptr_t private,
+			      const struct iio_chan_spec *chan,
+			      const char *buf, size_t len)
+{
+	struct admv1013_state *st = iio_priv(indio_dev);
+	unsigned int data, addr, msk;
+	int ret;
+
+	ret = kstrtou32(buf, 10, &data);
+	if (ret)
+		return ret;
+
+	if (chan->differential) {
+		data = FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_FINE_MSK, data);
+
+		switch ((u32)private) {
+		case ADMV1013_RFMOD_I:
+			ret = admv1013_spi_update_bits(st, ADMV1013_REG_LO_AMP_I,
+						       ADMV1013_LOAMP_PH_ADJ_FINE_MSK,
+						       data);
+			if (ret)
+				return ret;
+			break;
+		case ADMV1013_RFMOD_Q:
+			ret = admv1013_spi_update_bits(st, ADMV1013_REG_LO_AMP_Q,
+						       ADMV1013_LOAMP_PH_ADJ_FINE_MSK,
+						       data);
+			if (ret)
+				return ret;
+			break;
+		default:
+			return -EINVAL;
+		}
+	} else {
+		switch ((u32)private) {
+		case ADMV1013_RFMOD_I:
+			addr = ADMV1013_REG_OFFSET_ADJUST_I;
+			break;
+		case ADMV1013_RFMOD_Q:
+			addr = ADMV1013_REG_OFFSET_ADJUST_Q;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		if (!chan->channel) {
+			msk = ADMV1013_MIXER_OFF_ADJ_P_MSK;
+			data = FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_P_MSK, data);
+		} else {
+			msk = ADMV1013_MIXER_OFF_ADJ_N_MSK;
+			data = FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_N_MSK, data);
+		}
+
+		ret = admv1013_spi_update_bits(st, addr, msk, data);
+	}
+
+	return ret ? ret : len;
+}
+
+static int admv1013_update_quad_filters(struct admv1013_state *st)
+{
+	unsigned int filt_raw;
+	u64 rate = clk_get_rate(st->clkin);
+
+	if (rate >= (5400 * HZ_PER_MHZ) && rate <= (7000 * HZ_PER_MHZ))
+		filt_raw = 15;
+	else if (rate >= (5400 * HZ_PER_MHZ) && rate <= (8000 * HZ_PER_MHZ))
+		filt_raw = 10;
+	else if (rate >= (6600 * HZ_PER_MHZ) && rate <= (9200 * HZ_PER_MHZ))
+		filt_raw = 5;
+	else
+		filt_raw = 0;
+
+	return __admv1013_spi_update_bits(st, ADMV1013_REG_QUAD,
+					ADMV1013_QUAD_FILTERS_MSK,
+					FIELD_PREP(ADMV1013_QUAD_FILTERS_MSK, filt_raw));
+}
+
+static int admv1013_update_mixer_vgate(struct admv1013_state *st)
+{
+	unsigned int vcm, mixer_vgate;
+
+	vcm = regulator_get_voltage(st->reg);
+
+	if (vcm >= 0 && vcm < 1800000)
+		mixer_vgate = (2389 * vcm / 1000000 + 8100) / 100;
+	else if (vcm > 1800000 && vcm < 2600000)
+		mixer_vgate = (2375 * vcm / 1000000 + 125) / 100;
+	else
+		return -EINVAL;
+
+	return __admv1013_spi_update_bits(st, ADMV1013_REG_LO_AMP_I,
+				 ADMV1013_MIXER_VGATE_MSK,
+				 FIELD_PREP(ADMV1013_MIXER_VGATE_MSK, mixer_vgate));
+}
+
+static int admv1013_reg_access(struct iio_dev *indio_dev,
+			       unsigned int reg,
+			       unsigned int write_val,
+			       unsigned int *read_val)
+{
+	struct admv1013_state *st = iio_priv(indio_dev);
+
+	if (read_val)
+		return admv1013_spi_read(st, reg, read_val);
+	else
+		return admv1013_spi_write(st, reg, write_val);
+}
+
+static const struct iio_info admv1013_info = {
+	.debugfs_reg_access = &admv1013_reg_access,
+};
+
+static int admv1013_freq_change(struct notifier_block *nb, unsigned long action, void *data)
+{
+	struct admv1013_state *st = container_of(nb, struct admv1013_state, nb);
+	int ret;
+
+	if (action == POST_RATE_CHANGE) {
+		mutex_lock(&st->lock);
+		ret = notifier_from_errno(admv1013_update_quad_filters(st));
+		mutex_unlock(&st->lock);
+		return ret;
+	}
+
+	return NOTIFY_OK;
+}
+
+#define _ADMV1013_EXT_INFO(_name, _shared, _ident) { \
+		.name = _name, \
+		.read = admv1013_read, \
+		.write = admv1013_write, \
+		.private = _ident, \
+		.shared = _shared, \
+}
+
+static const struct iio_enum admv1013_mode_enum = {
+	.items = admv1013_modes,
+	.num_items = ARRAY_SIZE(admv1013_modes),
+	.get = admv1013_get_mode,
+	.set = admv1013_set_mode,
+};
+
+static const struct iio_chan_spec_ext_info admv1013_ext_info[] = {
+	_ADMV1013_EXT_INFO("i", IIO_SEPARATE, ADMV1013_RFMOD_I),
+	_ADMV1013_EXT_INFO("q", IIO_SEPARATE, ADMV1013_RFMOD_Q),
+	IIO_ENUM("freq_mode", IIO_SHARED_BY_ALL, &admv1013_mode_enum),
+	IIO_ENUM_AVAILABLE("freq_mode", IIO_SHARED_BY_ALL, &admv1013_mode_enum),
+	{ },
+};
+
+#define ADMV1013_CHAN_PHASE(_channel, _channel2, _admv1013_ext_info) {		\
+	.type = IIO_ALTVOLTAGE,					\
+	.output = 0,						\
+	.indexed = 1,						\
+	.channel2 = _channel2,					\
+	.channel = _channel,					\
+	.differential = 1,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE),		\
+	.ext_info = _admv1013_ext_info,				\
+	}
+
+#define ADMV1013_CHAN_CALIB(_channel, _admv1013_ext_info) {\
+	.type = IIO_ALTVOLTAGE,					\
+	.output = 0,						\
+	.indexed = 1,						\
+	.channel = _channel,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE),		\
+	.ext_info = _admv1013_ext_info,				\
+	}
+
+static const struct iio_chan_spec admv1013_channels[] = {
+	ADMV1013_CHAN_PHASE(0, 1, admv1013_ext_info),
+	ADMV1013_CHAN_CALIB(0, admv1013_ext_info),
+	ADMV1013_CHAN_CALIB(1, admv1013_ext_info),
+};
+
+static int admv1013_init(struct admv1013_state *st)
+{
+	int ret;
+	unsigned int data;
+	struct spi_device *spi = st->spi;
+
+	/* Perform a software reset */
+	ret = __admv1013_spi_update_bits(st, ADMV1013_REG_SPI_CONTROL,
+					 ADMV1013_SPI_SOFT_RESET_MSK,
+					 FIELD_PREP(ADMV1013_SPI_SOFT_RESET_MSK, 1));
+	if (ret)
+		return ret;
+
+	ret = __admv1013_spi_update_bits(st, ADMV1013_REG_SPI_CONTROL,
+					 ADMV1013_SPI_SOFT_RESET_MSK,
+					 FIELD_PREP(ADMV1013_SPI_SOFT_RESET_MSK, 0));
+	if (ret)
+		return ret;
+
+	ret = __admv1013_spi_read(st, ADMV1013_REG_SPI_CONTROL, &data);
+	if (ret)
+		return ret;
+
+	data = FIELD_GET(ADMV1013_CHIP_ID_MSK, data);
+	if (data != ADMV1013_CHIP_ID) {
+		dev_err(&spi->dev, "Invalid Chip ID.\n");
+		return -EINVAL;
+	}
+
+	ret = __admv1013_spi_write(st, ADMV1013_REG_VVA_TEMP_COMP, 0xE700);
+	if (ret)
+		return ret;
+
+	data = FIELD_PREP(ADMV1013_QUAD_SE_MODE_MSK, st->quad_se_mode);
+
+	ret = __admv1013_spi_update_bits(st, ADMV1013_REG_QUAD,
+					 ADMV1013_QUAD_SE_MODE_MSK, data);
+	if (ret)
+		return ret;
+
+	ret = admv1013_update_mixer_vgate(st);
+	if (ret)
+		return ret;
+
+	ret = admv1013_update_quad_filters(st);
+	if (ret)
+		return ret;
+
+	return __admv1013_spi_update_bits(st, ADMV1013_REG_ENABLE,
+					  ADMV1013_DET_EN_MSK,  st->det_en);
+}
+
+static void admv1013_clk_disable(void *data)
+{
+	clk_disable_unprepare(data);
+}
+
+static void admv1013_reg_disable(void *data)
+{
+	regulator_disable(data);
+}
+
+static void admv1013_powerdown(void *data)
+{
+	unsigned int enable_reg, enable_reg_msk;
+
+	/* Disable all components in the Enable Register */
+	enable_reg_msk = ADMV1013_VGA_PD_MSK |
+			ADMV1013_MIXER_PD_MSK |
+			ADMV1013_QUAD_PD_MSK |
+			ADMV1013_BG_PD_MSK |
+			ADMV1013_MIXER_IF_EN_MSK |
+			ADMV1013_DET_EN_MSK;
+
+	enable_reg = FIELD_PREP(ADMV1013_VGA_PD_MSK, 1) |
+			FIELD_PREP(ADMV1013_MIXER_PD_MSK, 1) |
+			FIELD_PREP(ADMV1013_QUAD_PD_MSK, 7) |
+			FIELD_PREP(ADMV1013_BG_PD_MSK, 1) |
+			FIELD_PREP(ADMV1013_MIXER_IF_EN_MSK, 0) |
+			FIELD_PREP(ADMV1013_DET_EN_MSK, 0);
+
+	admv1013_spi_update_bits(data, ADMV1013_REG_ENABLE, enable_reg_msk, enable_reg);
+}
+
+static int admv1013_properties_parse(struct admv1013_state *st)
+{
+	int ret;
+	const char *str;
+	struct spi_device *spi = st->spi;
+
+	st->det_en = device_property_read_bool(&spi->dev, "adi,detector-enable");
+
+	ret = device_property_read_string(&spi->dev, "adi,quad-se-mode", &str);
+	if (ret)
+		st->quad_se_mode = ADMV1013_SE_MODE_DIFF;
+
+	if (!strcmp(str, "diff"))
+		st->quad_se_mode = ADMV1013_SE_MODE_DIFF;
+	else if (!strcmp(str, "se-pos"))
+		st->quad_se_mode = ADMV1013_SE_MODE_POS;
+	else if (!strcmp(str, "se-neg"))
+		st->quad_se_mode = ADMV1013_SE_MODE_NEG;
+	else
+		return -EINVAL;
+
+	st->reg = devm_regulator_get(&spi->dev, "vcm");
+	if (IS_ERR(st->reg))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->reg),
+				     "failed to get the common-mode voltage\n");
+
+	st->clkin = devm_clk_get(&spi->dev, "lo_in");
+	if (IS_ERR(st->clkin))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->clkin),
+				     "failed to get the LO input clock\n");
+
+	return 0;
+}
+
+static int admv1013_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct admv1013_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	indio_dev->info = &admv1013_info;
+	indio_dev->name = "admv1013";
+	indio_dev->channels = admv1013_channels;
+	indio_dev->num_channels = ARRAY_SIZE(admv1013_channels);
+
+	st->spi = spi;
+
+	ret = admv1013_properties_parse(st);
+	if (ret)
+		return ret;
+
+	ret = regulator_enable(st->reg);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to enable specified Common-Mode Voltage!\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(&spi->dev, admv1013_reg_disable,
+				       st->reg);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(st->clkin);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(&spi->dev, admv1013_clk_disable, st->clkin);
+	if (ret)
+		return ret;
+
+	st->nb.notifier_call = admv1013_freq_change;
+	ret = devm_clk_notifier_register(&spi->dev, st->clkin, &st->nb);
+	if (ret)
+		return ret;
+
+	mutex_init(&st->lock);
+
+	ret = admv1013_init(st);
+	if (ret) {
+		dev_err(&spi->dev, "admv1013 init failed\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(&spi->dev, admv1013_powerdown, st);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id admv1013_id[] = {
+	{ "admv1013", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, admv1013_id);
+
+static const struct of_device_id admv1013_of_match[] = {
+	{ .compatible = "adi,admv1013" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, admv1013_of_match);
+
+static struct spi_driver admv1013_driver = {
+	.driver = {
+		.name = "admv1013",
+		.of_match_table = admv1013_of_match,
+	},
+	.probe = admv1013_probe,
+	.id_table = admv1013_id,
+};
+module_spi_driver(admv1013_driver);
+
+MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com");
+MODULE_DESCRIPTION("Analog Devices ADMV1013");
+MODULE_LICENSE("GPL v2");
-- 
2.34.0

[PATCH v5 2/3] dt-bindings: iio: frequency: add admv1013 doc

[Antoniu Miclaus]

Add device tree bindings for the ADMV1013 Upconverter.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 - remove powerdown properties from the dt bindings
 - update quad-se-mode property using strings enum
 .../bindings/iio/frequency/adi,admv1013.yaml  | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml

diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
new file mode 100644
index 000000000000..adbff58cbbf4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/frequency/adi,admv1013.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADMV1013 Microwave Upconverter
+
+maintainers:
+  - Antoniu Miclaus <antoniu.miclaus@analog.com>
+
+description: |
+   Wideband, microwave upconverter optimized for point to point microwave
+   radio designs operating in the 24 GHz to 44 GHz frequency range.
+
+   https://www.analog.com/en/products/admv1013.html
+
+properties:
+  compatible:
+    enum:
+      - adi,admv1013
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 1000000
+
+  clocks:
+    description:
+      Definition of the external clock.
+    minItems: 1
+
+  clock-names:
+    items:
+      - const: lo_in
+
+  clock-output-names:
+    maxItems: 1
+
+  vcm-supply:
+    description:
+      Analog voltage regulator.
+
+  adi,detector-enable:
+    description:
+      Enable the Envelope Detector available at output pins VENV_P and
+      VENV_N. Disable to reduce power consumption.
+    type: boolean
+
+  adi,quad-se-mode:
+    description:
+      Switch the LO path from differential to single-ended operation.
+      se-neg - Single-Ended Mode, Negative Side Disabled.
+      se-pos - Single-Ended Mode, Positive Side Disabled.
+      diff - Differential Mode.
+    enum: [se-neg, se-pos, diff]
+
+  '#clock-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - vcm-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      admv1013@0{
+        compatible = "adi,admv1013";
+        reg = <0>;
+        spi-max-frequency = <1000000>;
+        clocks = <&admv1013_lo>;
+        clock-names = "lo_in";
+        vcm-supply = <&vcm>;
+        adi,quad-se-mode = "diff";
+        adi,detector-enable;
+      };
+    };
+...
-- 
2.34.0

[PATCH v5 3/3] Documentation:ABI:testing:admv1013: add ABI docs

[Antoniu Miclaus]

Add documentation for the use of the Local Oscillator Feedthrough Offset
calibration.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v5:
 - rework the custom device attributes based on the feedback received in v4
 - add frequency translation modes custom attributes.
 .../testing/sysfs-bus-iio-frequency-admv1013  | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-frequency-admv1013

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-frequency-admv1013 b/Documentation/ABI/testing/sysfs-bus-iio-frequency-admv1013
new file mode 100644
index 000000000000..3ff80909f007
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-frequency-admv1013
@@ -0,0 +1,55 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage0-1_phase_i
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write raw value for the Local Oscillatior path quadrature I phase shift.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage0-1_phase_q
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write raw value for the Local Oscillatior path quadrature Q phase shift.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage0_calibbias_i
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write raw value for the Local Oscillatior Feedthrough Offset Calibration I Positive
+		side.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage0_calibbias_q
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write raw value for the Local Oscillatior Feedthrough Offset Calibration Q Positive
+		side.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage1_calibbias_i
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write raw value for the Local Oscillatior Feedthrough Offset Calibration I Negative
+		side.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage1_calibbias_q
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write raw value for the Local Oscillatior Feedthrough Offset Calibration Q Negative
+		side.
+
+What:		/sys/bus/iio/devices/iio:deviceX/freq_mode_available
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Reading this returns the valid values that can be written to the freq_mode attribute.
+
+		- if -> Intermediate Frequency
+		- iq -> Quadrature I/Q mode.
+
+What:		/sys/bus/iio/devices/iio:deviceX/freq_mode
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute configures the frequency mode.
+		Reading returns the actual mode.
-- 
2.34.0

Re: [PATCH v5 3/3] Documentation:ABI:testing:admv1013: add ABI docs

[Jonathan Cameron]

On Tue, 23 Nov 2021 13:53:36 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add documentation for the use of the Local Oscillator Feedthrough Offset
> calibration.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

Hi Antoniu

Nearly there, but I think the naming isn't quite consistent with normal ABI
and thinking on it a little more, not sure the freq_mode stuff should be
userspace controllable as it will be reflected in the wiring.

Thanks,

Jonathan


> ---
> changes in v5:
>  - rework the custom device attributes based on the feedback received in v4
>  - add frequency translation modes custom attributes.
>  .../testing/sysfs-bus-iio-frequency-admv1013  | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-frequency-admv1013
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-frequency-admv1013 b/Documentation/ABI/testing/sysfs-bus-iio-frequency-admv1013
> new file mode 100644
> index 000000000000..3ff80909f007
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-frequency-admv1013
> @@ -0,0 +1,55 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage0-1_phase_i

So why phase_i, rather than i_phase?

My logic is that 0-1 is the channel index, i it he modifier and phase is the info_mask element
saying what we are actually modifying about this channel.

So much as we'd have in_accel0_x_scale as with 0 as the index, x as the modifier and 
scale as the 'what' about the channel, the ordering should be the other way around to
what you have.  I and Q are defined as modifiers already so we should remain consistent
with that rather than defining a 'what' as phase_i applied to an unmodified channel
which is what i think this currently corresponds to.

> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write raw value for the Local Oscillatior path quadrature I phase shift.

Raw?  Phase is already documented in the main ABI/testing/sysfs-bus-iio documentation as
being in radians.  That needs to be true here as well so we have consistent ABI.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage0-1_phase_q
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write raw value for the Local Oscillatior path quadrature Q phase shift.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage0_calibbias_i
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write raw value for the Local Oscillatior Feedthrough Offset Calibration I Positive
> +		side.

I'd drop the raw from this as well, though calibbias is often unit free so no need to say anything
about units for this one.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage0_calibbias_q
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write raw value for the Local Oscillatior Feedthrough Offset Calibration Q Positive
> +		side.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage1_calibbias_i
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write raw value for the Local Oscillatior Feedthrough Offset Calibration I Negative
> +		side.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_altvoltage1_calibbias_q
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write raw value for the Local Oscillatior Feedthrough Offset Calibration Q Negative
> +		side.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/freq_mode_available
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Reading this returns the valid values that can be written to the freq_mode attribute.

Silly question.  How is this the mode of the 'frequency'.  IIRC it's the type of input signal
being provided.   Speaking of which, this is a characteristic of the wiring so I think
this is something that should be in DT unless I'm missing something and should not
be in the control of userspace... (hopefully I didn't say the other way around in an earlier review!)


> +
> +		- if -> Intermediate Frequency
> +		- iq -> Quadrature I/Q mode.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/freq_mode
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute configures the frequency mode.
> +		Reading returns the actual mode.

Re: [PATCH v5 2/3] dt-bindings: iio: frequency: add admv1013 doc

[Jonathan Cameron]

On Tue, 23 Nov 2021 13:53:35 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add device tree bindings for the ADMV1013 Upconverter.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

Hi Antoniu

One comment inline + as noted in the ABI, I think the freq_mode
belongs in here as something like adi,input-mode or something like that.

Thanks,

Jonathan

> ---
>  - remove powerdown properties from the dt bindings
>  - update quad-se-mode property using strings enum
>  .../bindings/iio/frequency/adi,admv1013.yaml  | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> new file mode 100644
> index 000000000000..adbff58cbbf4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/frequency/adi,admv1013.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADMV1013 Microwave Upconverter
> +
> +maintainers:
> +  - Antoniu Miclaus <antoniu.miclaus@analog.com>
> +
> +description: |
> +   Wideband, microwave upconverter optimized for point to point microwave
> +   radio designs operating in the 24 GHz to 44 GHz frequency range.
> +
> +   https://www.analog.com/en/products/admv1013.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,admv1013
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 1000000
> +
> +  clocks:
> +    description:
> +      Definition of the external clock.
> +    minItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: lo_in
> +
> +  clock-output-names:
> +    maxItems: 1

Does it provide a clock?  Whilst there is a high frequency output I'm not
sure anyone would term it a clock or that it would be helfpul to manage
it like that... 

> +
> +  vcm-supply:
> +    description:
> +      Analog voltage regulator.
> +
> +  adi,detector-enable:
> +    description:
> +      Enable the Envelope Detector available at output pins VENV_P and
> +      VENV_N. Disable to reduce power consumption.
> +    type: boolean
> +
> +  adi,quad-se-mode:
> +    description:
> +      Switch the LO path from differential to single-ended operation.
> +      se-neg - Single-Ended Mode, Negative Side Disabled.
> +      se-pos - Single-Ended Mode, Positive Side Disabled.
> +      diff - Differential Mode.
> +    enum: [se-neg, se-pos, diff]
> +
> +  '#clock-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - vcm-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      admv1013@0{
> +        compatible = "adi,admv1013";
> +        reg = <0>;
> +        spi-max-frequency = <1000000>;
> +        clocks = <&admv1013_lo>;
> +        clock-names = "lo_in";
> +        vcm-supply = <&vcm>;
> +        adi,quad-se-mode = "diff";
> +        adi,detector-enable;
> +      };
> +    };
> +...

Re: [PATCH v5 1/3] iio: frequency: admv1013: add support for ADMV1013

[Jonathan Cameron]

On Tue, 23 Nov 2021 13:53:34 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> The ADMV1013 is a wideband, microwave upconverter optimized
> for point to point microwave radio designs operating in the
> 24 GHz to 44 GHz radio frequency (RF) range.
> 
> Datasheet:
> https://www.analog.com/media/en/technical-documentation/data-sheets/ADMV1013.pdf
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

Hi Antoniu,

Clearly you are exploring new territory here so thanks for persisting and
I think we are getting close to where we want to be with this.


Other than one question over whether the generated ABI using
extinfo matches what you have in the docs, the remaining changes are all ABI
/ dt binding review related which will have impacts in here as well.

hmm. Side note, my kernel.org email is blocking for some reason so sending
using a different account. Hopefully it will make it!

> +
> +#define _ADMV1013_EXT_INFO(_name, _shared, _ident) { \
> +		.name = _name, \
> +		.read = admv1013_read, \
> +		.write = admv1013_write, \
> +		.private = _ident, \
> +		.shared = _shared, \
> +}
> +
> +static const struct iio_enum admv1013_mode_enum = {
> +	.items = admv1013_modes,
> +	.num_items = ARRAY_SIZE(admv1013_modes),
> +	.get = admv1013_get_mode,
> +	.set = admv1013_set_mode,
> +};
> +
> +static const struct iio_chan_spec_ext_info admv1013_ext_info[] = {
> +	_ADMV1013_EXT_INFO("i", IIO_SEPARATE, ADMV1013_RFMOD_I),
> +	_ADMV1013_EXT_INFO("q", IIO_SEPARATE, ADMV1013_RFMOD_Q),
> +	IIO_ENUM("freq_mode", IIO_SHARED_BY_ALL, &admv1013_mode_enum),
> +	IIO_ENUM_AVAILABLE("freq_mode", IIO_SHARED_BY_ALL, &admv1013_mode_enum),
> +	{ },
> +};
> +
> +#define ADMV1013_CHAN_PHASE(_channel, _channel2, _admv1013_ext_info) {		\
> +	.type = IIO_ALTVOLTAGE,					\
> +	.output = 0,						\
> +	.indexed = 1,						\
> +	.channel2 = _channel2,					\
> +	.channel = _channel,					\
> +	.differential = 1,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE),		\
> +	.ext_info = _admv1013_ext_info,				\
> +	}
> +
> +#define ADMV1013_CHAN_CALIB(_channel, _admv1013_ext_info) {\
> +	.type = IIO_ALTVOLTAGE,					\
> +	.output = 0,						\
> +	.indexed = 1,						\
> +	.channel = _channel,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE),		\

This has me a little confused.  How do these map to the
0-1_phase_i etc that you have in the ABI docs?
Unless I'm completely forgetting how this works this will give us
one attribute for phase and another one for _i

Perhaps it's worth providing a sysfs directory listing for this one
given it's rather unusual.

> +	.ext_info = _admv1013_ext_info,				\
> +	}
> +
> +static const struct iio_chan_spec admv1013_channels[] = {
> +	ADMV1013_CHAN_PHASE(0, 1, admv1013_ext_info),
> +	ADMV1013_CHAN_CALIB(0, admv1013_ext_info),
> +	ADMV1013_CHAN_CALIB(1, admv1013_ext_info),
> +};
> +