* [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013
@ 2021-10-27 9:23 Antoniu Miclaus
2021-10-27 9:23 ` [PATCH v2 2/2] dt-bindings: iio: frequency: add admv1013 doc Antoniu Miclaus
2021-10-27 17:43 ` [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013 Jonathan Cameron
0 siblings, 2 replies; 10+ messages in thread
From: Antoniu Miclaus @ 2021-10-27 9:23 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 v2:
- Kconfig: put all dependencies in one line
- remove `indio_dev->dev.parent = &spi->dev`
drivers/iio/frequency/Kconfig | 11 +
drivers/iio/frequency/Makefile | 1 +
drivers/iio/frequency/admv1013.c | 578 +++++++++++++++++++++++++++++++
3 files changed, 590 insertions(+)
create mode 100644 drivers/iio/frequency/admv1013.c
diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
index 240b81502512..6a1950a0b8fa 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 && 64BIT
+ 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..91254605013c
--- /dev/null
+++ b/drivers/iio/frequency/admv1013.c
@@ -0,0 +1,578 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ADMV1013 driver
+ *
+ * Copyright 2021 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.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/notifier.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.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_I Map */
+#define ADMV1013_LOAMP_PH_ADJ_I_FINE_MSK GENMASK(13, 7)
+#define ADMV1013_MIXER_VGATE_MSK GENMASK(6, 0)
+
+/* ADMV1013_REG_LO_AMP_Q Map */
+#define ADMV1013_LOAMP_PH_ADJ_Q_FINE_MSK GENMASK(13, 7)
+
+/* ADMV1013_REG_OFFSET_ADJUST_I Map */
+#define ADMV1013_MIXER_OFF_ADJ_I_P_MSK GENMASK(15, 9)
+#define ADMV1013_MIXER_OFF_ADJ_I_N_MSK GENMASK(8, 2)
+
+/* ADMV1013_REG_OFFSET_ADJUST_Q Map */
+#define ADMV1013_MIXER_OFF_ADJ_Q_P_MSK GENMASK(15, 9)
+#define ADMV1013_MIXER_OFF_ADJ_Q_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 */
+ 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;
+ int ret;
+
+ switch (info) {
+ case IIO_CHAN_INFO_OFFSET:
+ if (chan->channel2 == IIO_MOD_I) {
+ ret = admv1013_spi_read(st, ADMV1013_REG_OFFSET_ADJUST_I, &data);
+ if (ret)
+ return ret;
+
+ *val = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_I_P_MSK, data);
+ *val2 = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_I_N_MSK, data);
+ } else {
+ ret = admv1013_spi_read(st, ADMV1013_REG_OFFSET_ADJUST_Q, &data);
+ if (ret)
+ return ret;
+
+ *val = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_Q_P_MSK, data);
+ *val2 = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_Q_N_MSK, data);
+ }
+
+ return IIO_VAL_INT_MULTIPLE;
+ case IIO_CHAN_INFO_PHASE:
+ if (chan->channel2 == IIO_MOD_I) {
+ ret = admv1013_spi_read(st, ADMV1013_REG_LO_AMP_I, &data);
+ if (ret)
+ return ret;
+
+ *val = FIELD_GET(ADMV1013_LOAMP_PH_ADJ_I_FINE_MSK, data);
+ } else {
+ ret = admv1013_spi_read(st, ADMV1013_REG_LO_AMP_Q, &data);
+ if (ret)
+ return ret;
+
+ *val = FIELD_GET(ADMV1013_LOAMP_PH_ADJ_Q_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_OFFSET:
+ val2 /= 100000;
+
+ if (chan->channel2 == IIO_MOD_I)
+ ret = admv1013_spi_update_bits(st, ADMV1013_REG_OFFSET_ADJUST_I,
+ ADMV1013_MIXER_OFF_ADJ_I_P_MSK |
+ ADMV1013_MIXER_OFF_ADJ_I_N_MSK,
+ FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_I_P_MSK, val) |
+ FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_I_N_MSK, val2));
+ else
+ ret = admv1013_spi_update_bits(st, ADMV1013_REG_OFFSET_ADJUST_Q,
+ ADMV1013_MIXER_OFF_ADJ_Q_P_MSK |
+ ADMV1013_MIXER_OFF_ADJ_Q_N_MSK,
+ FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_Q_P_MSK, val) |
+ FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_Q_N_MSK, val2));
+
+ 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_I_FINE_MSK,
+ FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_I_FINE_MSK, val));
+ else
+ return admv1013_spi_update_bits(st, ADMV1013_REG_LO_AMP_Q,
+ ADMV1013_LOAMP_PH_ADJ_Q_FINE_MSK,
+ FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_Q_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 >= 5400000000 && rate <= 7000000000)
+ filt_raw = 15;
+ else if (rate >= 5400000000 && rate <= 8000000000)
+ filt_raw = 10;
+ else if (rate >= 6600000000 && rate <= 9200000000)
+ 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);
+ int ret;
+
+ if (read_val)
+ ret = admv1013_spi_read(st, reg, read_val);
+ else
+ ret = admv1013_spi_write(st, reg, write_val);
+
+ return ret;
+}
+
+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;
+}
+
+static void admv1013_clk_notifier_unreg(void *data)
+{
+ struct admv1013_state *st = data;
+
+ clk_notifier_unregister(st->clkin, &st->nb);
+}
+
+#define ADMV1013_CHAN(_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) | \
+ BIT(IIO_CHAN_INFO_OFFSET) \
+ }
+
+static const struct iio_chan_spec admv1013_channels[] = {
+ ADMV1013_CHAN(0, I),
+ ADMV1013_CHAN(0, 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-pd");
+ st->mixer_pd = device_property_read_bool(&spi->dev, "adi,mixer-pd");
+ st->quad_pd = device_property_read_bool(&spi->dev, "adi,quad-pd");
+ st->bg_pd = device_property_read_bool(&spi->dev, "adi,bg-pd");
+ st->mixer_if_en = device_property_read_bool(&spi->dev, "adi,mixer-if-en");
+ st->det_en = device_property_read_bool(&spi->dev, "adi,det-en");
+
+ 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 = clk_notifier_register(st->clkin, &st->nb);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(&spi->dev, admv1013_clk_notifier_unreg, st);
+ 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] 10+ messages in thread
* [PATCH v2 2/2] dt-bindings: iio: frequency: add admv1013 doc
2021-10-27 9:23 [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013 Antoniu Miclaus
@ 2021-10-27 9:23 ` Antoniu Miclaus
2021-10-27 17:05 ` Jonathan Cameron
2021-10-27 17:43 ` [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013 Jonathan Cameron
1 sibling, 1 reply; 10+ messages in thread
From: Antoniu Miclaus @ 2021-10-27 9:23 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>
---
no changes in v2.
.../bindings/iio/frequency/adi,admv1013.yaml | 110 ++++++++++++++++++
1 file changed, 110 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..7c22202e1ffd
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
@@ -0,0 +1,110 @@
+# 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-pd:
+ description:
+ Power Down the Voltage Gain Amplifier Circuit.
+ type: boolean
+
+ adi,mixer-pd:
+ description:
+ Power Down the Mixer Circuit.
+ type: boolean
+
+ adi,quad-pd:
+ description:
+ Power Down the Quadrupler.
+ type: boolean
+
+ adi,bg-pd:
+ description:
+ Power Down the Transmitter Band Gap.
+ type: boolean
+
+ adi,mixer-if-en:
+ description:
+ Enable the Intermediate Frequency Mode.
+ type: boolean
+
+ adi,det-en:
+ description:
+ Enable the Envelope Detector.
+ type: boolean
+
+ adi,quad-se-mode:
+ description:
+ Switch the LO path from differential to single-ended operation.
+ $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-en;
+ adi,det-en;
+ };
+ };
+...
--
2.33.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: iio: frequency: add admv1013 doc
2021-10-27 9:23 ` [PATCH v2 2/2] dt-bindings: iio: frequency: add admv1013 doc Antoniu Miclaus
@ 2021-10-27 17:05 ` Jonathan Cameron
0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2021-10-27 17:05 UTC (permalink / raw)
To: Antoniu Miclaus; +Cc: robh+dt, linux-iio, devicetree, linux-kernel, nuno.sa
On Wed, 27 Oct 2021 12:23:33 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> Add device tree bindings for the ADMV1013 Upconverter.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Hi Antoniu,
There are quite a few properties in here that don't feel to me
like they should be in the device tree. However, I don't know that
much about this type of device, so perhaps they just need more
documentation to reflect how they are describing characteristics
of the circuits around the device rather than runtime decisions...
Jonathan
> ---
> no changes in v2.
> .../bindings/iio/frequency/adi,admv1013.yaml | 110 ++++++++++++++++++
> 1 file changed, 110 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..7c22202e1ffd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/frequency/adi,admv1013.yaml
> @@ -0,0 +1,110 @@
> +# 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-pd:
> + description:
This lot all sound like things we should be adjusting at runtime
as a matter of policy rather than setting in device tree.
If not, how are they related to how the device is wired up?
> + Power Down the Voltage Gain Amplifier Circuit.
> + type: boolean
> +
> + adi,mixer-pd:
> + description:
> + Power Down the Mixer Circuit.
> + type: boolean
> +
> + adi,quad-pd:
> + description:
> + Power Down the Quadrupler.
pd is not clear, if we do want these in dt then spell it out
> + type: boolean
> +
> + adi,bg-pd:
> + description:
> + Power Down the Transmitter Band Gap.
> + type: boolean
> +
> + adi,mixer-if-en:
> + description:
> + Enable the Intermediate Frequency Mode.
> + type: boolean
> +
> + adi,det-en:
> + description:
> + Enable the Envelope Detector.
> + type: boolean
> +
> + adi,quad-se-mode:
> + description:
> + Switch the LO path from differential to single-ended operation.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [6, 9, 12]
Why these values? The description sounds boolean...
> +
> + '#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-en;
> + adi,det-en;
> + };
> + };
> +...
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-10-27 9:23 [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013 Antoniu Miclaus
2021-10-27 9:23 ` [PATCH v2 2/2] dt-bindings: iio: frequency: add admv1013 doc Antoniu Miclaus
@ 2021-10-27 17:43 ` Jonathan Cameron
2021-10-28 10:08 ` Miclaus, Antoniu
1 sibling, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2021-10-27 17:43 UTC (permalink / raw)
To: Antoniu Miclaus; +Cc: robh+dt, linux-iio, devicetree, linux-kernel, nuno.sa
On Wed, 27 Oct 2021 12:23:32 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> The ADMV1013 is a wideband, microwave upconverter optimized
> for point to point microwave radio designs operating in the
> 24 GHz to 44 GHz radio frequency (RF) range.
>
> Datasheet:
> https://www.analog.com/media/en/technical-documentation/data-sheets/ADMV1013.pdf
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Hi Antoniu.
A few small things inline, but main issue in here is the use of the IIO_VAL_INT_MULTIPLE
There are very very few uses for that type (1 IIRC) and it isn't to combine multiple
values into a single sysfs attribute as shown here. As such those offset interfaces
need a rethink. We may well have to define some new ABI to support them but I don't
see a reason to not maintain the normal sysfs rule of one value per attribute.
..
> 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..91254605013c
> --- /dev/null
> +++ b/drivers/iio/frequency/admv1013.c
> @@ -0,0 +1,578 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ADMV1013 driver
> + *
> + * Copyright 2021 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.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>
Recheck this list. Should have
property.h and mod_devicetable.h
> +#include <linux/notifier.h>
> +#include <linux/regmap.h>
and not regmap as you aren't using it.
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <asm/unaligned.h>
...
> +/* ADMV1013_REG_OFFSET_ADJUST_Q Map */
> +#define ADMV1013_MIXER_OFF_ADJ_Q_P_MSK GENMASK(15, 9)
> +#define ADMV1013_MIXER_OFF_ADJ_Q_N_MSK GENMASK(8, 2)
Given these two registers have the same form, could you use generic naming
and just have them defined once?
> +
> +/* 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 */
Also against concurrent access to data. Maybe other state in software as well?
This comment needs to cover everything the lock protects - if it were just
serialization of accesses to the device then the spi subsystem would handle
that fine for us.
> + 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_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;
> + int ret;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_OFFSET:
> + if (chan->channel2 == IIO_MOD_I) {
> + ret = admv1013_spi_read(st, ADMV1013_REG_OFFSET_ADJUST_I, &data);
> + if (ret)
> + return ret;
> +
> + *val = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_I_P_MSK, data);
> + *val2 = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_I_N_MSK, data);
I mention above about generic naming for these masks. Advantage is then that this
code can be
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;
*val = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_P_MSK, data);
*val2 = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_Q_MSK, data);
return IIO_VAL_INT_MULTIPLE;
However, returning multiple integers is normally reserved for things like
quaternions where the individual parts have no meaning except when they are all present.
It's not intended for pairs like this. It should also only be used with the special
read_raw_multi callback.
So we need to rethink this interface. I'm not entirely sure what it is
doing so I'm open to suggestions on what the interface should be!
The description on the datasheet suggest to me these are for calibration tweaking
in which case they should be related to calibbias not offset as they aren't something
we should apply to a raw value in userspace (which is what offset is for).
> + } else {
> + ret = admv1013_spi_read(st, ADMV1013_REG_OFFSET_ADJUST_Q, &data);
> + if (ret)
> + return ret;
> +
> + *val = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_Q_P_MSK, data);
> + *val2 = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_Q_N_MSK, data);
> + }
> +
> + return IIO_VAL_INT_MULTIPLE;
> + case IIO_CHAN_INFO_PHASE:
> + if (chan->channel2 == IIO_MOD_I) {
> + ret = admv1013_spi_read(st, ADMV1013_REG_LO_AMP_I, &data);
> + if (ret)
> + return ret;
> +
> + *val = FIELD_GET(ADMV1013_LOAMP_PH_ADJ_I_FINE_MSK, data);
As above, the masks match for these two branches of if / else, so with a generic
name you should be able to share more code and only have to select the right register.
> + } else {
> + ret = admv1013_spi_read(st, ADMV1013_REG_LO_AMP_Q, &data);
> + if (ret)
> + return ret;
> +
> + *val = FIELD_GET(ADMV1013_LOAMP_PH_ADJ_Q_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_OFFSET:
> + val2 /= 100000;
> +
> + if (chan->channel2 == IIO_MOD_I)
> + ret = admv1013_spi_update_bits(st, ADMV1013_REG_OFFSET_ADJUST_I,
> + ADMV1013_MIXER_OFF_ADJ_I_P_MSK |
> + ADMV1013_MIXER_OFF_ADJ_I_N_MSK,
> + FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_I_P_MSK, val) |
> + FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_I_N_MSK, val2));
As above, this isn't in inline with the normal ABI conventions so needs a rethink. As far as I can
establish these two values are independent though the datasheet provides limited information.
> + else
> + ret = admv1013_spi_update_bits(st, ADMV1013_REG_OFFSET_ADJUST_Q,
> + ADMV1013_MIXER_OFF_ADJ_Q_P_MSK |
> + ADMV1013_MIXER_OFF_ADJ_Q_N_MSK,
> + FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_Q_P_MSK, val) |
> + FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_Q_N_MSK, val2));
> +
> + 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_I_FINE_MSK,
> + FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_I_FINE_MSK, val));
> + else
> + return admv1013_spi_update_bits(st, ADMV1013_REG_LO_AMP_Q,
> + ADMV1013_LOAMP_PH_ADJ_Q_FINE_MSK,
> + FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_Q_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 >= 5400000000 && rate <= 7000000000)
To reduce chance of 0s issues you could use the HZ_PER_MHZ definition in include/linux/units.h
Nice to avoid counting so many zeros when reviewing.
> + filt_raw = 15;
> + else if (rate >= 5400000000 && rate <= 8000000000)
> + filt_raw = 10;
> + else if (rate >= 6600000000 && rate <= 9200000000)
> + 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_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);
> + int ret;
> +
> + if (read_val)
> + ret = admv1013_spi_read(st, reg, read_val);
return amdv1013_spi_read() etc is a bit more concise for now
loss of readability.
> + else
> + ret = admv1013_spi_write(st, reg, write_val);
> +
> + return ret;
> +}
> +
...
> +
> +#define ADMV1013_CHAN(_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) | \
> + BIT(IIO_CHAN_INFO_OFFSET) \
> + }
> +
> +static const struct iio_chan_spec admv1013_channels[] = {
> + ADMV1013_CHAN(0, I),
> + ADMV1013_CHAN(0, Q),
> +};
...
> +
> +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-pd");
> + st->mixer_pd = device_property_read_bool(&spi->dev, "adi,mixer-pd");
> + st->quad_pd = device_property_read_bool(&spi->dev, "adi,quad-pd");
> + st->bg_pd = device_property_read_bool(&spi->dev, "adi,bg-pd");
> + st->mixer_if_en = device_property_read_bool(&spi->dev, "adi,mixer-if-en");
> + st->det_en = device_property_read_bool(&spi->dev, "adi,det-en");
Comments on these in the binding document.
> +
> + 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 = clk_notifier_register(st->clkin, &st->nb);
There seems to be a devm_clk_notifier_registers() which you should use.
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(&spi->dev, admv1013_clk_notifier_unreg, st);
> + 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);
> +}
> +
...
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-10-27 17:43 ` [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013 Jonathan Cameron
@ 2021-10-28 10:08 ` Miclaus, Antoniu
2021-10-28 10:31 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Miclaus, Antoniu @ 2021-10-28 10:08 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: robh+dt, linux-iio, devicetree, linux-kernel, Sa, Nuno
Hello Jonathan,
Thanks for the review!
Regarding the interface for the Mixer Offset adjustments:
ADMV1013_MIXER_OFF_ADJ_P
ADMV1013_MIXER_OFF_ADJ_N
These parameters are related to the LO feedthrough offset calibration.
(LO and sideband suppression)
On this matter, my suggestion would be to add IIO calibration types, something like:
IIO_CHAN_INFO_CALIBFEEDTROUGH_POS
IIO_CHAN_INFO_CALIBFEEDTROUGH_NEG
Looking forward to your feedback.
Regards,
--
Antoniu Miclăuş
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Wednesday, October 27, 2021 8:43 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: robh+dt@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 v2 1/2] iio: frequency: admv1013: add support for
> ADMV1013
>
> [External]
>
> On Wed, 27 Oct 2021 12:23:32 +0300
> Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
>
> > The ADMV1013 is a wideband, microwave upconverter optimized
> > for point to point microwave radio designs operating in the
> > 24 GHz to 44 GHz radio frequency (RF) range.
> >
> > Datasheet:
> > https://www.analog.com/media/en/technical-documentation/data-
> sheets/ADMV1013.pdf
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Hi Antoniu.
>
> A few small things inline, but main issue in here is the use of the
> IIO_VAL_INT_MULTIPLE
> There are very very few uses for that type (1 IIRC) and it isn't to combine
> multiple
> values into a single sysfs attribute as shown here. As such those offset
> interfaces
> need a rethink. We may well have to define some new ABI to support them
> but I don't
> see a reason to not maintain the normal sysfs rule of one value per attribute.
>
>
> ..
>
> > 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..91254605013c
> > --- /dev/null
> > +++ b/drivers/iio/frequency/admv1013.c
> > @@ -0,0 +1,578 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * ADMV1013 driver
> > + *
> > + * Copyright 2021 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.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>
> Recheck this list. Should have
> property.h and mod_devicetable.h
>
> > +#include <linux/notifier.h>
> > +#include <linux/regmap.h>
>
> and not regmap as you aren't using it.
>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include <asm/unaligned.h>
>
> ...
>
> > +/* ADMV1013_REG_OFFSET_ADJUST_Q Map */
> > +#define ADMV1013_MIXER_OFF_ADJ_Q_P_MSK
> GENMASK(15, 9)
> > +#define ADMV1013_MIXER_OFF_ADJ_Q_N_MSK GENMASK(8,
> 2)
> Given these two registers have the same form, could you use generic naming
> and just have them defined once?
>
> > +
> > +/* 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 */
>
> Also against concurrent access to data. Maybe other state in software as
> well?
> This comment needs to cover everything the lock protects - if it were just
> serialization of accesses to the device then the spi subsystem would handle
> that fine for us.
>
> > + 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_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;
> > + int ret;
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_OFFSET:
> > + if (chan->channel2 == IIO_MOD_I) {
> > + ret = admv1013_spi_read(st,
> ADMV1013_REG_OFFSET_ADJUST_I, &data);
> > + if (ret)
> > + return ret;
> > +
> > + *val =
> FIELD_GET(ADMV1013_MIXER_OFF_ADJ_I_P_MSK, data);
> > + *val2 =
> FIELD_GET(ADMV1013_MIXER_OFF_ADJ_I_N_MSK, data);
>
> I mention above about generic naming for these masks. Advantage is then
> that this
> code can be
>
> 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;
>
> *val = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_P_MSK,
> data);
> *val2 = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_Q_MSK,
> data);
>
> return IIO_VAL_INT_MULTIPLE;
>
> However, returning multiple integers is normally reserved for things like
> quaternions where the individual parts have no meaning except when they
> are all present.
> It's not intended for pairs like this. It should also only be used with the special
> read_raw_multi callback.
>
> So we need to rethink this interface. I'm not entirely sure what it is
> doing so I'm open to suggestions on what the interface should be!
> The description on the datasheet suggest to me these are for calibration
> tweaking
> in which case they should be related to calibbias not offset as they aren't
> something
> we should apply to a raw value in userspace (which is what offset is for).
>
>
> > + } else {
> > + ret = admv1013_spi_read(st,
> ADMV1013_REG_OFFSET_ADJUST_Q, &data);
> > + if (ret)
> > + return ret;
> > +
> > + *val =
> FIELD_GET(ADMV1013_MIXER_OFF_ADJ_Q_P_MSK, data);
> > + *val2 =
> FIELD_GET(ADMV1013_MIXER_OFF_ADJ_Q_N_MSK, data);
> > + }
> > +
> > + return IIO_VAL_INT_MULTIPLE;
> > + case IIO_CHAN_INFO_PHASE:
> > + if (chan->channel2 == IIO_MOD_I) {
> > + ret = admv1013_spi_read(st,
> ADMV1013_REG_LO_AMP_I, &data);
> > + if (ret)
> > + return ret;
> > +
> > + *val =
> FIELD_GET(ADMV1013_LOAMP_PH_ADJ_I_FINE_MSK, data);
>
> As above, the masks match for these two branches of if / else, so with a
> generic
> name you should be able to share more code and only have to select the
> right register.
>
> > + } else {
> > + ret = admv1013_spi_read(st,
> ADMV1013_REG_LO_AMP_Q, &data);
> > + if (ret)
> > + return ret;
> > +
> > + *val =
> FIELD_GET(ADMV1013_LOAMP_PH_ADJ_Q_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_OFFSET:
> > + val2 /= 100000;
> > +
> > + if (chan->channel2 == IIO_MOD_I)
> > + ret = admv1013_spi_update_bits(st,
> ADMV1013_REG_OFFSET_ADJUST_I,
> > +
> ADMV1013_MIXER_OFF_ADJ_I_P_MSK |
> > +
> ADMV1013_MIXER_OFF_ADJ_I_N_MSK,
> > +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_I_P_MSK, val) |
> > +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_I_N_MSK, val2));
>
> As above, this isn't in inline with the normal ABI conventions so needs a
> rethink. As far as I can
> establish these two values are independent though the datasheet provides
> limited information.
>
> > + else
> > + ret = admv1013_spi_update_bits(st,
> ADMV1013_REG_OFFSET_ADJUST_Q,
> > +
> ADMV1013_MIXER_OFF_ADJ_Q_P_MSK |
> > +
> ADMV1013_MIXER_OFF_ADJ_Q_N_MSK,
> > +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_Q_P_MSK, val) |
> > +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_Q_N_MSK, val2));
> > +
> > + 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_I_FINE_MSK,
> > +
> FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_I_FINE_MSK, val));
> > + else
> > + return admv1013_spi_update_bits(st,
> ADMV1013_REG_LO_AMP_Q,
> > +
> ADMV1013_LOAMP_PH_ADJ_Q_FINE_MSK,
> > +
> FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_Q_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 >= 5400000000 && rate <= 7000000000)
>
> To reduce chance of 0s issues you could use the HZ_PER_MHZ definition in
> include/linux/units.h
> Nice to avoid counting so many zeros when reviewing.
>
> > + filt_raw = 15;
> > + else if (rate >= 5400000000 && rate <= 8000000000)
> > + filt_raw = 10;
> > + else if (rate >= 6600000000 && rate <= 9200000000)
> > + 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_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);
> > + int ret;
> > +
> > + if (read_val)
> > + ret = admv1013_spi_read(st, reg, read_val);
>
> return amdv1013_spi_read() etc is a bit more concise for now
> loss of readability.
>
> > + else
> > + ret = admv1013_spi_write(st, reg, write_val);
> > +
> > + return ret;
> > +}
> > +
>
> ...
>
>
> > +
> > +#define ADMV1013_CHAN(_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) | \
> > + BIT(IIO_CHAN_INFO_OFFSET) \
> > + }
> > +
> > +static const struct iio_chan_spec admv1013_channels[] = {
> > + ADMV1013_CHAN(0, I),
> > + ADMV1013_CHAN(0, Q),
> > +};
>
> ...
>
> > +
> > +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-pd");
> > + st->mixer_pd = device_property_read_bool(&spi->dev, "adi,mixer-
> pd");
> > + st->quad_pd = device_property_read_bool(&spi->dev, "adi,quad-
> pd");
> > + st->bg_pd = device_property_read_bool(&spi->dev, "adi,bg-pd");
> > + st->mixer_if_en = device_property_read_bool(&spi->dev,
> "adi,mixer-if-en");
> > + st->det_en = device_property_read_bool(&spi->dev, "adi,det-en");
>
> Comments on these in the binding document.
>
> > +
> > + 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 = clk_notifier_register(st->clkin, &st->nb);
>
> There seems to be a devm_clk_notifier_registers() which you should use.
>
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(&spi->dev,
> admv1013_clk_notifier_unreg, st);
> > + 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);
> > +}
> > +
>
> ...
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-10-28 10:08 ` Miclaus, Antoniu
@ 2021-10-28 10:31 ` Jonathan Cameron
2021-10-29 7:49 ` Sa, Nuno
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2021-10-28 10:31 UTC (permalink / raw)
To: Miclaus, Antoniu
Cc: robh+dt, linux-iio, devicetree, linux-kernel, Sa, Nuno,
Lars-Peter Clausen
On Thu, 28 Oct 2021 10:08:08 +0000
"Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
> Hello Jonathan,
>
> Thanks for the review!
>
> Regarding the interface for the Mixer Offset adjustments:
> ADMV1013_MIXER_OFF_ADJ_P
> ADMV1013_MIXER_OFF_ADJ_N
>
> These parameters are related to the LO feedthrough offset calibration.
> (LO and sideband suppression)
>
> On this matter, my suggestion would be to add IIO calibration types, something like:
> IIO_CHAN_INFO_CALIBFEEDTROUGH_POS
> IIO_CHAN_INFO_CALIBFEEDTROUGH_NEG
These sound too specific to me - we want an interface that is potentially useful
in other places. They are affecting the 'channel' which here is
simply an alt voltage channel, but I'm assuming it's something like
separate analog tweaks to the positive and negative of the differential pair?
Current channel is represented as a single index, but one route to this would be
to have it as a differential pair.
out_altvoltage0-1_phase
for the attribute that applies at the level of the differential pair and
out_altvoltage0_calibbias
out_altvoltage1_calibbias
For the P and N signal specific attributes.
I'm kind of guessing what these tweaks actually map to though so that may
not make sense.
Lars, guessing you are more familiar with this sort of device than me,
what do you think makes sense here?
Thanks,
Jonathan
>
> Looking forward to your feedback.
>
> Regards,
> --
> Antoniu Miclăuş
>
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Wednesday, October 27, 2021 8:43 PM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > Cc: robh+dt@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 v2 1/2] iio: frequency: admv1013: add support for
> > ADMV1013
> >
> > [External]
> >
> > On Wed, 27 Oct 2021 12:23:32 +0300
> > Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> >
> > > The ADMV1013 is a wideband, microwave upconverter optimized
> > > for point to point microwave radio designs operating in the
> > > 24 GHz to 44 GHz radio frequency (RF) range.
> > >
> > > Datasheet:
> > > https://www.analog.com/media/en/technical-documentation/data-
> > sheets/ADMV1013.pdf
> > >
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > Hi Antoniu.
> >
> > A few small things inline, but main issue in here is the use of the
> > IIO_VAL_INT_MULTIPLE
> > There are very very few uses for that type (1 IIRC) and it isn't to combine
> > multiple
> > values into a single sysfs attribute as shown here. As such those offset
> > interfaces
> > need a rethink. We may well have to define some new ABI to support them
> > but I don't
> > see a reason to not maintain the normal sysfs rule of one value per attribute.
> >
> >
> > ..
> >
> > > 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..91254605013c
> > > --- /dev/null
> > > +++ b/drivers/iio/frequency/admv1013.c
> > > @@ -0,0 +1,578 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * ADMV1013 driver
> > > + *
> > > + * Copyright 2021 Analog Devices Inc.
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bitops.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>
> > Recheck this list. Should have
> > property.h and mod_devicetable.h
> >
> > > +#include <linux/notifier.h>
> > > +#include <linux/regmap.h>
> >
> > and not regmap as you aren't using it.
> >
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/spi/spi.h>
> > > +
> > > +#include <asm/unaligned.h>
> >
> > ...
> >
> > > +/* ADMV1013_REG_OFFSET_ADJUST_Q Map */
> > > +#define ADMV1013_MIXER_OFF_ADJ_Q_P_MSK
> > GENMASK(15, 9)
> > > +#define ADMV1013_MIXER_OFF_ADJ_Q_N_MSK GENMASK(8,
> > 2)
> > Given these two registers have the same form, could you use generic naming
> > and just have them defined once?
> >
> > > +
> > > +/* 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 */
> >
> > Also against concurrent access to data. Maybe other state in software as
> > well?
> > This comment needs to cover everything the lock protects - if it were just
> > serialization of accesses to the device then the spi subsystem would handle
> > that fine for us.
> >
> > > + 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_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;
> > > + int ret;
> > > +
> > > + switch (info) {
> > > + case IIO_CHAN_INFO_OFFSET:
> > > + if (chan->channel2 == IIO_MOD_I) {
> > > + ret = admv1013_spi_read(st,
> > ADMV1013_REG_OFFSET_ADJUST_I, &data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + *val =
> > FIELD_GET(ADMV1013_MIXER_OFF_ADJ_I_P_MSK, data);
> > > + *val2 =
> > FIELD_GET(ADMV1013_MIXER_OFF_ADJ_I_N_MSK, data);
> >
> > I mention above about generic naming for these masks. Advantage is then
> > that this
> > code can be
> >
> > 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;
> >
> > *val = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_P_MSK,
> > data);
> > *val2 = FIELD_GET(ADMV1013_MIXER_OFF_ADJ_Q_MSK,
> > data);
> >
> > return IIO_VAL_INT_MULTIPLE;
> >
> > However, returning multiple integers is normally reserved for things like
> > quaternions where the individual parts have no meaning except when they
> > are all present.
> > It's not intended for pairs like this. It should also only be used with the special
> > read_raw_multi callback.
> >
> > So we need to rethink this interface. I'm not entirely sure what it is
> > doing so I'm open to suggestions on what the interface should be!
> > The description on the datasheet suggest to me these are for calibration
> > tweaking
> > in which case they should be related to calibbias not offset as they aren't
> > something
> > we should apply to a raw value in userspace (which is what offset is for).
> >
> >
> > > + } else {
> > > + ret = admv1013_spi_read(st,
> > ADMV1013_REG_OFFSET_ADJUST_Q, &data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + *val =
> > FIELD_GET(ADMV1013_MIXER_OFF_ADJ_Q_P_MSK, data);
> > > + *val2 =
> > FIELD_GET(ADMV1013_MIXER_OFF_ADJ_Q_N_MSK, data);
> > > + }
> > > +
> > > + return IIO_VAL_INT_MULTIPLE;
> > > + case IIO_CHAN_INFO_PHASE:
> > > + if (chan->channel2 == IIO_MOD_I) {
> > > + ret = admv1013_spi_read(st,
> > ADMV1013_REG_LO_AMP_I, &data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + *val =
> > FIELD_GET(ADMV1013_LOAMP_PH_ADJ_I_FINE_MSK, data);
> >
> > As above, the masks match for these two branches of if / else, so with a
> > generic
> > name you should be able to share more code and only have to select the
> > right register.
> >
> > > + } else {
> > > + ret = admv1013_spi_read(st,
> > ADMV1013_REG_LO_AMP_Q, &data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + *val =
> > FIELD_GET(ADMV1013_LOAMP_PH_ADJ_Q_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_OFFSET:
> > > + val2 /= 100000;
> > > +
> > > + if (chan->channel2 == IIO_MOD_I)
> > > + ret = admv1013_spi_update_bits(st,
> > ADMV1013_REG_OFFSET_ADJUST_I,
> > > +
> > ADMV1013_MIXER_OFF_ADJ_I_P_MSK |
> > > +
> > ADMV1013_MIXER_OFF_ADJ_I_N_MSK,
> > > +
> > FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_I_P_MSK, val) |
> > > +
> > FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_I_N_MSK, val2));
> >
> > As above, this isn't in inline with the normal ABI conventions so needs a
> > rethink. As far as I can
> > establish these two values are independent though the datasheet provides
> > limited information.
> >
> > > + else
> > > + ret = admv1013_spi_update_bits(st,
> > ADMV1013_REG_OFFSET_ADJUST_Q,
> > > +
> > ADMV1013_MIXER_OFF_ADJ_Q_P_MSK |
> > > +
> > ADMV1013_MIXER_OFF_ADJ_Q_N_MSK,
> > > +
> > FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_Q_P_MSK, val) |
> > > +
> > FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_Q_N_MSK, val2));
> > > +
> > > + 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_I_FINE_MSK,
> > > +
> > FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_I_FINE_MSK, val));
> > > + else
> > > + return admv1013_spi_update_bits(st,
> > ADMV1013_REG_LO_AMP_Q,
> > > +
> > ADMV1013_LOAMP_PH_ADJ_Q_FINE_MSK,
> > > +
> > FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_Q_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 >= 5400000000 && rate <= 7000000000)
> >
> > To reduce chance of 0s issues you could use the HZ_PER_MHZ definition in
> > include/linux/units.h
> > Nice to avoid counting so many zeros when reviewing.
> >
> > > + filt_raw = 15;
> > > + else if (rate >= 5400000000 && rate <= 8000000000)
> > > + filt_raw = 10;
> > > + else if (rate >= 6600000000 && rate <= 9200000000)
> > > + 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_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);
> > > + int ret;
> > > +
> > > + if (read_val)
> > > + ret = admv1013_spi_read(st, reg, read_val);
> >
> > return amdv1013_spi_read() etc is a bit more concise for now
> > loss of readability.
> >
> > > + else
> > > + ret = admv1013_spi_write(st, reg, write_val);
> > > +
> > > + return ret;
> > > +}
> > > +
> >
> > ...
> >
> >
> > > +
> > > +#define ADMV1013_CHAN(_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) | \
> > > + BIT(IIO_CHAN_INFO_OFFSET) \
> > > + }
> > > +
> > > +static const struct iio_chan_spec admv1013_channels[] = {
> > > + ADMV1013_CHAN(0, I),
> > > + ADMV1013_CHAN(0, Q),
> > > +};
> >
> > ...
> >
> > > +
> > > +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-pd");
> > > + st->mixer_pd = device_property_read_bool(&spi->dev, "adi,mixer-
> > pd");
> > > + st->quad_pd = device_property_read_bool(&spi->dev, "adi,quad-
> > pd");
> > > + st->bg_pd = device_property_read_bool(&spi->dev, "adi,bg-pd");
> > > + st->mixer_if_en = device_property_read_bool(&spi->dev,
> > "adi,mixer-if-en");
> > > + st->det_en = device_property_read_bool(&spi->dev, "adi,det-en");
> >
> > Comments on these in the binding document.
> >
> > > +
> > > + 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 = clk_notifier_register(st->clkin, &st->nb);
> >
> > There seems to be a devm_clk_notifier_registers() which you should use.
> >
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = devm_add_action_or_reset(&spi->dev,
> > admv1013_clk_notifier_unreg, st);
> > > + 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);
> > > +}
> > > +
> >
> > ...
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-10-28 10:31 ` Jonathan Cameron
@ 2021-10-29 7:49 ` Sa, Nuno
2021-10-30 15:14 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Sa, Nuno @ 2021-10-29 7:49 UTC (permalink / raw)
To: Jonathan Cameron, Miclaus, Antoniu
Cc: robh+dt, linux-iio, devicetree, linux-kernel, Lars-Peter Clausen
Hi Jonathan,
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Thursday, October 28, 2021 12:31 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: robh+dt@kernel.org; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Sa, Nuno
> <Nuno.Sa@analog.com>; Lars-Peter Clausen <lars@metafoo.de>
> Subject: Re: [PATCH v2 1/2] iio: frequency: admv1013: add support for
> ADMV1013
>
> On Thu, 28 Oct 2021 10:08:08 +0000
> "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
>
> > Hello Jonathan,
> >
> > Thanks for the review!
> >
> > Regarding the interface for the Mixer Offset adjustments:
> > ADMV1013_MIXER_OFF_ADJ_P
> > ADMV1013_MIXER_OFF_ADJ_N
> >
> > These parameters are related to the LO feedthrough offset
> calibration.
> > (LO and sideband suppression)
> >
> > On this matter, my suggestion would be to add IIO calibration types,
> something like:
> > IIO_CHAN_INFO_CALIBFEEDTROUGH_POS
> > IIO_CHAN_INFO_CALIBFEEDTROUGH_NEG
>
> These sound too specific to me - we want an interface that is
> potentially useful
> in other places. They are affecting the 'channel' which here is
> simply an alt voltage channel, but I'm assuming it's something like
> separate analog tweaks to the positive and negative of the differential
> pair?
That's also my understanding. This kind of calibration seems to be very
specific for TX local oscillators...
> Current channel is represented as a single index, but one route to this
> would be
> to have it as a differential pair.
>
> out_altvoltage0-1_phase
> for the attribute that applies at the level of the differential pair and
>
> out_altvoltage0_calibbias
> out_altvoltage1_calibbias
> For the P and N signal specific attributes.
Honestly, I'm not very enthusiastic with having out_altvoltage{0|1} as the
this applies to a single channel... Having this with separate indexes feels
odd to me. Even though we have the phase with "0-1", this can be a place
for misuse and confusion...
I was thinking about modifiers (to kind of represent differential channels)
but I don't think it would work out here... What about IIO_CHAN_INFO_CALIBBIAS_P
and IIO_CHAN_INFO_CALIBBIAS_N? Or maybe just something like
IIO_CHAN_INFO_CALIBBIAS_DIFF and internally in IIO we would automatically
create both P and N sysfs files since I don't think it makes senses in any case to
just define one of the calibrations... Anyways, these are my 5 cents :)
- Nuno Sá
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-10-29 7:49 ` Sa, Nuno
@ 2021-10-30 15:14 ` Jonathan Cameron
2021-11-02 10:00 ` Sa, Nuno
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2021-10-30 15:14 UTC (permalink / raw)
To: Sa, Nuno
Cc: Miclaus, Antoniu, robh+dt, linux-iio, devicetree, linux-kernel,
Lars-Peter Clausen
On Fri, 29 Oct 2021 07:49:41 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> Hi Jonathan,
>
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Thursday, October 28, 2021 12:31 PM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > Cc: robh+dt@kernel.org; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Sa, Nuno
> > <Nuno.Sa@analog.com>; Lars-Peter Clausen <lars@metafoo.de>
> > Subject: Re: [PATCH v2 1/2] iio: frequency: admv1013: add support for
> > ADMV1013
> >
> > On Thu, 28 Oct 2021 10:08:08 +0000
> > "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
> >
> > > Hello Jonathan,
> > >
> > > Thanks for the review!
> > >
> > > Regarding the interface for the Mixer Offset adjustments:
> > > ADMV1013_MIXER_OFF_ADJ_P
> > > ADMV1013_MIXER_OFF_ADJ_N
> > >
> > > These parameters are related to the LO feedthrough offset
> > calibration.
> > > (LO and sideband suppression)
> > >
> > > On this matter, my suggestion would be to add IIO calibration types,
> > something like:
> > > IIO_CHAN_INFO_CALIBFEEDTROUGH_POS
> > > IIO_CHAN_INFO_CALIBFEEDTROUGH_NEG
> >
> > These sound too specific to me - we want an interface that is
> > potentially useful
> > in other places. They are affecting the 'channel' which here is
> > simply an alt voltage channel, but I'm assuming it's something like
> > separate analog tweaks to the positive and negative of the differential
> > pair?
>
> That's also my understanding. This kind of calibration seems to be very
> specific for TX local oscillators...
>
> > Current channel is represented as a single index, but one route to this
> > would be
> > to have it as a differential pair.
> >
> > out_altvoltage0-1_phase
> > for the attribute that applies at the level of the differential pair and
> >
> > out_altvoltage0_calibbias
> > out_altvoltage1_calibbias
> > For the P and N signal specific attributes.
>
> Honestly, I'm not very enthusiastic with having out_altvoltage{0|1} as the
> this applies to a single channel... Having this with separate indexes feels
> odd to me. Even though we have the phase with "0-1", this can be a place
> for misuse and confusion...
>
> I was thinking about modifiers (to kind of represent differential channels)
> but I don't think it would work out here... What about IIO_CHAN_INFO_CALIBBIAS_P
> and IIO_CHAN_INFO_CALIBBIAS_N? Or maybe just something like
> IIO_CHAN_INFO_CALIBBIAS_DIFF and internally in IIO we would automatically
> create both P and N sysfs files since I don't think it makes senses in any case to
> just define one of the calibrations... Anyways, these are my 5 cents :)
Definitely not a modifier. It's almost arguable that these are different
characteristics of the channel so I can live with the ABI perhaps, but
unless this is a very common thing I'm not sure I want to burn 2 chan info
bits for them. Note we are running very low on those anyway without changing
how those are handled. We will need to tackle that anyway, but let's not
tie that to this driver.
I don't like the idea of adding core magic to spin multiple files from one
without more usecases. So for now use extended attributes for these if
we go this way.
I think I still prefer the separate channels approach. Note this is how
we deal with devices that are capable of either single ended or differential
operation. The channel numbering is reflecting the wire in both cases.
However, I'm not sure we've ever made it clear the ABI would apply like this.
We may have devices that use this interface but expect it to not apply to
the differential case....
Jonathan
>
> - Nuno Sá
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-10-30 15:14 ` Jonathan Cameron
@ 2021-11-02 10:00 ` Sa, Nuno
2021-11-03 17:25 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Sa, Nuno @ 2021-11-02 10:00 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Miclaus, Antoniu, robh+dt, linux-iio, devicetree, linux-kernel,
Lars-Peter Clausen
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, October 30, 2021 5:15 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; Lars-Peter
> Clausen <lars@metafoo.de>
> Subject: Re: [PATCH v2 1/2] iio: frequency: admv1013: add support for
> ADMV1013
>
> [External]
>
> On Fri, 29 Oct 2021 07:49:41 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
>
> > Hi Jonathan,
> >
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Thursday, October 28, 2021 12:31 PM
> > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > Cc: robh+dt@kernel.org; linux-iio@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Sa,
> Nuno
> > > <Nuno.Sa@analog.com>; Lars-Peter Clausen <lars@metafoo.de>
> > > Subject: Re: [PATCH v2 1/2] iio: frequency: admv1013: add support
> for
> > > ADMV1013
> > >
> > > On Thu, 28 Oct 2021 10:08:08 +0000
> > > "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
> > >
> > > > Hello Jonathan,
> > > >
> > > > Thanks for the review!
> > > >
> > > > Regarding the interface for the Mixer Offset adjustments:
> > > > ADMV1013_MIXER_OFF_ADJ_P
> > > > ADMV1013_MIXER_OFF_ADJ_N
> > > >
> > > > These parameters are related to the LO feedthrough offset
> > > calibration.
> > > > (LO and sideband suppression)
> > > >
> > > > On this matter, my suggestion would be to add IIO calibration
> types,
> > > something like:
> > > > IIO_CHAN_INFO_CALIBFEEDTROUGH_POS
> > > > IIO_CHAN_INFO_CALIBFEEDTROUGH_NEG
> > >
> > > These sound too specific to me - we want an interface that is
> > > potentially useful
> > > in other places. They are affecting the 'channel' which here is
> > > simply an alt voltage channel, but I'm assuming it's something like
> > > separate analog tweaks to the positive and negative of the
> differential
> > > pair?
> >
> > That's also my understanding. This kind of calibration seems to be
> very
> > specific for TX local oscillators...
> >
> > > Current channel is represented as a single index, but one route to
> this
> > > would be
> > > to have it as a differential pair.
> > >
> > > out_altvoltage0-1_phase
> > > for the attribute that applies at the level of the differential pair and
> > >
> > > out_altvoltage0_calibbias
> > > out_altvoltage1_calibbias
> > > For the P and N signal specific attributes.
> >
> > Honestly, I'm not very enthusiastic with having out_altvoltage{0|1}
> as the
> > this applies to a single channel... Having this with separate indexes
> feels
> > odd to me. Even though we have the phase with "0-1", this can be a
> place
> > for misuse and confusion...
> >
> > I was thinking about modifiers (to kind of represent differential
> channels)
> > but I don't think it would work out here... What about
> IIO_CHAN_INFO_CALIBBIAS_P
> > and IIO_CHAN_INFO_CALIBBIAS_N? Or maybe just something like
> > IIO_CHAN_INFO_CALIBBIAS_DIFF and internally in IIO we would
> automatically
> > create both P and N sysfs files since I don't think it makes senses in
> any case to
> > just define one of the calibrations... Anyways, these are my 5 cents :)
>
> Definitely not a modifier. It's almost arguable that these are different
> characteristics of the channel so I can live with the ABI perhaps, but
> unless this is a very common thing I'm not sure I want to burn 2 chan
> info
> bits for them. Note we are running very low on those anyway without
> changing
> how those are handled. We will need to tackle that anyway, but let's
> not
> tie that to this driver.
Hmm, Honestly I was not even thinking about the mask size and used
bits. But I guess it's very unlikely for a driver to define lots of bits in one
of the masks (just curious)?
> I don't like the idea of adding core magic to spin multiple files from one
> without more usecases. So for now use extended attributes for these
> if
> we go this way.
>
> I think I still prefer the separate channels approach. Note this is how
> we deal with devices that are capable of either single ended or
> differential
> operation. The channel numbering is reflecting the wire in both cases.
> However, I'm not sure we've ever made it clear the ABI would apply
> like this.
> We may have devices that use this interface but expect it to not apply
> to
> the differential case....
>
Well, you actually gave me something to think about over the weekend and
I'm getting more convinced with the ABI you purposed here. Getting all
the bits in one differential channel could lead to having to "duplicate" bit masks
to differentiate between P and N. Or we would have to do some non obvious
handling in the core as I was suggesting.
With your ABI, the "single ended" files already differentiate the pair. The only
thing we might be missing is to have a clear rule in the ABI docs. Like, if we have
out_altvoltageX-Y_phase and then
out_altvoltageX_calibbias
out_altvoltageY_calibbias,
it should be clear that X is the N part of the pair while Y is P. Or the other way
around... The point is to have a clear rule.
However, looking at the new series spin, it looks to me that we have an issue
that Antoniu might have to address in the series... These channels are both differential
and use modifiers and If I'm not missing nothing, we use channel2 for both cases.
I will leave a comment in the series which might be better...
- Nuno Sá
>
> >
> > - Nuno Sá
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013
2021-11-02 10:00 ` Sa, Nuno
@ 2021-11-03 17:25 ` Jonathan Cameron
0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2021-11-03 17:25 UTC (permalink / raw)
To: Sa, Nuno
Cc: Miclaus, Antoniu, robh+dt, linux-iio, devicetree, linux-kernel,
Lars-Peter Clausen
On Tue, 2 Nov 2021 10:00:58 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, October 30, 2021 5:15 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; Lars-Peter
> > Clausen <lars@metafoo.de>
> > Subject: Re: [PATCH v2 1/2] iio: frequency: admv1013: add support for
> > ADMV1013
> >
> > [External]
> >
> > On Fri, 29 Oct 2021 07:49:41 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >
> > > Hi Jonathan,
> > >
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Thursday, October 28, 2021 12:31 PM
> > > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > > Cc: robh+dt@kernel.org; linux-iio@vger.kernel.org;
> > > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Sa,
> > Nuno
> > > > <Nuno.Sa@analog.com>; Lars-Peter Clausen <lars@metafoo.de>
> > > > Subject: Re: [PATCH v2 1/2] iio: frequency: admv1013: add support
> > for
> > > > ADMV1013
> > > >
> > > > On Thu, 28 Oct 2021 10:08:08 +0000
> > > > "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:
> > > >
> > > > > Hello Jonathan,
> > > > >
> > > > > Thanks for the review!
> > > > >
> > > > > Regarding the interface for the Mixer Offset adjustments:
> > > > > ADMV1013_MIXER_OFF_ADJ_P
> > > > > ADMV1013_MIXER_OFF_ADJ_N
> > > > >
> > > > > These parameters are related to the LO feedthrough offset
> > > > calibration.
> > > > > (LO and sideband suppression)
> > > > >
> > > > > On this matter, my suggestion would be to add IIO calibration
> > types,
> > > > something like:
> > > > > IIO_CHAN_INFO_CALIBFEEDTROUGH_POS
> > > > > IIO_CHAN_INFO_CALIBFEEDTROUGH_NEG
> > > >
> > > > These sound too specific to me - we want an interface that is
> > > > potentially useful
> > > > in other places. They are affecting the 'channel' which here is
> > > > simply an alt voltage channel, but I'm assuming it's something like
> > > > separate analog tweaks to the positive and negative of the
> > differential
> > > > pair?
> > >
> > > That's also my understanding. This kind of calibration seems to be
> > very
> > > specific for TX local oscillators...
> > >
> > > > Current channel is represented as a single index, but one route to
> > this
> > > > would be
> > > > to have it as a differential pair.
> > > >
> > > > out_altvoltage0-1_phase
> > > > for the attribute that applies at the level of the differential pair and
> > > >
> > > > out_altvoltage0_calibbias
> > > > out_altvoltage1_calibbias
> > > > For the P and N signal specific attributes.
> > >
> > > Honestly, I'm not very enthusiastic with having out_altvoltage{0|1}
> > as the
> > > this applies to a single channel... Having this with separate indexes
> > feels
> > > odd to me. Even though we have the phase with "0-1", this can be a
> > place
> > > for misuse and confusion...
> > >
> > > I was thinking about modifiers (to kind of represent differential
> > channels)
> > > but I don't think it would work out here... What about
> > IIO_CHAN_INFO_CALIBBIAS_P
> > > and IIO_CHAN_INFO_CALIBBIAS_N? Or maybe just something like
> > > IIO_CHAN_INFO_CALIBBIAS_DIFF and internally in IIO we would
> > automatically
> > > create both P and N sysfs files since I don't think it makes senses in
> > any case to
> > > just define one of the calibrations... Anyways, these are my 5 cents :)
> >
> > Definitely not a modifier. It's almost arguable that these are different
> > characteristics of the channel so I can live with the ABI perhaps, but
> > unless this is a very common thing I'm not sure I want to burn 2 chan
> > info
> > bits for them. Note we are running very low on those anyway without
> > changing
> > how those are handled. We will need to tackle that anyway, but let's
> > not
> > tie that to this driver.
>
> Hmm, Honestly I was not even thinking about the mask size and used
> bits. But I guess it's very unlikely for a driver to define lots of bits in one
> of the masks (just curious)?
It's a fairly small set in most cases, but as it is a bitmap that doesn't
help us. We still need to fit in an unsigned long (so 32 bits).
Any change to how we do this may be painful. We might just jump to the
BIT_ULL() approach for now and be a bit more resistant to adding more
entries in future.
>
> > I don't like the idea of adding core magic to spin multiple files from one
> > without more usecases. So for now use extended attributes for these
> > if
> > we go this way.
> >
> > I think I still prefer the separate channels approach. Note this is how
> > we deal with devices that are capable of either single ended or
> > differential
> > operation. The channel numbering is reflecting the wire in both cases.
> > However, I'm not sure we've ever made it clear the ABI would apply
> > like this.
> > We may have devices that use this interface but expect it to not apply
> > to
> > the differential case....
> >
>
> Well, you actually gave me something to think about over the weekend and
> I'm getting more convinced with the ABI you purposed here. Getting all
> the bits in one differential channel could lead to having to "duplicate" bit masks
> to differentiate between P and N. Or we would have to do some non obvious
> handling in the core as I was suggesting.
>
> With your ABI, the "single ended" files already differentiate the pair. The only
> thing we might be missing is to have a clear rule in the ABI docs. Like, if we have
>
> out_altvoltageX-Y_phase and then
>
> out_altvoltageX_calibbias
> out_altvoltageY_calibbias,
>
> it should be clear that X is the N part of the pair while Y is P. Or the other way
> around... The point is to have a clear rule.
differential channel is X-Y so P-N would be how I would interpret it.
>
> However, looking at the new series spin, it looks to me that we have an issue
> that Antoniu might have to address in the series... These channels are both differential
> and use modifiers and If I'm not missing nothing, we use channel2 for both cases.
ouch. That indeed is going to blow up. Can't have modified differential channels
and that is very hard to work around because of lack of space in the events format.
I couldn't think of a reason why we'd have differential modified channels when
I made this stuff up...
Jonathan
>
> I will leave a comment in the series which might be better...
>
> - Nuno Sá
> >
> > >
> > > - Nuno Sá
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-11-03 17:21 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-27 9:23 [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013 Antoniu Miclaus
2021-10-27 9:23 ` [PATCH v2 2/2] dt-bindings: iio: frequency: add admv1013 doc Antoniu Miclaus
2021-10-27 17:05 ` Jonathan Cameron
2021-10-27 17:43 ` [PATCH v2 1/2] iio: frequency: admv1013: add support for ADMV1013 Jonathan Cameron
2021-10-28 10:08 ` Miclaus, Antoniu
2021-10-28 10:31 ` Jonathan Cameron
2021-10-29 7:49 ` Sa, Nuno
2021-10-30 15:14 ` Jonathan Cameron
2021-11-02 10:00 ` Sa, Nuno
2021-11-03 17:25 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.