[PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[Antoniu Miclaus]

The ADRF6780 is a silicon germanium (SiGe) design, wideband,
microwave upconverter optimized for point to point microwave
radio designs operating in the 5.9 GHz to 23.6 GHz frequency
range.

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

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v6:
 - fix warning detected by kernel test robot
 drivers/iio/frequency/Kconfig    |  12 +
 drivers/iio/frequency/Makefile   |   1 +
 drivers/iio/frequency/adrf6780.c | 474 +++++++++++++++++++++++++++++++
 3 files changed, 487 insertions(+)
 create mode 100644 drivers/iio/frequency/adrf6780.c

diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
index 240b81502512..2c9e0559e8a4 100644
--- a/drivers/iio/frequency/Kconfig
+++ b/drivers/iio/frequency/Kconfig
@@ -49,5 +49,17 @@ config ADF4371
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called adf4371.
+
+config ADRF6780
+        tristate "Analog Devices ADRF6780 Microwave Upconverter"
+        depends on SPI
+        depends on COMMON_CLK
+        help
+          Say yes here to build support for Analog Devices ADRF6780
+          5.9 GHz to 23.6 GHz, Wideband, Microwave Upconverter.
+
+          To compile this driver as a module, choose M here: the
+          module will be called adrf6780.
+
 endmenu
 endmenu
diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
index 518b1e50caef..ae3136c79202 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_ADRF6780) += adrf6780.o
diff --git a/drivers/iio/frequency/adrf6780.c b/drivers/iio/frequency/adrf6780.c
new file mode 100644
index 000000000000..6643dbc541c7
--- /dev/null
+++ b/drivers/iio/frequency/adrf6780.c
@@ -0,0 +1,474 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ADRF6780 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/delay.h>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/spi/spi.h>
+
+#include <asm/unaligned.h>
+
+/* ADRF6780 Register Map */
+#define ADRF6780_REG_CONTROL			0x00
+#define ADRF6780_REG_ALARM_READBACK		0x01
+#define ADRF6780_REG_ALARM_MASKS		0x02
+#define ADRF6780_REG_ENABLE			0x03
+#define ADRF6780_REG_LINEARIZE			0x04
+#define ADRF6780_REG_LO_PATH			0x05
+#define ADRF6780_REG_ADC_CONTROL		0x06
+#define ADRF6780_REG_ADC_OUTPUT			0x0C
+
+/* ADRF6780_REG_CONTROL Map */
+#define ADRF6780_PARITY_EN_MSK			BIT(15)
+#define ADRF6780_SOFT_RESET_MSK			BIT(14)
+#define ADRF6780_CHIP_ID_MSK			GENMASK(11, 4)
+#define ADRF6780_CHIP_ID			0xA
+#define ADRF6780_CHIP_REVISION_MSK		GENMASK(3, 0)
+
+/* ADRF6780_REG_ALARM_READBACK Map */
+#define ADRF6780_PARITY_ERROR_MSK		BIT(15)
+#define ADRF6780_TOO_FEW_ERRORS_MSK		BIT(14)
+#define ADRF6780_TOO_MANY_ERRORS_MSK		BIT(13)
+#define ADRF6780_ADDRESS_RANGE_ERROR_MSK	BIT(12)
+
+/* ADRF6780_REG_ENABLE Map */
+#define ADRF6780_VGA_BUFFER_EN_MSK		BIT(8)
+#define ADRF6780_DETECTOR_EN_MSK		BIT(7)
+#define ADRF6780_LO_BUFFER_EN_MSK		BIT(6)
+#define ADRF6780_IF_MODE_EN_MSK			BIT(5)
+#define ADRF6780_IQ_MODE_EN_MSK			BIT(4)
+#define ADRF6780_LO_X2_EN_MSK			BIT(3)
+#define ADRF6780_LO_PPF_EN_MSK			BIT(2)
+#define ADRF6780_LO_EN_MSK			BIT(1)
+#define ADRF6780_UC_BIAS_EN_MSK			BIT(0)
+
+/* ADRF6780_REG_LINEARIZE Map */
+#define ADRF6780_RDAC_LINEARIZE_MSK		GENMASK(7, 0)
+
+/* ADRF6780_REG_LO_PATH Map */
+#define ADRF6780_LO_SIDEBAND_MSK		BIT(10)
+#define ADRF6780_Q_PATH_PHASE_ACCURACY_MSK	GENMASK(7, 4)
+#define ADRF6780_I_PATH_PHASE_ACCURACY_MSK	GENMASK(3, 0)
+
+/* ADRF6780_REG_ADC_CONTROL Map */
+#define ADRF6780_VDET_OUTPUT_SELECT_MSK		BIT(3)
+#define ADRF6780_ADC_START_MSK			BIT(2)
+#define ADRF6780_ADC_EN_MSK			BIT(1)
+#define ADRF6780_ADC_CLOCK_EN_MSK		BIT(0)
+
+/* ADRF6780_REG_ADC_OUTPUT Map */
+#define ADRF6780_ADC_STATUS_MSK			BIT(8)
+#define ADRF6780_ADC_VALUE_MSK			GENMASK(7, 0)
+
+struct adrf6780_dev {
+	struct spi_device	*spi;
+	struct clk		*clkin;
+	/* Protect against concurrent accesses to the device */
+	struct mutex		lock;
+	bool			vga_buff_en;
+	bool			lo_buff_en;
+	bool			if_mode_en;
+	bool			iq_mode_en;
+	bool			lo_x2_en;
+	bool			lo_ppf_en;
+	bool			lo_en;
+	bool			uc_bias_en;
+	bool			lo_sideband;
+	bool			vdet_out_en;
+	u8			data[3] ____cacheline_aligned;
+};
+
+static int adrf6780_spi_read(struct adrf6780_dev *dev, unsigned int reg,
+			      unsigned int *val)
+{
+	int ret;
+	struct spi_transfer t = {0};
+
+	dev->data[0] = 0x80 | (reg << 1);
+	dev->data[1] = 0x0;
+	dev->data[2] = 0x0;
+
+	t.rx_buf = &dev->data[0];
+	t.tx_buf = &dev->data[0];
+	t.len = 3;
+
+	ret = spi_sync_transfer(dev->spi, &t, 1);
+	if (ret)
+		return ret;
+
+	*val = (get_unaligned_be24(&dev->data[0]) >> 1) & GENMASK(15, 0);
+
+	return ret;
+}
+
+static int adrf6780_spi_write(struct adrf6780_dev *dev,
+				      unsigned int reg,
+				      unsigned int val)
+{
+	put_unaligned_be24((val << 1) | (reg << 17), &dev->data[0]);
+
+	return spi_write(dev->spi, &dev->data[0], 3);
+}
+
+static int adrf6780_spi_update_bits(struct adrf6780_dev *dev, unsigned int reg,
+			       unsigned int mask, unsigned int val)
+{
+	int ret;
+	unsigned int data, temp;
+
+	ret = adrf6780_spi_read(dev, reg, &data);
+	if (ret)
+		return ret;
+
+	temp = (data & ~mask) | (val & mask);
+
+	return adrf6780_spi_write(dev, reg, temp);
+}
+
+static int adrf6780_read_voltage_raw(struct adrf6780_dev *dev, unsigned int *read_val)
+{
+	int ret;
+
+	mutex_lock(&dev->lock);
+
+	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
+					ADRF6780_ADC_EN_MSK |
+					ADRF6780_ADC_CLOCK_EN_MSK |
+					ADRF6780_ADC_START_MSK,
+					FIELD_PREP(ADRF6780_ADC_EN_MSK, 1) |
+					FIELD_PREP(ADRF6780_ADC_CLOCK_EN_MSK, 1) |
+					FIELD_PREP(ADRF6780_ADC_START_MSK, 1));
+	if (ret)
+		goto exit;
+
+	usleep_range(200, 250);
+
+	ret = adrf6780_spi_read(dev, ADRF6780_REG_ADC_OUTPUT, read_val);
+	if (ret)
+		goto exit;
+
+	if (!(read_val & ADRF6780_ADC_STATUS_MSK)) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
+					ADRF6780_ADC_START_MSK,
+					FIELD_PREP(ADRF6780_ADC_START_MSK, 0));
+	if (ret)
+		goto exit;
+
+	ret = adrf6780_spi_read(dev, ADRF6780_REG_ADC_OUTPUT, read_val);
+
+exit:
+	mutex_unlock(&dev->lock);
+	return ret;
+}
+
+static int adrf6780_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long info)
+{
+	struct adrf6780_dev *dev = iio_priv(indio_dev);
+	unsigned int data;
+	int ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = adrf6780_read_voltage_raw(dev, &data);
+		if (ret)
+			return ret;
+
+		*val = data & ADRF6780_ADC_VALUE_MSK;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = adrf6780_spi_read(dev, ADRF6780_REG_LINEARIZE, &data);
+		if (ret)
+			return ret;
+
+		*val = data & ADRF6780_RDAC_LINEARIZE_MSK;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_PHASE:
+		ret = adrf6780_spi_read(dev, ADRF6780_REG_LO_PATH, &data);
+		if (ret)
+			return ret;
+
+		switch (chan->channel2) {
+		case IIO_MOD_I:
+			*val = data & ADRF6780_I_PATH_PHASE_ACCURACY_MSK;
+
+			return IIO_VAL_INT;
+		case IIO_MOD_Q:
+			*val = FIELD_GET(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, data);
+
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adrf6780_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long info)
+{
+	struct adrf6780_dev *dev = iio_priv(indio_dev);
+	int ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		return adrf6780_spi_write(dev, ADRF6780_REG_LINEARIZE, val);
+	case IIO_CHAN_INFO_PHASE:
+		switch (chan->channel2) {
+		case IIO_MOD_I:
+			mutex_lock(&dev->lock);
+			ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
+							ADRF6780_I_PATH_PHASE_ACCURACY_MSK,
+							FIELD_PREP(ADRF6780_I_PATH_PHASE_ACCURACY_MSK, val));
+			mutex_unlock(&dev->lock);
+
+			return ret;
+		case IIO_MOD_Q:
+			mutex_lock(&dev->lock);
+			ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
+							ADRF6780_Q_PATH_PHASE_ACCURACY_MSK,
+							FIELD_PREP(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, val));
+			mutex_unlock(&dev->lock);
+
+			return ret;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adrf6780_reg_access(struct iio_dev *indio_dev,
+				unsigned int reg,
+				unsigned int write_val,
+				unsigned int *read_val)
+{
+	struct adrf6780_dev *dev = iio_priv(indio_dev);
+
+	if (read_val)
+		return adrf6780_spi_read(dev, reg, read_val);
+	else
+		return adrf6780_spi_write(dev, reg, write_val);
+}
+
+static const struct iio_info adrf6780_info = {
+	.read_raw = adrf6780_read_raw,
+	.write_raw = adrf6780_write_raw,
+	.debugfs_reg_access = &adrf6780_reg_access,
+};
+
+#define ADRF6780_CHAN(_channel) {			\
+	.type = IIO_VOLTAGE,				\
+	.output = 1,					\
+	.indexed = 1,					\
+	.channel = _channel,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
+		BIT(IIO_CHAN_INFO_SCALE)		\
+}
+
+#define ADRF6780_CHAN_IQ(_channel, rf_comp) {			\
+	.type = IIO_ALTVOLTAGE,					\
+	.modified = 1,						\
+	.output = 1,						\
+	.indexed = 1,						\
+	.channel2 = IIO_MOD_##rf_comp,				\
+	.channel = _channel,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE)		\
+}
+
+static const struct iio_chan_spec adrf6780_channels[] = {
+	ADRF6780_CHAN(0),
+	ADRF6780_CHAN_IQ(0, I),
+	ADRF6780_CHAN_IQ(0, Q),
+};
+
+static int adrf6780_reset(struct adrf6780_dev *dev)
+{
+	int ret;
+	struct spi_device *spi = dev->spi;
+
+	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
+				 ADRF6780_SOFT_RESET_MSK,
+				 FIELD_PREP(ADRF6780_SOFT_RESET_MSK, 1));
+	if (ret) {
+		dev_err(&spi->dev, "ADRF6780 SPI software reset failed.\n");
+		return ret;
+	}
+
+	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
+				 ADRF6780_SOFT_RESET_MSK,
+				 FIELD_PREP(ADRF6780_SOFT_RESET_MSK, 0));
+	if (ret) {
+		dev_err(&spi->dev, "ADRF6780 SPI software reset disable failed.\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int adrf6780_init(struct adrf6780_dev *dev)
+{
+	int ret;
+	unsigned int chip_id, enable_reg, enable_reg_msk;
+	struct spi_device *spi = dev->spi;
+
+	/* Perform a software reset */
+	ret = adrf6780_reset(dev);
+	if (ret)
+		return ret;
+
+	ret = adrf6780_spi_read(dev, ADRF6780_REG_CONTROL, &chip_id);
+	if (ret)
+		return ret;
+
+	chip_id = FIELD_GET(ADRF6780_CHIP_ID_MSK, chip_id);
+	if (chip_id != ADRF6780_CHIP_ID) {
+		dev_err(&spi->dev, "ADRF6780 Invalid Chip ID.\n");
+		return -EINVAL;
+	}
+
+	enable_reg_msk = ADRF6780_VGA_BUFFER_EN_MSK |
+			ADRF6780_DETECTOR_EN_MSK |
+			ADRF6780_LO_BUFFER_EN_MSK |
+			ADRF6780_IF_MODE_EN_MSK |
+			ADRF6780_IQ_MODE_EN_MSK |
+			ADRF6780_LO_X2_EN_MSK |
+			ADRF6780_LO_PPF_EN_MSK |
+			ADRF6780_LO_EN_MSK |
+			ADRF6780_UC_BIAS_EN_MSK;
+
+	enable_reg = FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, dev->vga_buff_en) |
+			FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, 1) |
+			FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, dev->lo_buff_en) |
+			FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, dev->if_mode_en) |
+			FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, dev->iq_mode_en) |
+			FIELD_PREP(ADRF6780_LO_X2_EN_MSK, dev->lo_x2_en) |
+			FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, dev->lo_ppf_en) |
+			FIELD_PREP(ADRF6780_LO_EN_MSK, dev->lo_en) |
+			FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, dev->uc_bias_en);
+
+	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ENABLE, enable_reg_msk, enable_reg);
+	if (ret)
+		return ret;
+
+	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
+						ADRF6780_LO_SIDEBAND_MSK,
+						FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, dev->lo_sideband));
+	if (ret)
+		return ret;
+
+	return adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
+						ADRF6780_VDET_OUTPUT_SELECT_MSK,
+						FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, dev->vdet_out_en));
+}
+
+static void adrf6780_clk_disable(void *data)
+{
+	clk_disable_unprepare(data);
+}
+
+static void adrf6780_dt_parse(struct adrf6780_dev *dev)
+{
+	struct spi_device *spi = dev->spi;
+
+	dev->vga_buff_en = device_property_read_bool(&spi->dev, "adi,vga-buff-en");
+	dev->lo_buff_en = device_property_read_bool(&spi->dev, "adi,lo-buff-en");
+	dev->if_mode_en = device_property_read_bool(&spi->dev, "adi,if-mode-en");
+	dev->iq_mode_en = device_property_read_bool(&spi->dev, "adi,iq-mode-en");
+	dev->lo_x2_en = device_property_read_bool(&spi->dev, "adi,lo-x2-en");
+	dev->lo_ppf_en = device_property_read_bool(&spi->dev, "adi,lo-ppf-en");
+	dev->lo_en = device_property_read_bool(&spi->dev, "adi,lo-en");
+	dev->uc_bias_en = device_property_read_bool(&spi->dev, "adi,uc-bias-en");
+	dev->lo_sideband = device_property_read_bool(&spi->dev, "adi,lo-sideband");
+	dev->vdet_out_en = device_property_read_bool(&spi->dev, "adi,vdet-out-en");
+}
+
+static int adrf6780_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct adrf6780_dev *dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*dev));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	dev = iio_priv(indio_dev);
+
+	indio_dev->info = &adrf6780_info;
+	indio_dev->name = "adrf6780";
+	indio_dev->channels = adrf6780_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adrf6780_channels);
+
+	dev->spi = spi;
+
+	adrf6780_dt_parse(dev);
+
+	dev->clkin = devm_clk_get(&spi->dev, "lo_in");
+	if (IS_ERR(dev->clkin))
+		return PTR_ERR(dev->clkin);
+
+	ret = clk_prepare_enable(dev->clkin);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_add_action_or_reset(&spi->dev, adrf6780_clk_disable, dev->clkin);
+	if (ret < 0)
+		return ret;
+
+	mutex_init(&dev->lock);
+
+	ret = adrf6780_init(dev);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id adrf6780_id[] = {
+	{ "adrf6780", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, adrf6780_id);
+
+static const struct of_device_id adrf6780_of_match[] = {
+	{ .compatible = "adi,adrf6780" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, adrf6780_of_match);
+
+static struct spi_driver adrf6780_driver = {
+	.driver = {
+		.name = "adrf6780",
+		.of_match_table = adrf6780_of_match,
+	},
+	.probe = adrf6780_probe,
+	.id_table = adrf6780_id,
+};
+module_spi_driver(adrf6780_driver);
+
+MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com");
+MODULE_DESCRIPTION("Analog Devices ADRF6780");
+MODULE_LICENSE("GPL v2");
-- 
2.32.0

[PATCH v6 2/2] dt-bindings: iio: frequency: add adrf6780 doc

[Antoniu Miclaus]

Add device tree bindings for the ADRF6780 Upconverter.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
no changes in v6
 .../bindings/iio/frequency/adi,adrf6780.yaml  | 119 ++++++++++++++++++
 1 file changed, 119 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml

diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
new file mode 100644
index 000000000000..65cb3bee4aca
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
@@ -0,0 +1,119 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/frequency/adi,adrf6780.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADRF6780 Microwave Upconverter
+
+maintainers:
+  - Antoniu Miclaus <antoniu.miclaus@analog.com>
+
+description: |
+   Wideband, microwave upconverter optimized for point to point microwave
+   radio designs operating in the 5.9 GHz to 23.6 GHz frequency range.
+
+   https://www.analog.com/en/products/adrf6780.html
+
+properties:
+  compatible:
+    enum:
+      - adi,adrf6780
+
+  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
+
+  adi,vga-buff-en:
+    description:
+      VGA Buffer Enable.
+    type: boolean
+
+  adi,lo-buff-en:
+    description:
+      LO Buffer Enable.
+    type: boolean
+
+  adi,if-mode-en:
+    description:
+      IF Mode Enable.
+    type: boolean
+
+  adi,iq-mode-en:
+    description:
+      IQ Mode Enable.
+    type: boolean
+
+  adi,lo-x2-en:
+    description:
+      LO x2 Enable.
+    type: boolean
+
+  adi,lo-ppf-en:
+    description:
+      LO x1 Enable.
+    type: boolean
+
+  adi,lo-en:
+    description:
+      LO Enable.
+    type: boolean
+
+  adi,uc-bias-en:
+    description:
+      UC Bias Enable.
+    type: boolean
+
+  adi,lo-sideband:
+    description:
+      Switch to the Other LO Sideband.
+    type: boolean
+
+  adi,vdet-out-en:
+    description:
+      VDET Output Select Enable.
+    type: boolean
+
+  '#clock-cells':
+    const: 0
+
+dependencies:
+  adi,lo-x2-en: [ "adi,lo-en" ]
+  adi,lo-ppf-en: [ "adi,lo-en" ]
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      adrf6780@0 {
+        compatible = "adi,adrf6780";
+        reg = <0>;
+        spi-max-frequency = <1000000>;
+        clocks = <&adrf6780_lo>;
+        clock-names = "lo_in";
+      };
+    };
+...
-- 
2.32.0

Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[Andy Shevchenko]

On Fri, Jul 16, 2021 at 2:43 PM Antoniu Miclaus
<antoniu.miclaus@analog.com> wrote:
>
> The ADRF6780 is a silicon germanium (SiGe) design, wideband,
> microwave upconverter optimized for point to point microwave
> radio designs operating in the 5.9 GHz to 23.6 GHz frequency
> range.

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

Is it one line? If not, please put on one line and drop below the
blank line so it will go as a tag.

>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

First question is why not to use the regmap API (I have heard it has
gained support of 17 bit)?

...

> +        depends on COMMON_CLK

Is it mandatory for any function inside the device?

...

> +static int adrf6780_spi_read(struct adrf6780_dev *dev, unsigned int reg,
> +                             unsigned int *val)
> +{
> +       int ret;
> +       struct spi_transfer t = {0};

> +       dev->data[0] = 0x80 | (reg << 1);

This 0x80 I guess is pretty much standard and regmap SPI supports it.

> +       dev->data[1] = 0x0;
> +       dev->data[2] = 0x0;
> +
> +       t.rx_buf = &dev->data[0];
> +       t.tx_buf = &dev->data[0];
> +       t.len = 3;
> +
> +       ret = spi_sync_transfer(dev->spi, &t, 1);
> +       if (ret)
> +               return ret;
> +
> +       *val = (get_unaligned_be24(&dev->data[0]) >> 1) & GENMASK(15, 0);
> +
> +       return ret;
> +}

...

> +       usleep_range(200, 250);

Needs a comment.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[Jonathan Cameron]

On Fri, 16 Jul 2021 14:42:09 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> The ADRF6780 is a silicon germanium (SiGe) design, wideband,
> microwave upconverter optimized for point to point microwave
> radio designs operating in the 5.9 GHz to 23.6 GHz frequency
> range.
> 
> Datasheet:
> https://www.analog.com/media/en/technical-documentation/data-sheets/ADRF6780.pdf
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Hi Antoniu

As it looks like we'll have a v7, a few trivial comments from me.

...

> +
> +static int adrf6780_init(struct adrf6780_dev *dev)
> +{
> +	int ret;
> +	unsigned int chip_id, enable_reg, enable_reg_msk;
> +	struct spi_device *spi = dev->spi;
> +
> +	/* Perform a software reset */
> +	ret = adrf6780_reset(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = adrf6780_spi_read(dev, ADRF6780_REG_CONTROL, &chip_id);
> +	if (ret)
> +		return ret;
> +
> +	chip_id = FIELD_GET(ADRF6780_CHIP_ID_MSK, chip_id);
> +	if (chip_id != ADRF6780_CHIP_ID) {
> +		dev_err(&spi->dev, "ADRF6780 Invalid Chip ID.\n");
> +		return -EINVAL;
> +	}
> +
> +	enable_reg_msk = ADRF6780_VGA_BUFFER_EN_MSK |
> +			ADRF6780_DETECTOR_EN_MSK |
> +			ADRF6780_LO_BUFFER_EN_MSK |
> +			ADRF6780_IF_MODE_EN_MSK |
> +			ADRF6780_IQ_MODE_EN_MSK |
> +			ADRF6780_LO_X2_EN_MSK |
> +			ADRF6780_LO_PPF_EN_MSK |
> +			ADRF6780_LO_EN_MSK |
> +			ADRF6780_UC_BIAS_EN_MSK;
> +
> +	enable_reg = FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, dev->vga_buff_en) |
> +			FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, 1) |
> +			FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, dev->lo_buff_en) |
> +			FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, dev->if_mode_en) |
> +			FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, dev->iq_mode_en) |
> +			FIELD_PREP(ADRF6780_LO_X2_EN_MSK, dev->lo_x2_en) |
> +			FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, dev->lo_ppf_en) |
> +			FIELD_PREP(ADRF6780_LO_EN_MSK, dev->lo_en) |
> +			FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, dev->uc_bias_en);

