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 <[email protected]>
---
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 <[email protected]");
+MODULE_DESCRIPTION("Analog Devices ADRF6780");
+MODULE_LICENSE("GPL v2");
--
2.32.0
Add device tree bindings for the ADRF6780 Upconverter.
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Antoniu Miclaus <[email protected]>
---
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 <[email protected]>
+
+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
On Fri, Jul 16, 2021 at 2:43 PM Antoniu Miclaus
<[email protected]> 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 <[email protected]>
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
On Fri, 16 Jul 2021 14:42:09 +0300
Antoniu Miclaus <[email protected]> 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 <[email protected]>
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");
> +}
On Fri, 16 Jul 2021 14:42:09 +0300
Antoniu Miclaus <[email protected]> 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 <[email protected]>
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 <[email protected]");
> +MODULE_DESCRIPTION("Analog Devices ADRF6780");
> +MODULE_LICENSE("GPL v2");
On Fri, 16 Jul 2021 14:42:10 +0300
Antoniu Miclaus <[email protected]> wrote:
> Add device tree bindings for the ADRF6780 Upconverter.
>
> Reviewed-by: Rob Herring <[email protected]>
> Signed-off-by: Antoniu Miclaus <[email protected]>
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 <[email protected]>
> +
> +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";
> + };
> + };
> +...
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 <[email protected]>
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/[email protected]
Hello Jonathan,
> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Saturday, July 17, 2021 4:26 PM
> To: Miclaus, Antoniu <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]
> 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 <[email protected]> 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 <[email protected]>
> 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?
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 <[email protected]>
> Sent: Saturday, July 17, 2021 5:22 PM
> To: Miclaus, Antoniu <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Rob Herring <[email protected]>
> 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 <[email protected]> wrote:
>
> > Add device tree bindings for the ADRF6780 Upconverter.
> >
> > Reviewed-by: Rob Herring <[email protected]>
> > Signed-off-by: Antoniu Miclaus <[email protected]>
>
> 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 <[email protected]>
> > +
> > +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";
> > + };
> > + };
> > +...
On Tue, Jul 20, 2021 at 4:17 PM Miclaus, Antoniu
<[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Friday, July 16, 2021 5:53 PM
> > On Fri, Jul 16, 2021 at 2:43 PM Antoniu Miclaus
> > <[email protected]> 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
Hello Andy,
Thanks for the review:
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Friday, July 16, 2021 5:53 PM
> To: Miclaus, Antoniu <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]
> 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
> <[email protected]> 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 <[email protected]>
>
> 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
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Tuesday, July 20, 2021 5:08 PM
> To: Miclaus, Antoniu <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]
> 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
> <[email protected]> wrote:
> > > From: Andy Shevchenko <[email protected]>
> > > Sent: Friday, July 16, 2021 5:53 PM
> > > On Fri, Jul 16, 2021 at 2:43 PM Antoniu Miclaus
> > > <[email protected]> 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
On Tue, Jul 20, 2021 at 5:33 PM Miclaus, Antoniu
<[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Tuesday, July 20, 2021 5:08 PM
> > On Tue, Jul 20, 2021 at 4:17 PM Miclaus, Antoniu
> > <[email protected]> wrote:
> > > > From: Andy Shevchenko <[email protected]>
> > > > Sent: Friday, July 16, 2021 5:53 PM
> > > > On Fri, Jul 16, 2021 at 2:43 PM Antoniu Miclaus
> > > > <[email protected]> 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
On Tue, 20 Jul 2021 13:34:08 +0000
"Miclaus, Antoniu" <[email protected]> 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