* [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013
@ 2021-11-01 10:04 Antoniu Miclaus
2021-11-01 10:04 ` [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc Antoniu Miclaus
2021-11-02 10:09 ` [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013 Sa, Nuno
0 siblings, 2 replies; 15+ messages in thread
From: Antoniu Miclaus @ 2021-11-01 10:04 UTC (permalink / raw)
To: jic23, robh+dt, linux-iio, devicetree, linux-kernel
Cc: nuno.sa, Antoniu Miclaus
The ADMV1013 is a wideband, microwave upconverter optimized
for point to point microwave radio designs operating in the
24 GHz to 44 GHz radio frequency (RF) range.
Datasheet:
https://www.analog.com/media/en/technical-documentation/data-sheets/ADMV1013.pdf
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
changes in v3:
- fix includes
- make masks more generic where possible
- improve mutex attribute description
- rework IIO channels and remove IIO_VAL_INT_MULTIPLE return
- use HZ_PER_MHZ macro
- improve readability in the `int admv1013_reg_access` function
- use `devm_clk_notifier_registers()`
---
drivers/iio/frequency/Kconfig | 11 +
drivers/iio/frequency/Makefile | 1 +
drivers/iio/frequency/admv1013.c | 571 +++++++++++++++++++++++++++++++
3 files changed, 583 insertions(+)
create mode 100644 drivers/iio/frequency/admv1013.c
diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
index 240b81502512..411b3b961e46 100644
--- a/drivers/iio/frequency/Kconfig
+++ b/drivers/iio/frequency/Kconfig
@@ -49,5 +49,16 @@ config ADF4371
To compile this driver as a module, choose M here: the
module will be called adf4371.
+
+config ADMV1013
+ tristate "Analog Devices ADMV1013 Microwave Upconverter"
+ depends on SPI && COMMON_CLK
+ help
+ Say yes here to build support for Analog Devices ADMV1013
+ 24 GHz to 44 GHz, Wideband, Microwave Upconverter.
+
+ To compile this driver as a module, choose M here: the
+ module will be called admv1013.
+
endmenu
endmenu
diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
index 518b1e50caef..559922a8196e 100644
--- a/drivers/iio/frequency/Makefile
+++ b/drivers/iio/frequency/Makefile
@@ -7,3 +7,4 @@
obj-$(CONFIG_AD9523) += ad9523.o
obj-$(CONFIG_ADF4350) += adf4350.o
obj-$(CONFIG_ADF4371) += adf4371.o
+obj-$(CONFIG_ADMV1013) += admv1013.o
diff --git a/drivers/iio/frequency/admv1013.c b/drivers/iio/frequency/admv1013.c
new file mode 100644
index 000000000000..6451a2cc7c52
--- /dev/null
+++ b/drivers/iio/frequency/admv1013.c
@@ -0,0 +1,571 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ADMV1013 driver
+ *
+ * Copyright 2021 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/notifier.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/units.h>
+
+#include <asm/unaligned.h>
+
+/* ADMV1013 Register Map */
+#define ADMV1013_REG_SPI_CONTROL 0x00
+#define ADMV1013_REG_ALARM 0x01
+#define ADMV1013_REG_ALARM_MASKS 0x02
+#define ADMV1013_REG_ENABLE 0x03
+#define ADMV1013_REG_LO_AMP_I 0x05
+#define ADMV1013_REG_LO_AMP_Q 0x06
+#define ADMV1013_REG_OFFSET_ADJUST_I 0x07
+#define ADMV1013_REG_OFFSET_ADJUST_Q 0x08
+#define ADMV1013_REG_QUAD 0x09
+#define ADMV1013_REG_VVA_TEMP_COMP 0x0A
+
+/* ADMV1013_REG_SPI_CONTROL Map */
+#define ADMV1013_PARITY_EN_MSK BIT(15)
+#define ADMV1013_SPI_SOFT_RESET_MSK BIT(14)
+#define ADMV1013_CHIP_ID_MSK GENMASK(11, 4)
+#define ADMV1013_CHIP_ID 0xA
+#define ADMV1013_REVISION_ID_MSK GENMASK(3, 0)
+
+/* ADMV1013_REG_ALARM Map */
+#define ADMV1013_PARITY_ERROR_MSK BIT(15)
+#define ADMV1013_TOO_FEW_ERRORS_MSK BIT(14)
+#define ADMV1013_TOO_MANY_ERRORS_MSK BIT(13)
+#define ADMV1013_ADDRESS_RANGE_ERROR_MSK BIT(12)
+
+/* ADMV1013_REG_ENABLE Map */
+#define ADMV1013_VGA_PD_MSK BIT(15)
+#define ADMV1013_MIXER_PD_MSK BIT(14)
+#define ADMV1013_QUAD_PD_MSK GENMASK(13, 11)
+#define ADMV1013_BG_PD_MSK BIT(10)
+#define ADMV1013_MIXER_IF_EN_MSK BIT(7)
+#define ADMV1013_DET_EN_MSK BIT(5)
+
+/* ADMV1013_REG_LO_AMP Map */
+#define ADMV1013_LOAMP_PH_ADJ_FINE_MSK GENMASK(13, 7)
+#define ADMV1013_MIXER_VGATE_MSK GENMASK(6, 0)
+
+/* ADMV1013_REG_OFFSET_ADJUST Map */
+#define ADMV1013_MIXER_OFF_ADJ_P_MSK GENMASK(15, 9)
+#define ADMV1013_MIXER_OFF_ADJ_N_MSK GENMASK(8, 2)
+
+/* ADMV1013_REG_QUAD Map */
+#define ADMV1013_QUAD_SE_MODE_MSK GENMASK(9, 6)
+#define ADMV1013_QUAD_FILTERS_MSK GENMASK(3, 0)
+
+/* ADMV1013_REG_VVA_TEMP_COMP Map */
+#define ADMV1013_VVA_TEMP_COMP_MSK GENMASK(15, 0)
+
+struct admv1013_state {
+ struct spi_device *spi;
+ struct clk *clkin;
+ /* Protect against concurrent accesses to the device and to data */
+ struct mutex lock;
+ struct regulator *reg;
+ struct notifier_block nb;
+ unsigned int quad_se_mode;
+ bool vga_pd;
+ bool mixer_pd;
+ bool quad_pd;
+ bool bg_pd;
+ bool mixer_if_en;
+ bool det_en;
+ u8 data[3] ____cacheline_aligned;
+};
+
+static int __admv1013_spi_read(struct admv1013_state *st, unsigned int reg,
+ unsigned int *val)
+{
+ int ret;
+ struct spi_transfer t = {0};
+
+ st->data[0] = 0x80 | (reg << 1);
+ st->data[1] = 0x0;
+ st->data[2] = 0x0;
+
+ t.rx_buf = &st->data[0];
+ t.tx_buf = &st->data[0];
+ t.len = 3;
+
+ ret = spi_sync_transfer(st->spi, &t, 1);
+ if (ret)
+ return ret;
+
+ *val = (get_unaligned_be24(&st->data[0]) >> 1) & GENMASK(15, 0);
+
+ return ret;
+}
+
+static int admv1013_spi_read(struct admv1013_state *st, unsigned int reg,
+ unsigned int *val)
+{
+ int ret;
+
+ mutex_lock(&st->lock);
+ ret = __admv1013_spi_read(st, reg, val);
+ mutex_unlock(&st->lock);
+
+ return ret;
+}
+
+static int __admv1013_spi_write(struct admv1013_state *st,
+ unsigned int reg,
+ unsigned int val)
+{
+ put_unaligned_be24((val << 1) | (reg << 17), &st->data[0]);
+
+ return spi_write(st->spi, &st->data[0], 3);
+}
+
+static int admv1013_spi_write(struct admv1013_state *st, unsigned int reg,
+ unsigned int val)
+{
+ int ret;
+
+ mutex_lock(&st->lock);
+ ret = __admv1013_spi_write(st, reg, val);
+ mutex_unlock(&st->lock);
+
+ return ret;
+}
+
+static int __admv1013_spi_update_bits(struct admv1013_state *st, unsigned int reg,
+ unsigned int mask, unsigned int val)
+{
+ int ret;
+ unsigned int data, temp;
+
+ ret = __admv1013_spi_read(st, reg, &data);
+ if (ret)
+ return ret;
+
+ temp = (data & ~mask) | (val & mask);
+
+ return __admv1013_spi_write(st, reg, temp);
+}
+
+static int admv1013_spi_update_bits(struct admv1013_state *st, unsigned int reg,
+ unsigned int mask, unsigned int val)
+{
+ int ret;
+
+ mutex_lock(&st->lock);
+ ret = __admv1013_spi_update_bits(st, reg, mask, val);
+ mutex_unlock(&st->lock);
+
+ return ret;
+}
+
+static int admv1013_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long info)
+{
+ struct admv1013_state *st = iio_priv(indio_dev);
+ unsigned int data, addr;
+ int ret;
+
+ switch (info) {
+ case IIO_CHAN_INFO_CALIBBIAS:
+ if (chan->channel2 == IIO_MOD_I)
+ addr = ADMV1013_REG_OFFSET_ADJUST_I;
+ else
+ addr = ADMV1013_REG_OFFSET_ADJUST_Q;
+
+ ret = admv1013_spi_read(st, addr, &data);
+ if (ret)
+ return ret;
+
+ if (!(chan->channel))
+ *val = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_P_MSK, data);
+ else
+ *val = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_N_MSK, data);
+
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_PHASE:
+ if (chan->channel2 == IIO_MOD_I)
+ addr = ADMV1013_REG_LO_AMP_I;
+ else
+ addr = ADMV1013_REG_LO_AMP_Q;
+
+ ret = admv1013_spi_read(st, addr, &data);
+ if (ret)
+ return ret;
+
+ *val = FIELD_GET(ADMV1013_LOAMP_PH_ADJ_FINE_MSK, data);
+
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int admv1013_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long info)
+{
+ struct admv1013_state *st = iio_priv(indio_dev);
+ int ret;
+
+ switch (info) {
+ case IIO_CHAN_INFO_CALIBBIAS:
+ if (chan->channel2 == IIO_MOD_I) {
+ if (!(chan->channel))
+ ret = admv1013_spi_update_bits(st, ADMV1013_REG_OFFSET_ADJUST_I,
+ ADMV1013_MIXER_OFF_ADJ_P_MSK,
+ FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_P_MSK, val));
+ else
+ ret = admv1013_spi_update_bits(st, ADMV1013_REG_OFFSET_ADJUST_I,
+ ADMV1013_MIXER_OFF_ADJ_N_MSK,
+ FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_N_MSK, val));
+ } else {
+ if (!(chan->channel))
+ ret = admv1013_spi_update_bits(st, ADMV1013_REG_OFFSET_ADJUST_Q,
+ ADMV1013_MIXER_OFF_ADJ_P_MSK,
+ FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_P_MSK, val));
+ else
+ ret = admv1013_spi_update_bits(st, ADMV1013_REG_OFFSET_ADJUST_Q,
+ ADMV1013_MIXER_OFF_ADJ_N_MSK,
+ FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_N_MSK, val));
+ }
+
+ return ret;
+ case IIO_CHAN_INFO_PHASE:
+ if (chan->channel2 == IIO_MOD_I)
+ return admv1013_spi_update_bits(st, ADMV1013_REG_LO_AMP_I,
+ ADMV1013_LOAMP_PH_ADJ_FINE_MSK,
+ FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_FINE_MSK, val));
+ else
+ return admv1013_spi_update_bits(st, ADMV1013_REG_LO_AMP_Q,
+ ADMV1013_LOAMP_PH_ADJ_FINE_MSK,
+ FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_FINE_MSK, val));
+ default:
+ return -EINVAL;
+ }
+}
+
+static int admv1013_update_quad_filters(struct admv1013_state *st)
+{
+ unsigned int filt_raw;
+ u64 rate = clk_get_rate(st->clkin);
+
+ if (rate >= (5400 * HZ_PER_MHZ) && rate <= (7000 * HZ_PER_MHZ))
+ filt_raw = 15;
+ else if (rate >= (5400 * HZ_PER_MHZ) && rate <= (8000 * HZ_PER_MHZ))
+ filt_raw = 10;
+ else if (rate >= (6600 * HZ_PER_MHZ) && rate <= (9200 * HZ_PER_MHZ))
+ filt_raw = 5;
+ else
+ filt_raw = 0;
+
+ return __admv1013_spi_update_bits(st, ADMV1013_REG_QUAD,
+ ADMV1013_QUAD_FILTERS_MSK,
+ FIELD_PREP(ADMV1013_QUAD_FILTERS_MSK, filt_raw));
+}
+
+static int admv1013_update_mixer_vgate(struct admv1013_state *st)
+{
+ unsigned int vcm, mixer_vgate;
+
+ vcm = regulator_get_voltage(st->reg);
+
+ if (vcm >= 0 && vcm < 1800000)
+ mixer_vgate = (2389 * vcm / 1000000 + 8100) / 100;
+ else if (vcm > 1800000 && vcm < 2600000)
+ mixer_vgate = (2375 * vcm / 1000000 + 125) / 100;
+ else
+ return -EINVAL;
+
+ return __admv1013_spi_update_bits(st, ADMV1013_REG_LO_AMP_I,
+ ADMV1013_MIXER_VGATE_MSK,
+ FIELD_PREP(ADMV1013_MIXER_VGATE_MSK, mixer_vgate));
+}
+
+static int admv1013_reg_access(struct iio_dev *indio_dev,
+ unsigned int reg,
+ unsigned int write_val,
+ unsigned int *read_val)
+{
+ struct admv1013_state *st = iio_priv(indio_dev);
+
+ if (read_val)
+ return admv1013_spi_read(st, reg, read_val);
+ else
+ return admv1013_spi_write(st, reg, write_val);
+}
+
+static const struct iio_info admv1013_info = {
+ .read_raw = admv1013_read_raw,
+ .write_raw = admv1013_write_raw,
+ .debugfs_reg_access = &admv1013_reg_access,
+};
+
+static int admv1013_freq_change(struct notifier_block *nb, unsigned long action, void *data)
+{
+ struct admv1013_state *st = container_of(nb, struct admv1013_state, nb);
+ int ret;
+
+ if (action == POST_RATE_CHANGE) {
+ mutex_lock(&st->lock);
+ ret = notifier_from_errno(admv1013_update_quad_filters(st));
+ mutex_unlock(&st->lock);
+ return ret;
+ }
+
+ return NOTIFY_OK;
+}
+
+#define ADMV1013_CHAN_PHASE(_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) \
+ }
+
+#define ADMV1013_CHAN_CALIB(_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_CALIBBIAS) \
+ }
+
+static const struct iio_chan_spec admv1013_channels[] = {
+ ADMV1013_CHAN_PHASE(0, I),
+ ADMV1013_CHAN_PHASE(0, Q),
+ ADMV1013_CHAN_CALIB(0, I),
+ ADMV1013_CHAN_CALIB(0, Q),
+ ADMV1013_CHAN_CALIB(1, I),
+ ADMV1013_CHAN_CALIB(1, Q),
+};
+
+static int admv1013_init(struct admv1013_state *st)
+{
+ int ret;
+ unsigned int chip_id, enable_reg, enable_reg_msk;
+ struct spi_device *spi = st->spi;
+
+ /* Perform a software reset */
+ ret = __admv1013_spi_update_bits(st, ADMV1013_REG_SPI_CONTROL,
+ ADMV1013_SPI_SOFT_RESET_MSK,
+ FIELD_PREP(ADMV1013_SPI_SOFT_RESET_MSK, 1));
+ if (ret)
+ return ret;
+
+ ret = __admv1013_spi_update_bits(st, ADMV1013_REG_SPI_CONTROL,
+ ADMV1013_SPI_SOFT_RESET_MSK,
+ FIELD_PREP(ADMV1013_SPI_SOFT_RESET_MSK, 0));
+ if (ret)
+ return ret;
+
+ ret = __admv1013_spi_read(st, ADMV1013_REG_SPI_CONTROL, &chip_id);
+ if (ret)
+ return ret;
+
+ chip_id = FIELD_GET(ADMV1013_CHIP_ID_MSK, chip_id);
+ if (chip_id != ADMV1013_CHIP_ID) {
+ dev_err(&spi->dev, "Invalid Chip ID.\n");
+ return -EINVAL;
+ }
+
+ ret = __admv1013_spi_write(st, ADMV1013_REG_VVA_TEMP_COMP, 0xE700);
+ if (ret)
+ return ret;
+
+ ret = __admv1013_spi_update_bits(st, ADMV1013_REG_QUAD,
+ ADMV1013_QUAD_SE_MODE_MSK,
+ FIELD_PREP(ADMV1013_QUAD_SE_MODE_MSK, st->quad_se_mode));
+ if (ret)
+ return ret;
+
+ ret = admv1013_update_mixer_vgate(st);
+ if (ret)
+ return ret;
+
+ ret = admv1013_update_quad_filters(st);
+ if (ret)
+ return ret;
+
+ enable_reg_msk = ADMV1013_VGA_PD_MSK |
+ ADMV1013_MIXER_PD_MSK |
+ ADMV1013_QUAD_PD_MSK |
+ ADMV1013_BG_PD_MSK |
+ ADMV1013_MIXER_IF_EN_MSK |
+ ADMV1013_DET_EN_MSK;
+
+ enable_reg = FIELD_PREP(ADMV1013_VGA_PD_MSK, st->vga_pd) |
+ FIELD_PREP(ADMV1013_MIXER_PD_MSK, st->mixer_pd) |
+ FIELD_PREP(ADMV1013_QUAD_PD_MSK, st->quad_pd ? 7 : 0) |
+ FIELD_PREP(ADMV1013_BG_PD_MSK, st->bg_pd) |
+ FIELD_PREP(ADMV1013_MIXER_IF_EN_MSK, st->mixer_if_en) |
+ FIELD_PREP(ADMV1013_DET_EN_MSK, st->det_en);
+
+ return __admv1013_spi_update_bits(st, ADMV1013_REG_ENABLE, enable_reg_msk, enable_reg);
+}
+
+static void admv1013_clk_disable(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
+static void admv1013_reg_disable(void *data)
+{
+ regulator_disable(data);
+}
+
+static void admv1013_powerdown(void *data)
+{
+ unsigned int enable_reg, enable_reg_msk;
+
+ /* Disable all components in the Enable Register */
+ enable_reg_msk = ADMV1013_VGA_PD_MSK |
+ ADMV1013_MIXER_PD_MSK |
+ ADMV1013_QUAD_PD_MSK |
+ ADMV1013_BG_PD_MSK |
+ ADMV1013_MIXER_IF_EN_MSK |
+ ADMV1013_DET_EN_MSK;
+
+ enable_reg = FIELD_PREP(ADMV1013_VGA_PD_MSK, 1) |
+ FIELD_PREP(ADMV1013_MIXER_PD_MSK, 1) |
+ FIELD_PREP(ADMV1013_QUAD_PD_MSK, 7) |
+ FIELD_PREP(ADMV1013_BG_PD_MSK, 1) |
+ FIELD_PREP(ADMV1013_MIXER_IF_EN_MSK, 0) |
+ FIELD_PREP(ADMV1013_DET_EN_MSK, 0);
+
+ admv1013_spi_update_bits(data, ADMV1013_REG_ENABLE, enable_reg_msk, enable_reg);
+}
+
+static int admv1013_properties_parse(struct admv1013_state *st)
+{
+ int ret;
+ struct spi_device *spi = st->spi;
+
+ st->vga_pd = device_property_read_bool(&spi->dev, "adi,vga-powerdown");
+ st->mixer_pd = device_property_read_bool(&spi->dev, "adi,mixer-powerdown");
+ st->quad_pd = device_property_read_bool(&spi->dev, "adi,quad-powerdown");
+ st->bg_pd = device_property_read_bool(&spi->dev, "adi,bg-powerdown");
+ st->mixer_if_en = device_property_read_bool(&spi->dev, "adi,mixer-if-enable");
+ st->det_en = device_property_read_bool(&spi->dev, "adi,detector-enable");
+
+ ret = device_property_read_u32(&spi->dev, "adi,quad-se-mode", &st->quad_se_mode);
+ if (ret)
+ st->quad_se_mode = 12;
+
+ st->reg = devm_regulator_get(&spi->dev, "vcm");
+ if (IS_ERR(st->reg))
+ return dev_err_probe(&spi->dev, PTR_ERR(st->reg),
+ "failed to get the common-mode voltage\n");
+
+ st->clkin = devm_clk_get(&spi->dev, "lo_in");
+ if (IS_ERR(st->clkin))
+ return dev_err_probe(&spi->dev, PTR_ERR(st->clkin),
+ "failed to get the LO input clock\n");
+
+ return 0;
+}
+
+static int admv1013_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct admv1013_state *st;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ st = iio_priv(indio_dev);
+
+ indio_dev->info = &admv1013_info;
+ indio_dev->name = "admv1013";
+ indio_dev->channels = admv1013_channels;
+ indio_dev->num_channels = ARRAY_SIZE(admv1013_channels);
+
+ st->spi = spi;
+
+ ret = admv1013_properties_parse(st);
+ if (ret)
+ return ret;
+
+ ret = regulator_enable(st->reg);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to enable specified Common-Mode Voltage!\n");
+ return ret;
+ }
+
+ ret = devm_add_action_or_reset(&spi->dev, admv1013_reg_disable,
+ st->reg);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(st->clkin);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(&spi->dev, admv1013_clk_disable, st->clkin);
+ if (ret)
+ return ret;
+
+ st->nb.notifier_call = admv1013_freq_change;
+ ret = devm_clk_notifier_register(&spi->dev, st->clkin, &st->nb);
+ if (ret)
+ return ret;
+
+ mutex_init(&st->lock);
+
+ ret = admv1013_init(st);
+ if (ret) {
+ dev_err(&spi->dev, "admv1013 init failed\n");
+ return ret;
+ }
+
+ ret = devm_add_action_or_reset(&spi->dev, admv1013_powerdown, st);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id admv1013_id[] = {
+ { "admv1013", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(spi, admv1013_id);
+
+static const struct of_device_id admv1013_of_match[] = {
+ { .compatible = "adi,admv1013" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, admv1013_of_match);
+
+static struct spi_driver admv1013_driver = {
+ .driver = {
+ .name = "admv1013",
+ .of_match_table = admv1013_of_match,
+ },
+ .probe = admv1013_probe,
+ .id_table = admv1013_id,
+};
+module_spi_driver(admv1013_driver);
+
+MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com");
+MODULE_DESCRIPTION("Analog Devices ADMV1013");
+MODULE_LICENSE("GPL v2");
--
2.33.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
2021-11-01 10:04 [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013 Antoniu Miclaus
@ 2021-11-01 10:04 ` Antoniu Miclaus
2021-11-02 17:50 ` Rob Herring
2021-11-02 10:09 ` [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013 Sa, Nuno
1 sibling, 1 reply; 15+ messages in thread
From: Antoniu Miclaus @ 2021-11-01 10:04 UTC (permalink / raw)
To: jic23, robh+dt, linux-iio, devicetree, linux-kernel
Cc: nuno.sa, Antoniu Miclaus
Add device tree bindings for the ADMV1013 Upconverter.
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
.../bindings/iio/frequency/adi,admv1013.yaml | 119 ++++++++++++++++++
1 file changed, 119 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
new file mode 100644
index 000000000000..47993253a586
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.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,admv1013.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADMV1013 Microwave Upconverter
+
+maintainers:
+ - Antoniu Miclaus <antoniu.miclaus@analog.com>
+
+description: |
+ Wideband, microwave upconverter optimized for point to point microwave
+ radio designs operating in the 24 GHz to 44 GHz frequency range.
+
+ https://www.analog.com/en/products/admv1013.html
+
+properties:
+ compatible:
+ enum:
+ - adi,admv1013
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 1000000
+
+ clocks:
+ description:
+ Definition of the external clock.
+ minItems: 1
+
+ clock-names:
+ items:
+ - const: lo_in
+
+ clock-output-names:
+ maxItems: 1
+
+ vcm-supply:
+ description:
+ Analog voltage regulator.
+
+ adi,vga-powerdown:
+ description:
+ Power Down the Voltage Gain Amplifier Circuit available at
+ BG_RBIAS2 pin.
+ type: boolean
+
+ adi,mixer-powerdown:
+ description:
+ Power Down the Mixer Circuit. Enable to put the block in
+ a power down state.
+ type: boolean
+
+ adi,quad-powerdown:
+ description:
+ Power Down the Quadrupler. Enable to put the block in
+ a power down state.
+ type: boolean
+
+ adi,bg-powerdown:
+ description:
+ Power Down the Transmitter Band Gap. Enable to put the part in
+ a power down state.
+ type: boolean
+
+ adi,mixer-if-enable:
+ description:
+ Enable the Intermediate Frequency Mode. Either IF Mode or I/Q Mode
+ can be enabled at a time.
+ type: boolean
+
+ adi,detector-enable:
+ description:
+ Enable the Envelope Detector available at output pins VENV_P and
+ VENV_N. Disable to reduce power consumption.
+ type: boolean
+
+ adi,quad-se-mode:
+ description:
+ Switch the LO path from differential to single-ended operation.
+ Set value 6 for Single-Ended Mode, Negative Side Disabled.
+ Set value 9 for Single-Ended Mode, Positive Side Disabled.
+ Set value 12 for Differential Mode.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [6, 9, 12]
+
+ '#clock-cells':
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - vcm-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ admv1013@0{
+ compatible = "adi,admv1013";
+ reg = <0>;
+ spi-max-frequency = <1000000>;
+ clocks = <&admv1013_lo>;
+ clock-names = "lo_in";
+ vcm-supply = <&vcm>;
+ adi,quad-se-mode = <12>;
+ adi,mixer-if-enable;
+ adi,detector-enable;
+ };
+ };
+...
--
2.33.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* RE: [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-11-01 10:04 [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013 Antoniu Miclaus
2021-11-01 10:04 ` [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc Antoniu Miclaus
@ 2021-11-02 10:09 ` Sa, Nuno
2021-11-03 20:03 ` Jonathan Cameron
1 sibling, 1 reply; 15+ messages in thread
From: Sa, Nuno @ 2021-11-02 10:09 UTC (permalink / raw)
To: Miclaus, Antoniu, jic23, robh+dt, linux-iio, devicetree, linux-kernel
Cc: Miclaus, Antoniu
> From: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Sent: Monday, November 1, 2021 11:04 AM
> To: jic23@kernel.org; robh+dt@kernel.org; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Sa, Nuno <Nuno.Sa@analog.com>; Miclaus, Antoniu
> <Antoniu.Miclaus@analog.com>
> Subject: [PATCH v3 1/2] iio: frequency: admv1013: add support for
> ADMV1013
>
> The ADMV1013 is a wideband, microwave upconverter optimized
> for point to point microwave radio designs operating in the
> 24 GHz to 44 GHz radio frequency (RF) range.
>
> Datasheet:
> https://www.analog.com/media/en/technical-documentation/data-
> sheets/ADMV1013.pdf
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> changes in v3:
> - fix includes
> - make masks more generic where possible
> - improve mutex attribute description
> - rework IIO channels and remove IIO_VAL_INT_MULTIPLE return
> - use HZ_PER_MHZ macro
> - improve readability in the `int admv1013_reg_access` function
> - use `devm_clk_notifier_registers()`
> ---
> drivers/iio/frequency/Kconfig | 11 +
> drivers/iio/frequency/Makefile | 1 +
> drivers/iio/frequency/admv1013.c | 571
> +++++++++++++++++++++++++++++++
> 3 files changed, 583 insertions(+)
> create mode 100644 drivers/iio/frequency/admv1013.c
>
> diff --git a/drivers/iio/frequency/Kconfig
> b/drivers/iio/frequency/Kconfig
> index 240b81502512..411b3b961e46 100644
> --- a/drivers/iio/frequency/Kconfig
> +++ b/drivers/iio/frequency/Kconfig
> @@ -49,5 +49,16 @@ config ADF4371
>
> To compile this driver as a module, choose M here: the
> module will be called adf4371.
> +
> +config ADMV1013
> + tristate "Analog Devices ADMV1013 Microwave Upconverter"
> + depends on SPI && COMMON_CLK
> + help
> + Say yes here to build support for Analog Devices ADMV1013
> + 24 GHz to 44 GHz, Wideband, Microwave Upconverter.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called admv1013.
> +
> endmenu
> endmenu
> diff --git a/drivers/iio/frequency/Makefile
> b/drivers/iio/frequency/Makefile
> index 518b1e50caef..559922a8196e 100644
> --- a/drivers/iio/frequency/Makefile
> +++ b/drivers/iio/frequency/Makefile
> @@ -7,3 +7,4 @@
> obj-$(CONFIG_AD9523) += ad9523.o
> obj-$(CONFIG_ADF4350) += adf4350.o
> obj-$(CONFIG_ADF4371) += adf4371.o
> +obj-$(CONFIG_ADMV1013) += admv1013.o
> diff --git a/drivers/iio/frequency/admv1013.c
> b/drivers/iio/frequency/admv1013.c
> new file mode 100644
> index 000000000000..6451a2cc7c52
> --- /dev/null
> +++ b/drivers/iio/frequency/admv1013.c
> @@ -0,0 +1,571 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ADMV1013 driver
> + *
> + * Copyright 2021 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/notifier.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
> +
> +#include <asm/unaligned.h>
> +
> +/* ADMV1013 Register Map */
> +#define ADMV1013_REG_SPI_CONTROL 0x00
> +#define ADMV1013_REG_ALARM 0x01
> +#define ADMV1013_REG_ALARM_MASKS 0x02
> +#define ADMV1013_REG_ENABLE 0x03
> +#define ADMV1013_REG_LO_AMP_I 0x05
> +#define ADMV1013_REG_LO_AMP_Q 0x06
> +#define ADMV1013_REG_OFFSET_ADJUST_I 0x07
> +#define ADMV1013_REG_OFFSET_ADJUST_Q 0x08
> +#define ADMV1013_REG_QUAD 0x09
> +#define ADMV1013_REG_VVA_TEMP_COMP 0x0A
> +
> +/* ADMV1013_REG_SPI_CONTROL Map */
> +#define ADMV1013_PARITY_EN_MSK BIT(15)
> +#define ADMV1013_SPI_SOFT_RESET_MSK BIT(14)
> +#define ADMV1013_CHIP_ID_MSK GENMASK(11, 4)
> +#define ADMV1013_CHIP_ID 0xA
> +#define ADMV1013_REVISION_ID_MSK GENMASK(3, 0)
> +
> +/* ADMV1013_REG_ALARM Map */
> +#define ADMV1013_PARITY_ERROR_MSK BIT(15)
> +#define ADMV1013_TOO_FEW_ERRORS_MSK BIT(14)
> +#define ADMV1013_TOO_MANY_ERRORS_MSK BIT(13)
> +#define ADMV1013_ADDRESS_RANGE_ERROR_MSK BIT(12)
> +
> +/* ADMV1013_REG_ENABLE Map */
> +#define ADMV1013_VGA_PD_MSK BIT(15)
> +#define ADMV1013_MIXER_PD_MSK BIT(14)
> +#define ADMV1013_QUAD_PD_MSK
> GENMASK(13, 11)
> +#define ADMV1013_BG_PD_MSK BIT(10)
> +#define ADMV1013_MIXER_IF_EN_MSK BIT(7)
> +#define ADMV1013_DET_EN_MSK BIT(5)
> +
> +/* ADMV1013_REG_LO_AMP Map */
> +#define ADMV1013_LOAMP_PH_ADJ_FINE_MSK
> GENMASK(13, 7)
> +#define ADMV1013_MIXER_VGATE_MSK GENMASK(6, 0)
> +
> +/* ADMV1013_REG_OFFSET_ADJUST Map */
> +#define ADMV1013_MIXER_OFF_ADJ_P_MSK
> GENMASK(15, 9)
> +#define ADMV1013_MIXER_OFF_ADJ_N_MSK
> GENMASK(8, 2)
> +
> +/* ADMV1013_REG_QUAD Map */
> +#define ADMV1013_QUAD_SE_MODE_MSK GENMASK(9, 6)
> +#define ADMV1013_QUAD_FILTERS_MSK GENMASK(3, 0)
> +
> +/* ADMV1013_REG_VVA_TEMP_COMP Map */
> +#define ADMV1013_VVA_TEMP_COMP_MSK
> GENMASK(15, 0)
> +
> +struct admv1013_state {
> + struct spi_device *spi;
> + struct clk *clkin;
> + /* Protect against concurrent accesses to the device and to
> data */
> + struct mutex lock;
> + struct regulator *reg;
> + struct notifier_block nb;
> + unsigned int quad_se_mode;
> + bool vga_pd;
> + bool mixer_pd;
> + bool quad_pd;
> + bool bg_pd;
> + bool mixer_if_en;
> + bool det_en;
> + u8 data[3] ____cacheline_aligned;
> +};
> +
> +static int __admv1013_spi_read(struct admv1013_state *st, unsigned
> int reg,
> + unsigned int *val)
> +{
> + int ret;
> + struct spi_transfer t = {0};
> +
> + st->data[0] = 0x80 | (reg << 1);
> + st->data[1] = 0x0;
> + st->data[2] = 0x0;
> +
> + t.rx_buf = &st->data[0];
> + t.tx_buf = &st->data[0];
> + t.len = 3;
> +
> + ret = spi_sync_transfer(st->spi, &t, 1);
> + if (ret)
> + return ret;
> +
> + *val = (get_unaligned_be24(&st->data[0]) >> 1) &
> GENMASK(15, 0);
> +
> + return ret;
> +}
> +
> +static int admv1013_spi_read(struct admv1013_state *st, unsigned
> int reg,
> + unsigned int *val)
> +{
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = __admv1013_spi_read(st, reg, val);
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> +static int __admv1013_spi_write(struct admv1013_state *st,
> + unsigned int reg,
> + unsigned int val)
> +{
> + put_unaligned_be24((val << 1) | (reg << 17), &st->data[0]);
> +
> + return spi_write(st->spi, &st->data[0], 3);
> +}
> +
> +static int admv1013_spi_write(struct admv1013_state *st, unsigned
> int reg,
> + unsigned int val)
> +{
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = __admv1013_spi_write(st, reg, val);
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> +static int __admv1013_spi_update_bits(struct admv1013_state *st,
> unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + int ret;
> + unsigned int data, temp;
> +
> + ret = __admv1013_spi_read(st, reg, &data);
> + if (ret)
> + return ret;
> +
> + temp = (data & ~mask) | (val & mask);
> +
> + return __admv1013_spi_write(st, reg, temp);
> +}
> +
> +static int admv1013_spi_update_bits(struct admv1013_state *st,
> unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = __admv1013_spi_update_bits(st, reg, mask, val);
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> +static int admv1013_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long info)
> +{
> + struct admv1013_state *st = iio_priv(indio_dev);
> + unsigned int data, addr;
> + int ret;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_CALIBBIAS:
> + if (chan->channel2 == IIO_MOD_I)
> + addr = ADMV1013_REG_OFFSET_ADJUST_I;
> + else
> + addr = ADMV1013_REG_OFFSET_ADJUST_Q;
> +
> + ret = admv1013_spi_read(st, addr, &data);
> + if (ret)
> + return ret;
> +
> + if (!(chan->channel))
> + *val =
> FIELD_GET(ADMV1013_MIXER_OFF_ADJ_P_MSK, data);
> + else
> + *val =
> FIELD_GET(ADMV1013_MIXER_OFF_ADJ_N_MSK, data);
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_PHASE:
> + if (chan->channel2 == IIO_MOD_I)
> + addr = ADMV1013_REG_LO_AMP_I;
> + else
> + addr = ADMV1013_REG_LO_AMP_Q;
> +
> + ret = admv1013_spi_read(st, addr, &data);
> + if (ret)
> + return ret;
> +
> + *val =
> FIELD_GET(ADMV1013_LOAMP_PH_ADJ_FINE_MSK, data);
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int admv1013_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long info)
> +{
> + struct admv1013_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_CALIBBIAS:
> + if (chan->channel2 == IIO_MOD_I) {
> + if (!(chan->channel))
> + ret = admv1013_spi_update_bits(st,
> ADMV1013_REG_OFFSET_ADJUST_I,
> +
> ADMV1013_MIXER_OFF_ADJ_P_MSK,
> +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_P_MSK, val));
> + else
> + ret = admv1013_spi_update_bits(st,
> ADMV1013_REG_OFFSET_ADJUST_I,
> +
> ADMV1013_MIXER_OFF_ADJ_N_MSK,
> +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_N_MSK, val));
> + } else {
> + if (!(chan->channel))
> + ret = admv1013_spi_update_bits(st,
> ADMV1013_REG_OFFSET_ADJUST_Q,
> +
> ADMV1013_MIXER_OFF_ADJ_P_MSK,
> +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_P_MSK, val));
> + else
> + ret = admv1013_spi_update_bits(st,
> ADMV1013_REG_OFFSET_ADJUST_Q,
> +
> ADMV1013_MIXER_OFF_ADJ_N_MSK,
> +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_N_MSK, val));
> + }
> +
> + return ret;
> + case IIO_CHAN_INFO_PHASE:
> + if (chan->channel2 == IIO_MOD_I)
> + return admv1013_spi_update_bits(st,
> ADMV1013_REG_LO_AMP_I,
> +
> ADMV1013_LOAMP_PH_ADJ_FINE_MSK,
> +
> FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_FINE_MSK, val));
> + else
> + return admv1013_spi_update_bits(st,
> ADMV1013_REG_LO_AMP_Q,
> +
> ADMV1013_LOAMP_PH_ADJ_FINE_MSK,
> +
> FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_FINE_MSK, val));
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int admv1013_update_quad_filters(struct admv1013_state *st)
> +{
> + unsigned int filt_raw;
> + u64 rate = clk_get_rate(st->clkin);
> +
> + if (rate >= (5400 * HZ_PER_MHZ) && rate <= (7000 *
> HZ_PER_MHZ))
> + filt_raw = 15;
> + else if (rate >= (5400 * HZ_PER_MHZ) && rate <= (8000 *
> HZ_PER_MHZ))
> + filt_raw = 10;
> + else if (rate >= (6600 * HZ_PER_MHZ) && rate <= (9200 *
> HZ_PER_MHZ))
> + filt_raw = 5;
> + else
> + filt_raw = 0;
> +
> + return __admv1013_spi_update_bits(st,
> ADMV1013_REG_QUAD,
> +
> ADMV1013_QUAD_FILTERS_MSK,
> +
> FIELD_PREP(ADMV1013_QUAD_FILTERS_MSK, filt_raw));
> +}
> +
> +static int admv1013_update_mixer_vgate(struct admv1013_state *st)
> +{
> + unsigned int vcm, mixer_vgate;
> +
> + vcm = regulator_get_voltage(st->reg);
> +
> + if (vcm >= 0 && vcm < 1800000)
> + mixer_vgate = (2389 * vcm / 1000000 + 8100) / 100;
> + else if (vcm > 1800000 && vcm < 2600000)
> + mixer_vgate = (2375 * vcm / 1000000 + 125) / 100;
> + else
> + return -EINVAL;
> +
> + return __admv1013_spi_update_bits(st,
> ADMV1013_REG_LO_AMP_I,
> + ADMV1013_MIXER_VGATE_MSK,
> +
> FIELD_PREP(ADMV1013_MIXER_VGATE_MSK, mixer_vgate));
> +}
> +
> +static int admv1013_reg_access(struct iio_dev *indio_dev,
> + unsigned int reg,
> + unsigned int write_val,
> + unsigned int *read_val)
> +{
> + struct admv1013_state *st = iio_priv(indio_dev);
> +
> + if (read_val)
> + return admv1013_spi_read(st, reg, read_val);
> + else
> + return admv1013_spi_write(st, reg, write_val);
> +}
> +
> +static const struct iio_info admv1013_info = {
> + .read_raw = admv1013_read_raw,
> + .write_raw = admv1013_write_raw,
> + .debugfs_reg_access = &admv1013_reg_access,
> +};
> +
> +static int admv1013_freq_change(struct notifier_block *nb, unsigned
> long action, void *data)
> +{
> + struct admv1013_state *st = container_of(nb, struct
> admv1013_state, nb);
> + int ret;
> +
> + if (action == POST_RATE_CHANGE) {
> + mutex_lock(&st->lock);
> + ret =
> notifier_from_errno(admv1013_update_quad_filters(st));
> + mutex_unlock(&st->lock);
> + return ret;
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> +#define ADMV1013_CHAN_PHASE(_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) \
> + }
> +
> +#define ADMV1013_CHAN_CALIB(_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_CALIBBIAS) \
> + }
> +
> +static const struct iio_chan_spec admv1013_channels[] = {
> + ADMV1013_CHAN_PHASE(0, I),
> + ADMV1013_CHAN_PHASE(0, Q),
> + ADMV1013_CHAN_CALIB(0, I),
> + ADMV1013_CHAN_CALIB(0, Q),
> + ADMV1013_CHAN_CALIB(1, I),
> + ADMV1013_CHAN_CALIB(1, Q),
> +};
> +
Hmm, If I'm not missing nothing this leads to something like:
out_altvoltage0_i_phase
out_altvoltage0_q_phase
out_altvoltage0_i_calibbias
out_altvoltage0_q_calibbias
out_altvoltage1_i_calibbias
out_altvoltage1_q_calibbias
To me it is really non obvious that index 1 also applies to the same
channel. I see that we have this like this probably because we
can't use modified and differential at the same time, right?
Jonathan, I'm not sure what should be the done here... From the top of my
head I guess we can either:
* drop the modifiers (not perfect but also not that bad IMO),
* tweak something adding extended info (not really neat)
* or handling this in the core with a new variable. Of course, we would need
to be careful to not break existing drivers...
Not sure if labels could also be something to use... Any suggestion from your
side?
- Nuno Sá
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
2021-11-01 10:04 ` [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc Antoniu Miclaus
@ 2021-11-02 17:50 ` Rob Herring
2021-11-03 8:09 ` Miclaus, Antoniu
0 siblings, 1 reply; 15+ messages in thread
From: Rob Herring @ 2021-11-02 17:50 UTC (permalink / raw)
To: Antoniu Miclaus; +Cc: jic23, linux-iio, devicetree, linux-kernel, nuno.sa
On Mon, Nov 01, 2021 at 12:04:20PM +0200, Antoniu Miclaus wrote:
> Add device tree bindings for the ADMV1013 Upconverter.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> .../bindings/iio/frequency/adi,admv1013.yaml | 119 ++++++++++++++++++
> 1 file changed, 119 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> new file mode 100644
> index 000000000000..47993253a586
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.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,admv1013.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADMV1013 Microwave Upconverter
> +
> +maintainers:
> + - Antoniu Miclaus <antoniu.miclaus@analog.com>
> +
> +description: |
> + Wideband, microwave upconverter optimized for point to point microwave
> + radio designs operating in the 24 GHz to 44 GHz frequency range.
> +
> + https://www.analog.com/en/products/admv1013.html
> +
> +properties:
> + compatible:
> + enum:
> + - adi,admv1013
> +
> + reg:
> + maxItems: 1
> +
> + spi-max-frequency:
> + maximum: 1000000
> +
> + clocks:
> + description:
> + Definition of the external clock.
> + minItems: 1
> +
> + clock-names:
> + items:
> + - const: lo_in
> +
> + clock-output-names:
> + maxItems: 1
> +
> + vcm-supply:
> + description:
> + Analog voltage regulator.
> +
> + adi,vga-powerdown:
> + description:
> + Power Down the Voltage Gain Amplifier Circuit available at
> + BG_RBIAS2 pin.
> + type: boolean
> +
> + adi,mixer-powerdown:
> + description:
> + Power Down the Mixer Circuit. Enable to put the block in
> + a power down state.
> + type: boolean
> +
> + adi,quad-powerdown:
> + description:
> + Power Down the Quadrupler. Enable to put the block in
> + a power down state.
> + type: boolean
> +
> + adi,bg-powerdown:
> + description:
> + Power Down the Transmitter Band Gap. Enable to put the part in
> + a power down state.
> + type: boolean
> +
> + adi,mixer-if-enable:
> + description:
> + Enable the Intermediate Frequency Mode. Either IF Mode or I/Q Mode
> + can be enabled at a time.
> + type: boolean
> +
> + adi,detector-enable:
> + description:
> + Enable the Envelope Detector available at output pins VENV_P and
> + VENV_N. Disable to reduce power consumption.
> + type: boolean
> +
> + adi,quad-se-mode:
> + description:
> + Switch the LO path from differential to single-ended operation.
> + Set value 6 for Single-Ended Mode, Negative Side Disabled.
> + Set value 9 for Single-Ended Mode, Positive Side Disabled.
> + Set value 12 for Differential Mode.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [6, 9, 12]
All these vendor properties are fixed based on the board design or
something a user may want to change? The latter does not belong in DT.
Rob
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
2021-11-02 17:50 ` Rob Herring
@ 2021-11-03 8:09 ` Miclaus, Antoniu
2021-11-03 14:30 ` Miclaus, Antoniu
0 siblings, 1 reply; 15+ messages in thread
From: Miclaus, Antoniu @ 2021-11-03 8:09 UTC (permalink / raw)
To: Rob Herring; +Cc: jic23, linux-iio, devicetree, linux-kernel, Sa, Nuno
Hello Rob,
These properties are fixed and available in the datasheet (binary format):
https://www.analog.com/media/en/technical-documentation/data-sheets/ADMV1013.pdf
Please see Page 37 of 39, Table 15, QUAD_SE_MODE.
Regards,
--
Antoniu Miclăuş
> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, November 2, 2021 7:51 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: jic23@kernel.org; linux-iio@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
>
> [External]
>
> On Mon, Nov 01, 2021 at 12:04:20PM +0200, Antoniu Miclaus wrote:
> > Add device tree bindings for the ADMV1013 Upconverter.
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> > .../bindings/iio/frequency/adi,admv1013.yaml | 119
> ++++++++++++++++++
> > 1 file changed, 119 insertions(+)
> > create mode 100644
> Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> >
> > diff --git
> a/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > new file mode 100644
> > index 000000000000..47993253a586
> > --- /dev/null
> > +++
> b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.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,admv1013.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKQGXrugy$
> > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKYugV1fM$
> > +
> > +title: ADMV1013 Microwave Upconverter
> > +
> > +maintainers:
> > + - Antoniu Miclaus <antoniu.miclaus@analog.com>
> > +
> > +description: |
> > + Wideband, microwave upconverter optimized for point to point
> microwave
> > + radio designs operating in the 24 GHz to 44 GHz frequency range.
> > +
> > + https://www.analog.com/en/products/admv1013.html
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - adi,admv1013
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + spi-max-frequency:
> > + maximum: 1000000
> > +
> > + clocks:
> > + description:
> > + Definition of the external clock.
> > + minItems: 1
> > +
> > + clock-names:
> > + items:
> > + - const: lo_in
> > +
> > + clock-output-names:
> > + maxItems: 1
> > +
> > + vcm-supply:
> > + description:
> > + Analog voltage regulator.
> > +
> > + adi,vga-powerdown:
> > + description:
> > + Power Down the Voltage Gain Amplifier Circuit available at
> > + BG_RBIAS2 pin.
> > + type: boolean
> > +
> > + adi,mixer-powerdown:
> > + description:
> > + Power Down the Mixer Circuit. Enable to put the block in
> > + a power down state.
> > + type: boolean
> > +
> > + adi,quad-powerdown:
> > + description:
> > + Power Down the Quadrupler. Enable to put the block in
> > + a power down state.
> > + type: boolean
> > +
> > + adi,bg-powerdown:
> > + description:
> > + Power Down the Transmitter Band Gap. Enable to put the part in
> > + a power down state.
> > + type: boolean
> > +
> > + adi,mixer-if-enable:
> > + description:
> > + Enable the Intermediate Frequency Mode. Either IF Mode or I/Q Mode
> > + can be enabled at a time.
> > + type: boolean
> > +
> > + adi,detector-enable:
> > + description:
> > + Enable the Envelope Detector available at output pins VENV_P and
> > + VENV_N. Disable to reduce power consumption.
> > + type: boolean
> > +
> > + adi,quad-se-mode:
> > + description:
> > + Switch the LO path from differential to single-ended operation.
> > + Set value 6 for Single-Ended Mode, Negative Side Disabled.
> > + Set value 9 for Single-Ended Mode, Positive Side Disabled.
> > + Set value 12 for Differential Mode.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [6, 9, 12]
>
> All these vendor properties are fixed based on the board design or
> something a user may want to change? The latter does not belong in DT.
>
> Rob
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
2021-11-03 8:09 ` Miclaus, Antoniu
@ 2021-11-03 14:30 ` Miclaus, Antoniu
2021-11-04 18:26 ` Jonathan Cameron
0 siblings, 1 reply; 15+ messages in thread
From: Miclaus, Antoniu @ 2021-11-03 14:30 UTC (permalink / raw)
To: Rob Herring; +Cc: jic23, linux-iio, devicetree, linux-kernel, Sa, Nuno
Example:
In the setup that we tested the driver, we had a clock chip that was hardware-routed only to the positive side of the local oscillator input (LOP pin) from admv1013.
Therefore, I think keeping the property in the DT might be useful.
Regards,
--
Antoniu Miclăuş
> -----Original Message-----
> From: Miclaus, Antoniu
> Sent: Wednesday, November 3, 2021 10:09 AM
> To: Rob Herring <robh@kernel.org>
> Cc: jic23@kernel.org; linux-iio@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> Subject: RE: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
>
> Hello Rob,
>
> These properties are fixed and available in the datasheet (binary format):
> https://www.analog.com/media/en/technical-documentation/data-
> sheets/ADMV1013.pdf
>
> Please see Page 37 of 39, Table 15, QUAD_SE_MODE.
>
> Regards,
> --
> Antoniu Miclăuş
>
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Tuesday, November 2, 2021 7:51 PM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > Cc: jic23@kernel.org; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
> >
> > [External]
> >
> > On Mon, Nov 01, 2021 at 12:04:20PM +0200, Antoniu Miclaus wrote:
> > > Add device tree bindings for the ADMV1013 Upconverter.
> > >
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > ---
> > > .../bindings/iio/frequency/adi,admv1013.yaml | 119
> > ++++++++++++++++++
> > > 1 file changed, 119 insertions(+)
> > > create mode 100644
> > Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > >
> > > diff --git
> > a/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > new file mode 100644
> > > index 000000000000..47993253a586
> > > --- /dev/null
> > > +++
> > b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.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,admv1013.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKQGXrugy$
> > > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-
> >
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKYugV1fM$
> > > +
> > > +title: ADMV1013 Microwave Upconverter
> > > +
> > > +maintainers:
> > > + - Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > +
> > > +description: |
> > > + Wideband, microwave upconverter optimized for point to point
> > microwave
> > > + radio designs operating in the 24 GHz to 44 GHz frequency range.
> > > +
> > > + https://www.analog.com/en/products/admv1013.html
> > > +
> > > +properties:
> > > + compatible:
> > > + enum:
> > > + - adi,admv1013
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + spi-max-frequency:
> > > + maximum: 1000000
> > > +
> > > + clocks:
> > > + description:
> > > + Definition of the external clock.
> > > + minItems: 1
> > > +
> > > + clock-names:
> > > + items:
> > > + - const: lo_in
> > > +
> > > + clock-output-names:
> > > + maxItems: 1
> > > +
> > > + vcm-supply:
> > > + description:
> > > + Analog voltage regulator.
> > > +
> > > + adi,vga-powerdown:
> > > + description:
> > > + Power Down the Voltage Gain Amplifier Circuit available at
> > > + BG_RBIAS2 pin.
> > > + type: boolean
> > > +
> > > + adi,mixer-powerdown:
> > > + description:
> > > + Power Down the Mixer Circuit. Enable to put the block in
> > > + a power down state.
> > > + type: boolean
> > > +
> > > + adi,quad-powerdown:
> > > + description:
> > > + Power Down the Quadrupler. Enable to put the block in
> > > + a power down state.
> > > + type: boolean
> > > +
> > > + adi,bg-powerdown:
> > > + description:
> > > + Power Down the Transmitter Band Gap. Enable to put the part in
> > > + a power down state.
> > > + type: boolean
> > > +
> > > + adi,mixer-if-enable:
> > > + description:
> > > + Enable the Intermediate Frequency Mode. Either IF Mode or I/Q
> Mode
> > > + can be enabled at a time.
> > > + type: boolean
> > > +
> > > + adi,detector-enable:
> > > + description:
> > > + Enable the Envelope Detector available at output pins VENV_P and
> > > + VENV_N. Disable to reduce power consumption.
> > > + type: boolean
> > > +
> > > + adi,quad-se-mode:
> > > + description:
> > > + Switch the LO path from differential to single-ended operation.
> > > + Set value 6 for Single-Ended Mode, Negative Side Disabled.
> > > + Set value 9 for Single-Ended Mode, Positive Side Disabled.
> > > + Set value 12 for Differential Mode.
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + enum: [6, 9, 12]
> >
> > All these vendor properties are fixed based on the board design or
> > something a user may want to change? The latter does not belong in DT.
> >
> > Rob
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-11-02 10:09 ` [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013 Sa, Nuno
@ 2021-11-03 20:03 ` Jonathan Cameron
2021-11-04 8:11 ` Sa, Nuno
0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2021-11-03 20:03 UTC (permalink / raw)
To: Sa, Nuno; +Cc: Miclaus, Antoniu, robh+dt, linux-iio, devicetree, linux-kernel
On Tue, 2 Nov 2021 10:09:15 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > +#define ADMV1013_CHAN_PHASE(_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) \
> > + }
> > +
> > +#define ADMV1013_CHAN_CALIB(_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_CALIBBIAS) \
> > + }
> > +
> > +static const struct iio_chan_spec admv1013_channels[] = {
> > + ADMV1013_CHAN_PHASE(0, I),
> > + ADMV1013_CHAN_PHASE(0, Q),
> > + ADMV1013_CHAN_CALIB(0, I),
> > + ADMV1013_CHAN_CALIB(0, Q),
> > + ADMV1013_CHAN_CALIB(1, I),
> > + ADMV1013_CHAN_CALIB(1, Q),
> > +};
> > +
>
> Hmm, If I'm not missing nothing this leads to something like:
>
> out_altvoltage0_i_phase
> out_altvoltage0_q_phase
> out_altvoltage0_i_calibbias
> out_altvoltage0_q_calibbias
> out_altvoltage1_i_calibbias
> out_altvoltage1_q_calibbias
>
> To me it is really non obvious that index 1 also applies to the same
> channel. I see that we have this like this probably because we
> can't use modified and differential at the same time, right?
>
Indeed, this is the problem you mentioned in the discussion on v2
My suggestion of making it clear it is a differential channel and then
applying calibbias to the parts doesn't work as it would need to
have both modifiers and a second channel index.
As for why that is an issue, it comes down to trying to keep the
event interface descriptive, but still minimal. We basically ran
out of bits and at the time I couldn't think of a reason we'd want
both differential and a modifier...
> Jonathan, I'm not sure what should be the done here... From the top of my
> head I guess we can either:
>
> * drop the modifiers (not perfect but also not that bad IMO)
> * tweak something adding extended info (not really neat)
True, it's not neat but might be the way forwards for 'now'.. We don't have
events or anything on this driver, so we could make it look right without any
risk of not being able to extend it. However it will probably come back to bite
us and userspace may not expect whatever we do.
Horrible though it is you could use extend_name - which was originally intended
to let us 'label special purpose channels'. It was a bad idea, but is there and
not going way any time soon.
If you did the differential thing and set extend_name = "i" etc that
might get us around the internal issue of reusing channel2 for mod and differential
channel.
> * or handling this in the core with a new variable. Of course, we would need
> to be careful to not break existing drivers...
We would end up something hardly ever used so I'm doubtful that's a good
idea until we have some visibility of how common it would be.
>
> Not sure if labels could also be something to use... Any suggestion from your
> side?
Let me think a bit more on this for a day or two...
Jonathan
>
> - Nuno Sá
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-11-03 20:03 ` Jonathan Cameron
@ 2021-11-04 8:11 ` Sa, Nuno
2021-11-04 18:23 ` Jonathan Cameron
0 siblings, 1 reply; 15+ messages in thread
From: Sa, Nuno @ 2021-11-04 8:11 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Miclaus, Antoniu, robh+dt, linux-iio, devicetree, linux-kernel
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Wednesday, November 3, 2021 9:04 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>;
> robh+dt@kernel.org; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] iio: frequency: admv1013: add support for
> ADMV1013
>
> [External]
>
> On Tue, 2 Nov 2021 10:09:15 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
>
> > > +#define ADMV1013_CHAN_PHASE(_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) \
> > > + }
> > > +
> > > +#define ADMV1013_CHAN_CALIB(_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_CALIBBIAS) \
> > > + }
> > > +
> > > +static const struct iio_chan_spec admv1013_channels[] = {
> > > + ADMV1013_CHAN_PHASE(0, I),
> > > + ADMV1013_CHAN_PHASE(0, Q),
> > > + ADMV1013_CHAN_CALIB(0, I),
> > > + ADMV1013_CHAN_CALIB(0, Q),
> > > + ADMV1013_CHAN_CALIB(1, I),
> > > + ADMV1013_CHAN_CALIB(1, Q),
> > > +};
> > > +
> >
> > Hmm, If I'm not missing nothing this leads to something like:
> >
> > out_altvoltage0_i_phase
> > out_altvoltage0_q_phase
> > out_altvoltage0_i_calibbias
> > out_altvoltage0_q_calibbias
> > out_altvoltage1_i_calibbias
> > out_altvoltage1_q_calibbias
> >
> > To me it is really non obvious that index 1 also applies to the same
> > channel. I see that we have this like this probably because we
> > can't use modified and differential at the same time, right?
> >
>
> Indeed, this is the problem you mentioned in the discussion on v2
> My suggestion of making it clear it is a differential channel and then
> applying calibbias to the parts doesn't work as it would need to
> have both modifiers and a second channel index.
> As for why that is an issue, it comes down to trying to keep the
> event interface descriptive, but still minimal. We basically ran
> out of bits and at the time I couldn't think of a reason we'd want
> both differential and a modifier...
I'm not really seeing the issue with the event interface but that is mostly
because I still never had to deal with it, so I never looked that deeply into
the code. Might be a good time know :)
> > Jonathan, I'm not sure what should be the done here... From the top
> of my
> > head I guess we can either:
> >
> > * drop the modifiers (not perfect but also not that bad IMO)
> > * tweak something adding extended info (not really neat)
> True, it's not neat but might be the way forwards for 'now'.. We don't
> have
> events or anything on this driver, so we could make it look right
> without any
> risk of not being able to extend it. However it will probably come back
> to bite
> us and userspace may not expect whatever we do.
> Horrible though it is you could use extend_name - which was originally
> intended
> to let us 'label special purpose channels'. It was a bad idea, but is there
> and
> not going way any time soon.
>
> If you did the differential thing and set extend_name = "i" etc that
> might get us around the internal issue of reusing channel2 for mod and
> differential
> channel.
Couldn't we use the label to achieve kind of the same? Or do you think
that having the "i" and "q" in the filenames is really mandatory? I can
understand if you think they are as they are valid modifiers. OTOH,
IIRC, with the latest patches from Paul, adding the extended_name will
automatically get us the label sysfs file which might be a little odd but I
guess that's already true for all the legacy drivers using it... So yeah,
between this or extended info, we might have our "band aid" for this.
> > * or handling this in the core with a new variable. Of course, we
> would need
> > to be careful to not break existing drivers...
>
> We would end up something hardly ever used so I'm doubtful that's a
> good
> idea until we have some visibility of how common it would be.
True, most likely we would end up with a public variable only used in
this use case... Better to wait if some users like this pop up.
- Nuno Sá
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-11-04 8:11 ` Sa, Nuno
@ 2021-11-04 18:23 ` Jonathan Cameron
2021-11-05 9:11 ` Sa, Nuno
0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2021-11-04 18:23 UTC (permalink / raw)
To: Sa, Nuno; +Cc: Miclaus, Antoniu, robh+dt, linux-iio, devicetree, linux-kernel
On Thu, 4 Nov 2021 08:11:12 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Wednesday, November 3, 2021 9:04 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>;
> > robh+dt@kernel.org; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v3 1/2] iio: frequency: admv1013: add support for
> > ADMV1013
> >
> > [External]
> >
> > On Tue, 2 Nov 2021 10:09:15 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >
> > > > +#define ADMV1013_CHAN_PHASE(_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) \
> > > > + }
> > > > +
> > > > +#define ADMV1013_CHAN_CALIB(_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_CALIBBIAS) \
> > > > + }
> > > > +
> > > > +static const struct iio_chan_spec admv1013_channels[] = {
> > > > + ADMV1013_CHAN_PHASE(0, I),
> > > > + ADMV1013_CHAN_PHASE(0, Q),
> > > > + ADMV1013_CHAN_CALIB(0, I),
> > > > + ADMV1013_CHAN_CALIB(0, Q),
> > > > + ADMV1013_CHAN_CALIB(1, I),
> > > > + ADMV1013_CHAN_CALIB(1, Q),
> > > > +};
> > > > +
> > >
> > > Hmm, If I'm not missing nothing this leads to something like:
> > >
> > > out_altvoltage0_i_phase
> > > out_altvoltage0_q_phase
> > > out_altvoltage0_i_calibbias
> > > out_altvoltage0_q_calibbias
> > > out_altvoltage1_i_calibbias
> > > out_altvoltage1_q_calibbias
> > >
> > > To me it is really non obvious that index 1 also applies to the same
> > > channel. I see that we have this like this probably because we
> > > can't use modified and differential at the same time, right?
> > >
> >
> > Indeed, this is the problem you mentioned in the discussion on v2
> > My suggestion of making it clear it is a differential channel and then
> > applying calibbias to the parts doesn't work as it would need to
> > have both modifiers and a second channel index.
> > As for why that is an issue, it comes down to trying to keep the
> > event interface descriptive, but still minimal. We basically ran
> > out of bits and at the time I couldn't think of a reason we'd want
> > both differential and a modifier...
>
> I'm not really seeing the issue with the event interface but that is mostly
> because I still never had to deal with it, so I never looked that deeply into
> the code. Might be a good time know :)
not that it really matters for this discussion, but meh - I know where
to look :)
/**
* IIO_EVENT_CODE() - create event identifier
* @chan_type: Type of the channel. Should be one of enum iio_chan_type.
* @diff: Whether the event is for an differential channel or not.
* @modifier: Modifier for the channel. Should be one of enum iio_modifier.
* @direction: Direction of the event. One of enum iio_event_direction.
* @type: Type of the event. Should be one of enum iio_event_type.
* @chan: Channel number for non-differential channels.
* @chan1: First channel number for differential channels.
* @chan2: Second channel number for differential channels.
*/
#define IIO_EVENT_CODE(chan_type, diff, modifier, direction, \
type, chan, chan1, chan2) \
(((u64)type << 56) | ((u64)diff << 55) | \
((u64)direction << 48) | ((u64)modifier << 40) | \
((u64)chan_type << 32) | (((u16)chan2) << 16) | ((u16)chan1) | \
((u16)chan))
I'd forgotten the odd chan vs chan1 bit :)
Otherwise, key thing is we are running out of space in the 64 bits that
are pushed through the event kfifo.
>
> > > Jonathan, I'm not sure what should be the done here... From the top
> > of my
> > > head I guess we can either:
> > >
> > > * drop the modifiers (not perfect but also not that bad IMO)
> > > * tweak something adding extended info (not really neat)
> > True, it's not neat but might be the way forwards for 'now'.. We don't
> > have
> > events or anything on this driver, so we could make it look right
> > without any
> > risk of not being able to extend it. However it will probably come back
> > to bite
> > us and userspace may not expect whatever we do.
> > Horrible though it is you could use extend_name - which was originally
> > intended
> > to let us 'label special purpose channels'. It was a bad idea, but is there
> > and
> > not going way any time soon.
> >
> > If you did the differential thing and set extend_name = "i" etc that
> > might get us around the internal issue of reusing channel2 for mod and
> > differential
> > channel.
>
> Couldn't we use the label to achieve kind of the same? Or do you think
> that having the "i" and "q" in the filenames is really mandatory?
https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-bus-iio#L135
The i and q are ABI already, so we are stuck with them if we want to maintain
consistency.
> I can
> understand if you think they are as they are valid modifiers. OTOH,
> IIRC, with the latest patches from Paul, adding the extended_name will
> automatically get us the label sysfs file which might be a little odd but I
> guess that's already true for all the legacy drivers using it... So yeah,
> between this or extended info, we might have our "band aid" for this.
Ah good point. Paul's stuff was to allow userspace to basically skip these
fields which in this case would be bad...
extended info is the best plan for now. We can always change that over to
a more standard option at a later date it turns out to be necessary.
Jonathan
>
> > > * or handling this in the core with a new variable. Of course, we
> > would need
> > > to be careful to not break existing drivers...
> >
> > We would end up something hardly ever used so I'm doubtful that's a
> > good
> > idea until we have some visibility of how common it would be.
>
> True, most likely we would end up with a public variable only used in
> this use case... Better to wait if some users like this pop up.
>
> - Nuno Sá
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
2021-11-03 14:30 ` Miclaus, Antoniu
@ 2021-11-04 18:26 ` Jonathan Cameron
2021-11-05 8:38 ` Miclaus, Antoniu
0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2021-11-04 18:26 UTC (permalink / raw)
To: Miclaus, Antoniu
Cc: Rob Herring, linux-iio, devicetree, linux-kernel, Sa, Nuno
On Wed, 3 Nov 2021 14:30:56 +0000
"Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
> Example:
> In the setup that we tested the driver, we had a clock chip that was hardware-routed only to the positive side of the local oscillator input (LOP pin) from admv1013.
>
> Therefore, I think keeping the property in the DT might be useful.
I think Rob's question was more general than that one property... See below.
>
> Regards,
> --
> Antoniu Miclăuş
>
> > -----Original Message-----
> > From: Miclaus, Antoniu
> > Sent: Wednesday, November 3, 2021 10:09 AM
> > To: Rob Herring <robh@kernel.org>
> > Cc: jic23@kernel.org; linux-iio@vger.kernel.org; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > Subject: RE: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
> >
> > Hello Rob,
> >
> > These properties are fixed and available in the datasheet (binary format):
> > https://www.analog.com/media/en/technical-documentation/data-
> > sheets/ADMV1013.pdf
> >
> > Please see Page 37 of 39, Table 15, QUAD_SE_MODE.
> >
> > Regards,
> > --
> > Antoniu Miclăuş
> >
> > > -----Original Message-----
> > > From: Rob Herring <robh@kernel.org>
> > > Sent: Tuesday, November 2, 2021 7:51 PM
> > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > Cc: jic23@kernel.org; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > > Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
> > >
> > > [External]
> > >
> > > On Mon, Nov 01, 2021 at 12:04:20PM +0200, Antoniu Miclaus wrote:
> > > > Add device tree bindings for the ADMV1013 Upconverter.
> > > >
> > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > > ---
> > > > .../bindings/iio/frequency/adi,admv1013.yaml | 119
> > > ++++++++++++++++++
> > > > 1 file changed, 119 insertions(+)
> > > > create mode 100644
> > > Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > >
> > > > diff --git
> > > a/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > new file mode 100644
> > > > index 000000000000..47993253a586
> > > > --- /dev/null
> > > > +++
> > > b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.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,admv1013.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKQGXrugy$
> > > > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-
> > >
> > schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKYugV1fM$
> > > > +
> > > > +title: ADMV1013 Microwave Upconverter
> > > > +
> > > > +maintainers:
> > > > + - Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > > +
> > > > +description: |
> > > > + Wideband, microwave upconverter optimized for point to point
> > > microwave
> > > > + radio designs operating in the 24 GHz to 44 GHz frequency range.
> > > > +
> > > > + https://www.analog.com/en/products/admv1013.html
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > + enum:
> > > > + - adi,admv1013
> > > > +
> > > > + reg:
> > > > + maxItems: 1
> > > > +
> > > > + spi-max-frequency:
> > > > + maximum: 1000000
> > > > +
> > > > + clocks:
> > > > + description:
> > > > + Definition of the external clock.
> > > > + minItems: 1
> > > > +
> > > > + clock-names:
> > > > + items:
> > > > + - const: lo_in
> > > > +
> > > > + clock-output-names:
> > > > + maxItems: 1
> > > > +
> > > > + vcm-supply:
> > > > + description:
> > > > + Analog voltage regulator.
> > > > +
> > > > + adi,vga-powerdown:
> > > > + description:
> > > > + Power Down the Voltage Gain Amplifier Circuit available at
> > > > + BG_RBIAS2 pin.
> > > > + type: boolean
What wiring would make it sensible to always have this powered down?
If we can describe that rather than vga-powerdown then that is what should
be in the binding. If there isn't any wiring based justification and this
is just turning off part of the device, then it should not be in the binding.
> > > > +
> > > > + adi,mixer-powerdown:
> > > > + description:
> > > > + Power Down the Mixer Circuit. Enable to put the block in
> > > > + a power down state.
Same for all these other power downs.
> > > > + type: boolean
> > > > +
> > > > + adi,quad-powerdown:
> > > > + description:
> > > > + Power Down the Quadrupler. Enable to put the block in
> > > > + a power down state.
> > > > + type: boolean
> > > > +
> > > > + adi,bg-powerdown:
> > > > + description:
> > > > + Power Down the Transmitter Band Gap. Enable to put the part in
> > > > + a power down state.
> > > > + type: boolean
> > > > +
> > > > + adi,mixer-if-enable:
> > > > + description:
> > > > + Enable the Intermediate Frequency Mode. Either IF Mode or I/Q
> > Mode
> > > > + can be enabled at a time.
> > > > + type: boolean
> > > > +
> > > > + adi,detector-enable:
> > > > + description:
> > > > + Enable the Envelope Detector available at output pins VENV_P and
> > > > + VENV_N. Disable to reduce power consumption.
> > > > + type: boolean
> > > > +
> > > > + adi,quad-se-mode:
> > > > + description:
> > > > + Switch the LO path from differential to single-ended operation.
> > > > + Set value 6 for Single-Ended Mode, Negative Side Disabled.
> > > > + Set value 9 for Single-Ended Mode, Positive Side Disabled.
> > > > + Set value 12 for Differential Mode.
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + enum: [6, 9, 12]
> > >
> > > All these vendor properties are fixed based on the board design or
> > > something a user may want to change? The latter does not belong in DT.
> > >
> > > Rob
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
2021-11-04 18:26 ` Jonathan Cameron
@ 2021-11-05 8:38 ` Miclaus, Antoniu
2021-11-05 17:10 ` Jonathan Cameron
0 siblings, 1 reply; 15+ messages in thread
From: Miclaus, Antoniu @ 2021-11-05 8:38 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Rob Herring, linux-iio, devicetree, linux-kernel, Sa, Nuno
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Thursday, November 4, 2021 8:27 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: Rob Herring <robh@kernel.org>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Sa, Nuno
> <Nuno.Sa@analog.com>
> Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
>
> [External]
>
> On Wed, 3 Nov 2021 14:30:56 +0000
> "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
>
> > Example:
> > In the setup that we tested the driver, we had a clock chip that was
> hardware-routed only to the positive side of the local oscillator input (LOP
> pin) from admv1013.
> >
> > Therefore, I think keeping the property in the DT might be useful.
>
> I think Rob's question was more general than that one property... See below.
> >
> > Regards,
> > --
> > Antoniu Miclăuş
> >
> > > -----Original Message-----
> > > From: Miclaus, Antoniu
> > > Sent: Wednesday, November 3, 2021 10:09 AM
> > > To: Rob Herring <robh@kernel.org>
> > > Cc: jic23@kernel.org; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > > Subject: RE: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013
> doc
> > >
> > > Hello Rob,
> > >
> > > These properties are fixed and available in the datasheet (binary format):
> > > https://www.analog.com/media/en/technical-documentation/data-
> > > sheets/ADMV1013.pdf
> > >
> > > Please see Page 37 of 39, Table 15, QUAD_SE_MODE.
> > >
> > > Regards,
> > > --
> > > Antoniu Miclăuş
> > >
> > > > -----Original Message-----
> > > > From: Rob Herring <robh@kernel.org>
> > > > Sent: Tuesday, November 2, 2021 7:51 PM
> > > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > > Cc: jic23@kernel.org; linux-iio@vger.kernel.org;
> > > devicetree@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > > > Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013
> doc
> > > >
> > > > [External]
> > > >
> > > > On Mon, Nov 01, 2021 at 12:04:20PM +0200, Antoniu Miclaus wrote:
> > > > > Add device tree bindings for the ADMV1013 Upconverter.
> > > > >
> > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > > > ---
> > > > > .../bindings/iio/frequency/adi,admv1013.yaml | 119
> > > > ++++++++++++++++++
> > > > > 1 file changed, 119 insertions(+)
> > > > > create mode 100644
> > > > Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > >
> > > > > diff --git
> > > >
> a/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > >
> b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..47993253a586
> > > > > --- /dev/null
> > > > > +++
> > > >
> b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.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,admv1013.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > > > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKQGXrugy$
> > > > > +$schema:
> https://urldefense.com/v3/__http://devicetree.org/meta-
> > > >
> > >
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > > > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKYugV1fM$
> > > > > +
> > > > > +title: ADMV1013 Microwave Upconverter
> > > > > +
> > > > > +maintainers:
> > > > > + - Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > > > +
> > > > > +description: |
> > > > > + Wideband, microwave upconverter optimized for point to point
> > > > microwave
> > > > > + radio designs operating in the 24 GHz to 44 GHz frequency range.
> > > > > +
> > > > > + https://www.analog.com/en/products/admv1013.html
> > > > > +
> > > > > +properties:
> > > > > + compatible:
> > > > > + enum:
> > > > > + - adi,admv1013
> > > > > +
> > > > > + reg:
> > > > > + maxItems: 1
> > > > > +
> > > > > + spi-max-frequency:
> > > > > + maximum: 1000000
> > > > > +
> > > > > + clocks:
> > > > > + description:
> > > > > + Definition of the external clock.
> > > > > + minItems: 1
> > > > > +
> > > > > + clock-names:
> > > > > + items:
> > > > > + - const: lo_in
> > > > > +
> > > > > + clock-output-names:
> > > > > + maxItems: 1
> > > > > +
> > > > > + vcm-supply:
> > > > > + description:
> > > > > + Analog voltage regulator.
> > > > > +
> > > > > + adi,vga-powerdown:
> > > > > + description:
> > > > > + Power Down the Voltage Gain Amplifier Circuit available at
> > > > > + BG_RBIAS2 pin.
> > > > > + type: boolean
>
> What wiring would make it sensible to always have this powered down?
> If we can describe that rather than vga-powerdown then that is what should
> be in the binding. If there isn't any wiring based justification and this
> is just turning off part of the device, then it should not be in the binding.
>
This part is similar to ADRF6780 that has also an Enable Register that can power down circuit blocks within the part:
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/drivers/iio/frequency/adrf6780.c?h=togreg
I preferred to expose these properties in the device tree, rather than creating custom device attributes in the driver.
But I guess these properties can be managed also only from debug fs.
I can adapt the code based on your decision. Looking forward to your feedback.
> > > > > +
> > > > > + adi,mixer-powerdown:
> > > > > + description:
> > > > > + Power Down the Mixer Circuit. Enable to put the block in
> > > > > + a power down state.
>
> Same for all these other power downs.
>
> > > > > + type: boolean
> > > > > +
> > > > > + adi,quad-powerdown:
> > > > > + description:
> > > > > + Power Down the Quadrupler. Enable to put the block in
> > > > > + a power down state.
> > > > > + type: boolean
> > > > > +
> > > > > + adi,bg-powerdown:
> > > > > + description:
> > > > > + Power Down the Transmitter Band Gap. Enable to put the part in
> > > > > + a power down state.
> > > > > + type: boolean
> > > > > +
> > > > > + adi,mixer-if-enable:
> > > > > + description:
> > > > > + Enable the Intermediate Frequency Mode. Either IF Mode or I/Q
> > > Mode
> > > > > + can be enabled at a time.
> > > > > + type: boolean
> > > > > +
> > > > > + adi,detector-enable:
> > > > > + description:
> > > > > + Enable the Envelope Detector available at output pins VENV_P
> and
> > > > > + VENV_N. Disable to reduce power consumption.
> > > > > + type: boolean
> > > > > +
> > > > > + adi,quad-se-mode:
> > > > > + description:
> > > > > + Switch the LO path from differential to single-ended operation.
> > > > > + Set value 6 for Single-Ended Mode, Negative Side Disabled.
> > > > > + Set value 9 for Single-Ended Mode, Positive Side Disabled.
> > > > > + Set value 12 for Differential Mode.
> > > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > > + enum: [6, 9, 12]
> > > >
> > > > All these vendor properties are fixed based on the board design or
> > > > something a user may want to change? The latter does not belong in
> DT.
> > > >
> > > > Rob
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-11-04 18:23 ` Jonathan Cameron
@ 2021-11-05 9:11 ` Sa, Nuno
0 siblings, 0 replies; 15+ messages in thread
From: Sa, Nuno @ 2021-11-05 9:11 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Miclaus, Antoniu, robh+dt, linux-iio, devicetree, linux-kernel
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Thursday, November 4, 2021 7:23 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>;
> robh+dt@kernel.org; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] iio: frequency: admv1013: add support for
> ADMV1013
>
> [External]
>
> On Thu, 4 Nov 2021 08:11:12 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
>
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Wednesday, November 3, 2021 9:04 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>;
> > > robh+dt@kernel.org; linux-iio@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v3 1/2] iio: frequency: admv1013: add support
> for
> > > ADMV1013
> > >
> > > [External]
> > >
> > > On Tue, 2 Nov 2021 10:09:15 +0000
> > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > >
> > > > > +#define ADMV1013_CHAN_PHASE(_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)
> \
> > > > > + }
> > > > > +
> > > > > +#define ADMV1013_CHAN_CALIB(_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_CALIBBIAS) \
> > > > > + }
> > > > > +
> > > > > +static const struct iio_chan_spec admv1013_channels[] = {
> > > > > + ADMV1013_CHAN_PHASE(0, I),
> > > > > + ADMV1013_CHAN_PHASE(0, Q),
> > > > > + ADMV1013_CHAN_CALIB(0, I),
> > > > > + ADMV1013_CHAN_CALIB(0, Q),
> > > > > + ADMV1013_CHAN_CALIB(1, I),
> > > > > + ADMV1013_CHAN_CALIB(1, Q),
> > > > > +};
> > > > > +
> > > >
> > > > Hmm, If I'm not missing nothing this leads to something like:
> > > >
> > > > out_altvoltage0_i_phase
> > > > out_altvoltage0_q_phase
> > > > out_altvoltage0_i_calibbias
> > > > out_altvoltage0_q_calibbias
> > > > out_altvoltage1_i_calibbias
> > > > out_altvoltage1_q_calibbias
> > > >
> > > > To me it is really non obvious that index 1 also applies to the same
> > > > channel. I see that we have this like this probably because we
> > > > can't use modified and differential at the same time, right?
> > > >
> > >
> > > Indeed, this is the problem you mentioned in the discussion on v2
> > > My suggestion of making it clear it is a differential channel and then
> > > applying calibbias to the parts doesn't work as it would need to
> > > have both modifiers and a second channel index.
> > > As for why that is an issue, it comes down to trying to keep the
> > > event interface descriptive, but still minimal. We basically ran
> > > out of bits and at the time I couldn't think of a reason we'd want
> > > both differential and a modifier...
> >
> > I'm not really seeing the issue with the event interface but that is
> mostly
> > because I still never had to deal with it, so I never looked that deeply
> into
> > the code. Might be a good time know :)
>
> not that it really matters for this discussion, but meh - I know where
> to look :)
>
> /**
> * IIO_EVENT_CODE() - create event identifier
> * @chan_type: Type of the channel. Should be one of enum
> iio_chan_type.
> * @diff: Whether the event is for an differential channel or not.
> * @modifier: Modifier for the channel. Should be one of enum
> iio_modifier.
> * @direction: Direction of the event. One of enum
> iio_event_direction.
> * @type: Type of the event. Should be one of enum
> iio_event_type.
> * @chan: Channel number for non-differential channels.
> * @chan1: First channel number for differential channels.
> * @chan2: Second channel number for differential channels.
> */
> #define IIO_EVENT_CODE(chan_type, diff, modifier, direction,
> \
> type, chan, chan1, chan2) \
> (((u64)type << 56) | ((u64)diff << 55) | \
> ((u64)direction << 48) | ((u64)modifier << 40) | \
> ((u64)chan_type << 32) | (((u16)chan2) << 16) | ((u16)chan1) |
> \
> ((u16)chan))
>
> I'd forgotten the odd chan vs chan1 bit :)
>
> Otherwise, key thing is we are running out of space in the 64 bits that
> are pushed through the event kfifo.
Yeah, it took me a bit to remember to look at the uapi but then I found
those defines...
- Nuno Sá
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
2021-11-05 8:38 ` Miclaus, Antoniu
@ 2021-11-05 17:10 ` Jonathan Cameron
2021-11-08 10:01 ` Miclaus, Antoniu
0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2021-11-05 17:10 UTC (permalink / raw)
To: Miclaus, Antoniu
Cc: Jonathan Cameron, Rob Herring, linux-iio, devicetree,
linux-kernel, Sa, Nuno
On Fri, 5 Nov 2021 08:38:42 +0000
"Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Thursday, November 4, 2021 8:27 PM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > Cc: Rob Herring <robh@kernel.org>; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Sa, Nuno
> > <Nuno.Sa@analog.com>
> > Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
> >
> > [External]
> >
> > On Wed, 3 Nov 2021 14:30:56 +0000
> > "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
> >
> > > Example:
> > > In the setup that we tested the driver, we had a clock chip that was
> > hardware-routed only to the positive side of the local oscillator input (LOP
> > pin) from admv1013.
> > >
> > > Therefore, I think keeping the property in the DT might be useful.
> >
> > I think Rob's question was more general than that one property... See below.
> > >
> > > Regards,
> > > --
> > > Antoniu Miclăuş
> > >
> > > > -----Original Message-----
> > > > From: Miclaus, Antoniu
> > > > Sent: Wednesday, November 3, 2021 10:09 AM
> > > > To: Rob Herring <robh@kernel.org>
> > > > Cc: jic23@kernel.org; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > > > Subject: RE: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013
> > doc
> > > >
> > > > Hello Rob,
> > > >
> > > > These properties are fixed and available in the datasheet (binary format):
> > > > https://www.analog.com/media/en/technical-documentation/data-
> > > > sheets/ADMV1013.pdf
> > > >
> > > > Please see Page 37 of 39, Table 15, QUAD_SE_MODE.
> > > >
> > > > Regards,
> > > > --
> > > > Antoniu Miclăuş
> > > >
> > > > > -----Original Message-----
> > > > > From: Rob Herring <robh@kernel.org>
> > > > > Sent: Tuesday, November 2, 2021 7:51 PM
> > > > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > > > Cc: jic23@kernel.org; linux-iio@vger.kernel.org;
> > > > devicetree@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013
> > doc
> > > > >
> > > > > [External]
> > > > >
> > > > > On Mon, Nov 01, 2021 at 12:04:20PM +0200, Antoniu Miclaus wrote:
> > > > > > Add device tree bindings for the ADMV1013 Upconverter.
> > > > > >
> > > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > > > > ---
> > > > > > .../bindings/iio/frequency/adi,admv1013.yaml | 119
> > > > > ++++++++++++++++++
> > > > > > 1 file changed, 119 insertions(+)
> > > > > > create mode 100644
> > > > > Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > > >
> > > > > > diff --git
> > > > >
> > a/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > >
> > b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..47993253a586
> > > > > > --- /dev/null
> > > > > > +++
> > > > >
> > b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.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,admv1013.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > > > > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKQGXrugy$
> > > > > > +$schema:
> > https://urldefense.com/v3/__http://devicetree.org/meta-
> > > > >
> > > >
> > schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > > > > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKYugV1fM$
> > > > > > +
> > > > > > +title: ADMV1013 Microwave Upconverter
> > > > > > +
> > > > > > +maintainers:
> > > > > > + - Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > > > > +
> > > > > > +description: |
> > > > > > + Wideband, microwave upconverter optimized for point to point
> > > > > microwave
> > > > > > + radio designs operating in the 24 GHz to 44 GHz frequency range.
> > > > > > +
> > > > > > + https://www.analog.com/en/products/admv1013.html
> > > > > > +
> > > > > > +properties:
> > > > > > + compatible:
> > > > > > + enum:
> > > > > > + - adi,admv1013
> > > > > > +
> > > > > > + reg:
> > > > > > + maxItems: 1
> > > > > > +
> > > > > > + spi-max-frequency:
> > > > > > + maximum: 1000000
> > > > > > +
> > > > > > + clocks:
> > > > > > + description:
> > > > > > + Definition of the external clock.
> > > > > > + minItems: 1
> > > > > > +
> > > > > > + clock-names:
> > > > > > + items:
> > > > > > + - const: lo_in
> > > > > > +
> > > > > > + clock-output-names:
> > > > > > + maxItems: 1
> > > > > > +
> > > > > > + vcm-supply:
> > > > > > + description:
> > > > > > + Analog voltage regulator.
> > > > > > +
> > > > > > + adi,vga-powerdown:
> > > > > > + description:
> > > > > > + Power Down the Voltage Gain Amplifier Circuit available at
> > > > > > + BG_RBIAS2 pin.
> > > > > > + type: boolean
> >
> > What wiring would make it sensible to always have this powered down?
> > If we can describe that rather than vga-powerdown then that is what should
> > be in the binding. If there isn't any wiring based justification and this
> > is just turning off part of the device, then it should not be in the binding.
> >
> This part is similar to ADRF6780 that has also an Enable Register that can power down circuit blocks within the part:
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/drivers/iio/frequency/adrf6780.c?h=togreg
>
Hmm. Guess we weren't super awake with that one :(
> I preferred to expose these properties in the device tree, rather than creating custom device attributes in the driver.
>
> But I guess these properties can be managed also only from debug fs.
The ideal situation is to control these based on what the device is being used for
and not provide explicit control to userspace or via dt.
So if it doesn't make sense to power up a unit because it isn't wired to anything
then express the wiring in DT.
If it makes sense to power down a unit because it isn't being used currently
due to what features are in use, then power it down automatically. Runtime pm
handles a lot of these cases by letting you do autopowerdown if not used after
a certain period. That way, we don't end up powering up and down rapidly but
only do it if we have go reason to believe it is worth powering down.
Could we use either of these approaches here?
Jonathan
>
> I can adapt the code based on your decision. Looking forward to your feedback.
> > > > > > +
> > > > > > + adi,mixer-powerdown:
> > > > > > + description:
> > > > > > + Power Down the Mixer Circuit. Enable to put the block in
> > > > > > + a power down state.
> >
> > Same for all these other power downs.
> >
> > > > > > + type: boolean
> > > > > > +
> > > > > > + adi,quad-powerdown:
> > > > > > + description:
> > > > > > + Power Down the Quadrupler. Enable to put the block in
> > > > > > + a power down state.
> > > > > > + type: boolean
> > > > > > +
> > > > > > + adi,bg-powerdown:
> > > > > > + description:
> > > > > > + Power Down the Transmitter Band Gap. Enable to put the part in
> > > > > > + a power down state.
> > > > > > + type: boolean
> > > > > > +
> > > > > > + adi,mixer-if-enable:
> > > > > > + description:
> > > > > > + Enable the Intermediate Frequency Mode. Either IF Mode or I/Q
> > > > Mode
> > > > > > + can be enabled at a time.
> > > > > > + type: boolean
> > > > > > +
> > > > > > + adi,detector-enable:
> > > > > > + description:
> > > > > > + Enable the Envelope Detector available at output pins VENV_P
> > and
> > > > > > + VENV_N. Disable to reduce power consumption.
> > > > > > + type: boolean
> > > > > > +
> > > > > > + adi,quad-se-mode:
> > > > > > + description:
> > > > > > + Switch the LO path from differential to single-ended operation.
> > > > > > + Set value 6 for Single-Ended Mode, Negative Side Disabled.
> > > > > > + Set value 9 for Single-Ended Mode, Positive Side Disabled.
> > > > > > + Set value 12 for Differential Mode.
> > > > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > + enum: [6, 9, 12]
> > > > >
> > > > > All these vendor properties are fixed based on the board design or
> > > > > something a user may want to change? The latter does not belong in
> > DT.
> > > > >
> > > > > Rob
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
2021-11-05 17:10 ` Jonathan Cameron
@ 2021-11-08 10:01 ` Miclaus, Antoniu
2021-11-13 16:24 ` Jonathan Cameron
0 siblings, 1 reply; 15+ messages in thread
From: Miclaus, Antoniu @ 2021-11-08 10:01 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, Rob Herring, linux-iio, devicetree,
linux-kernel, Sa, Nuno
--
Antoniu Miclăuş
> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Friday, November 5, 2021 7:10 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Rob Herring <robh@kernel.org>;
> linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
>
> [External]
>
> On Fri, 5 Nov 2021 08:38:42 +0000
> "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
>
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Thursday, November 4, 2021 8:27 PM
> > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > Cc: Rob Herring <robh@kernel.org>; linux-iio@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Sa, Nuno
> > > <Nuno.Sa@analog.com>
> > > Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013
> doc
> > >
> > > [External]
> > >
> > > On Wed, 3 Nov 2021 14:30:56 +0000
> > > "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
> > >
> > > > Example:
> > > > In the setup that we tested the driver, we had a clock chip that was
> > > hardware-routed only to the positive side of the local oscillator input (LOP
> > > pin) from admv1013.
> > > >
> > > > Therefore, I think keeping the property in the DT might be useful.
> > >
> > > I think Rob's question was more general than that one property... See
> below.
> > > >
> > > > Regards,
> > > > --
> > > > Antoniu Miclăuş
> > > >
> > > > > -----Original Message-----
> > > > > From: Miclaus, Antoniu
> > > > > Sent: Wednesday, November 3, 2021 10:09 AM
> > > > > To: Rob Herring <robh@kernel.org>
> > > > > Cc: jic23@kernel.org; linux-iio@vger.kernel.org;
> > > devicetree@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Subject: RE: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013
> > > doc
> > > > >
> > > > > Hello Rob,
> > > > >
> > > > > These properties are fixed and available in the datasheet (binary
> format):
> > > > > https://www.analog.com/media/en/technical-documentation/data-
> > > > > sheets/ADMV1013.pdf
> > > > >
> > > > > Please see Page 37 of 39, Table 15, QUAD_SE_MODE.
> > > > >
> > > > > Regards,
> > > > > --
> > > > > Antoniu Miclăuş
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Rob Herring <robh@kernel.org>
> > > > > > Sent: Tuesday, November 2, 2021 7:51 PM
> > > > > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > > > > Cc: jic23@kernel.org; linux-iio@vger.kernel.org;
> > > > > devicetree@vger.kernel.org;
> > > > > > linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add
> admv1013
> > > doc
> > > > > >
> > > > > > [External]
> > > > > >
> > > > > > On Mon, Nov 01, 2021 at 12:04:20PM +0200, Antoniu Miclaus wrote:
> > > > > > > Add device tree bindings for the ADMV1013 Upconverter.
> > > > > > >
> > > > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > > > > > ---
> > > > > > > .../bindings/iio/frequency/adi,admv1013.yaml | 119
> > > > > > ++++++++++++++++++
> > > > > > > 1 file changed, 119 insertions(+)
> > > > > > > create mode 100644
> > > > > >
> Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > > > >
> > > > > > > diff --git
> > > > > >
> > >
> a/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > > >
> > >
> b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..47993253a586
> > > > > > > --- /dev/null
> > > > > > > +++
> > > > > >
> > >
> b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.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,admv1013.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > > > > > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKQGXrugy$
> > > > > > > +$schema:
> > > https://urldefense.com/v3/__http://devicetree.org/meta-
> > > > > >
> > > > >
> > >
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > > > > > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKYugV1fM$
> > > > > > > +
> > > > > > > +title: ADMV1013 Microwave Upconverter
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > + - Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > > > > > +
> > > > > > > +description: |
> > > > > > > + Wideband, microwave upconverter optimized for point to point
> > > > > > microwave
> > > > > > > + radio designs operating in the 24 GHz to 44 GHz frequency
> range.
> > > > > > > +
> > > > > > > + https://www.analog.com/en/products/admv1013.html
> > > > > > > +
> > > > > > > +properties:
> > > > > > > + compatible:
> > > > > > > + enum:
> > > > > > > + - adi,admv1013
> > > > > > > +
> > > > > > > + reg:
> > > > > > > + maxItems: 1
> > > > > > > +
> > > > > > > + spi-max-frequency:
> > > > > > > + maximum: 1000000
> > > > > > > +
> > > > > > > + clocks:
> > > > > > > + description:
> > > > > > > + Definition of the external clock.
> > > > > > > + minItems: 1
> > > > > > > +
> > > > > > > + clock-names:
> > > > > > > + items:
> > > > > > > + - const: lo_in
> > > > > > > +
> > > > > > > + clock-output-names:
> > > > > > > + maxItems: 1
> > > > > > > +
> > > > > > > + vcm-supply:
> > > > > > > + description:
> > > > > > > + Analog voltage regulator.
> > > > > > > +
> > > > > > > + adi,vga-powerdown:
> > > > > > > + description:
> > > > > > > + Power Down the Voltage Gain Amplifier Circuit available at
> > > > > > > + BG_RBIAS2 pin.
> > > > > > > + type: boolean
> > >
> > > What wiring would make it sensible to always have this powered down?
> > > If we can describe that rather than vga-powerdown then that is what
> should
> > > be in the binding. If there isn't any wiring based justification and this
> > > is just turning off part of the device, then it should not be in the binding.
> > >
> > This part is similar to ADRF6780 that has also an Enable Register that can
> power down circuit blocks within the part:
> >
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/g
> it/jic23/iio.git/tree/drivers/iio/frequency/adrf6780.c?h=togreg__;!!A3Ni8CS0
> y2Y!sZfM9jaILdXhyFOXGx3KQHQOxNJaKopL7sEw2F-
> xzEWT0xHdoIW5sUEYE9ezdwHxPPWy$
> >
>
> Hmm. Guess we weren't super awake with that one :(
>
> > I preferred to expose these properties in the device tree, rather than
> creating custom device attributes in the driver.
> >
> > But I guess these properties can be managed also only from debug fs.
>
> The ideal situation is to control these based on what the device is being used
> for
> and not provide explicit control to userspace or via dt.
>
> So if it doesn't make sense to power up a unit because it isn't wired to
> anything
> then express the wiring in DT.
All these powerdown properties have individual supply pins assigned (VCC_MIXER,
VCC_BG, VCC_QUAD... datasheet - page 8 of 39).
So I guess we can consider to enable/disable these blocks based on the wiring.
Looking forward to your feedback.
>
> If it makes sense to power down a unit because it isn't being used currently
> due to what features are in use, then power it down automatically. Runtime
> pm
> handles a lot of these cases by letting you do autopowerdown if not used
> after
> a certain period. That way, we don't end up powering up and down rapidly
> but
> only do it if we have go reason to believe it is worth powering down.
>
> Could we use either of these approaches here?
>
> Jonathan
> >
> > I can adapt the code based on your decision. Looking forward to your
> feedback.
> > > > > > > +
> > > > > > > + adi,mixer-powerdown:
> > > > > > > + description:
> > > > > > > + Power Down the Mixer Circuit. Enable to put the block in
> > > > > > > + a power down state.
> > >
> > > Same for all these other power downs.
> > >
> > > > > > > + type: boolean
> > > > > > > +
> > > > > > > + adi,quad-powerdown:
> > > > > > > + description:
> > > > > > > + Power Down the Quadrupler. Enable to put the block in
> > > > > > > + a power down state.
> > > > > > > + type: boolean
> > > > > > > +
> > > > > > > + adi,bg-powerdown:
> > > > > > > + description:
> > > > > > > + Power Down the Transmitter Band Gap. Enable to put the
> part in
> > > > > > > + a power down state.
> > > > > > > + type: boolean
> > > > > > > +
> > > > > > > + adi,mixer-if-enable:
> > > > > > > + description:
> > > > > > > + Enable the Intermediate Frequency Mode. Either IF Mode or
> I/Q
> > > > > Mode
> > > > > > > + can be enabled at a time.
> > > > > > > + type: boolean
> > > > > > > +
> > > > > > > + adi,detector-enable:
> > > > > > > + description:
> > > > > > > + Enable the Envelope Detector available at output pins
> VENV_P
> > > and
> > > > > > > + VENV_N. Disable to reduce power consumption.
> > > > > > > + type: boolean
> > > > > > > +
> > > > > > > + adi,quad-se-mode:
> > > > > > > + description:
> > > > > > > + Switch the LO path from differential to single-ended
> operation.
> > > > > > > + Set value 6 for Single-Ended Mode, Negative Side Disabled.
> > > > > > > + Set value 9 for Single-Ended Mode, Positive Side Disabled.
> > > > > > > + Set value 12 for Differential Mode.
> > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > + enum: [6, 9, 12]
> > > > > >
> > > > > > All these vendor properties are fixed based on the board design or
> > > > > > something a user may want to change? The latter does not belong
> in
> > > DT.
> > > > > >
> > > > > > Rob
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
2021-11-08 10:01 ` Miclaus, Antoniu
@ 2021-11-13 16:24 ` Jonathan Cameron
0 siblings, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2021-11-13 16:24 UTC (permalink / raw)
To: Miclaus, Antoniu
Cc: Jonathan Cameron, Rob Herring, linux-iio, devicetree,
linux-kernel, Sa, Nuno
On Mon, 8 Nov 2021 10:01:04 +0000
"Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
> --
> Antoniu Miclăuş
>
> > -----Original Message-----
> > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Sent: Friday, November 5, 2021 7:10 PM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>; Rob Herring <robh@kernel.org>;
> > linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc
> >
> > [External]
> >
> > On Fri, 5 Nov 2021 08:38:42 +0000
> > "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Thursday, November 4, 2021 8:27 PM
> > > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > > Cc: Rob Herring <robh@kernel.org>; linux-iio@vger.kernel.org;
> > > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Sa, Nuno
> > > > <Nuno.Sa@analog.com>
> > > > Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013
> > doc
> > > >
> > > > [External]
> > > >
> > > > On Wed, 3 Nov 2021 14:30:56 +0000
> > > > "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
> > > >
> > > > > Example:
> > > > > In the setup that we tested the driver, we had a clock chip that was
> > > > hardware-routed only to the positive side of the local oscillator input (LOP
> > > > pin) from admv1013.
> > > > >
> > > > > Therefore, I think keeping the property in the DT might be useful.
> > > >
> > > > I think Rob's question was more general than that one property... See
> > below.
> > > > >
> > > > > Regards,
> > > > > --
> > > > > Antoniu Miclăuş
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Miclaus, Antoniu
> > > > > > Sent: Wednesday, November 3, 2021 10:09 AM
> > > > > > To: Rob Herring <robh@kernel.org>
> > > > > > Cc: jic23@kernel.org; linux-iio@vger.kernel.org;
> > > > devicetree@vger.kernel.org;
> > > > > > linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > Subject: RE: [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013
> > > > doc
> > > > > >
> > > > > > Hello Rob,
> > > > > >
> > > > > > These properties are fixed and available in the datasheet (binary
> > format):
> > > > > > https://www.analog.com/media/en/technical-documentation/data-
> > > > > > sheets/ADMV1013.pdf
> > > > > >
> > > > > > Please see Page 37 of 39, Table 15, QUAD_SE_MODE.
> > > > > >
> > > > > > Regards,
> > > > > > --
> > > > > > Antoniu Miclăuş
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Rob Herring <robh@kernel.org>
> > > > > > > Sent: Tuesday, November 2, 2021 7:51 PM
> > > > > > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > > > > > Cc: jic23@kernel.org; linux-iio@vger.kernel.org;
> > > > > > devicetree@vger.kernel.org;
> > > > > > > linux-kernel@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > > Subject: Re: [PATCH v3 2/2] dt-bindings: iio: frequency: add
> > admv1013
> > > > doc
> > > > > > >
> > > > > > > [External]
> > > > > > >
> > > > > > > On Mon, Nov 01, 2021 at 12:04:20PM +0200, Antoniu Miclaus wrote:
> > > > > > > > Add device tree bindings for the ADMV1013 Upconverter.
> > > > > > > >
> > > > > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > > > > > > ---
> > > > > > > > .../bindings/iio/frequency/adi,admv1013.yaml | 119
> > > > > > > ++++++++++++++++++
> > > > > > > > 1 file changed, 119 insertions(+)
> > > > > > > > create mode 100644
> > > > > > >
> > Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > > > > >
> > > > > > > > diff --git
> > > > > > >
> > > >
> > a/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > > > >
> > > >
> > b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..47993253a586
> > > > > > > > --- /dev/null
> > > > > > > > +++
> > > > > > >
> > > >
> > b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.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,admv1013.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > > > > > > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKQGXrugy$
> > > > > > > > +$schema:
> > > > https://urldefense.com/v3/__http://devicetree.org/meta-
> > > > > > >
> > > > > >
> > > >
> > schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!uTDPalOgj6YS_vZ6bsDSbA_Qna6Q
> > > > > > > OwMpoRxzo6nn06i5TNuGWZEk9PvtbC6SKYugV1fM$
> > > > > > > > +
> > > > > > > > +title: ADMV1013 Microwave Upconverter
> > > > > > > > +
> > > > > > > > +maintainers:
> > > > > > > > + - Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > > > > > > +
> > > > > > > > +description: |
> > > > > > > > + Wideband, microwave upconverter optimized for point to point
> > > > > > > microwave
> > > > > > > > + radio designs operating in the 24 GHz to 44 GHz frequency
> > range.
> > > > > > > > +
> > > > > > > > + https://www.analog.com/en/products/admv1013.html
> > > > > > > > +
> > > > > > > > +properties:
> > > > > > > > + compatible:
> > > > > > > > + enum:
> > > > > > > > + - adi,admv1013
> > > > > > > > +
> > > > > > > > + reg:
> > > > > > > > + maxItems: 1
> > > > > > > > +
> > > > > > > > + spi-max-frequency:
> > > > > > > > + maximum: 1000000
> > > > > > > > +
> > > > > > > > + clocks:
> > > > > > > > + description:
> > > > > > > > + Definition of the external clock.
> > > > > > > > + minItems: 1
> > > > > > > > +
> > > > > > > > + clock-names:
> > > > > > > > + items:
> > > > > > > > + - const: lo_in
> > > > > > > > +
> > > > > > > > + clock-output-names:
> > > > > > > > + maxItems: 1
> > > > > > > > +
> > > > > > > > + vcm-supply:
> > > > > > > > + description:
> > > > > > > > + Analog voltage regulator.
> > > > > > > > +
> > > > > > > > + adi,vga-powerdown:
> > > > > > > > + description:
> > > > > > > > + Power Down the Voltage Gain Amplifier Circuit available at
> > > > > > > > + BG_RBIAS2 pin.
> > > > > > > > + type: boolean
> > > >
> > > > What wiring would make it sensible to always have this powered down?
> > > > If we can describe that rather than vga-powerdown then that is what
> > should
> > > > be in the binding. If there isn't any wiring based justification and this
> > > > is just turning off part of the device, then it should not be in the binding.
> > > >
> > > This part is similar to ADRF6780 that has also an Enable Register that can
> > power down circuit blocks within the part:
> > >
> > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/g
> > it/jic23/iio.git/tree/drivers/iio/frequency/adrf6780.c?h=togreg__;!!A3Ni8CS0
> > y2Y!sZfM9jaILdXhyFOXGx3KQHQOxNJaKopL7sEw2F-
> > xzEWT0xHdoIW5sUEYE9ezdwHxPPWy$
> > >
> >
> > Hmm. Guess we weren't super awake with that one :(
> >
> > > I preferred to expose these properties in the device tree, rather than
> > creating custom device attributes in the driver.
> > >
> > > But I guess these properties can be managed also only from debug fs.
> >
> > The ideal situation is to control these based on what the device is being used
> > for
> > and not provide explicit control to userspace or via dt.
> >
> > So if it doesn't make sense to power up a unit because it isn't wired to
> > anything
> > then express the wiring in DT.
>
> All these powerdown properties have individual supply pins assigned (VCC_MIXER,
> VCC_BG, VCC_QUAD... datasheet - page 8 of 39).
> So I guess we can consider to enable/disable these blocks based on the wiring.
Hmm. You could treat those as optional regulators so as to figure out if they are
connected or not. If that seems sensible to you just make sure you call out the
fact they will control if the block is enabled by their presence.
Whilst we use presence or not of vref-supply in many drivers to identify if an
internal regulator should be used, in that case we have to be able to read the
reference voltage to establish scale so it needs to be specified.
For these cases it is a little less clear as they are fixed supplies and we only
care if they are there or not. As such a stub supply would be be fine if we weren't
using these as indicators of whether to power down internal blocks.
You are a lot more familiar with this device than I am so I'll leave the decision
to you, but make sure you give a clear reasoning for why a particular choice and
that the dt documentation etc clearly explains it.
>
> Looking forward to your feedback.
>
> >
> > If it makes sense to power down a unit because it isn't being used currently
> > due to what features are in use, then power it down automatically. Runtime
> > pm
> > handles a lot of these cases by letting you do autopowerdown if not used
> > after
> > a certain period. That way, we don't end up powering up and down rapidly
> > but
> > only do it if we have go reason to believe it is worth powering down.
> >
> > Could we use either of these approaches here?
> >
> > Jonathan
> > >
> > > I can adapt the code based on your decision. Looking forward to your
> > feedback.
> > > > > > > > +
> > > > > > > > + adi,mixer-powerdown:
> > > > > > > > + description:
> > > > > > > > + Power Down the Mixer Circuit. Enable to put the block in
> > > > > > > > + a power down state.
> > > >
> > > > Same for all these other power downs.
> > > >
> > > > > > > > + type: boolean
> > > > > > > > +
> > > > > > > > + adi,quad-powerdown:
> > > > > > > > + description:
> > > > > > > > + Power Down the Quadrupler. Enable to put the block in
> > > > > > > > + a power down state.
> > > > > > > > + type: boolean
> > > > > > > > +
> > > > > > > > + adi,bg-powerdown:
> > > > > > > > + description:
> > > > > > > > + Power Down the Transmitter Band Gap. Enable to put the
> > part in
> > > > > > > > + a power down state.
> > > > > > > > + type: boolean
> > > > > > > > +
> > > > > > > > + adi,mixer-if-enable:
> > > > > > > > + description:
> > > > > > > > + Enable the Intermediate Frequency Mode. Either IF Mode or
> > I/Q
> > > > > > Mode
> > > > > > > > + can be enabled at a time.
> > > > > > > > + type: boolean
> > > > > > > > +
> > > > > > > > + adi,detector-enable:
> > > > > > > > + description:
> > > > > > > > + Enable the Envelope Detector available at output pins
> > VENV_P
> > > > and
> > > > > > > > + VENV_N. Disable to reduce power consumption.
> > > > > > > > + type: boolean
> > > > > > > > +
> > > > > > > > + adi,quad-se-mode:
> > > > > > > > + description:
> > > > > > > > + Switch the LO path from differential to single-ended
> > operation.
> > > > > > > > + Set value 6 for Single-Ended Mode, Negative Side Disabled.
> > > > > > > > + Set value 9 for Single-Ended Mode, Positive Side Disabled.
> > > > > > > > + Set value 12 for Differential Mode.
> > > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > > + enum: [6, 9, 12]
> > > > > > >
> > > > > > > All these vendor properties are fixed based on the board design or
> > > > > > > something a user may want to change? The latter does not belong
> > in
> > > > DT.
> > > > > > >
> > > > > > > Rob
> > >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-11-13 16:19 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-01 10:04 [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013 Antoniu Miclaus
2021-11-01 10:04 ` [PATCH v3 2/2] dt-bindings: iio: frequency: add admv1013 doc Antoniu Miclaus
2021-11-02 17:50 ` Rob Herring
2021-11-03 8:09 ` Miclaus, Antoniu
2021-11-03 14:30 ` Miclaus, Antoniu
2021-11-04 18:26 ` Jonathan Cameron
2021-11-05 8:38 ` Miclaus, Antoniu
2021-11-05 17:10 ` Jonathan Cameron
2021-11-08 10:01 ` Miclaus, Antoniu
2021-11-13 16:24 ` Jonathan Cameron
2021-11-02 10:09 ` [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013 Sa, Nuno
2021-11-03 20:03 ` Jonathan Cameron
2021-11-04 8:11 ` Sa, Nuno
2021-11-04 18:23 ` Jonathan Cameron
2021-11-05 9:11 ` Sa, Nuno
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).