Here we are probably turning on a bunch of stuff which will result in power usage.
Would it be sensible to turn it off again in remove path?  (devm managed should be fine).


> +
> +	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ENABLE, enable_reg_msk, enable_reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
> +						ADRF6780_LO_SIDEBAND_MSK,
> +						FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, dev->lo_sideband));
> +	if (ret)
> +		return ret;
> +
> +	return adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
> +						ADRF6780_VDET_OUTPUT_SELECT_MSK,
> +						FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, dev->vdet_out_en));
> +}
> +

> +static void adrf6780_dt_parse(struct adrf6780_dev *dev)

Trivial nitpick, but given this is all device property based (great)
the dt naming is now misleading.  Perhaps _properties_parse() ?

> +{
> +	struct spi_device *spi = dev->spi;
> +
> +	dev->vga_buff_en = device_property_read_bool(&spi->dev, "adi,vga-buff-en");
> +	dev->lo_buff_en = device_property_read_bool(&spi->dev, "adi,lo-buff-en");
> +	dev->if_mode_en = device_property_read_bool(&spi->dev, "adi,if-mode-en");
> +	dev->iq_mode_en = device_property_read_bool(&spi->dev, "adi,iq-mode-en");
> +	dev->lo_x2_en = device_property_read_bool(&spi->dev, "adi,lo-x2-en");
> +	dev->lo_ppf_en = device_property_read_bool(&spi->dev, "adi,lo-ppf-en");
> +	dev->lo_en = device_property_read_bool(&spi->dev, "adi,lo-en");
> +	dev->uc_bias_en = device_property_read_bool(&spi->dev, "adi,uc-bias-en");
> +	dev->lo_sideband = device_property_read_bool(&spi->dev, "adi,lo-sideband");
> +	dev->vdet_out_en = device_property_read_bool(&spi->dev, "adi,vdet-out-en");
> +}

Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[Jonathan Cameron]

