* [PATCH 0/2] iio: frequency: adrf6780: add support for ADRF6780
@ 2021-06-29 14:23 Antoniu Miclaus
2021-06-29 14:23 ` [PATCH 1/2] " Antoniu Miclaus
2021-06-29 14:23 ` [PATCH 2/2] dt-bindings: iio: frequency: add adrf6780 doc Antoniu Miclaus
0 siblings, 2 replies; 7+ messages in thread
From: Antoniu Miclaus @ 2021-06-29 14:23 UTC (permalink / raw)
To: linux-iio, linux-kernel, jic23, devicetree, robh+dt; +Cc: Antoniu Miclaus
Add support for the ADRF6780 microwave upconverter.
Antoniu Miclaus (2):
iio: frequency: adrf6780: add support for ADRF6780
dt-bindings: iio: frequency: add adrf6780 doc
.../bindings/iio/frequency/adi,adrf6780.yaml | 133 +++++
drivers/iio/frequency/Kconfig | 13 +
drivers/iio/frequency/Makefile | 1 +
drivers/iio/frequency/adrf6780.c | 534 ++++++++++++++++++
4 files changed, 681 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
create mode 100644 drivers/iio/frequency/adrf6780.c
--
2.32.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-06-29 14:23 [PATCH 0/2] iio: frequency: adrf6780: add support for ADRF6780 Antoniu Miclaus
@ 2021-06-29 14:23 ` Antoniu Miclaus
2021-06-30 8:38 ` Alexandru Ardelean
2021-06-29 14:23 ` [PATCH 2/2] dt-bindings: iio: frequency: add adrf6780 doc Antoniu Miclaus
1 sibling, 1 reply; 7+ messages in thread
From: Antoniu Miclaus @ 2021-06-29 14:23 UTC (permalink / raw)
To: linux-iio, linux-kernel, jic23, devicetree, robh+dt; +Cc: Antoniu Miclaus
Add support for the ADRF6780 microwave upconverter.
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
drivers/iio/frequency/Kconfig | 13 +
drivers/iio/frequency/Makefile | 1 +
drivers/iio/frequency/adrf6780.c | 534 +++++++++++++++++++++++++++++++
3 files changed, 548 insertions(+)
create mode 100644 drivers/iio/frequency/adrf6780.c
diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
index 240b81502512..fc9751c48f59 100644
--- a/drivers/iio/frequency/Kconfig
+++ b/drivers/iio/frequency/Kconfig
@@ -49,5 +49,18 @@ config ADF4371
To compile this driver as a module, choose M here: the
module will be called adf4371.
+
+config ADRF6780
+ tristate "Analog Devices ADRF6780 Microwave Upconverter"
+ depends on SPI
+ depends on COMMON_CLK
+ depends on OF
+ help
+ Say yes here to build support for Analog Devices ADRF6780
+ 5.9 GHz to 23.6 GHz, Wideband, Microwave Upconverter.
+
+ To compile this driver as a module, choose M here: the
+ module will be called adrf6780.
+
endmenu
endmenu
diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
index 518b1e50caef..ae3136c79202 100644
--- a/drivers/iio/frequency/Makefile
+++ b/drivers/iio/frequency/Makefile
@@ -7,3 +7,4 @@
obj-$(CONFIG_AD9523) += ad9523.o
obj-$(CONFIG_ADF4350) += adf4350.o
obj-$(CONFIG_ADF4371) += adf4371.o
+obj-$(CONFIG_ADRF6780) += adrf6780.o
diff --git a/drivers/iio/frequency/adrf6780.c b/drivers/iio/frequency/adrf6780.c
new file mode 100644
index 000000000000..c492c4e4adf1
--- /dev/null
+++ b/drivers/iio/frequency/adrf6780.c
@@ -0,0 +1,534 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ADRF6780 driver
+ *
+ * Copyright 2021 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+/* ADRF6780 Register Map */
+#define ADRF6780_REG_CONTROL 0x00
+#define ADRF6780_REG_ALARM_READBACK 0x01
+#define ADRF6780_REG_ALARM_MASKS 0x02
+#define ADRF6780_REG_ENABLE 0x03
+#define ADRF6780_REG_LINEARIZE 0x04
+#define ADRF6780_REG_LO_PATH 0x05
+#define ADRF6780_REG_ADC_CONTROL 0x06
+#define ADRF6780_REG_ADC_OUTPUT 0x0C
+
+/* ADRF6780_REG_CONTROL Map */
+#define ADRF6780_PARITY_EN_MSK BIT(15)
+#define ADRF6780_PARITY_EN(x) FIELD_PREP(ADRF6780_PARITY_EN_MSK, x)
+#define ADRF6780_SOFT_RESET_MSK BIT(14)
+#define ADRF6780_SOFT_RESET(x) FIELD_PREP(ADRF6780_SOFT_RESET_MSK, x)
+#define ADRF6780_CHIP_ID_MSK GENMASK(11, 4)
+#define ADRF6780_CHIP_ID 0xA
+#define ADRF6780_CHIP_REVISION_MSK GENMASK(3, 0)
+#define ADRF6780_CHIP_REVISION(x) FIELD_PREP(ADRF6780_CHIP_REVISION_MSK, x)
+
+/* ADRF6780_REG_ALARM_READBACK Map */
+#define ADRF6780_PARITY_ERROR_MSK BIT(15)
+#define ADRF6780_PARITY_ERROR(x) FIELD_PREP(ADRF6780_PARITY_ERROR_MSK, x)
+#define ADRF6780_TOO_FEW_ERRORS_MSK BIT(14)
+#define ADRF6780_TOO_FEW_ERRORS(x) FIELD_PREP(ADRF6780_TOO_FEW_ERRORS_MSK, x)
+#define ADRF6780_TOO_MANY_ERRORS_MSK BIT(13)
+#define ADRF6780_TOO_MANY_ERRORS(x) FIELD_PREP(ADRF6780_TOO_MANY_ERRORS_MSK, x)
+#define ADRF6780_ADDRESS_RANGE_ERROR_MSK BIT(12)
+#define ADRF6780_ADDRESS_RANGE_ERROR(x) FIELD_PREP(ADRF6780_ADDRESS_RANGE_ERROR_MSK, x)
+
+/* ADRF6780_REG_ENABLE Map */
+#define ADRF6780_VGA_BUFFER_EN_MSK BIT(8)
+#define ADRF6780_VGA_BUFFER_EN(x) FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, x)
+#define ADRF6780_DETECTOR_EN_MSK BIT(7)
+#define ADRF6780_DETECTOR_EN(x) FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, x)
+#define ADRF6780_LO_BUFFER_EN_MSK BIT(6)
+#define ADRF6780_LO_BUFFER_EN(x) FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, x)
+#define ADRF6780_IF_MODE_EN_MSK BIT(5)
+#define ADRF6780_IF_MODE_EN(x) FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, x)
+#define ADRF6780_IQ_MODE_EN_MSK BIT(4)
+#define ADRF6780_IQ_MODE_EN(x) FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, x)
+#define ADRF6780_LO_X2_EN_MSK BIT(3)
+#define ADRF6780_LO_X2_EN(x) FIELD_PREP(ADRF6780_LO_X2_EN_MSK, x)
+#define ADRF6780_LO_PPF_EN_MSK BIT(2)
+#define ADRF6780_LO_PPF_EN(x) FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, x)
+#define ADRF6780_LO_EN_MSK BIT(1)
+#define ADRF6780_LO_EN(x) FIELD_PREP(ADRF6780_LO_EN_MSK, x)
+#define ADRF6780_UC_BIAS_EN_MSK BIT(0)
+#define ADRF6780_UC_BIAS_EN(x) FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, x)
+
+/* ADRF6780_REG_LINEARIZE Map */
+#define ADRF6780_RDAC_LINEARIZE_MSK GENMASK(7, 0)
+#define ADRF6780_RDAC_LINEARIZE(x) FIELD_PREP(ADRF6780_RDAC_LINEARIZE_MSK, x)
+
+/* ADRF6780_REG_LO_PATH Map */
+#define ADRF6780_LO_SIDEBAND_MSK BIT(10)
+#define ADRF6780_LO_SIDEBAND(x) FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, x)
+#define ADRF6780_Q_PATH_PHASE_ACCURACY_MSK GENMASK(7, 4)
+#define ADRF6780_Q_PATH_PHASE_ACCURACY(x) FIELD_PREP(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, x)
+#define ADRF6780_I_PATH_PHASE_ACCURACY_MSK GENMASK(3, 0)
+#define ADRF6780_I_PATH_PHASE_ACCURACY(x) FIELD_PREP(ADRF6780_I_PATH_PHASE_ACCURACY_MSK, x)
+
+/* ADRF6780_REG_ADC_CONTROL Map */
+#define ADRF6780_VDET_OUTPUT_SELECT_MSK BIT(3)
+#define ADRF6780_VDET_OUTPUT_SELECT(x) FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, x)
+#define ADRF6780_ADC_START_MSK BIT(2)
+#define ADRF6780_ADC_START(x) FIELD_PREP(ADRF6780_ADC_START_MSK, x)
+#define ADRF6780_ADC_EN_MSK BIT(1)
+#define ADRF6780_ADC_EN(x) FIELD_PREP(ADRF6780_ADC_EN_MSK, x)
+#define ADRF6780_ADC_CLOCK_EN_MSK BIT(0)
+#define ADRF6780_ADC_CLOCK_EN(x) FIELD_PREP(ADRF6780_ADC_CLOCK_EN_MSK, x)
+
+/* ADRF6780_REG_ADC_OUTPUT Map */
+#define ADRF6780_ADC_STATUS_MSK BIT(8)
+#define ADRF6780_ADC_STATUS(x) FIELD_PREP(ADRF6780_ADC_STATUS_MSK, x)
+#define ADRF6780_ADC_VALUE_MSK GENMASK(7, 0)
+#define ADRF6780_ADC_VALUE(x) FIELD_PREP(ADRF6780_ADC_VALUE_MSK, x)
+
+enum supported_parts {
+ ADRF6780,
+};
+
+struct adrf6780_dev {
+ struct spi_device *spi;
+ struct clk *clkin;
+ /* Protect against concurrent accesses to the device */
+ struct mutex lock;
+ bool parity_en;
+ bool vga_buff_en;
+ bool det_en;
+ bool lo_buff_en;
+ bool if_mode_en;
+ bool iq_mode_en;
+ bool lo_x2_en;
+ bool lo_ppf_en;
+ bool lo_en;
+ bool uc_bias_en;
+ bool lo_sideband;
+ bool vdet_out_en;
+};
+
+static int adrf6780_spi_read(struct adrf6780_dev *dev, unsigned int reg,
+ unsigned int *val)
+{
+ int ret;
+ unsigned int cnt, temp;
+ struct spi_transfer t = {0};
+ u8 data[3];
+
+ data[0] = 0x80 | (reg << 1);
+ data[1] = 0x0;
+ data[2] = 0x0;
+
+ t.rx_buf = &data[0];
+ t.tx_buf = &data[0];
+ t.len = 3;
+
+ ret = spi_sync_transfer(dev->spi, &t, 1);
+ if (ret < 0)
+ return ret;
+
+ temp = ((data[0] | 0x80 | (reg << 1)) << 16) |
+ (data[1] << 8) | data[2];
+
+ if (dev->parity_en) {
+ cnt = hweight_long(temp);
+ if (!(cnt % 2))
+ return -EINVAL;
+ }
+
+ *val = (temp >> 1) & 0xFFFF;
+
+ return ret;
+}
+
+static int adrf6780_spi_write(struct adrf6780_dev *dev,
+ unsigned int reg,
+ unsigned int val)
+{
+ unsigned int cnt;
+ u8 data[3];
+
+ val = (val << 1);
+
+ if (dev->parity_en) {
+ cnt = hweight_long((reg << 17) | val);
+ if (cnt % 2 == 0)
+ val |= 0x1;
+ }
+
+ data[0] = (reg << 1) | (val >> 16);
+ data[1] = val >> 8;
+ data[2] = val;
+
+ return spi_write(dev->spi, &data[0], 3);
+}
+
+static int __adrf6780_spi_update_bits(struct adrf6780_dev *dev, unsigned int reg,
+ unsigned int mask, unsigned int val)
+{
+ int ret;
+ unsigned int data, temp;
+
+ ret = adrf6780_spi_read(dev, reg, &data);
+ if (ret < 0)
+ return ret;
+
+ temp = (data & ~mask) | (val & mask);
+
+ return adrf6780_spi_write(dev, reg, temp);
+}
+
+static int adrf6780_spi_update_bits(struct adrf6780_dev *dev, unsigned int reg,
+ unsigned int mask, unsigned int val)
+{
+ int ret;
+
+ mutex_lock(&dev->lock);
+ ret = __adrf6780_spi_update_bits(dev, reg, mask, val);
+ mutex_unlock(&dev->lock);
+ return ret;
+}
+
+static int adrf6780_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long info)
+{
+ struct adrf6780_dev *dev = iio_priv(indio_dev);
+ unsigned int data;
+ int ret;
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ mutex_lock(&dev->lock);
+
+ ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ENABLE,
+ ADRF6780_DETECTOR_EN_MSK,
+ ADRF6780_DETECTOR_EN(1));
+ if (ret < 0)
+ goto exit;
+
+ ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
+ ADRF6780_ADC_EN_MSK,
+ ADRF6780_ADC_EN(1));
+ if (ret < 0)
+ goto exit;
+
+ ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
+ ADRF6780_ADC_CLOCK_EN_MSK,
+ ADRF6780_ADC_CLOCK_EN(1));
+ if (ret < 0)
+ goto exit;
+
+ ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
+ ADRF6780_ADC_START_MSK,
+ ADRF6780_ADC_START(1));
+ if (ret < 0)
+ goto exit;
+
+ usleep_range(200, 250);
+
+ ret = adrf6780_spi_read(dev, ADRF6780_REG_ADC_OUTPUT, &data);
+ if (ret < 0)
+ goto exit;
+
+ if (!(data & ADRF6780_ADC_STATUS_MSK)) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
+ ADRF6780_ADC_START_MSK,
+ ADRF6780_ADC_START(0));
+ if (ret < 0)
+ goto exit;
+
+ ret = adrf6780_spi_read(dev, ADRF6780_REG_ADC_OUTPUT, &data);
+ if (ret < 0)
+ goto exit;
+
+ mutex_unlock(&dev->lock);
+
+ *val = data & ADRF6780_ADC_VALUE_MSK;
+
+ return IIO_VAL_INT;
+exit:
+ mutex_unlock(&dev->lock);
+ return ret;
+ case IIO_CHAN_INFO_SCALE:
+ ret = adrf6780_spi_read(dev, ADRF6780_REG_LINEARIZE, &data);
+ if (ret < 0)
+ return ret;
+
+ *val = data & ADRF6780_RDAC_LINEARIZE_MSK;
+
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_PHASE:
+ ret = adrf6780_spi_read(dev, ADRF6780_REG_LO_PATH, &data);
+ if (ret < 0)
+ return ret;
+
+ if (chan->channel2 == IIO_MOD_I)
+ *val = data & ADRF6780_I_PATH_PHASE_ACCURACY_MSK;
+ else
+ *val = (data & ADRF6780_Q_PATH_PHASE_ACCURACY_MSK) >> 4;
+
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adrf6780_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long info)
+{
+ struct adrf6780_dev *dev = iio_priv(indio_dev);
+ int ret;
+
+ switch (info) {
+ case IIO_CHAN_INFO_SCALE:
+ return adrf6780_spi_write(dev, ADRF6780_REG_LINEARIZE, val);
+ case IIO_CHAN_INFO_PHASE:
+ if (chan->channel2 == IIO_MOD_I)
+ ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
+ ADRF6780_I_PATH_PHASE_ACCURACY_MSK,
+ ADRF6780_I_PATH_PHASE_ACCURACY(val));
+ else
+ ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
+ ADRF6780_Q_PATH_PHASE_ACCURACY_MSK,
+ ADRF6780_Q_PATH_PHASE_ACCURACY(val));
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int adrf6780_reg_access(struct iio_dev *indio_dev,
+ unsigned int reg,
+ unsigned int write_val,
+ unsigned int *read_val)
+{
+ struct adrf6780_dev *dev = iio_priv(indio_dev);
+
+ if (read_val)
+ return adrf6780_spi_read(dev, reg, read_val);
+ else
+ return adrf6780_spi_write(dev, reg, write_val);
+}
+
+static const struct iio_info adrf6780_info = {
+ .read_raw = adrf6780_read_raw,
+ .write_raw = adrf6780_write_raw,
+ .debugfs_reg_access = &adrf6780_reg_access,
+};
+
+#define ADRF6780_CHAN(_channel) { \
+ .type = IIO_VOLTAGE, \
+ .output = 1, \
+ .indexed = 1, \
+ .channel = _channel, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE) \
+}
+
+#define ADRF6780_CHAN_IQ(_channel, rf_comp) { \
+ .type = IIO_ALTVOLTAGE, \
+ .modified = 1, \
+ .output = 1, \
+ .indexed = 1, \
+ .channel2 = IIO_MOD_##rf_comp, \
+ .channel = _channel, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PHASE) \
+}
+
+static const struct iio_chan_spec adrf6780_channels[] = {
+ ADRF6780_CHAN(0),
+ ADRF6780_CHAN_IQ(0, I),
+ ADRF6780_CHAN_IQ(0, Q),
+};
+
+static int adrf6780_init(struct adrf6780_dev *dev)
+{
+ int ret;
+ unsigned int chip_id, enable_reg, enable_reg_msk;
+ struct spi_device *spi = dev->spi;
+ bool temp_parity = dev->parity_en;
+
+ dev->parity_en = false;
+
+ /* Perform a software reset */
+ ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
+ ADRF6780_SOFT_RESET_MSK,
+ ADRF6780_SOFT_RESET(1));
+ if (ret < 0) {
+ dev_err(&spi->dev, "ADRF6780 SPI software reset failed.\n");
+ return ret;
+ }
+
+ ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
+ ADRF6780_SOFT_RESET_MSK,
+ ADRF6780_SOFT_RESET(0));
+ if (ret < 0) {
+ dev_err(&spi->dev, "ADRF6780 SPI software reset disable failed.\n");
+ return ret;
+ }
+
+ ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
+ ADRF6780_PARITY_EN_MSK,
+ ADRF6780_PARITY_EN(temp_parity));
+ if (ret < 0)
+ return ret;
+
+ dev->parity_en = temp_parity;
+
+ ret = adrf6780_spi_read(dev, ADRF6780_REG_CONTROL, &chip_id);
+ if (ret < 0)
+ return ret;
+
+ chip_id = (chip_id & ADRF6780_CHIP_ID_MSK) >> 4;
+ if (chip_id != ADRF6780_CHIP_ID) {
+ dev_err(&spi->dev, "ADRF6780 Invalid Chip ID.\n");
+ return -EINVAL;
+ }
+
+ enable_reg_msk = ADRF6780_VGA_BUFFER_EN_MSK |
+ ADRF6780_DETECTOR_EN_MSK |
+ ADRF6780_LO_BUFFER_EN_MSK |
+ ADRF6780_IF_MODE_EN_MSK |
+ ADRF6780_IQ_MODE_EN_MSK |
+ ADRF6780_LO_X2_EN_MSK |
+ ADRF6780_LO_PPF_EN_MSK |
+ ADRF6780_LO_EN_MSK |
+ ADRF6780_UC_BIAS_EN_MSK;
+
+ enable_reg = ADRF6780_VGA_BUFFER_EN(dev->vga_buff_en) |
+ ADRF6780_DETECTOR_EN(dev->det_en) |
+ ADRF6780_LO_BUFFER_EN(dev->lo_buff_en) |
+ ADRF6780_IF_MODE_EN(dev->if_mode_en) |
+ ADRF6780_IQ_MODE_EN(dev->iq_mode_en) |
+ ADRF6780_LO_X2_EN(dev->lo_x2_en) |
+ ADRF6780_LO_PPF_EN(dev->lo_ppf_en) |
+ ADRF6780_LO_EN(dev->lo_en) |
+ ADRF6780_UC_BIAS_EN(dev->uc_bias_en);
+
+ ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ENABLE, enable_reg_msk, enable_reg);
+ if (ret < 0)
+ return ret;
+
+ ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
+ ADRF6780_LO_SIDEBAND_MSK,
+ ADRF6780_LO_SIDEBAND(dev->lo_sideband));
+ if (ret < 0)
+ return ret;
+
+ return __adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
+ ADRF6780_VDET_OUTPUT_SELECT_MSK,
+ ADRF6780_VDET_OUTPUT_SELECT(dev->vdet_out_en));
+}
+
+static void adrf6780_clk_disable(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
+static int adrf6780_dt_parse(struct adrf6780_dev *dev)
+{
+ struct spi_device *spi = dev->spi;
+
+ dev->parity_en = of_property_read_bool(spi->dev.of_node, "adi,parity-en");
+ dev->vga_buff_en = of_property_read_bool(spi->dev.of_node, "adi,vga-buff-en");
+ dev->det_en = of_property_read_bool(spi->dev.of_node, "adi,det-en");
+ dev->lo_buff_en = of_property_read_bool(spi->dev.of_node, "adi,lo-buff-en");
+ dev->if_mode_en = of_property_read_bool(spi->dev.of_node, "adi,if-mode-en");
+ dev->iq_mode_en = of_property_read_bool(spi->dev.of_node, "adi,iq-mode-en");
+ dev->lo_x2_en = of_property_read_bool(spi->dev.of_node, "adi,lo-x2-en");
+ dev->lo_ppf_en = of_property_read_bool(spi->dev.of_node, "adi,lo-ppf-en");
+ dev->lo_en = of_property_read_bool(spi->dev.of_node, "adi,lo-en");
+ dev->uc_bias_en = of_property_read_bool(spi->dev.of_node, "adi,uc-bias-en");
+ dev->lo_sideband = of_property_read_bool(spi->dev.of_node, "adi,lo-sideband");
+ dev->vdet_out_en = of_property_read_bool(spi->dev.of_node, "adi,vdet-out-en");
+
+ dev->clkin = devm_clk_get(&spi->dev, "lo_in");
+ if (IS_ERR(dev->clkin))
+ return PTR_ERR(dev->clkin);
+
+ return 0;
+}
+
+static int adrf6780_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct adrf6780_dev *dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*dev));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ dev = iio_priv(indio_dev);
+
+ indio_dev->dev.parent = &spi->dev;
+ indio_dev->info = &adrf6780_info;
+ indio_dev->name = "adrf6780";
+ indio_dev->channels = adrf6780_channels;
+ indio_dev->num_channels = ARRAY_SIZE(adrf6780_channels);
+
+ dev->spi = spi;
+
+ ret = adrf6780_dt_parse(dev);
+ if (ret < 0)
+ return ret;
+
+ ret = clk_prepare_enable(dev->clkin);
+ if (ret < 0)
+ return ret;
+
+ ret = devm_add_action_or_reset(&spi->dev, adrf6780_clk_disable, dev->clkin);
+ if (ret < 0)
+ return ret;
+
+ mutex_init(&dev->lock);
+
+ ret = adrf6780_init(dev);
+ if (ret < 0)
+ return ret;
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id adrf6780_id[] = {
+ { "adrf6780", ADRF6780 },
+ {}
+};
+MODULE_DEVICE_TABLE(spi, adrf6780_id);
+
+static const struct of_device_id adrf6780_of_match[] = {
+ { .compatible = "adi,adrf6780" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, adrf6780_of_match);
+
+static struct spi_driver adrf6780_driver = {
+ .driver = {
+ .name = "adrf6780",
+ .of_match_table = adrf6780_of_match,
+ },
+ .probe = adrf6780_probe,
+ .id_table = adrf6780_id,
+};
+module_spi_driver(adrf6780_driver);
+
+MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com");
+MODULE_DESCRIPTION("Analog Devices ADRF6780");
+MODULE_LICENSE("GPL v2");
--
2.32.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] dt-bindings: iio: frequency: add adrf6780 doc
2021-06-29 14:23 [PATCH 0/2] iio: frequency: adrf6780: add support for ADRF6780 Antoniu Miclaus
2021-06-29 14:23 ` [PATCH 1/2] " Antoniu Miclaus
@ 2021-06-29 14:23 ` Antoniu Miclaus
1 sibling, 0 replies; 7+ messages in thread
From: Antoniu Miclaus @ 2021-06-29 14:23 UTC (permalink / raw)
To: linux-iio, linux-kernel, jic23, devicetree, robh+dt; +Cc: Antoniu Miclaus
Add device tree bindings for the ADRF6780 Upconverter.
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
.../bindings/iio/frequency/adi,adrf6780.yaml | 133 ++++++++++++++++++
1 file changed, 133 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
new file mode 100644
index 000000000000..008f76095a63
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
@@ -0,0 +1,133 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/frequency/adi,adrf6780.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADRF6780 Microwave Upconverter
+
+maintainers:
+- Antoniu Miclaus <antoniu.miclaus@analog.com>
+
+description: |
+ wideband, microwave upconverter optimized for point to point microwave
+ radio designs operating in the 5.9 GHz to 23.6 GHz frequency range.
+ https://www.analog.com/en/products/adrf6780.html
+
+properties:
+ compatible:
+ enum:
+ - adi,adrf6780
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 1000000
+
+ clocks:
+ description:
+ Definition of the external clock (see clock/clock-bindings.txt)
+ minItems: 1
+
+ clock-names:
+ description:
+ Must be "lo_in"
+ maxItems: 1
+
+ clock-output-names:
+ maxItems: 1
+
+ adi,parity-en:
+ description:
+ Enable Parity for Write execution.
+ type: boolean
+
+ adi,vga-buff-en:
+ description:
+ VGA Buffer Enable.
+ type: boolean
+
+ adi,det-en:
+ description:
+ Detector Enable.
+ type: boolean
+
+ adi,lo-buff-en:
+ description:
+ LO Buffer Enable.
+ type: boolean
+
+ adi,if-mode-en:
+ description:
+ IF Mode Enable.
+ type: boolean
+
+ adi,iq-mode-en:
+ description:
+ IQ Mode Enable.
+ type: boolean
+
+ adi,lo-x2-en:
+ description:
+ LO x2 Enable.
+ type: boolean
+
+ adi,lo-ppf-en:
+ description:
+ LO x1 Enable.
+ type: boolean
+
+ adi,lo-en:
+ description:
+ LO Enable.
+ type: boolean
+
+ adi,uc-bias-en:
+ description:
+ UC Bias Enable.
+ type: boolean
+
+ adi,lo-sideband:
+ description:
+ Switch to the Other LO Sideband.
+ type: boolean
+
+ adi,vdet-out-en:
+ description:
+ VDET Output Select Enable.
+ type: boolean
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ '#clock-cells':
+ const: 0
+
+required:
+- compatible
+- reg
+- clocks
+- clock-names
+
+additionalProperties: false
+
+examples:
+- |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ adrf6780@0{
+ compatible = "adi,adrf6780";
+ reg = <0>;
+ spi-max-frequency = <1000000>;
+ clocks = <&adrf6780_lo>;
+ clock-names = "lo_in";
+ adi,parity-en;
+ };
+ };
+...
+
--
2.32.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-06-29 14:23 ` [PATCH 1/2] " Antoniu Miclaus
@ 2021-06-30 8:38 ` Alexandru Ardelean
2021-06-30 12:04 ` Sa, Nuno
0 siblings, 1 reply; 7+ messages in thread
From: Alexandru Ardelean @ 2021-06-30 8:38 UTC (permalink / raw)
To: Antoniu Miclaus
Cc: linux-iio, LKML, Jonathan Cameron, devicetree, Rob Herring
On Tue, Jun 29, 2021 at 5:25 PM Antoniu Miclaus
<antoniu.miclaus@analog.com> wrote:
>
> Add support for the ADRF6780 microwave upconverter.
Hey,
A few comments inline.
The description of the commit could have a bit more information.
Maybe a short description of the chip (typically I'd adapt something
from the datasheet).
And maybe a link to the datasheet.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> drivers/iio/frequency/Kconfig | 13 +
> drivers/iio/frequency/Makefile | 1 +
> drivers/iio/frequency/adrf6780.c | 534 +++++++++++++++++++++++++++++++
> 3 files changed, 548 insertions(+)
> create mode 100644 drivers/iio/frequency/adrf6780.c
>
> diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
> index 240b81502512..fc9751c48f59 100644
> --- a/drivers/iio/frequency/Kconfig
> +++ b/drivers/iio/frequency/Kconfig
> @@ -49,5 +49,18 @@ config ADF4371
>
> To compile this driver as a module, choose M here: the
> module will be called adf4371.
> +
> +config ADRF6780
> + tristate "Analog Devices ADRF6780 Microwave Upconverter"
> + depends on SPI
> + depends on COMMON_CLK
> + depends on OF
> + help
> + Say yes here to build support for Analog Devices ADRF6780
> + 5.9 GHz to 23.6 GHz, Wideband, Microwave Upconverter.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called adrf6780.
> +
> endmenu
> endmenu
> diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
> index 518b1e50caef..ae3136c79202 100644
> --- a/drivers/iio/frequency/Makefile
> +++ b/drivers/iio/frequency/Makefile
> @@ -7,3 +7,4 @@
> obj-$(CONFIG_AD9523) += ad9523.o
> obj-$(CONFIG_ADF4350) += adf4350.o
> obj-$(CONFIG_ADF4371) += adf4371.o
> +obj-$(CONFIG_ADRF6780) += adrf6780.o
> diff --git a/drivers/iio/frequency/adrf6780.c b/drivers/iio/frequency/adrf6780.c
> new file mode 100644
> index 000000000000..c492c4e4adf1
> --- /dev/null
> +++ b/drivers/iio/frequency/adrf6780.c
> @@ -0,0 +1,534 @@
> +// SPDX-License-Identifier: GPL-2.0+
about the licensing;
SPDX-License-Identifier: GPL-2.0+ == MODULE_LICENSE("GPL v2");
SPDX-License-Identifier: GPL-2.0 == MODULE_LICENSE("GPL v2");
I usually don't care about this licensing details, but it seems to be
important elsewhere.
> +/*
> + * ADRF6780 driver
This could be "Analog Devices ADRF6780 driver"
> + *
> + * Copyright 2021 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
Not all these headers look used.
For one thing, regmap.h doesn't look used at all.
Maybe trim the list.
> +
> +/* ADRF6780 Register Map */
> +#define ADRF6780_REG_CONTROL 0x00
> +#define ADRF6780_REG_ALARM_READBACK 0x01
> +#define ADRF6780_REG_ALARM_MASKS 0x02
> +#define ADRF6780_REG_ENABLE 0x03
> +#define ADRF6780_REG_LINEARIZE 0x04
> +#define ADRF6780_REG_LO_PATH 0x05
> +#define ADRF6780_REG_ADC_CONTROL 0x06
> +#define ADRF6780_REG_ADC_OUTPUT 0x0C
> +
> +/* ADRF6780_REG_CONTROL Map */
> +#define ADRF6780_PARITY_EN_MSK BIT(15)
> +#define ADRF6780_PARITY_EN(x) FIELD_PREP(ADRF6780_PARITY_EN_MSK, x)
> +#define ADRF6780_SOFT_RESET_MSK BIT(14)
> +#define ADRF6780_SOFT_RESET(x) FIELD_PREP(ADRF6780_SOFT_RESET_MSK, x)
> +#define ADRF6780_CHIP_ID_MSK GENMASK(11, 4)
> +#define ADRF6780_CHIP_ID 0xA
> +#define ADRF6780_CHIP_REVISION_MSK GENMASK(3, 0)
> +#define ADRF6780_CHIP_REVISION(x) FIELD_PREP(ADRF6780_CHIP_REVISION_MSK, x)
> +
> +/* ADRF6780_REG_ALARM_READBACK Map */
> +#define ADRF6780_PARITY_ERROR_MSK BIT(15)
> +#define ADRF6780_PARITY_ERROR(x) FIELD_PREP(ADRF6780_PARITY_ERROR_MSK, x)
> +#define ADRF6780_TOO_FEW_ERRORS_MSK BIT(14)
> +#define ADRF6780_TOO_FEW_ERRORS(x) FIELD_PREP(ADRF6780_TOO_FEW_ERRORS_MSK, x)
> +#define ADRF6780_TOO_MANY_ERRORS_MSK BIT(13)
> +#define ADRF6780_TOO_MANY_ERRORS(x) FIELD_PREP(ADRF6780_TOO_MANY_ERRORS_MSK, x)
> +#define ADRF6780_ADDRESS_RANGE_ERROR_MSK BIT(12)
> +#define ADRF6780_ADDRESS_RANGE_ERROR(x) FIELD_PREP(ADRF6780_ADDRESS_RANGE_ERROR_MSK, x)
> +
> +/* ADRF6780_REG_ENABLE Map */
> +#define ADRF6780_VGA_BUFFER_EN_MSK BIT(8)
> +#define ADRF6780_VGA_BUFFER_EN(x) FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, x)
> +#define ADRF6780_DETECTOR_EN_MSK BIT(7)
> +#define ADRF6780_DETECTOR_EN(x) FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, x)
> +#define ADRF6780_LO_BUFFER_EN_MSK BIT(6)
> +#define ADRF6780_LO_BUFFER_EN(x) FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, x)
> +#define ADRF6780_IF_MODE_EN_MSK BIT(5)
> +#define ADRF6780_IF_MODE_EN(x) FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, x)
> +#define ADRF6780_IQ_MODE_EN_MSK BIT(4)
> +#define ADRF6780_IQ_MODE_EN(x) FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, x)
> +#define ADRF6780_LO_X2_EN_MSK BIT(3)
> +#define ADRF6780_LO_X2_EN(x) FIELD_PREP(ADRF6780_LO_X2_EN_MSK, x)
> +#define ADRF6780_LO_PPF_EN_MSK BIT(2)
> +#define ADRF6780_LO_PPF_EN(x) FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, x)
> +#define ADRF6780_LO_EN_MSK BIT(1)
> +#define ADRF6780_LO_EN(x) FIELD_PREP(ADRF6780_LO_EN_MSK, x)
> +#define ADRF6780_UC_BIAS_EN_MSK BIT(0)
> +#define ADRF6780_UC_BIAS_EN(x) FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, x)
> +
> +/* ADRF6780_REG_LINEARIZE Map */
> +#define ADRF6780_RDAC_LINEARIZE_MSK GENMASK(7, 0)
> +#define ADRF6780_RDAC_LINEARIZE(x) FIELD_PREP(ADRF6780_RDAC_LINEARIZE_MSK, x)
> +
> +/* ADRF6780_REG_LO_PATH Map */
> +#define ADRF6780_LO_SIDEBAND_MSK BIT(10)
> +#define ADRF6780_LO_SIDEBAND(x) FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, x)
> +#define ADRF6780_Q_PATH_PHASE_ACCURACY_MSK GENMASK(7, 4)
> +#define ADRF6780_Q_PATH_PHASE_ACCURACY(x) FIELD_PREP(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, x)
> +#define ADRF6780_I_PATH_PHASE_ACCURACY_MSK GENMASK(3, 0)
> +#define ADRF6780_I_PATH_PHASE_ACCURACY(x) FIELD_PREP(ADRF6780_I_PATH_PHASE_ACCURACY_MSK, x)
> +
> +/* ADRF6780_REG_ADC_CONTROL Map */
> +#define ADRF6780_VDET_OUTPUT_SELECT_MSK BIT(3)
> +#define ADRF6780_VDET_OUTPUT_SELECT(x) FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, x)
> +#define ADRF6780_ADC_START_MSK BIT(2)
> +#define ADRF6780_ADC_START(x) FIELD_PREP(ADRF6780_ADC_START_MSK, x)
> +#define ADRF6780_ADC_EN_MSK BIT(1)
> +#define ADRF6780_ADC_EN(x) FIELD_PREP(ADRF6780_ADC_EN_MSK, x)
> +#define ADRF6780_ADC_CLOCK_EN_MSK BIT(0)
> +#define ADRF6780_ADC_CLOCK_EN(x) FIELD_PREP(ADRF6780_ADC_CLOCK_EN_MSK, x)
> +
> +/* ADRF6780_REG_ADC_OUTPUT Map */
> +#define ADRF6780_ADC_STATUS_MSK BIT(8)
> +#define ADRF6780_ADC_STATUS(x) FIELD_PREP(ADRF6780_ADC_STATUS_MSK, x)
> +#define ADRF6780_ADC_VALUE_MSK GENMASK(7, 0)
> +#define ADRF6780_ADC_VALUE(x) FIELD_PREP(ADRF6780_ADC_VALUE_MSK, x)
The indentation for the bit-values doesn't look consistent in all places.
> +
> +enum supported_parts {
> + ADRF6780,
> +};
This enum doesn't seem used anywhere
> +
> +struct adrf6780_dev {
> + struct spi_device *spi;
> + struct clk *clkin;
> + /* Protect against concurrent accesses to the device */
> + struct mutex lock;
> + bool parity_en;
Maybe remove this parity check.
There are many drivers that support some form of simple error
checking, but in the kernel this is typically left up to the SPI
framework.
So, I'd just disable the error checking entirely.
> + bool vga_buff_en;
> + bool det_en;
> + bool lo_buff_en;
> + bool if_mode_en;
> + bool iq_mode_en;
> + bool lo_x2_en;
> + bool lo_ppf_en;
> + bool lo_en;
> + bool uc_bias_en;
> + bool lo_sideband;
> + bool vdet_out_en;
> +};
> +
> +static int adrf6780_spi_read(struct adrf6780_dev *dev, unsigned int reg,
> + unsigned int *val)
> +{
> + int ret;
> + unsigned int cnt, temp;
> + struct spi_transfer t = {0};
> + u8 data[3];
> +
> + data[0] = 0x80 | (reg << 1);
> + data[1] = 0x0;
> + data[2] = 0x0;
> +
> + t.rx_buf = &data[0];
> + t.tx_buf = &data[0];
> + t.len = 3;
> +
> + ret = spi_sync_transfer(dev->spi, &t, 1);
> + if (ret < 0)
> + return ret;
> +
> + temp = ((data[0] | 0x80 | (reg << 1)) << 16) |
> + (data[1] << 8) | data[2];
> +
> + if (dev->parity_en) {
> + cnt = hweight_long(temp);
> + if (!(cnt % 2))
> + return -EINVAL;
> + }
> +
> + *val = (temp >> 1) & 0xFFFF;
> +
> + return ret;
> +}
> +
> +static int adrf6780_spi_write(struct adrf6780_dev *dev,
> + unsigned int reg,
> + unsigned int val)
> +{
> + unsigned int cnt;
> + u8 data[3];
> +
> + val = (val << 1);
> +
> + if (dev->parity_en) {
> + cnt = hweight_long((reg << 17) | val);
> + if (cnt % 2 == 0)
> + val |= 0x1;
> + }
> +
> + data[0] = (reg << 1) | (val >> 16);
> + data[1] = val >> 8;
> + data[2] = val;
> +
> + return spi_write(dev->spi, &data[0], 3);
> +}
> +
> +static int __adrf6780_spi_update_bits(struct adrf6780_dev *dev, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + int ret;
> + unsigned int data, temp;
> +
> + ret = adrf6780_spi_read(dev, reg, &data);
> + if (ret < 0)
> + return ret;
> +
> + temp = (data & ~mask) | (val & mask);
> +
> + return adrf6780_spi_write(dev, reg, temp);
> +}
> +
> +static int adrf6780_spi_update_bits(struct adrf6780_dev *dev, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + int ret;
> +
> + mutex_lock(&dev->lock);
> + ret = __adrf6780_spi_update_bits(dev, reg, mask, val);
> + mutex_unlock(&dev->lock);
> + return ret;
> +}
> +
> +static int adrf6780_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long info)
> +{
> + struct adrf6780_dev *dev = iio_priv(indio_dev);
> + unsigned int data;
> + int ret;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&dev->lock);
> +
> + ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ENABLE,
> + ADRF6780_DETECTOR_EN_MSK,
> + ADRF6780_DETECTOR_EN(1));
The detector seems to be enabled here regardless of the 'det_en' value.
And it doesn't seem to put the value back to a 'det_en' state.
But the question is, if it would make sense to always enable the detector?
In any case enabling the detector in IIO_CHAN_INFO_RAW doesn't look
like a good idea.
> + if (ret < 0)
> + goto exit;
> +
> + ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
> + ADRF6780_ADC_EN_MSK,
> + ADRF6780_ADC_EN(1));
> + if (ret < 0)
> + goto exit;
> +
> + ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
> + ADRF6780_ADC_CLOCK_EN_MSK,
> + ADRF6780_ADC_CLOCK_EN(1));
> + if (ret < 0)
> + goto exit;
> +
> + ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
> + ADRF6780_ADC_START_MSK,
> + ADRF6780_ADC_START(1));
> + if (ret < 0)
> + goto exit;
This looks like 3 SPI operations on the same register.
Would it work to group them in a single operation?
> +
> + usleep_range(200, 250);
> +
> + ret = adrf6780_spi_read(dev, ADRF6780_REG_ADC_OUTPUT, &data);
> + if (ret < 0)
> + goto exit;
> +
> + if (!(data & ADRF6780_ADC_STATUS_MSK)) {
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
> + ADRF6780_ADC_START_MSK,
> + ADRF6780_ADC_START(0));
> + if (ret < 0)
> + goto exit;
> +
> + ret = adrf6780_spi_read(dev, ADRF6780_REG_ADC_OUTPUT, &data);
> + if (ret < 0)
> + goto exit;
> +
> + mutex_unlock(&dev->lock);
> +
> + *val = data & ADRF6780_ADC_VALUE_MSK;
> +
> + return IIO_VAL_INT;
> +exit:
> + mutex_unlock(&dev->lock);
> + return ret;
> + case IIO_CHAN_INFO_SCALE:
> + ret = adrf6780_spi_read(dev, ADRF6780_REG_LINEARIZE, &data);
> + if (ret < 0)
> + return ret;
> +
> + *val = data & ADRF6780_RDAC_LINEARIZE_MSK;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_PHASE:
> + ret = adrf6780_spi_read(dev, ADRF6780_REG_LO_PATH, &data);
> + if (ret < 0)
> + return ret;
> +
> + if (chan->channel2 == IIO_MOD_I)
> + *val = data & ADRF6780_I_PATH_PHASE_ACCURACY_MSK;
> + else
> + *val = (data & ADRF6780_Q_PATH_PHASE_ACCURACY_MSK) >> 4;
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int adrf6780_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long info)
> +{
> + struct adrf6780_dev *dev = iio_priv(indio_dev);
> + int ret;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SCALE:
> + return adrf6780_spi_write(dev, ADRF6780_REG_LINEARIZE, val);
> + case IIO_CHAN_INFO_PHASE:
> + if (chan->channel2 == IIO_MOD_I)
> + ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
> + ADRF6780_I_PATH_PHASE_ACCURACY_MSK,
> + ADRF6780_I_PATH_PHASE_ACCURACY(val));
> + else
> + ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
> + ADRF6780_Q_PATH_PHASE_ACCURACY_MSK,
> + ADRF6780_Q_PATH_PHASE_ACCURACY(val));
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int adrf6780_reg_access(struct iio_dev *indio_dev,
> + unsigned int reg,
> + unsigned int write_val,
> + unsigned int *read_val)
> +{
> + struct adrf6780_dev *dev = iio_priv(indio_dev);
> +
> + if (read_val)
> + return adrf6780_spi_read(dev, reg, read_val);
> + else
> + return adrf6780_spi_write(dev, reg, write_val);
> +}
> +
> +static const struct iio_info adrf6780_info = {
> + .read_raw = adrf6780_read_raw,
> + .write_raw = adrf6780_write_raw,
> + .debugfs_reg_access = &adrf6780_reg_access,
> +};
> +
> +#define ADRF6780_CHAN(_channel) { \
> + .type = IIO_VOLTAGE, \
> + .output = 1, \
> + .indexed = 1, \
> + .channel = _channel, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) \
> +}
> +
> +#define ADRF6780_CHAN_IQ(_channel, rf_comp) { \
> + .type = IIO_ALTVOLTAGE, \
> + .modified = 1, \
> + .output = 1, \
> + .indexed = 1, \
> + .channel2 = IIO_MOD_##rf_comp, \
> + .channel = _channel, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PHASE) \
> +}
> +
> +static const struct iio_chan_spec adrf6780_channels[] = {
> + ADRF6780_CHAN(0),
> + ADRF6780_CHAN_IQ(0, I),
> + ADRF6780_CHAN_IQ(0, Q),
> +};
> +
> +static int adrf6780_init(struct adrf6780_dev *dev)
> +{
> + int ret;
> + unsigned int chip_id, enable_reg, enable_reg_msk;
> + struct spi_device *spi = dev->spi;
> + bool temp_parity = dev->parity_en;
> +
> + dev->parity_en = false;
> +
> + /* Perform a software reset */
> + ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
> + ADRF6780_SOFT_RESET_MSK,
> + ADRF6780_SOFT_RESET(1));
> + if (ret < 0) {
> + dev_err(&spi->dev, "ADRF6780 SPI software reset failed.\n");
> + return ret;
> + }
> +
> + ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
> + ADRF6780_SOFT_RESET_MSK,
> + ADRF6780_SOFT_RESET(0));
> + if (ret < 0) {
> + dev_err(&spi->dev, "ADRF6780 SPI software reset disable failed.\n");
> + return ret;
> + }
I'd create a adrf6780_reset() function.
The driver has a reset pin, which [optionally] can be implemented [now
or later].
Typically, these resets are implemented is:
if (gpio)
gpio_reset()
return
software_reset()
> +
> + ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
> + ADRF6780_PARITY_EN_MSK,
> + ADRF6780_PARITY_EN(temp_parity));
> + if (ret < 0)
> + return ret;
> +
> + dev->parity_en = temp_parity;
> +
> + ret = adrf6780_spi_read(dev, ADRF6780_REG_CONTROL, &chip_id);
> + if (ret < 0)
> + return ret;
> +
> + chip_id = (chip_id & ADRF6780_CHIP_ID_MSK) >> 4;
> + if (chip_id != ADRF6780_CHIP_ID) {
> + dev_err(&spi->dev, "ADRF6780 Invalid Chip ID.\n");
> + return -EINVAL;
> + }
> +
> + enable_reg_msk = ADRF6780_VGA_BUFFER_EN_MSK |
> + ADRF6780_DETECTOR_EN_MSK |
> + ADRF6780_LO_BUFFER_EN_MSK |
> + ADRF6780_IF_MODE_EN_MSK |
> + ADRF6780_IQ_MODE_EN_MSK |
> + ADRF6780_LO_X2_EN_MSK |
> + ADRF6780_LO_PPF_EN_MSK |
> + ADRF6780_LO_EN_MSK |
> + ADRF6780_UC_BIAS_EN_MSK;
> +
> + enable_reg = ADRF6780_VGA_BUFFER_EN(dev->vga_buff_en) |
> + ADRF6780_DETECTOR_EN(dev->det_en) |
> + ADRF6780_LO_BUFFER_EN(dev->lo_buff_en) |
> + ADRF6780_IF_MODE_EN(dev->if_mode_en) |
> + ADRF6780_IQ_MODE_EN(dev->iq_mode_en) |
> + ADRF6780_LO_X2_EN(dev->lo_x2_en) |
> + ADRF6780_LO_PPF_EN(dev->lo_ppf_en) |
> + ADRF6780_LO_EN(dev->lo_en) |
> + ADRF6780_UC_BIAS_EN(dev->uc_bias_en);
> +
> + ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_ENABLE, enable_reg_msk, enable_reg);
> + if (ret < 0)
> + return ret;
> +
> + ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
> + ADRF6780_LO_SIDEBAND_MSK,
> + ADRF6780_LO_SIDEBAND(dev->lo_sideband));
> + if (ret < 0)
> + return ret;
> +
> + return __adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL,
> + ADRF6780_VDET_OUTPUT_SELECT_MSK,
> + ADRF6780_VDET_OUTPUT_SELECT(dev->vdet_out_en));
> +}
> +
> +static void adrf6780_clk_disable(void *data)
> +{
> + clk_disable_unprepare(data);
> +}
> +
> +static int adrf6780_dt_parse(struct adrf6780_dev *dev)
> +{
> + struct spi_device *spi = dev->spi;
> +
> + dev->parity_en = of_property_read_bool(spi->dev.of_node, "adi,parity-en");
> + dev->vga_buff_en = of_property_read_bool(spi->dev.of_node, "adi,vga-buff-en");
> + dev->det_en = of_property_read_bool(spi->dev.of_node, "adi,det-en");
> + dev->lo_buff_en = of_property_read_bool(spi->dev.of_node, "adi,lo-buff-en");
> + dev->if_mode_en = of_property_read_bool(spi->dev.of_node, "adi,if-mode-en");
> + dev->iq_mode_en = of_property_read_bool(spi->dev.of_node, "adi,iq-mode-en");
> + dev->lo_x2_en = of_property_read_bool(spi->dev.of_node, "adi,lo-x2-en");
> + dev->lo_ppf_en = of_property_read_bool(spi->dev.of_node, "adi,lo-ppf-en");
> + dev->lo_en = of_property_read_bool(spi->dev.of_node, "adi,lo-en");
> + dev->uc_bias_en = of_property_read_bool(spi->dev.of_node, "adi,uc-bias-en");
> + dev->lo_sideband = of_property_read_bool(spi->dev.of_node, "adi,lo-sideband");
> + dev->vdet_out_en = of_property_read_bool(spi->dev.of_node, "adi,vdet-out-en");
I'm not sure this is the best way to control these from the DT.
So, the parity can be disabled/removed.
Maybe the detector can always be enabled.
The others may make sense to be enabled.
> +
> + dev->clkin = devm_clk_get(&spi->dev, "lo_in");
> + if (IS_ERR(dev->clkin))
> + return PTR_ERR(dev->clkin);
Initializing a clock in the adrf6780_dt_parse() function is weird.
Mabe move this to the main probe function.
> +
> + return 0;
> +}
> +
> +static int adrf6780_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct adrf6780_dev *dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*dev));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + dev = iio_priv(indio_dev);
> +
> + indio_dev->dev.parent = &spi->dev;
this assignment can be removed in the upstream version of this driver;
the assignment is done inside devm_iio_device_alloc()
> + indio_dev->info = &adrf6780_info;
> + indio_dev->name = "adrf6780";
> + indio_dev->channels = adrf6780_channels;
> + indio_dev->num_channels = ARRAY_SIZE(adrf6780_channels);
> +
> + dev->spi = spi;
> +
> + ret = adrf6780_dt_parse(dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = clk_prepare_enable(dev->clkin);
> + if (ret < 0)
> + return ret;
> +
> + ret = devm_add_action_or_reset(&spi->dev, adrf6780_clk_disable, dev->clkin);
> + if (ret < 0)
> + return ret;
> +
> + mutex_init(&dev->lock);
> +
> + ret = adrf6780_init(dev);
> + if (ret < 0)
> + return ret;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct spi_device_id adrf6780_id[] = {
> + { "adrf6780", ADRF6780 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, adrf6780_id);
> +
> +static const struct of_device_id adrf6780_of_match[] = {
> + { .compatible = "adi,adrf6780" },
> + {},
you can remove the comma from this null terminator
> +};
> +MODULE_DEVICE_TABLE(of, adrf6780_of_match);
> +
> +static struct spi_driver adrf6780_driver = {
> + .driver = {
> + .name = "adrf6780",
> + .of_match_table = adrf6780_of_match,
> + },
> + .probe = adrf6780_probe,
> + .id_table = adrf6780_id,
> +};
> +module_spi_driver(adrf6780_driver);
> +
> +MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com");
> +MODULE_DESCRIPTION("Analog Devices ADRF6780");
> +MODULE_LICENSE("GPL v2");
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-06-30 8:38 ` Alexandru Ardelean
@ 2021-06-30 12:04 ` Sa, Nuno
2021-07-02 7:09 ` Sa, Nuno
2021-07-03 16:18 ` Jonathan Cameron
0 siblings, 2 replies; 7+ messages in thread
From: Sa, Nuno @ 2021-06-30 12:04 UTC (permalink / raw)
To: Alexandru Ardelean, Miclaus, Antoniu
Cc: linux-iio, LKML, Jonathan Cameron, devicetree, Rob Herring
Hey Alex,
Thanks for the review,
> From: Alexandru Ardelean <ardeleanalex@gmail.com>
> Sent: Wednesday, June 30, 2021 10:38 AM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; LKML <linux-
> kernel@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> devicetree <devicetree@vger.kernel.org>; Rob Herring
> <robh+dt@kernel.org>
> Subject: Re: [PATCH 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
>
> On Tue, Jun 29, 2021 at 5:25 PM Antoniu Miclaus
> <antoniu.miclaus@analog.com> wrote:
> >
> > Add support for the ADRF6780 microwave upconverter.
>
> Hey,
>
> A few comments inline.
> The description of the commit could have a bit more information.
> Maybe a short description of the chip (typically I'd adapt something
> from the datasheet).
> And maybe a link to the datasheet.
>
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> > drivers/iio/frequency/Kconfig | 13 +
> > drivers/iio/frequency/Makefile | 1 +
> > drivers/iio/frequency/adrf6780.c | 534
> +++++++++++++++++++++++++++++++
> > 3 files changed, 548 insertions(+)
> > create mode 100644 drivers/iio/frequency/adrf6780.c
> >
> > diff --git a/drivers/iio/frequency/Kconfig
> b/drivers/iio/frequency/Kconfig
> > index 240b81502512..fc9751c48f59 100644
> > --- a/drivers/iio/frequency/Kconfig
> > +++ b/drivers/iio/frequency/Kconfig
> > @@ -49,5 +49,18 @@ config ADF4371
> >
> > To compile this driver as a module, choose M here: the
> > module will be called adf4371.
> > +
> > +config ADRF6780
> > + tristate "Analog Devices ADRF6780 Microwave Upconverter"
> > + depends on SPI
> > + depends on COMMON_CLK
> > + depends on OF
> > + help
> > + Say yes here to build support for Analog Devices ADRF6780
> > + 5.9 GHz to 23.6 GHz, Wideband, Microwave Upconverter.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called adrf6780.
> > +
> > endmenu
> > endmenu
> > diff --git a/drivers/iio/frequency/Makefile
> b/drivers/iio/frequency/Makefile
> > index 518b1e50caef..ae3136c79202 100644
> > --- a/drivers/iio/frequency/Makefile
> > +++ b/drivers/iio/frequency/Makefile
> > @@ -7,3 +7,4 @@
> > obj-$(CONFIG_AD9523) += ad9523.o
> > obj-$(CONFIG_ADF4350) += adf4350.o
> > obj-$(CONFIG_ADF4371) += adf4371.o
> > +obj-$(CONFIG_ADRF6780) += adrf6780.o
> > diff --git a/drivers/iio/frequency/adrf6780.c
> b/drivers/iio/frequency/adrf6780.c
> > new file mode 100644
> > index 000000000000..c492c4e4adf1
> > --- /dev/null
> > +++ b/drivers/iio/frequency/adrf6780.c
> > @@ -0,0 +1,534 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> about the licensing;
>
> SPDX-License-Identifier: GPL-2.0+ == MODULE_LICENSE("GPL v2");
> SPDX-License-Identifier: GPL-2.0 == MODULE_LICENSE("GPL v2");
>
> I usually don't care about this licensing details, but it seems to be
> important elsewhere.
>
> > +/*
> > + * ADRF6780 driver
>
> This could be "Analog Devices ADRF6780 driver"
>
> > + *
> > + * Copyright 2021 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/clk.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spi/spi.h>
>
> Not all these headers look used.
> For one thing, regmap.h doesn't look used at all.
> Maybe trim the list.
>
> > +
> > +/* ADRF6780 Register Map */
> > +#define ADRF6780_REG_CONTROL 0x00
> > +#define ADRF6780_REG_ALARM_READBACK 0x01
> > +#define ADRF6780_REG_ALARM_MASKS 0x02
> > +#define ADRF6780_REG_ENABLE 0x03
> > +#define ADRF6780_REG_LINEARIZE 0x04
> > +#define ADRF6780_REG_LO_PATH 0x05
> > +#define ADRF6780_REG_ADC_CONTROL 0x06
> > +#define ADRF6780_REG_ADC_OUTPUT 0x0C
> > +
> > +/* ADRF6780_REG_CONTROL Map */
> > +#define ADRF6780_PARITY_EN_MSK BIT(15)
> > +#define ADRF6780_PARITY_EN(x)
> FIELD_PREP(ADRF6780_PARITY_EN_MSK, x)
> > +#define ADRF6780_SOFT_RESET_MSK BIT(14)
> > +#define ADRF6780_SOFT_RESET(x)
> FIELD_PREP(ADRF6780_SOFT_RESET_MSK, x)
> > +#define ADRF6780_CHIP_ID_MSK GENMASK(11, 4)
> > +#define ADRF6780_CHIP_ID 0xA
> > +#define ADRF6780_CHIP_REVISION_MSK GENMASK(3, 0)
> > +#define ADRF6780_CHIP_REVISION(x)
> FIELD_PREP(ADRF6780_CHIP_REVISION_MSK, x)
> > +
> > +/* ADRF6780_REG_ALARM_READBACK Map */
> > +#define ADRF6780_PARITY_ERROR_MSK BIT(15)
> > +#define ADRF6780_PARITY_ERROR(x)
> FIELD_PREP(ADRF6780_PARITY_ERROR_MSK, x)
> > +#define ADRF6780_TOO_FEW_ERRORS_MSK BIT(14)
> > +#define ADRF6780_TOO_FEW_ERRORS(x)
> FIELD_PREP(ADRF6780_TOO_FEW_ERRORS_MSK, x)
> > +#define ADRF6780_TOO_MANY_ERRORS_MSK BIT(13)
> > +#define ADRF6780_TOO_MANY_ERRORS(x)
> FIELD_PREP(ADRF6780_TOO_MANY_ERRORS_MSK, x)
> > +#define ADRF6780_ADDRESS_RANGE_ERROR_MSK BIT(12)
> > +#define ADRF6780_ADDRESS_RANGE_ERROR(x)
> FIELD_PREP(ADRF6780_ADDRESS_RANGE_ERROR_MSK, x)
> > +
> > +/* ADRF6780_REG_ENABLE Map */
> > +#define ADRF6780_VGA_BUFFER_EN_MSK BIT(8)
> > +#define ADRF6780_VGA_BUFFER_EN(x)
> FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, x)
> > +#define ADRF6780_DETECTOR_EN_MSK BIT(7)
> > +#define ADRF6780_DETECTOR_EN(x)
> FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, x)
> > +#define ADRF6780_LO_BUFFER_EN_MSK BIT(6)
> > +#define ADRF6780_LO_BUFFER_EN(x)
> FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, x)
> > +#define ADRF6780_IF_MODE_EN_MSK BIT(5)
> > +#define ADRF6780_IF_MODE_EN(x)
> FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, x)
> > +#define ADRF6780_IQ_MODE_EN_MSK BIT(4)
> > +#define ADRF6780_IQ_MODE_EN(x)
> FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, x)
> > +#define ADRF6780_LO_X2_EN_MSK BIT(3)
> > +#define ADRF6780_LO_X2_EN(x)
> FIELD_PREP(ADRF6780_LO_X2_EN_MSK, x)
> > +#define ADRF6780_LO_PPF_EN_MSK BIT(2)
> > +#define ADRF6780_LO_PPF_EN(x)
> FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, x)
> > +#define ADRF6780_LO_EN_MSK BIT(1)
> > +#define ADRF6780_LO_EN(x)
> FIELD_PREP(ADRF6780_LO_EN_MSK, x)
> > +#define ADRF6780_UC_BIAS_EN_MSK BIT(0)
> > +#define ADRF6780_UC_BIAS_EN(x)
> FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, x)
> > +
> > +/* ADRF6780_REG_LINEARIZE Map */
> > +#define ADRF6780_RDAC_LINEARIZE_MSK GENMASK(7, 0)
> > +#define ADRF6780_RDAC_LINEARIZE(x)
> FIELD_PREP(ADRF6780_RDAC_LINEARIZE_MSK, x)
> > +
> > +/* ADRF6780_REG_LO_PATH Map */
> > +#define ADRF6780_LO_SIDEBAND_MSK BIT(10)
> > +#define ADRF6780_LO_SIDEBAND(x)
> FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, x)
> > +#define ADRF6780_Q_PATH_PHASE_ACCURACY_MSK
> GENMASK(7, 4)
> > +#define ADRF6780_Q_PATH_PHASE_ACCURACY(x)
> FIELD_PREP(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, x)
> > +#define ADRF6780_I_PATH_PHASE_ACCURACY_MSK
> GENMASK(3, 0)
> > +#define ADRF6780_I_PATH_PHASE_ACCURACY(x)
> FIELD_PREP(ADRF6780_I_PATH_PHASE_ACCURACY_MSK, x)
> > +
> > +/* ADRF6780_REG_ADC_CONTROL Map */
> > +#define ADRF6780_VDET_OUTPUT_SELECT_MSK BIT(3)
> > +#define ADRF6780_VDET_OUTPUT_SELECT(x)
> FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, x)
> > +#define ADRF6780_ADC_START_MSK BIT(2)
> > +#define ADRF6780_ADC_START(x)
> FIELD_PREP(ADRF6780_ADC_START_MSK, x)
> > +#define ADRF6780_ADC_EN_MSK BIT(1)
> > +#define ADRF6780_ADC_EN(x)
> FIELD_PREP(ADRF6780_ADC_EN_MSK, x)
> > +#define ADRF6780_ADC_CLOCK_EN_MSK BIT(0)
> > +#define ADRF6780_ADC_CLOCK_EN(x)
> FIELD_PREP(ADRF6780_ADC_CLOCK_EN_MSK, x)
> > +
> > +/* ADRF6780_REG_ADC_OUTPUT Map */
> > +#define ADRF6780_ADC_STATUS_MSK BIT(8)
> > +#define ADRF6780_ADC_STATUS(x)
> FIELD_PREP(ADRF6780_ADC_STATUS_MSK, x)
> > +#define ADRF6780_ADC_VALUE_MSK GENMASK(7, 0)
> > +#define ADRF6780_ADC_VALUE(x)
> FIELD_PREP(ADRF6780_ADC_VALUE_MSK, x)
>
> The indentation for the bit-values doesn't look consistent in all places.
>
> > +
> > +enum supported_parts {
> > + ADRF6780,
> > +};
>
> This enum doesn't seem used anywhere
>
> > +
> > +struct adrf6780_dev {
> > + struct spi_device *spi;
> > + struct clk *clkin;
> > + /* Protect against concurrent accesses to the device */
> > + struct mutex lock;
> > + bool parity_en;
>
> Maybe remove this parity check.
> There are many drivers that support some form of simple error
> checking, but in the kernel this is typically left up to the SPI
> framework.
> So, I'd just disable the error checking entirely.
I would just argue here... Though parity check is not the most reliable one
out there, it does give some extra checking on top of what the SPI framework
can give (AFAICT). As this part might be used is some sensitive applications, I
can imagine someone wanting this extra feature that does not really cost that
much (if something). However it looks like that on the write side, the only way
to know about parity issues (or other errors) is to make use of ALM pin.
Anyways, I do agree that having this as a devicetree parameter is, at least, arguable
(I don't think it is terribly wrong though). But I guess that, at least, as a module
parameter would be something more acceptable.
Or, as you suggested, It can also be removed and only implemented if someone
really cares/asks for it...
- Nuno Sá
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-06-30 12:04 ` Sa, Nuno
@ 2021-07-02 7:09 ` Sa, Nuno
2021-07-03 16:18 ` Jonathan Cameron
1 sibling, 0 replies; 7+ messages in thread
From: Sa, Nuno @ 2021-07-02 7:09 UTC (permalink / raw)
To: Sa, Nuno, Alexandru Ardelean, Miclaus, Antoniu
Cc: linux-iio, LKML, Jonathan Cameron, devicetree, Rob Herring
> From: Sa, Nuno <Nuno.Sa@analog.com>
> Sent: Wednesday, June 30, 2021 2:04 PM
> To: Alexandru Ardelean <ardeleanalex@gmail.com>; Miclaus, Antoniu
> <Antoniu.Miclaus@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; LKML <linux-
> kernel@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> devicetree <devicetree@vger.kernel.org>; Rob Herring
> <robh+dt@kernel.org>
> Subject: RE: [PATCH 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
>
> [External]
>
> Hey Alex,
>
> Thanks for the review,
>
> > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > Sent: Wednesday, June 30, 2021 10:38 AM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; LKML <linux-
> > kernel@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> > devicetree <devicetree@vger.kernel.org>; Rob Herring
> > <robh+dt@kernel.org>
> > Subject: Re: [PATCH 1/2] iio: frequency: adrf6780: add support for
> > ADRF6780
> >
> > On Tue, Jun 29, 2021 at 5:25 PM Antoniu Miclaus
> > <antoniu.miclaus@analog.com> wrote:
> > >
> > > Add support for the ADRF6780 microwave upconverter.
> >
> > Hey,
> >
> > A few comments inline.
> > The description of the commit could have a bit more information.
> > Maybe a short description of the chip (typically I'd adapt something
> > from the datasheet).
> > And maybe a link to the datasheet.
> >
> > >
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > ---
> > > drivers/iio/frequency/Kconfig | 13 +
> > > drivers/iio/frequency/Makefile | 1 +
> > > drivers/iio/frequency/adrf6780.c | 534
> > +++++++++++++++++++++++++++++++
> > > 3 files changed, 548 insertions(+)
> > > create mode 100644 drivers/iio/frequency/adrf6780.c
> > >
> > > diff --git a/drivers/iio/frequency/Kconfig
> > b/drivers/iio/frequency/Kconfig
> > > index 240b81502512..fc9751c48f59 100644
> > > --- a/drivers/iio/frequency/Kconfig
> > > +++ b/drivers/iio/frequency/Kconfig
> > > @@ -49,5 +49,18 @@ config ADF4371
> > >
> > > To compile this driver as a module, choose M here: the
> > > module will be called adf4371.
> > > +
> > > +config ADRF6780
> > > + tristate "Analog Devices ADRF6780 Microwave Upconverter"
> > > + depends on SPI
> > > + depends on COMMON_CLK
> > > + depends on OF
> > > + help
> > > + Say yes here to build support for Analog Devices ADRF6780
> > > + 5.9 GHz to 23.6 GHz, Wideband, Microwave Upconverter.
> > > +
> > > + To compile this driver as a module, choose M here: the
> > > + module will be called adrf6780.
> > > +
> > > endmenu
> > > endmenu
> > > diff --git a/drivers/iio/frequency/Makefile
> > b/drivers/iio/frequency/Makefile
> > > index 518b1e50caef..ae3136c79202 100644
> > > --- a/drivers/iio/frequency/Makefile
> > > +++ b/drivers/iio/frequency/Makefile
> > > @@ -7,3 +7,4 @@
> > > obj-$(CONFIG_AD9523) += ad9523.o
> > > obj-$(CONFIG_ADF4350) += adf4350.o
> > > obj-$(CONFIG_ADF4371) += adf4371.o
> > > +obj-$(CONFIG_ADRF6780) += adrf6780.o
> > > diff --git a/drivers/iio/frequency/adrf6780.c
> > b/drivers/iio/frequency/adrf6780.c
> > > new file mode 100644
> > > index 000000000000..c492c4e4adf1
> > > --- /dev/null
> > > +++ b/drivers/iio/frequency/adrf6780.c
> > > @@ -0,0 +1,534 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> >
> > about the licensing;
> >
> > SPDX-License-Identifier: GPL-2.0+ == MODULE_LICENSE("GPL
> v2");
> > SPDX-License-Identifier: GPL-2.0 == MODULE_LICENSE("GPL v2");
> >
> > I usually don't care about this licensing details, but it seems to be
> > important elsewhere.
> >
> > > +/*
> > > + * ADRF6780 driver
> >
> > This could be "Analog Devices ADRF6780 driver"
> >
> > > + *
> > > + * Copyright 2021 Analog Devices Inc.
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bits.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/clkdev.h>
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/spi/spi.h>
> >
> > Not all these headers look used.
> > For one thing, regmap.h doesn't look used at all.
> > Maybe trim the list.
> >
> > > +
> > > +/* ADRF6780 Register Map */
> > > +#define ADRF6780_REG_CONTROL 0x00
> > > +#define ADRF6780_REG_ALARM_READBACK 0x01
> > > +#define ADRF6780_REG_ALARM_MASKS 0x02
> > > +#define ADRF6780_REG_ENABLE 0x03
> > > +#define ADRF6780_REG_LINEARIZE 0x04
> > > +#define ADRF6780_REG_LO_PATH 0x05
> > > +#define ADRF6780_REG_ADC_CONTROL 0x06
> > > +#define ADRF6780_REG_ADC_OUTPUT 0x0C
> > > +
> > > +/* ADRF6780_REG_CONTROL Map */
> > > +#define ADRF6780_PARITY_EN_MSK BIT(15)
> > > +#define ADRF6780_PARITY_EN(x)
> > FIELD_PREP(ADRF6780_PARITY_EN_MSK, x)
> > > +#define ADRF6780_SOFT_RESET_MSK BIT(14)
> > > +#define ADRF6780_SOFT_RESET(x)
> > FIELD_PREP(ADRF6780_SOFT_RESET_MSK, x)
> > > +#define ADRF6780_CHIP_ID_MSK GENMASK(11, 4)
> > > +#define ADRF6780_CHIP_ID 0xA
> > > +#define ADRF6780_CHIP_REVISION_MSK GENMASK(3, 0)
> > > +#define ADRF6780_CHIP_REVISION(x)
> > FIELD_PREP(ADRF6780_CHIP_REVISION_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ALARM_READBACK Map */
> > > +#define ADRF6780_PARITY_ERROR_MSK BIT(15)
> > > +#define ADRF6780_PARITY_ERROR(x)
> > FIELD_PREP(ADRF6780_PARITY_ERROR_MSK, x)
> > > +#define ADRF6780_TOO_FEW_ERRORS_MSK BIT(14)
> > > +#define ADRF6780_TOO_FEW_ERRORS(x)
> > FIELD_PREP(ADRF6780_TOO_FEW_ERRORS_MSK, x)
> > > +#define ADRF6780_TOO_MANY_ERRORS_MSK BIT(13)
> > > +#define ADRF6780_TOO_MANY_ERRORS(x)
> > FIELD_PREP(ADRF6780_TOO_MANY_ERRORS_MSK, x)
> > > +#define ADRF6780_ADDRESS_RANGE_ERROR_MSK BIT(12)
> > > +#define ADRF6780_ADDRESS_RANGE_ERROR(x)
> > FIELD_PREP(ADRF6780_ADDRESS_RANGE_ERROR_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ENABLE Map */
> > > +#define ADRF6780_VGA_BUFFER_EN_MSK BIT(8)
> > > +#define ADRF6780_VGA_BUFFER_EN(x)
> > FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, x)
> > > +#define ADRF6780_DETECTOR_EN_MSK BIT(7)
> > > +#define ADRF6780_DETECTOR_EN(x)
> > FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, x)
> > > +#define ADRF6780_LO_BUFFER_EN_MSK BIT(6)
> > > +#define ADRF6780_LO_BUFFER_EN(x)
> > FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, x)
> > > +#define ADRF6780_IF_MODE_EN_MSK BIT(5)
> > > +#define ADRF6780_IF_MODE_EN(x)
> > FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, x)
> > > +#define ADRF6780_IQ_MODE_EN_MSK BIT(4)
> > > +#define ADRF6780_IQ_MODE_EN(x)
> > FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, x)
> > > +#define ADRF6780_LO_X2_EN_MSK BIT(3)
> > > +#define ADRF6780_LO_X2_EN(x)
> > FIELD_PREP(ADRF6780_LO_X2_EN_MSK, x)
> > > +#define ADRF6780_LO_PPF_EN_MSK BIT(2)
> > > +#define ADRF6780_LO_PPF_EN(x)
> > FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, x)
> > > +#define ADRF6780_LO_EN_MSK BIT(1)
> > > +#define ADRF6780_LO_EN(x)
> > FIELD_PREP(ADRF6780_LO_EN_MSK, x)
> > > +#define ADRF6780_UC_BIAS_EN_MSK BIT(0)
> > > +#define ADRF6780_UC_BIAS_EN(x)
> > FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, x)
> > > +
> > > +/* ADRF6780_REG_LINEARIZE Map */
> > > +#define ADRF6780_RDAC_LINEARIZE_MSK GENMASK(7, 0)
> > > +#define ADRF6780_RDAC_LINEARIZE(x)
> > FIELD_PREP(ADRF6780_RDAC_LINEARIZE_MSK, x)
> > > +
> > > +/* ADRF6780_REG_LO_PATH Map */
> > > +#define ADRF6780_LO_SIDEBAND_MSK BIT(10)
> > > +#define ADRF6780_LO_SIDEBAND(x)
> > FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, x)
> > > +#define ADRF6780_Q_PATH_PHASE_ACCURACY_MSK
> > GENMASK(7, 4)
> > > +#define ADRF6780_Q_PATH_PHASE_ACCURACY(x)
> > FIELD_PREP(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, x)
> > > +#define ADRF6780_I_PATH_PHASE_ACCURACY_MSK
> > GENMASK(3, 0)
> > > +#define ADRF6780_I_PATH_PHASE_ACCURACY(x)
> > FIELD_PREP(ADRF6780_I_PATH_PHASE_ACCURACY_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ADC_CONTROL Map */
> > > +#define ADRF6780_VDET_OUTPUT_SELECT_MSK BIT(3)
> > > +#define ADRF6780_VDET_OUTPUT_SELECT(x)
> > FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, x)
> > > +#define ADRF6780_ADC_START_MSK BIT(2)
> > > +#define ADRF6780_ADC_START(x)
> > FIELD_PREP(ADRF6780_ADC_START_MSK, x)
> > > +#define ADRF6780_ADC_EN_MSK BIT(1)
> > > +#define ADRF6780_ADC_EN(x)
> > FIELD_PREP(ADRF6780_ADC_EN_MSK, x)
> > > +#define ADRF6780_ADC_CLOCK_EN_MSK BIT(0)
> > > +#define ADRF6780_ADC_CLOCK_EN(x)
> > FIELD_PREP(ADRF6780_ADC_CLOCK_EN_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ADC_OUTPUT Map */
> > > +#define ADRF6780_ADC_STATUS_MSK BIT(8)
> > > +#define ADRF6780_ADC_STATUS(x)
> > FIELD_PREP(ADRF6780_ADC_STATUS_MSK, x)
> > > +#define ADRF6780_ADC_VALUE_MSK GENMASK(7, 0)
> > > +#define ADRF6780_ADC_VALUE(x)
> > FIELD_PREP(ADRF6780_ADC_VALUE_MSK, x)
> >
> > The indentation for the bit-values doesn't look consistent in all
> places.
> >
> > > +
> > > +enum supported_parts {
> > > + ADRF6780,
> > > +};
> >
> > This enum doesn't seem used anywhere
> >
> > > +
> > > +struct adrf6780_dev {
> > > + struct spi_device *spi;
> > > + struct clk *clkin;
> > > + /* Protect against concurrent accesses to the device */
> > > + struct mutex lock;
> > > + bool parity_en;
> >
> > Maybe remove this parity check.
> > There are many drivers that support some form of simple error
> > checking, but in the kernel this is typically left up to the SPI
> > framework.
> > So, I'd just disable the error checking entirely.
>
> I would just argue here... Though parity check is not the most reliable
> one
> out there, it does give some extra checking on top of what the SPI
> framework
> can give (AFAICT). As this part might be used is some sensitive
> applications, I
> can imagine someone wanting this extra feature that does not really
> cost that
> much (if something). However it looks like that on the write side, the
> only way
> to know about parity issues (or other errors) is to make use of ALM
> pin.
>
> Anyways, I do agree that having this as a devicetree parameter is, at
> least, arguable
> (I don't think it is terribly wrong though). But I guess that, at least, as a
> module
> parameter would be something more acceptable.
Nice that we did not went (removed in v2) with the module parameter. Just
found out yesterday [1] that they really not much appreciated in device drivers
(which actually makes sense).
[1]: https://lore.kernel.org/lkml/YN2MtVFOC+yd%2FrRZ@kroah.com/
- Nuno Sá
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-06-30 12:04 ` Sa, Nuno
2021-07-02 7:09 ` Sa, Nuno
@ 2021-07-03 16:18 ` Jonathan Cameron
1 sibling, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2021-07-03 16:18 UTC (permalink / raw)
To: Sa, Nuno
Cc: Alexandru Ardelean, Miclaus, Antoniu, linux-iio, LKML,
devicetree, Rob Herring
On Wed, 30 Jun 2021 12:04:16 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> Hey Alex,
>
> Thanks for the review,
>
> > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > Sent: Wednesday, June 30, 2021 10:38 AM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; LKML <linux-
> > kernel@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> > devicetree <devicetree@vger.kernel.org>; Rob Herring
> > <robh+dt@kernel.org>
> > Subject: Re: [PATCH 1/2] iio: frequency: adrf6780: add support for
> > ADRF6780
> >
> > On Tue, Jun 29, 2021 at 5:25 PM Antoniu Miclaus
> > <antoniu.miclaus@analog.com> wrote:
> > >
> > > Add support for the ADRF6780 microwave upconverter.
> >
> > Hey,
> >
> > A few comments inline.
> > The description of the commit could have a bit more information.
> > Maybe a short description of the chip (typically I'd adapt something
> > from the datasheet).
> > And maybe a link to the datasheet.
> >
> > >
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > ---
> > > drivers/iio/frequency/Kconfig | 13 +
> > > drivers/iio/frequency/Makefile | 1 +
> > > drivers/iio/frequency/adrf6780.c | 534
> > +++++++++++++++++++++++++++++++
> > > 3 files changed, 548 insertions(+)
> > > create mode 100644 drivers/iio/frequency/adrf6780.c
> > >
> > > diff --git a/drivers/iio/frequency/Kconfig
> > b/drivers/iio/frequency/Kconfig
> > > index 240b81502512..fc9751c48f59 100644
> > > --- a/drivers/iio/frequency/Kconfig
> > > +++ b/drivers/iio/frequency/Kconfig
> > > @@ -49,5 +49,18 @@ config ADF4371
> > >
> > > To compile this driver as a module, choose M here: the
> > > module will be called adf4371.
> > > +
> > > +config ADRF6780
> > > + tristate "Analog Devices ADRF6780 Microwave Upconverter"
> > > + depends on SPI
> > > + depends on COMMON_CLK
> > > + depends on OF
> > > + help
> > > + Say yes here to build support for Analog Devices ADRF6780
> > > + 5.9 GHz to 23.6 GHz, Wideband, Microwave Upconverter.
> > > +
> > > + To compile this driver as a module, choose M here: the
> > > + module will be called adrf6780.
> > > +
> > > endmenu
> > > endmenu
> > > diff --git a/drivers/iio/frequency/Makefile
> > b/drivers/iio/frequency/Makefile
> > > index 518b1e50caef..ae3136c79202 100644
> > > --- a/drivers/iio/frequency/Makefile
> > > +++ b/drivers/iio/frequency/Makefile
> > > @@ -7,3 +7,4 @@
> > > obj-$(CONFIG_AD9523) += ad9523.o
> > > obj-$(CONFIG_ADF4350) += adf4350.o
> > > obj-$(CONFIG_ADF4371) += adf4371.o
> > > +obj-$(CONFIG_ADRF6780) += adrf6780.o
> > > diff --git a/drivers/iio/frequency/adrf6780.c
> > b/drivers/iio/frequency/adrf6780.c
> > > new file mode 100644
> > > index 000000000000..c492c4e4adf1
> > > --- /dev/null
> > > +++ b/drivers/iio/frequency/adrf6780.c
> > > @@ -0,0 +1,534 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> >
> > about the licensing;
> >
> > SPDX-License-Identifier: GPL-2.0+ == MODULE_LICENSE("GPL v2");
> > SPDX-License-Identifier: GPL-2.0 == MODULE_LICENSE("GPL v2");
> >
> > I usually don't care about this licensing details, but it seems to be
> > important elsewhere.
> >
> > > +/*
> > > + * ADRF6780 driver
> >
> > This could be "Analog Devices ADRF6780 driver"
> >
> > > + *
> > > + * Copyright 2021 Analog Devices Inc.
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bits.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/clkdev.h>
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/spi/spi.h>
> >
> > Not all these headers look used.
> > For one thing, regmap.h doesn't look used at all.
> > Maybe trim the list.
> >
> > > +
> > > +/* ADRF6780 Register Map */
> > > +#define ADRF6780_REG_CONTROL 0x00
> > > +#define ADRF6780_REG_ALARM_READBACK 0x01
> > > +#define ADRF6780_REG_ALARM_MASKS 0x02
> > > +#define ADRF6780_REG_ENABLE 0x03
> > > +#define ADRF6780_REG_LINEARIZE 0x04
> > > +#define ADRF6780_REG_LO_PATH 0x05
> > > +#define ADRF6780_REG_ADC_CONTROL 0x06
> > > +#define ADRF6780_REG_ADC_OUTPUT 0x0C
> > > +
> > > +/* ADRF6780_REG_CONTROL Map */
> > > +#define ADRF6780_PARITY_EN_MSK BIT(15)
> > > +#define ADRF6780_PARITY_EN(x)
> > FIELD_PREP(ADRF6780_PARITY_EN_MSK, x)
> > > +#define ADRF6780_SOFT_RESET_MSK BIT(14)
> > > +#define ADRF6780_SOFT_RESET(x)
> > FIELD_PREP(ADRF6780_SOFT_RESET_MSK, x)
> > > +#define ADRF6780_CHIP_ID_MSK GENMASK(11, 4)
> > > +#define ADRF6780_CHIP_ID 0xA
> > > +#define ADRF6780_CHIP_REVISION_MSK GENMASK(3, 0)
> > > +#define ADRF6780_CHIP_REVISION(x)
> > FIELD_PREP(ADRF6780_CHIP_REVISION_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ALARM_READBACK Map */
> > > +#define ADRF6780_PARITY_ERROR_MSK BIT(15)
> > > +#define ADRF6780_PARITY_ERROR(x)
> > FIELD_PREP(ADRF6780_PARITY_ERROR_MSK, x)
> > > +#define ADRF6780_TOO_FEW_ERRORS_MSK BIT(14)
> > > +#define ADRF6780_TOO_FEW_ERRORS(x)
> > FIELD_PREP(ADRF6780_TOO_FEW_ERRORS_MSK, x)
> > > +#define ADRF6780_TOO_MANY_ERRORS_MSK BIT(13)
> > > +#define ADRF6780_TOO_MANY_ERRORS(x)
> > FIELD_PREP(ADRF6780_TOO_MANY_ERRORS_MSK, x)
> > > +#define ADRF6780_ADDRESS_RANGE_ERROR_MSK BIT(12)
> > > +#define ADRF6780_ADDRESS_RANGE_ERROR(x)
> > FIELD_PREP(ADRF6780_ADDRESS_RANGE_ERROR_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ENABLE Map */
> > > +#define ADRF6780_VGA_BUFFER_EN_MSK BIT(8)
> > > +#define ADRF6780_VGA_BUFFER_EN(x)
> > FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, x)
> > > +#define ADRF6780_DETECTOR_EN_MSK BIT(7)
> > > +#define ADRF6780_DETECTOR_EN(x)
> > FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, x)
> > > +#define ADRF6780_LO_BUFFER_EN_MSK BIT(6)
> > > +#define ADRF6780_LO_BUFFER_EN(x)
> > FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, x)
> > > +#define ADRF6780_IF_MODE_EN_MSK BIT(5)
> > > +#define ADRF6780_IF_MODE_EN(x)
> > FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, x)
> > > +#define ADRF6780_IQ_MODE_EN_MSK BIT(4)
> > > +#define ADRF6780_IQ_MODE_EN(x)
> > FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, x)
> > > +#define ADRF6780_LO_X2_EN_MSK BIT(3)
> > > +#define ADRF6780_LO_X2_EN(x)
> > FIELD_PREP(ADRF6780_LO_X2_EN_MSK, x)
> > > +#define ADRF6780_LO_PPF_EN_MSK BIT(2)
> > > +#define ADRF6780_LO_PPF_EN(x)
> > FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, x)
> > > +#define ADRF6780_LO_EN_MSK BIT(1)
> > > +#define ADRF6780_LO_EN(x)
> > FIELD_PREP(ADRF6780_LO_EN_MSK, x)
> > > +#define ADRF6780_UC_BIAS_EN_MSK BIT(0)
> > > +#define ADRF6780_UC_BIAS_EN(x)
> > FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, x)
> > > +
> > > +/* ADRF6780_REG_LINEARIZE Map */
> > > +#define ADRF6780_RDAC_LINEARIZE_MSK GENMASK(7, 0)
> > > +#define ADRF6780_RDAC_LINEARIZE(x)
> > FIELD_PREP(ADRF6780_RDAC_LINEARIZE_MSK, x)
> > > +
> > > +/* ADRF6780_REG_LO_PATH Map */
> > > +#define ADRF6780_LO_SIDEBAND_MSK BIT(10)
> > > +#define ADRF6780_LO_SIDEBAND(x)
> > FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, x)
> > > +#define ADRF6780_Q_PATH_PHASE_ACCURACY_MSK
> > GENMASK(7, 4)
> > > +#define ADRF6780_Q_PATH_PHASE_ACCURACY(x)
> > FIELD_PREP(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, x)
> > > +#define ADRF6780_I_PATH_PHASE_ACCURACY_MSK
> > GENMASK(3, 0)
> > > +#define ADRF6780_I_PATH_PHASE_ACCURACY(x)
> > FIELD_PREP(ADRF6780_I_PATH_PHASE_ACCURACY_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ADC_CONTROL Map */
> > > +#define ADRF6780_VDET_OUTPUT_SELECT_MSK BIT(3)
> > > +#define ADRF6780_VDET_OUTPUT_SELECT(x)
> > FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, x)
> > > +#define ADRF6780_ADC_START_MSK BIT(2)
> > > +#define ADRF6780_ADC_START(x)
> > FIELD_PREP(ADRF6780_ADC_START_MSK, x)
> > > +#define ADRF6780_ADC_EN_MSK BIT(1)
> > > +#define ADRF6780_ADC_EN(x)
> > FIELD_PREP(ADRF6780_ADC_EN_MSK, x)
> > > +#define ADRF6780_ADC_CLOCK_EN_MSK BIT(0)
> > > +#define ADRF6780_ADC_CLOCK_EN(x)
> > FIELD_PREP(ADRF6780_ADC_CLOCK_EN_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ADC_OUTPUT Map */
> > > +#define ADRF6780_ADC_STATUS_MSK BIT(8)
> > > +#define ADRF6780_ADC_STATUS(x)
> > FIELD_PREP(ADRF6780_ADC_STATUS_MSK, x)
> > > +#define ADRF6780_ADC_VALUE_MSK GENMASK(7, 0)
> > > +#define ADRF6780_ADC_VALUE(x)
> > FIELD_PREP(ADRF6780_ADC_VALUE_MSK, x)
> >
> > The indentation for the bit-values doesn't look consistent in all places.
> >
> > > +
> > > +enum supported_parts {
> > > + ADRF6780,
> > > +};
> >
> > This enum doesn't seem used anywhere
> >
> > > +
> > > +struct adrf6780_dev {
> > > + struct spi_device *spi;
> > > + struct clk *clkin;
> > > + /* Protect against concurrent accesses to the device */
> > > + struct mutex lock;
> > > + bool parity_en;
> >
> > Maybe remove this parity check.
> > There are many drivers that support some form of simple error
> > checking, but in the kernel this is typically left up to the SPI
> > framework.
> > So, I'd just disable the error checking entirely.
>
> I would just argue here... Though parity check is not the most reliable one
> out there, it does give some extra checking on top of what the SPI framework
> can give (AFAICT). As this part might be used is some sensitive applications, I
> can imagine someone wanting this extra feature that does not really cost that
> much (if something). However it looks like that on the write side, the only way
> to know about parity issues (or other errors) is to make use of ALM pin.
If there is hardware parity support and we can safely use it (ie. the ALM pin
is connected) then I would support it as always on.
Note I haven't looked at the code yet, so this is just a general principle.
Not something for a module parameter though. If it's really costly for some reason
we might provide a runtime control to disable it.
>
> Anyways, I do agree that having this as a devicetree parameter is, at least, arguable
> (I don't think it is terribly wrong though). But I guess that, at least, as a module
> parameter would be something more acceptable.
>
> Or, as you suggested, It can also be removed and only implemented if someone
> really cares/asks for it...
>
> - Nuno Sá
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-03 16:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-29 14:23 [PATCH 0/2] iio: frequency: adrf6780: add support for ADRF6780 Antoniu Miclaus
2021-06-29 14:23 ` [PATCH 1/2] " Antoniu Miclaus
2021-06-30 8:38 ` Alexandru Ardelean
2021-06-30 12:04 ` Sa, Nuno
2021-07-02 7:09 ` Sa, Nuno
2021-07-03 16:18 ` Jonathan Cameron
2021-06-29 14:23 ` [PATCH 2/2] dt-bindings: iio: frequency: add adrf6780 doc Antoniu Miclaus
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.