On Fri, 16 Jul 2021 14:42:09 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

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

Hmm. I didn't really take my time to read the datasheet properly until now
so wasn't totally sure what this device is doing.  Sorry about that!

Anyhow, I'd definitely likes some descriptive comments or stuff in the
patch description to explain what the channels are actually mapping to.
This is a rather different device than most of the frequency related
devices we've seen before.

I 'think' there is effectively only one output channel for this device
and the things about it we can control are
"phase" indirectly by tweaking the phase of the LO path.

The linearize control doesn't seem to be directly related the scale of
any channel, so not sure that's appropriate to map that way.

The ADC is not reading a voltage as is specified here, but rather something
that is almost linearly related to RF output power in dBm.

Perhaps if you give a listing of the resulting sysfs ABI and what it
controls here in the patch description of v7 we can see if we can figure
out a more appropriate mapping to existing ABI (or extension of the defined
ABI if necessary). I'm not totally convinced all the stuff in device tree
belongs there either - the LO x2 for example feels like it might be
more appropriate exposed to userspace in some fashion (it was this that
got me spending the time to work out what the device actually does!)


Jonathan




> ---
> changes in v6:
>  - fix warning detected by kernel test robot
>  drivers/iio/frequency/Kconfig    |  12 +
>  drivers/iio/frequency/Makefile   |   1 +
>  drivers/iio/frequency/adrf6780.c | 474 +++++++++++++++++++++++++++++++
>  3 files changed, 487 insertions(+)
>  create mode 100644 drivers/iio/frequency/adrf6780.c
> 
> diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
> index 240b81502512..2c9e0559e8a4 100644
> --- a/drivers/iio/frequency/Kconfig
> +++ b/drivers/iio/frequency/Kconfig
> @@ -49,5 +49,17 @@ config ADF4371
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called adf4371.
> +
> +config ADRF6780
> +        tristate "Analog Devices ADRF6780 Microwave Upconverter"
> +        depends on SPI
> +        depends on COMMON_CLK
> +        help
> +          Say yes here to build support for Analog Devices ADRF6780
> +          5.9 GHz to 23.6 GHz, Wideband, Microwave Upconverter.
> +
> +          To compile this driver as a module, choose M here: the
> +          module will be called adrf6780.
> +
>  endmenu
>  endmenu
> diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
> index 518b1e50caef..ae3136c79202 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_ADRF6780) += adrf6780.o
> diff --git a/drivers/iio/frequency/adrf6780.c b/drivers/iio/frequency/adrf6780.c
> new file mode 100644
> index 000000000000..6643dbc541c7
> --- /dev/null
> +++ b/drivers/iio/frequency/adrf6780.c
> @@ -0,0 +1,474 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ADRF6780 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/delay.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/spi/spi.h>
> +
> +#include <asm/unaligned.h>
> +
> +/* ADRF6780 Register Map */
> +#define ADRF6780_REG_CONTROL			0x00
> +#define ADRF6780_REG_ALARM_READBACK		0x01
> +#define ADRF6780_REG_ALARM_MASKS		0x02
> +#define ADRF6780_REG_ENABLE			0x03
> +#define ADRF6780_REG_LINEARIZE			0x04
> +#define ADRF6780_REG_LO_PATH			0x05
> +#define ADRF6780_REG_ADC_CONTROL		0x06
> +#define ADRF6780_REG_ADC_OUTPUT			0x0C
> +
> +/* ADRF6780_REG_CONTROL Map */
> +#define ADRF6780_PARITY_EN_MSK			BIT(15)
> +#define ADRF6780_SOFT_RESET_MSK			BIT(14)
> +#define ADRF6780_CHIP_ID_MSK			GENMASK(11, 4)
> +#define ADRF6780_CHIP_ID			0xA
> +#define ADRF6780_CHIP_REVISION_MSK		GENMASK(3, 0)
> +
> +/* ADRF6780_REG_ALARM_READBACK Map */
> +#define ADRF6780_PARITY_ERROR_MSK		BIT(15)
> +#define ADRF6780_TOO_FEW_ERRORS_MSK		BIT(14)
> +#define ADRF6780_TOO_MANY_ERRORS_MSK		BIT(13)
> +#define ADRF6780_ADDRESS_RANGE_ERROR_MSK	BIT(12)
> +
> +/* ADRF6780_REG_ENABLE Map */
> +#define ADRF6780_VGA_BUFFER_EN_MSK		BIT(8)
> +#define ADRF6780_DETECTOR_EN_MSK		BIT(7)
> +#define ADRF6780_LO_BUFFER_EN_MSK		BIT(6)
> +#define ADRF6780_IF_MODE_EN_MSK			BIT(5)
> +#define ADRF6780_IQ_MODE_EN_MSK			BIT(4)
> +#define ADRF6780_LO_X2_EN_MSK			BIT(3)
> +#define ADRF6780_LO_PPF_EN_MSK			BIT(2)
> +#define ADRF6780_LO_EN_MSK			BIT(1)
> +#define ADRF6780_UC_BIAS_EN_MSK			BIT(0)
> +
> +/* ADRF6780_REG_LINEARIZE Map */
> +#define ADRF6780_RDAC_LINEARIZE_MSK		GENMASK(7, 0)
> +
> +/* ADRF6780_REG_LO_PATH Map */
> +#define ADRF6780_LO_SIDEBAND_MSK		BIT(10)
> +#define ADRF6780_Q_PATH_PHASE_ACCURACY_MSK	GENMASK(7, 4)
> +#define ADRF6780_I_PATH_PHASE_ACCURACY_MSK	GENMASK(3, 0)
> +
> +/* ADRF6780_REG_ADC_CONTROL Map */
> +#define ADRF6780_VDET_OUTPUT_SELECT_MSK		BIT(3)
> +#define ADRF6780_ADC_START_MSK			BIT(2)
> +#define ADRF6780_ADC_EN_MSK			BIT(1)
> +#define ADRF6780_ADC_CLOCK_EN_MSK		BIT(0)
> +
> +/* ADRF6780_REG_ADC_OUTPUT Map */
> +#define ADRF6780_ADC_STATUS_MSK			BIT(8)
> +#define ADRF6780_ADC_VALUE_MSK			GENMASK(7, 0)
> +
> +struct adrf6780_dev {
> +	struct spi_device	*spi;
> +	struct clk		*clkin;
> +	/* Protect against concurrent accesses to the device */
> +	struct mutex		lock;
> +	bool			vga_buff_en;
> +	bool			lo_buff_en;
> +	bool			if_mode_en;
> +	bool			iq_mode_en;
> +	bool			lo_x2_en;
> +	bool			lo_ppf_en;
> +	bool			lo_en;
> +	bool			uc_bias_en;
> +	bool			lo_sideband;
> +	bool			vdet_out_en;
> +	u8			data[3] ____cacheline_aligned;
> +};
> +
> +static int adrf6780_spi_read(struct adrf6780_dev *dev, unsigned int reg,
> +			      unsigned int *val)
> +{
> +	int ret;
> +	struct spi_transfer t = {0};
> +
> +	dev->data[0] = 0x80 | (reg << 1);
> +	dev->data[1] = 0x0;
> +	dev->data[2] = 0x0;
> +
> +	t.rx_buf = &dev->data[0];
> +	t.tx_buf = &dev->data[0];
> +	t.len = 3;
> +
> +	ret = spi_sync_transfer(dev->spi, &t, 1);
> +	if (ret)
> +		return ret;
> +
> +	*val = (get_unaligned_be24(&dev->data[0]) >> 1) & GENMASK(15, 0);
> +
> +	return ret;
> +}
> +
> +static int adrf6780_spi_write(struct adrf6780_dev *dev,
> +				      unsigned int reg,
> +				      unsigned int val)
> +{
> +	put_unaligned_be24((val << 1) | (reg << 17), &dev->data[0]);
> +
> +	return spi_write(dev->spi, &dev->data[0], 3);
> +}
> +
> +static int adrf6780_spi_update_bits(struct adrf6780_dev *dev, unsigned int reg,
> +			       unsigned int mask, unsigned int val)
> +{
> +	int ret;
> +	unsigned int data, temp;
> +
> +	ret = adrf6780_spi_read(dev, reg, &data);
> +	if (ret)
> +		return ret;
> +
> +	temp = (data & ~mask) | (val & mask);
> +
> +	return adrf6780_spi_write(dev, reg, temp);
> +}
> +
> +static int adrf6780_read_voltage_raw(struct adrf6780_dev *dev, unsigned int *read_val)
> +{
> +	int ret;
> +
> +	mutex_lock(&dev->lock);
> +
> +	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
> +					ADRF6780_ADC_EN_MSK |
> +					ADRF6780_ADC_CLOCK_EN_MSK |
> +					ADRF6780_ADC_START_MSK,
> +					FIELD_PREP(ADRF6780_ADC_EN_MSK, 1) |
> +					FIELD_PREP(ADRF6780_ADC_CLOCK_EN_MSK, 1) |
> +					FIELD_PREP(ADRF6780_ADC_START_MSK, 1));
> +	if (ret)
> +		goto exit;
> +
> +	usleep_range(200, 250);
> +
> +	ret = adrf6780_spi_read(dev, ADRF6780_REG_ADC_OUTPUT, read_val);
> +	if (ret)
> +		goto exit;
> +
> +	if (!(read_val & ADRF6780_ADC_STATUS_MSK)) {
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
> +					ADRF6780_ADC_START_MSK,
> +					FIELD_PREP(ADRF6780_ADC_START_MSK, 0));
> +	if (ret)
> +		goto exit;
> +
> +	ret = adrf6780_spi_read(dev, ADRF6780_REG_ADC_OUTPUT, read_val);
> +
> +exit:
> +	mutex_unlock(&dev->lock);
> +	return ret;
> +}
> +
> +static int adrf6780_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long info)
> +{
> +	struct adrf6780_dev *dev = iio_priv(indio_dev);
> +	unsigned int data;
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = adrf6780_read_voltage_raw(dev, &data);
> +		if (ret)
> +			return ret;
> +
> +		*val = data & ADRF6780_ADC_VALUE_MSK;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = adrf6780_spi_read(dev, ADRF6780_REG_LINEARIZE, &data);
> +		if (ret)
> +			return ret;
> +
> +		*val = data & ADRF6780_RDAC_LINEARIZE_MSK;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_PHASE:
> +		ret = adrf6780_spi_read(dev, ADRF6780_REG_LO_PATH, &data);
> +		if (ret)
> +			return ret;
> +
> +		switch (chan->channel2) {
> +		case IIO_MOD_I:
> +			*val = data & ADRF6780_I_PATH_PHASE_ACCURACY_MSK;
> +
> +			return IIO_VAL_INT;
> +		case IIO_MOD_Q:
> +			*val = FIELD_GET(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, data);
> +
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int adrf6780_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long info)
> +{
> +	struct adrf6780_dev *dev = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return adrf6780_spi_write(dev, ADRF6780_REG_LINEARIZE, val);
> +	case IIO_CHAN_INFO_PHASE:
> +		switch (chan->channel2) {
> +		case IIO_MOD_I:
> +			mutex_lock(&dev->lock);
> +			ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
> +							ADRF6780_I_PATH_PHASE_ACCURACY_MSK,
> +							FIELD_PREP(ADRF6780_I_PATH_PHASE_ACCURACY_MSK, val));
> +			mutex_unlock(&dev->lock);
> +
> +			return ret;
> +		case IIO_MOD_Q:
> +			mutex_lock(&dev->lock);
> +			ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
> +							ADRF6780_Q_PATH_PHASE_ACCURACY_MSK,
> +							FIELD_PREP(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, val));
> +			mutex_unlock(&dev->lock);
> +
> +			return ret;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int adrf6780_reg_access(struct iio_dev *indio_dev,
> +				unsigned int reg,
> +				unsigned int write_val,
> +				unsigned int *read_val)
> +{
> +	struct adrf6780_dev *dev = iio_priv(indio_dev);
> +
> +	if (read_val)
> +		return adrf6780_spi_read(dev, reg, read_val);
> +	else
> +		return adrf6780_spi_write(dev, reg, write_val);
> +}
> +
> +static const struct iio_info adrf6780_info = {
> +	.read_raw = adrf6780_read_raw,
> +	.write_raw = adrf6780_write_raw,
> +	.debugfs_reg_access = &adrf6780_reg_access,
> +};
> +
> +#define ADRF6780_CHAN(_channel) {			\
> +	.type = IIO_VOLTAGE,				\
> +	.output = 1,					\
> +	.indexed = 1,					\
> +	.channel = _channel,				\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> +		BIT(IIO_CHAN_INFO_SCALE)		\
> +}
> +
> +#define ADRF6780_CHAN_IQ(_channel, rf_comp) {			\
> +	.type = IIO_ALTVOLTAGE,					\
> +	.modified = 1,						\
> +	.output = 1,						\
> +	.indexed = 1,						\
> +	.channel2 = IIO_MOD_##rf_comp,				\
> +	.channel = _channel,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE)		\
> +}
> +
> +static const struct iio_chan_spec adrf6780_channels[] = {
> +	ADRF6780_CHAN(0),
> +	ADRF6780_CHAN_IQ(0, I),
> +	ADRF6780_CHAN_IQ(0, Q),
> +};




> +
> +static int adrf6780_reset(struct adrf6780_dev *dev)
> +{
> +	int ret;
> +	struct spi_device *spi = dev->spi;
> +
> +	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
> +				 ADRF6780_SOFT_RESET_MSK,
> +				 FIELD_PREP(ADRF6780_SOFT_RESET_MSK, 1));
> +	if (ret) {
> +		dev_err(&spi->dev, "ADRF6780 SPI software reset failed.\n");
> +		return ret;
> +	}
> +
> +	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
> +				 ADRF6780_SOFT_RESET_MSK,
> +				 FIELD_PREP(ADRF6780_SOFT_RESET_MSK, 0));
> +	if (ret) {
> +		dev_err(&spi->dev, "ADRF6780 SPI software reset disable failed.\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int adrf6780_init(struct adrf6780_dev *dev)
> +{
> +	int ret;
> +	unsigned int chip_id, enable_reg, enable_reg_msk;
> +	struct spi_device *spi = dev->spi;
> +
> +	/* Perform a software reset */
> +	ret = adrf6780_reset(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = adrf6780_spi_read(dev, ADRF6780_REG_CONTROL, &chip_id);
> +	if (ret)
> +		return ret;
> +
> +	chip_id = FIELD_GET(ADRF6780_CHIP_ID_MSK, chip_id);
> +	if (chip_id != ADRF6780_CHIP_ID) {
> +		dev_err(&spi->dev, "ADRF6780 Invalid Chip ID.\n");
> +		return -EINVAL;
> +	}
> +
> +	enable_reg_msk = ADRF6780_VGA_BUFFER_EN_MSK |
> +			ADRF6780_DETECTOR_EN_MSK |
> +			ADRF6780_LO_BUFFER_EN_MSK |
> +			ADRF6780_IF_MODE_EN_MSK |
> +			ADRF6780_IQ_MODE_EN_MSK |
> +			ADRF6780_LO_X2_EN_MSK |
> +			ADRF6780_LO_PPF_EN_MSK |
> +			ADRF6780_LO_EN_MSK |
> +			ADRF6780_UC_BIAS_EN_MSK;
> +
> +	enable_reg = FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, dev->vga_buff_en) |
> +			FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, 1) |
> +			FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, dev->lo_buff_en) |
> +			FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, dev->if_mode_en) |
> +			FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, dev->iq_mode_en) |
> +			FIELD_PREP(ADRF6780_LO_X2_EN_MSK, dev->lo_x2_en) |
> +			FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, dev->lo_ppf_en) |
> +			FIELD_PREP(ADRF6780_LO_EN_MSK, dev->lo_en) |
> +			FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, dev->uc_bias_en);
> +
> +	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ENABLE, enable_reg_msk, enable_reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
> +						ADRF6780_LO_SIDEBAND_MSK,
> +						FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, dev->lo_sideband));
> +	if (ret)
> +		return ret;
> +
> +	return adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
> +						ADRF6780_VDET_OUTPUT_SELECT_MSK,
> +						FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, dev->vdet_out_en));
> +}
> +
> +static void adrf6780_clk_disable(void *data)
> +{
> +	clk_disable_unprepare(data);
> +}
> +
> +static void adrf6780_dt_parse(struct adrf6780_dev *dev)
> +{
> +	struct spi_device *spi = dev->spi;
> +
> +	dev->vga_buff_en = device_property_read_bool(&spi->dev, "adi,vga-buff-en");
> +	dev->lo_buff_en = device_property_read_bool(&spi->dev, "adi,lo-buff-en");
> +	dev->if_mode_en = device_property_read_bool(&spi->dev, "adi,if-mode-en");
> +	dev->iq_mode_en = device_property_read_bool(&spi->dev, "adi,iq-mode-en");
> +	dev->lo_x2_en = device_property_read_bool(&spi->dev, "adi,lo-x2-en");
> +	dev->lo_ppf_en = device_property_read_bool(&spi->dev, "adi,lo-ppf-en");
> +	dev->lo_en = device_property_read_bool(&spi->dev, "adi,lo-en");
> +	dev->uc_bias_en = device_property_read_bool(&spi->dev, "adi,uc-bias-en");
> +	dev->lo_sideband = device_property_read_bool(&spi->dev, "adi,lo-sideband");
> +	dev->vdet_out_en = device_property_read_bool(&spi->dev, "adi,vdet-out-en");
> +}
> +
> +static int adrf6780_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct adrf6780_dev *dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*dev));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	dev = iio_priv(indio_dev);
> +
> +	indio_dev->info = &adrf6780_info;
> +	indio_dev->name = "adrf6780";
> +	indio_dev->channels = adrf6780_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(adrf6780_channels);
> +
> +	dev->spi = spi;
> +
> +	adrf6780_dt_parse(dev);
> +
> +	dev->clkin = devm_clk_get(&spi->dev, "lo_in");
> +	if (IS_ERR(dev->clkin))
> +		return PTR_ERR(dev->clkin);
> +
> +	ret = clk_prepare_enable(dev->clkin);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(&spi->dev, adrf6780_clk_disable, dev->clkin);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_init(&dev->lock);
> +
> +	ret = adrf6780_init(dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct spi_device_id adrf6780_id[] = {
> +	{ "adrf6780", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, adrf6780_id);
> +
> +static const struct of_device_id adrf6780_of_match[] = {
> +	{ .compatible = "adi,adrf6780" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, adrf6780_of_match);
> +
> +static struct spi_driver adrf6780_driver = {
> +	.driver = {
> +		.name = "adrf6780",
> +		.of_match_table = adrf6780_of_match,
> +	},
> +	.probe = adrf6780_probe,
> +	.id_table = adrf6780_id,
> +};
> +module_spi_driver(adrf6780_driver);
> +
> +MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com");
> +MODULE_DESCRIPTION("Analog Devices ADRF6780");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v6 2/2] dt-bindings: iio: frequency: add adrf6780 doc

[Jonathan Cameron]

On Fri, 16 Jul 2021 14:42:10 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add device tree bindings for the ADRF6780 Upconverter.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

oops. Would have ideally gotten to taking a close look at this before v6 :(  
Sorry about that!  I don't suppose we have any other reviewers whose knowledge of
this sort of hardware is fresher and deeper than mine?  I'd like more eyes on
the next version of this if possible!

Jonathan

> ---
> no changes in v6
>  .../bindings/iio/frequency/adi,adrf6780.yaml  | 119 ++++++++++++++++++
>  1 file changed, 119 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
> new file mode 100644
> index 000000000000..65cb3bee4aca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
> @@ -0,0 +1,119 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/frequency/adi,adrf6780.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADRF6780 Microwave Upconverter
> +
> +maintainers:
> +  - Antoniu Miclaus <antoniu.miclaus@analog.com>
> +
> +description: |
> +   Wideband, microwave upconverter optimized for point to point microwave
> +   radio designs operating in the 5.9 GHz to 23.6 GHz frequency range.
> +
> +   https://www.analog.com/en/products/adrf6780.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adrf6780
> +
> +  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
> +
> +  adi,vga-buff-en:
> +    description:
> +      VGA Buffer Enable.

Ideally I'd like any acronyms spelt out in the descriptions.
(I assume this is variable gain amplifier?) The fun question from looking at
this in the datasheet is where is it in the functional diagram?  I'm not actually
sure where it might be.  Perhaps in the VVA, so on the VAAT input?

If you have a convenient path to point out to your datasheet people that it is
undocumented, that might be good to clean up :)
My guess is this is needed if the precision reference on the example circuit needs
a high impedance input, but I'm only guessing.


> +    type: boolean
> +
> +  adi,lo-buff-en:
> +    description:
> +      LO Buffer Enable.

What is LO and why might it need a buffer? (or is it always a good idea to turn this on
when using the device?)

> +    type: boolean
> +
> +  adi,if-mode-en:
> +    description:
> +      IF Mode Enable.
IF stands for what? Intermediate Frequency...

> +    type: boolean
> +
> +  adi,iq-mode-en:
> +    description:
> +      IQ Mode Enable.
> +    type: boolean

I struggled to figure this out from the datasheet, but is there a circumstance under which
if and iq mode might both be enabled? Nothing stops you setting the registers that way, but
it seems to be documented as one or the other...

For that matter, why probably want to describe this as "baseband IQ mode" given datasheet
refers to as baseband in most places other than the register definition.

> +
> +  adi,lo-x2-en:
> +    description:
> +      LO x2 Enable.
> +    type: boolean
> +
> +  adi,lo-ppf-en:
> +    description:
> +      LO x1 Enable.

This is curious. I agree that the register write documents it as x1 enable, but it seems
to be be enabling polyphase filters - what exactly their relationship is to x1 is not that
clear to me.  Perhaps one to query with the hardware people if possible!

> +    type: boolean
> +
> +  adi,lo-en:
> +    description:
> +      LO Enable.
> +    type: boolean

Would you ever have this off whilst running?  It's possible I'm missing something, but
I think you need that frequency path to be enabled to get anything at all to
happen.

> +
> +  adi,uc-bias-en:
> +    description:
> +      UC Bias Enable.
> +    type: boolean

This being completely undocumented apart from the register setting, I have 0 idea what
it actually is.   Any chance we can get some more details?

> +
> +  adi,lo-sideband:
> +    description:
> +      Switch to the Other LO Sideband.

Switch what to the other LO sideband?

> +    type: boolean
> +
> +  adi,vdet-out-en:

So my reading of the datasheet on this one didn't lead me to a completely clear answer.
Does turning this one effectively stop you using the internal ADC to measure the power
whilst exposing the signal on an external pin?

> +    description:
> +      VDET Output Select Enable.
> +    type: boolean
> +
> +  '#clock-cells':
> +    const: 0
> +
> +dependencies:
> +  adi,lo-x2-en: [ "adi,lo-en" ]
> +  adi,lo-ppf-en: [ "adi,lo-en" ]
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      adrf6780@0 {
> +        compatible = "adi,adrf6780";
> +        reg = <0>;
> +        spi-max-frequency = <1000000>;
> +        clocks = <&adrf6780_lo>;
> +        clock-names = "lo_in";
> +      };
> +    };
> +...

Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3110 bytes --]

Hi Antoniu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on iio/togreg]
[also build test ERROR on linux/master linus/master v5.14-rc1 next-20210716]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Antoniu-Miclaus/iio-frequency-adrf6780-add-support-for-ADRF6780/20210718-104320
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/59f8f05a22397c9b919c512d1e28cbad5caf78a7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Antoniu-Miclaus/iio-frequency-adrf6780-add-support-for-ADRF6780/20210718-104320
        git checkout 59f8f05a22397c9b919c512d1e28cbad5caf78a7
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=arc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/iio/frequency/adrf6780.c: In function 'adrf6780_read_voltage_raw':
>> drivers/iio/frequency/adrf6780.c:161:17: error: invalid operands to binary & (have 'unsigned int *' and 'long unsigned int')
     161 |  if (!(read_val & ADRF6780_ADC_STATUS_MSK)) {
         |                 ^


vim +161 drivers/iio/frequency/adrf6780.c

   138	
   139	static int adrf6780_read_voltage_raw(struct adrf6780_dev *dev, unsigned int *read_val)
   140	{
   141		int ret;
   142	
   143		mutex_lock(&dev->lock);
   144	
   145		ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
   146						ADRF6780_ADC_EN_MSK |
   147						ADRF6780_ADC_CLOCK_EN_MSK |
   148						ADRF6780_ADC_START_MSK,
   149						FIELD_PREP(ADRF6780_ADC_EN_MSK, 1) |
   150						FIELD_PREP(ADRF6780_ADC_CLOCK_EN_MSK, 1) |
   151						FIELD_PREP(ADRF6780_ADC_START_MSK, 1));
   152		if (ret)
   153			goto exit;
   154	
   155		usleep_range(200, 250);
   156	
   157		ret = adrf6780_spi_read(dev, ADRF6780_REG_ADC_OUTPUT, read_val);
   158		if (ret)
   159			goto exit;
   160	
 > 161		if (!(read_val & ADRF6780_ADC_STATUS_MSK)) {
   162			ret = -EINVAL;
   163			goto exit;
   164		}
   165	
   166		ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
   167						ADRF6780_ADC_START_MSK,
   168						FIELD_PREP(ADRF6780_ADC_START_MSK, 0));
   169		if (ret)
   170			goto exit;
   171	
   172		ret = adrf6780_spi_read(dev, ADRF6780_REG_ADC_OUTPUT, read_val);
   173	
   174	exit:
   175		mutex_unlock(&dev->lock);
   176		return ret;
   177	}
   178	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 68529 bytes --]

RE: [PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[Miclaus, Antoniu]

Hello Andy,

Thanks for the review:

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, July 16, 2021 5:53 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> jic23@kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
> 
> [External]
> 
> On Fri, Jul 16, 2021 at 2:43 PM Antoniu Miclaus
> <antoniu.miclaus@analog.com> wrote:
> >
> > The ADRF6780 is a silicon germanium (SiGe) design, wideband,
> > microwave upconverter optimized for point to point microwave
> > radio designs operating in the 5.9 GHz to 23.6 GHz frequency
> > range.
> 
> > Datasheet:
> > https://www.analog.com/media/en/technical-documentation/data-
> sheets/ADRF6780.pdf
> 
> Is it one line? If not, please put on one line and drop below the
> blank line so it will go as a tag.
Yes, it is one line.
> 
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> 
> First question is why not to use the regmap API (I have heard it has
> gained support of 17 bit)?

Initially that was the plan, but after this patch:
https://github.com/torvalds/linux/commit/4191f19792bf91267835eb090d970e9cd6277a65
the custom write formats for regmap allow the read only via cached registers.

Therefore, I preferred using spi transfers for write/read to/from the device.
> ...
> 
> > +        depends on COMMON_CLK
> 
> Is it mandatory for any function inside the device?

Yes. It will serve as LO input to the device.
> 
> ...
> 
> > +static int adrf6780_spi_read(struct adrf6780_dev *dev, unsigned int reg,
> > +                             unsigned int *val)
> > +{
> > +       int ret;
> > +       struct spi_transfer t = {0};
> 
> > +       dev->data[0] = 0x80 | (reg << 1);
> 
> This 0x80 I guess is pretty much standard and regmap SPI supports it.
> 
> > +       dev->data[1] = 0x0;
> > +       dev->data[2] = 0x0;
> > +
> > +       t.rx_buf = &dev->data[0];
> > +       t.tx_buf = &dev->data[0];
> > +       t.len = 3;
> > +
> > +       ret = spi_sync_transfer(dev->spi, &t, 1);
> > +       if (ret)
> > +               return ret;
> > +
> > +       *val = (get_unaligned_be24(&dev->data[0]) >> 1) & GENMASK(15, 0);
> > +
> > +       return ret;
> > +}
> 
> ...
> 
> > +       usleep_range(200, 250);
> 
> Needs a comment.
Will add in next patch version.
> 
> --
> With Best Regards,
> Andy Shevchenko

Regards,
Antoniu

RE: [PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[Miclaus, Antoniu]

Hello Jonathan,


> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, July 17, 2021 4:26 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
> 
> [External]
> 
> On Fri, 16 Jul 2021 14:42:09 +0300
> Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> 
> > The ADRF6780 is a silicon germanium (SiGe) design, wideband,
> > microwave upconverter optimized for point to point microwave
> > radio designs operating in the 5.9 GHz to 23.6 GHz frequency
> > range.
> >
> > Datasheet:
> > https://www.analog.com/media/en/technical-documentation/data-
> sheets/ADRF6780.pdf
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Hi Antoniu
> 
> As it looks like we'll have a v7, a few trivial comments from me.
> 
> ...
> 
> > +
> > +static int adrf6780_init(struct adrf6780_dev *dev)
> > +{
> > +	int ret;
> > +	unsigned int chip_id, enable_reg, enable_reg_msk;
> > +	struct spi_device *spi = dev->spi;
> > +
> > +	/* Perform a software reset */
> > +	ret = adrf6780_reset(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adrf6780_spi_read(dev, ADRF6780_REG_CONTROL, &chip_id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	chip_id = FIELD_GET(ADRF6780_CHIP_ID_MSK, chip_id);
> > +	if (chip_id != ADRF6780_CHIP_ID) {
> > +		dev_err(&spi->dev, "ADRF6780 Invalid Chip ID.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	enable_reg_msk = ADRF6780_VGA_BUFFER_EN_MSK |
> > +			ADRF6780_DETECTOR_EN_MSK |
> > +			ADRF6780_LO_BUFFER_EN_MSK |
> > +			ADRF6780_IF_MODE_EN_MSK |
> > +			ADRF6780_IQ_MODE_EN_MSK |
> > +			ADRF6780_LO_X2_EN_MSK |
> > +			ADRF6780_LO_PPF_EN_MSK |
> > +			ADRF6780_LO_EN_MSK |
> > +			ADRF6780_UC_BIAS_EN_MSK;
> > +
> > +	enable_reg = FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, dev-
> >vga_buff_en) |
> > +			FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, 1) |
> > +			FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, dev-
> >lo_buff_en) |
> > +			FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, dev-
> >if_mode_en) |
> > +			FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, dev-
> >iq_mode_en) |
> > +			FIELD_PREP(ADRF6780_LO_X2_EN_MSK, dev-
> >lo_x2_en) |
> > +			FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, dev-
> >lo_ppf_en) |
> > +			FIELD_PREP(ADRF6780_LO_EN_MSK, dev->lo_en) |
> > +			FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, dev-
> >uc_bias_en);
> 
> Here we are probably turning on a bunch of stuff which will result in power
> usage.
> Would it be sensible to turn it off again in remove path?  (devm managed
> should be fine).

Most of these attributes are enabled by default after device reset.
Taking into account this statement, is it still worth adding a 'custom' remove path?

> 
> 
> > +
> > +	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ENABLE,
> enable_reg_msk, enable_reg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
> > +
> 	ADRF6780_LO_SIDEBAND_MSK,
> > +
> 	FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, dev->lo_sideband));
> > +	if (ret)
> > +		return ret;
> > +
> > +	return adrf6780_spi_update_bits(dev,
> ADRF6780_REG_ADC_CONTROL,
> > +
> 	ADRF6780_VDET_OUTPUT_SELECT_MSK,
> > +
> 	FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, dev-
> >vdet_out_en));
> > +}
> > +
> 
> > +static void adrf6780_dt_parse(struct adrf6780_dev *dev)
> 
> Trivial nitpick, but given this is all device property based (great)
> the dt naming is now misleading.  Perhaps _properties_parse() ?
> 
> > +{
> > +	struct spi_device *spi = dev->spi;
> > +
> > +	dev->vga_buff_en = device_property_read_bool(&spi->dev,
> "adi,vga-buff-en");
> > +	dev->lo_buff_en = device_property_read_bool(&spi->dev, "adi,lo-
> buff-en");
> > +	dev->if_mode_en = device_property_read_bool(&spi->dev, "adi,if-
> mode-en");
> > +	dev->iq_mode_en = device_property_read_bool(&spi->dev, "adi,iq-
> mode-en");
> > +	dev->lo_x2_en = device_property_read_bool(&spi->dev, "adi,lo-x2-
> en");
> > +	dev->lo_ppf_en = device_property_read_bool(&spi->dev, "adi,lo-
> ppf-en");
> > +	dev->lo_en = device_property_read_bool(&spi->dev, "adi,lo-en");
> > +	dev->uc_bias_en = device_property_read_bool(&spi->dev, "adi,uc-
> bias-en");
> > +	dev->lo_sideband = device_property_read_bool(&spi->dev, "adi,lo-
> sideband");
> > +	dev->vdet_out_en = device_property_read_bool(&spi->dev,
> "adi,vdet-out-en");
> > +}

--
Antoniu Miclăuş

RE: [PATCH v6 2/2] dt-bindings: iio: frequency: add adrf6780 doc

[Miclaus, Antoniu]

Hello Jonathan,

Adding @Thahirally, Murtaza, maybe he can help us with some more details hardware-wise.

Unfortunately, the current mail format does not allow me to highlight the points of interest in the conversation.

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, July 17, 2021 5:22 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Rob Herring <robh@kernel.org>
> Subject: Re: [PATCH v6 2/2] dt-bindings: iio: frequency: add adrf6780 doc
> 
> [External]
> 
> On Fri, 16 Jul 2021 14:42:10 +0300
> Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> 
> > Add device tree bindings for the ADRF6780 Upconverter.
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> 
> oops. Would have ideally gotten to taking a close look at this before v6 :(
> Sorry about that!  I don't suppose we have any other reviewers whose
> knowledge of
> this sort of hardware is fresher and deeper than mine?  I'd like more eyes on
> the next version of this if possible!
> 
> Jonathan
> 
> > ---
> > no changes in v6
> >  .../bindings/iio/frequency/adi,adrf6780.yaml  | 119 ++++++++++++++++++
> >  1 file changed, 119 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
> >
> > diff --git
> a/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
> b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
> > new file mode 100644
> > index 000000000000..65cb3bee4aca
> > --- /dev/null
> > +++
> b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
> > @@ -0,0 +1,119 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> https://urldefense.com/v3/__http://devicetree.org/schemas/iio/frequency
> /adi,adrf6780.yaml*__;Iw!!A3Ni8CS0y2Y!tq2rTJBBpvfHI71YPxIn96552nFJvLYy
> U6rbHeP_e5sxgwDvLPHhZ_9NMjP_lD2kj1lJ$
> > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!tq2rTJBBpvfHI71YPxIn96552nFJvLY
> yU6rbHeP_e5sxgwDvLPHhZ_9NMjP_lJHL0noG$
> > +
> > +title: ADRF6780 Microwave Upconverter
> > +
> > +maintainers:
> > +  - Antoniu Miclaus <antoniu.miclaus@analog.com>
> > +
> > +description: |
> > +   Wideband, microwave upconverter optimized for point to point
> microwave
> > +   radio designs operating in the 5.9 GHz to 23.6 GHz frequency range.
> > +
> > +   https://www.analog.com/en/products/adrf6780.html
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,adrf6780
> > +
> > +  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
> > +
> > +  adi,vga-buff-en:
> > +    description:
> > +      VGA Buffer Enable.
> 
> Ideally I'd like any acronyms spelt out in the descriptions.
> (I assume this is variable gain amplifier?) The fun question from looking at
> this in the datasheet is where is it in the functional diagram?  I'm not actually
> sure where it might be.  Perhaps in the VVA, so on the VAAT input?
> 
> If you have a convenient path to point out to your datasheet people that it is
> undocumented, that might be good to clean up :)
> My guess is this is needed if the precision reference on the example circuit
> needs
> a high impedance input, but I'm only guessing.
> 
> 
> > +    type: boolean
> > +
> > +  adi,lo-buff-en:
> > +    description:
> > +      LO Buffer Enable.
> 
> What is LO and why might it need a buffer? (or is it always a good idea to turn
> this on
> when using the device?)
> 
> > +    type: boolean
> > +
> > +  adi,if-mode-en:
> > +    description:
> > +      IF Mode Enable.
> IF stands for what? Intermediate Frequency...
> 
> > +    type: boolean
> > +
> > +  adi,iq-mode-en:
> > +    description:
> > +      IQ Mode Enable.
> > +    type: boolean
> 
> I struggled to figure this out from the datasheet, but is there a circumstance
> under which
> if and iq mode might both be enabled? Nothing stops you setting the
> registers that way, but
> it seems to be documented as one or the other...
> 
> For that matter, why probably want to describe this as "baseband IQ mode"
> given datasheet
> refers to as baseband in most places other than the register definition.
> 
> > +
> > +  adi,lo-x2-en:
> > +    description:
> > +      LO x2 Enable.
> > +    type: boolean
> > +
> > +  adi,lo-ppf-en:
> > +    description:
> > +      LO x1 Enable.
> 
> This is curious. I agree that the register write documents it as x1 enable, but it
> seems
> to be be enabling polyphase filters - what exactly their relationship is to x1 is
> not that
> clear to me.  Perhaps one to query with the hardware people if possible!
> 
> > +    type: boolean
> > +
> > +  adi,lo-en:
> > +    description:
> > +      LO Enable.
> > +    type: boolean
> 
> Would you ever have this off whilst running?  It's possible I'm missing
> something, but
> I think you need that frequency path to be enabled to get anything at all to
> happen.
> 
> > +
> > +  adi,uc-bias-en:
> > +    description:
> > +      UC Bias Enable.
> > +    type: boolean
> 
> This being completely undocumented apart from the register setting, I have
> 0 idea what
> it actually is.   Any chance we can get some more details?
> 
> > +
> > +  adi,lo-sideband:
> > +    description:
> > +      Switch to the Other LO Sideband.
> 
> Switch what to the other LO sideband?
> 
> > +    type: boolean
> > +
> > +  adi,vdet-out-en:
> 
> So my reading of the datasheet on this one didn't lead me to a completely
> clear answer.
> Does turning this one effectively stop you using the internal ADC to measure
> the power
> whilst exposing the signal on an external pin?
> 
> > +    description:
> > +      VDET Output Select Enable.
> > +    type: boolean
> > +
> > +  '#clock-cells':
> > +    const: 0
> > +
> > +dependencies:
> > +  adi,lo-x2-en: [ "adi,lo-en" ]
> > +  adi,lo-ppf-en: [ "adi,lo-en" ]
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    spi {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      adrf6780@0 {
> > +        compatible = "adi,adrf6780";
> > +        reg = <0>;
> > +        spi-max-frequency = <1000000>;
> > +        clocks = <&adrf6780_lo>;
> > +        clock-names = "lo_in";
> > +      };
> > +    };
> > +...

Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[Andy Shevchenko]

On Tue, Jul 20, 2021 at 4:17 PM Miclaus, Antoniu
<Antoniu.Miclaus@analog.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Friday, July 16, 2021 5:53 PM
> > On Fri, Jul 16, 2021 at 2:43 PM Antoniu Miclaus
> > <antoniu.miclaus@analog.com> wrote:

...

> > First question is why not to use the regmap API (I have heard it has
> > gained support of 17 bit)?
>
> Initially that was the plan, but after this patch:
> https://github.com/torvalds/linux/commit/4191f19792bf91267835eb090d970e9cd6277a65
> the custom write formats for regmap allow the read only via cached registers.
>
> Therefore, I preferred using spi transfers for write/read to/from the device.

Not sure I follow you. That patch is upstream. Does it prevent you
from switching to regmap SPI API?

...

> > > +        depends on COMMON_CLK
> >
> > Is it mandatory for any function inside the device?
>
> Yes. It will serve as LO input to the device.

But can the device work without it (with limited functionality)?

--
With Best Regards,
Andy Shevchenko

RE: [PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[Miclaus, Antoniu]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Tuesday, July 20, 2021 5:08 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> jic23@kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
> 
> [External]
> 
> On Tue, Jul 20, 2021 at 4:17 PM Miclaus, Antoniu
> <Antoniu.Miclaus@analog.com> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Friday, July 16, 2021 5:53 PM
> > > On Fri, Jul 16, 2021 at 2:43 PM Antoniu Miclaus
> > > <antoniu.miclaus@analog.com> wrote:
> 
> ...
> 
> > > First question is why not to use the regmap API (I have heard it has
> > > gained support of 17 bit)?
> >
> > Initially that was the plan, but after this patch:
> >
> https://urldefense.com/v3/__https://github.com/torvalds/linux/commit/41
> 91f19792bf91267835eb090d970e9cd6277a65__;!!A3Ni8CS0y2Y!ptUV0YC2nfD
> 6AdH_y5U0ziyELl4B9pDL0ubkdpFHFtrFNE_NqUS_TWm_gE-SlHV315Ak$
> > the custom write formats for regmap allow the read only via cached
> registers.
> >
> > Therefore, I preferred using spi transfers for write/read to/from the
> device.
> 
> Not sure I follow you. That patch is upstream. Does it prevent you
> from switching to regmap SPI API?

It does not prevent me from switching to regmap SPI API.
It will prevent me from using regmap_read to read directly from device.
https://github.com/torvalds/linux/commit/4191f19792bf91267835eb090d970e9cd6277a65#diff-3e0b7d2f0a55adb6573693a514cb8024a81a55da848cb22632ab5fd4dd6dd4e4R39

> 
> ...
> 
> > > > +        depends on COMMON_CLK
> > >
> > > Is it mandatory for any function inside the device?
> >
> > Yes. It will serve as LO input to the device.
> 
> But can the device work without it (with limited functionality)?
This aspect is not clearly stated in the datasheet, but since the Enable register has the LO enabled by default, I considered this aspect as being mandatory.
I will try to find out more information from the hardware guys who developed the part.

> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[Andy Shevchenko]

On Tue, Jul 20, 2021 at 5:33 PM Miclaus, Antoniu
<Antoniu.Miclaus@analog.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Tuesday, July 20, 2021 5:08 PM
> > On Tue, Jul 20, 2021 at 4:17 PM Miclaus, Antoniu
> > <Antoniu.Miclaus@analog.com> wrote:
> > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Sent: Friday, July 16, 2021 5:53 PM
> > > > On Fri, Jul 16, 2021 at 2:43 PM Antoniu Miclaus
> > > > <antoniu.miclaus@analog.com> wrote:

...

> > > > First question is why not to use the regmap API (I have heard it has
> > > > gained support of 17 bit)?
> > >
> > > Initially that was the plan, but after this patch:
> > >
> > https://urldefense.com/v3/__https://github.com/torvalds/linux/commit/41
> > 91f19792bf91267835eb090d970e9cd6277a65__;!!A3Ni8CS0y2Y!ptUV0YC2nfD
> > 6AdH_y5U0ziyELl4B9pDL0ubkdpFHFtrFNE_NqUS_TWm_gE-SlHV315Ak$
> > > the custom write formats for regmap allow the read only via cached
> > registers.
> > >
> > > Therefore, I preferred using spi transfers for write/read to/from the
> > device.
> >
> > Not sure I follow you. That patch is upstream. Does it prevent you
> > from switching to regmap SPI API?
>
> It does not prevent me from switching to regmap SPI API.
> It will prevent me from using regmap_read to read directly from device.
> https://github.com/torvalds/linux/commit/4191f19792bf91267835eb090d970e9cd6277a65#diff-3e0b7d2f0a55adb6573693a514cb8024a81a55da848cb22632ab5fd4dd6dd4e4R39

How? If it's the case it's a huge regression in the API and must be fixed ASAP.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780

[Jonathan Cameron]

On Tue, 20 Jul 2021 13:34:08 +0000
"Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:

> Hello Jonathan,
...
> > > +
> > > +	enable_reg = FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, dev-
> > >vga_buff_en) |
> > > +			FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, 1) |
> > > +			FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, dev-
> > >lo_buff_en) |
> > > +			FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, dev-
> > >if_mode_en) |
> > > +			FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, dev-
> > >iq_mode_en) |
> > > +			FIELD_PREP(ADRF6780_LO_X2_EN_MSK, dev-
> > >lo_x2_en) |
> > > +			FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, dev-
> > >lo_ppf_en) |
> > > +			FIELD_PREP(ADRF6780_LO_EN_MSK, dev->lo_en) |
> > > +			FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, dev-
> > >uc_bias_en);  
> > 
> > Here we are probably turning on a bunch of stuff which will result in power
> > usage.
> > Would it be sensible to turn it off again in remove path?  (devm managed
> > should be fine).  
> 
> Most of these attributes are enabled by default after device reset.
> Taking into account this statement, is it still worth adding a 'custom' remove path?

Perhaps a nice to have if they save power.
Got to love hardware that eats power until you load a driver!

Ah well,

Jonathan