* [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780
@ 2021-07-02 11:12 Antoniu Miclaus
2021-07-02 11:12 ` [PATCH v4 2/2] dt-bindings: iio: frequency: add adrf6780 doc Antoniu Miclaus
2021-07-03 16:57 ` [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780 Jonathan Cameron
0 siblings, 2 replies; 12+ messages in thread
From: Antoniu Miclaus @ 2021-07-02 11:12 UTC (permalink / raw)
To: linux-iio, linux-kernel, jic23, devicetree, robh+dt; +Cc: Antoniu Miclaus
The ADRF6780 is a silicon germanium (SiGe) design, wideband,
microwave upconverter optimized for point to point microwave
radio designs operating in the 5.9 GHz to 23.6 GHz frequency
range.
Datasheet:
https://www.analog.com/media/en/technical-documentation/data-sheets/ADRF6780.pdf
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v4:
- change license to: GPL-2.0-only
drivers/iio/frequency/Kconfig | 13 +
drivers/iio/frequency/Makefile | 1 +
drivers/iio/frequency/adrf6780.c | 498 +++++++++++++++++++++++++++++++
3 files changed, 512 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..472a66f90c7f
--- /dev/null
+++ b/drivers/iio/frequency/adrf6780.c
@@ -0,0 +1,498 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ADRF6780 driver
+ *
+ * Copyright 2021 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/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)
+
+struct adrf6780_dev {
+ struct spi_device *spi;
+ struct clk *clkin;
+ /* Protect against concurrent accesses to the device */
+ struct mutex lock;
+ bool vga_buff_en;
+ bool lo_buff_en;
+ bool if_mode_en;
+ bool iq_mode_en;
+ bool lo_x2_en;
+ bool lo_ppf_en;
+ bool lo_en;
+ bool uc_bias_en;
+ bool lo_sideband;
+ bool vdet_out_en;
+};
+
+static int adrf6780_spi_read(struct adrf6780_dev *dev, unsigned int reg,
+ unsigned int *val)
+{
+ int ret;
+ unsigned int 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];
+
+ *val = (temp >> 1) & 0xFFFF;
+
+ return ret;
+}
+
+static int adrf6780_spi_write(struct adrf6780_dev *dev,
+ unsigned int reg,
+ unsigned int val)
+{
+ u8 data[3];
+
+ val = (val << 1);
+
+ 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_ADC_CONTROL,
+ ADRF6780_ADC_EN_MSK |
+ ADRF6780_ADC_CLOCK_EN_MSK |
+ ADRF6780_ADC_START_MSK,
+ ADRF6780_ADC_EN(1) |
+ ADRF6780_ADC_CLOCK_EN(1) |
+ 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_reset(struct adrf6780_dev *dev)
+{
+ int ret;
+ struct spi_device *spi = dev->spi;
+
+ ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
+ ADRF6780_SOFT_RESET_MSK,
+ 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;
+ }
+
+ return ret;
+}
+
+static int adrf6780_init(struct adrf6780_dev *dev)
+{
+ int ret;
+ unsigned int chip_id, enable_reg, enable_reg_msk;
+ struct spi_device *spi = dev->spi;
+
+ /* Perform a software reset */
+ ret = adrf6780_reset(dev);
+ if (ret < 0)
+ return ret;
+
+ 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(1) |
+ 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->vga_buff_en = of_property_read_bool(spi->dev.of_node, "adi,vga-buff-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");
+
+ 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->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;
+
+ dev->clkin = devm_clk_get(&spi->dev, "lo_in");
+ if (IS_ERR(dev->clkin))
+ return PTR_ERR(dev->clkin);
+
+ ret = clk_prepare_enable(dev->clkin);
+ if (ret < 0)
+ return ret;
+
+ ret = devm_add_action_or_reset(&spi->dev, adrf6780_clk_disable, dev->clkin);
+ if (ret < 0)
+ return ret;
+
+ mutex_init(&dev->lock);
+
+ ret = adrf6780_init(dev);
+ if (ret < 0)
+ return ret;
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id adrf6780_id[] = {
+ { "adrf6780", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(spi, adrf6780_id);
+
+static const struct of_device_id adrf6780_of_match[] = {
+ { .compatible = "adi,adrf6780" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, adrf6780_of_match);
+
+static struct spi_driver adrf6780_driver = {
+ .driver = {
+ .name = "adrf6780",
+ .of_match_table = adrf6780_of_match,
+ },
+ .probe = adrf6780_probe,
+ .id_table = adrf6780_id,
+};
+module_spi_driver(adrf6780_driver);
+
+MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com");
+MODULE_DESCRIPTION("Analog Devices ADRF6780");
+MODULE_LICENSE("GPL v2");
--
2.32.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v4 2/2] dt-bindings: iio: frequency: add adrf6780 doc
2021-07-02 11:12 [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780 Antoniu Miclaus
@ 2021-07-02 11:12 ` Antoniu Miclaus
2021-07-14 22:56 ` Rob Herring
2021-07-03 16:57 ` [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780 Jonathan Cameron
1 sibling, 1 reply; 12+ messages in thread
From: Antoniu Miclaus @ 2021-07-02 11:12 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>
---
changes in v4:
- fix dt_binding_check detected issues
- update license to GPL-2.0-only OR BSD-2-Clause
- add dependencies schema
- other minor fixes based on the review received
.../bindings/iio/frequency/adi,adrf6780.yaml | 119 ++++++++++++++++++
1 file changed, 119 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
new file mode 100644
index 000000000000..61a7a45837ae
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
@@ -0,0 +1,119 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/frequency/adi,adrf6780.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADRF6780 Microwave Upconverter
+
+maintainers:
+ - Antoniu Miclaus <antoniu.miclaus@analog.com>
+
+description: |
+ Wideband, microwave upconverter optimized for point to point microwave
+ radio designs operating in the 5.9 GHz to 23.6 GHz frequency range.
+
+ https://www.analog.com/en/products/adrf6780.html
+
+properties:
+ compatible:
+ enum:
+ - adi,adrf6780
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 1000000
+
+ clocks:
+ description:
+ Definition of the external clock.
+ minItems: 1
+
+ clock-names:
+ items:
+ - const: lo_in
+
+ clock-output-names:
+ maxItems: 1
+
+ adi,vga-buff-en:
+ description:
+ VGA Buffer Enable.
+ type: boolean
+
+ adi,lo-buff-en:
+ description:
+ LO Buffer Enable.
+ type: boolean
+
+ adi,if-mode-en:
+ description:
+ IF Mode Enable.
+ type: boolean
+
+ adi,iq-mode-en:
+ description:
+ IQ Mode Enable.
+ type: boolean
+
+ adi,lo-x2-en:
+ description:
+ LO x2 Enable.
+ type: boolean
+
+ adi,lo-ppf-en:
+ description:
+ LO x1 Enable.
+ type: boolean
+
+ adi,lo-en:
+ description:
+ LO Enable.
+ type: boolean
+
+ adi,uc-bias-en:
+ description:
+ UC Bias Enable.
+ type: boolean
+
+ adi,lo-sideband:
+ description:
+ Switch to the Other LO Sideband.
+ type: boolean
+
+ adi,vdet-out-en:
+ description:
+ VDET Output Select Enable.
+ type: boolean
+
+ '#clock-cells':
+ const: 0
+
+dependencies:
+ adi,lo-x2-en: [ "adi,lo-en" ]
+ adi,lo-ppf-en: [ "adi,lo-en" ]
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+examples:
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ adrf6780@0 {
+ compatible = "adi,adrf6780";
+ reg = <0>;
+ spi-max-frequency = <1000000>;
+ clocks = <&adrf6780_lo>;
+ clock-names = "lo_in";
+ };
+ };
+...
--
2.32.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-07-02 11:12 [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780 Antoniu Miclaus
2021-07-02 11:12 ` [PATCH v4 2/2] dt-bindings: iio: frequency: add adrf6780 doc Antoniu Miclaus
@ 2021-07-03 16:57 ` Jonathan Cameron
2021-07-05 10:18 ` Sa, Nuno
1 sibling, 1 reply; 12+ messages in thread
From: Jonathan Cameron @ 2021-07-03 16:57 UTC (permalink / raw)
To: Antoniu Miclaus; +Cc: linux-iio, linux-kernel, devicetree, robh+dt
On Fri, 2 Jul 2021 14:12:38 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> The ADRF6780 is a silicon germanium (SiGe) design, wideband,
> microwave upconverter optimized for point to point microwave
> radio designs operating in the 5.9 GHz to 23.6 GHz frequency
> range.
>
> Datasheet:
> https://www.analog.com/media/en/technical-documentation/data-sheets/ADRF6780.pdf
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Hi Antoniu,
Frequency drivers are fairly unusual so if you could add a listing of
the attributes in sysfs that would be great (it's nice practice anyway but
I don't insist on it!)
Various fairly minor comments inline.
Thanks,
Jonathan
> ---
> changes in v4:
> - change license to: GPL-2.0-only
> drivers/iio/frequency/Kconfig | 13 +
> drivers/iio/frequency/Makefile | 1 +
> drivers/iio/frequency/adrf6780.c | 498 +++++++++++++++++++++++++++++++
> 3 files changed, 512 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
Why? Pretty much everything seems to have defaults if not provided via OF.
I've asked for the generic firmware functions anyway, so you can drop this
for that reason if nothing else!
> + 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..472a66f90c7f
> --- /dev/null
> +++ b/drivers/iio/frequency/adrf6780.c
> @@ -0,0 +1,498 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ADRF6780 driver
> + *
> + * Copyright 2021 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
#include <linux/mod_devicetable.h>
> +#include <linux/spi/spi.h>
> +
> +/* 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)
Not used. In general, just use FIELD_PREP / FIELD_GET inline rather than having extra
macros like these. That approach is simpler for reviewers to follow.
> +
> +struct adrf6780_dev {
> + struct spi_device *spi;
> + struct clk *clkin;
> + /* Protect against concurrent accesses to the device */
> + struct mutex lock;
> + bool vga_buff_en;
> + bool lo_buff_en;
> + bool if_mode_en;
> + bool iq_mode_en;
> + bool lo_x2_en;
> + bool lo_ppf_en;
> + bool lo_en;
> + bool uc_bias_en;
> + bool lo_sideband;
> + bool vdet_out_en;
> +};
> +
> +static int adrf6780_spi_read(struct adrf6780_dev *dev, unsigned int reg,
> + unsigned int *val)
> +{
> + int ret;
> + unsigned int 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);
data needs to be dma safe.
> + if (ret < 0)
> + return ret;
> +
> + temp = ((data[0] | 0x80 | (reg << 1)) << 16) |
> + (data[1] << 8) | data[2];
Ouch. That's a bit nasty, but why are you writing the reg into
it? Looks like a get_unaligned_be24() >> 1 and a 16bit mask.
(use GENMASK(15, 0) for that to make it apparent what is happening.
> +
> + *val = (temp >> 1) & 0xFFFF;
> +
> + return ret;
> +}
> +
> +static int adrf6780_spi_write(struct adrf6780_dev *dev,
> + unsigned int reg,
> + unsigned int val)
> +{
> + u8 data[3];
> +
> + val = (val << 1);
> +
> + data[0] = (reg << 1) | (val >> 16);
> + data[1] = val >> 8;
> + data[2] = val;
An opportunity for
put_unaligned_be24() with a value of (I think)
(val << 1) | (reg << 17)
> +
> + return spi_write(dev->spi, &data[0], 3);
Needs a dma safe buffer, which basically means it can't be on the stack.
Lots of ways of handling that, but look for __cacheline_aligned in IIO drivers
to see the one we probably use mostly commonly in IIO drivers.
> +}
> +
> +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)
As this is ultimately getting the result of spi_sync_transfer() which never returns > 0
you can switch this to the more relaxed if (ret) That ends up more consistent
if you are going to do return adrf6780 below. (also only returns <= 0 but that's
not obvious given you have this check here on the equivalent read function that
hints that positive can occur.
> + 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)
This is so rarely called, that I'm not sure it is worth it existing (as opposed
to just having this code inline where needed).
The naming is currently inconsistent. adrf6780_spi_read() doesn't take the lock
but adrf6780_spi_update_bits() does.
So drop this function and rename unlocked version to adrf6780_spi_update_bits()
> +{
> + 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_ADC_CONTROL,
> + ADRF6780_ADC_EN_MSK |
> + ADRF6780_ADC_CLOCK_EN_MSK |
> + ADRF6780_ADC_START_MSK,
> + ADRF6780_ADC_EN(1) |
> + ADRF6780_ADC_CLOCK_EN(1) |
> + 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:
Having a label inside a switch like this often indicates that we should probably
have factored some code out another function. I think that's the case here.
Introduce a function called something like
adrf6780_read_voltage() that does everything between the locks. That way
you can unlock and return without considering if there is an error (in this outer
function). In the inner function, all the goto stuff becomes a direct return.
> + 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)
Where you have two options you are matching like here, I'd prefer to see a switch statement
(with a default that returns an error). That makes it immediately clear there are only
two valid options.
> + 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_reset(struct adrf6780_dev *dev)
> +{
> + int ret;
> + struct spi_device *spi = dev->spi;
> +
> + ret = __adrf6780_spi_update_bits(dev, ADRF6780_REG_CONTROL,
> + ADRF6780_SOFT_RESET_MSK,
> + 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;
> + }
> +
> + return ret;
return 0; Make the value obvious where you can. we can't get here with a positive
value but that isn't clear from looking at the function.
> +}
> +
> +static int adrf6780_init(struct adrf6780_dev *dev)
> +{
> + int ret;
> + unsigned int chip_id, enable_reg, enable_reg_msk;
> + struct spi_device *spi = dev->spi;
> +
> + /* Perform a software reset */
> + ret = adrf6780_reset(dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = adrf6780_spi_read(dev, ADRF6780_REG_CONTROL, &chip_id);
> + if (ret < 0)
> + return ret;
> +
> + chip_id = (chip_id & ADRF6780_CHIP_ID_MSK) >> 4;
FIELD_GET()
> + 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(1) |
> + 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));
I mention this above, but where possible make it clear that positive values don't occur
with if (ret) rather that if (ret < 0)
That way we don't end up suggesting this function might return a positive value.
> + 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->vga_buff_en = of_property_read_bool(spi->dev.of_node, "adi,vga-buff-en");
generic firmware functions preferred.
See include/linux/property.h
> + 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");
> +
> + return 0;
If there are no possible errors, then change the function to return void and
drop the error check at the caller.
> +}
> +
> +static int adrf6780_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct adrf6780_dev *dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*dev));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + dev = iio_priv(indio_dev);
> +
> + indio_dev->info = &adrf6780_info;
> + indio_dev->name = "adrf6780";
> + indio_dev->channels = adrf6780_channels;
> + indio_dev->num_channels = ARRAY_SIZE(adrf6780_channels);
> +
> + dev->spi = spi;
> +
> + ret = adrf6780_dt_parse(dev);
> + if (ret < 0)
> + return ret;
> +
> + dev->clkin = devm_clk_get(&spi->dev, "lo_in");
> + if (IS_ERR(dev->clkin))
> + return PTR_ERR(dev->clkin);
> +
> + ret = clk_prepare_enable(dev->clkin);
> + if (ret < 0)
> + return ret;
> +
> + ret = devm_add_action_or_reset(&spi->dev, adrf6780_clk_disable, dev->clkin);
> + if (ret < 0)
> + return ret;
> +
> + mutex_init(&dev->lock);
> +
> + ret = adrf6780_init(dev);
> + if (ret < 0)
> + return ret;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct spi_device_id adrf6780_id[] = {
> + { "adrf6780", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, adrf6780_id);
> +
> +static const struct of_device_id adrf6780_of_match[] = {
> + { .compatible = "adi,adrf6780" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, adrf6780_of_match);
> +
> +static struct spi_driver adrf6780_driver = {
> + .driver = {
> + .name = "adrf6780",
> + .of_match_table = adrf6780_of_match,
> + },
> + .probe = adrf6780_probe,
> + .id_table = adrf6780_id,
> +};
> +module_spi_driver(adrf6780_driver);
> +
> +MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com");
> +MODULE_DESCRIPTION("Analog Devices ADRF6780");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-07-03 16:57 ` [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780 Jonathan Cameron
@ 2021-07-05 10:18 ` Sa, Nuno
2021-07-06 9:04 ` Jonathan Cameron
0 siblings, 1 reply; 12+ messages in thread
From: Sa, Nuno @ 2021-07-05 10:18 UTC (permalink / raw)
To: Jonathan Cameron, Miclaus, Antoniu
Cc: linux-iio, linux-kernel, devicetree, robh+dt
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, July 3, 2021 6:57 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; robh+dt@kernel.org
> Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
>
> On Fri, 2 Jul 2021 14:12:38 +0300
> Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
>
> > The ADRF6780 is a silicon germanium (SiGe) design, wideband,
> > microwave upconverter optimized for point to point microwave
> > radio designs operating in the 5.9 GHz to 23.6 GHz frequency
> > range.
> >
> > Datasheet:
> > https://www.analog.com/media/en/technical-documentation/data-
> sheets/ADRF6780.pdf
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
>
> Hi Antoniu,
>
> Frequency drivers are fairly unusual so if you could add a listing of
> the attributes in sysfs that would be great (it's nice practice anyway but
> I don't insist on it!)
>
> Various fairly minor comments inline.
>
> Thanks,
>
> Jonathan
>
>
> > ---
> > changes in v4:
> > - change license to: GPL-2.0-only
> > drivers/iio/frequency/Kconfig | 13 +
> > drivers/iio/frequency/Makefile | 1 +
> > drivers/iio/frequency/adrf6780.c | 498
> +++++++++++++++++++++++++++++++
> > 3 files changed, 512 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
>
> Why? Pretty much everything seems to have defaults if not provided
> via OF.
> I've asked for the generic firmware functions anyway, so you can drop
> this
> for that reason if nothing else!
>
> > + 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..472a66f90c7f
> > --- /dev/null
> > +++ b/drivers/iio/frequency/adrf6780.c
> > @@ -0,0 +1,498 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * ADRF6780 driver
> > + *
> > + * Copyright 2021 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/clk.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
>
> #include <linux/mod_devicetable.h>
>
> > +#include <linux/spi/spi.h>
> > +
> > +/* 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)
>
> Not used. In general, just use FIELD_PREP / FIELD_GET inline rather
> than having extra
> macros like these. That approach is simpler for reviewers to follow.
>
> > +
> > +struct adrf6780_dev {
> > + struct spi_device *spi;
> > + struct clk *clkin;
> > + /* Protect against concurrent accesses to the device */
> > + struct mutex lock;
> > + bool vga_buff_en;
> > + bool lo_buff_en;
> > + bool if_mode_en;
> > + bool iq_mode_en;
> > + bool lo_x2_en;
> > + bool lo_ppf_en;
> > + bool lo_en;
> > + bool uc_bias_en;
> > + bool lo_sideband;
> > + bool vdet_out_en;
> > +};
> > +
> > +static int adrf6780_spi_read(struct adrf6780_dev *dev, unsigned int
> reg,
> > + unsigned int *val)
> > +{
> > + int ret;
> > + unsigned int 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);
>
> data needs to be dma safe.
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + temp = ((data[0] | 0x80 | (reg << 1)) << 16) |
> > + (data[1] << 8) | data[2];
>
> Ouch. That's a bit nasty, but why are you writing the reg into
> it? Looks like a get_unaligned_be24() >> 1 and a 16bit mask.
> (use GENMASK(15, 0) for that to make it apparent what is happening.
>
> > +
> > + *val = (temp >> 1) & 0xFFFF;
> > +
> > + return ret;
> > +}
> > +
> > +static int adrf6780_spi_write(struct adrf6780_dev *dev,
> > + unsigned int reg,
> > + unsigned int val)
> > +{
> > + u8 data[3];
> > +
> > + val = (val << 1);
> > +
> > + data[0] = (reg << 1) | (val >> 16);
> > + data[1] = val >> 8;
> > + data[2] = val;
>
> An opportunity for
> put_unaligned_be24() with a value of (I think)
>
> (val << 1) | (reg << 17)
>
>
> > +
> > + return spi_write(dev->spi, &data[0], 3);
>
> Needs a dma safe buffer, which basically means it can't be on the
> stack.
> Lots of ways of handling that, but look for __cacheline_aligned in IIO
> drivers
> to see the one we probably use mostly commonly in IIO drivers.
Hi Jonathan,
This is something I wanted to ask for some time so I will take the opportunity here :).
Is this something you prefer just not to risk at all and make it an hard requirement
(which is fair)? ...
I'm asking this because, tbh, I would be very surprised if any spi/i2c controller out there
is using dma for a 3byte transfer. I guess the overhead of setting it up is probably not
worth it...
For instance, in i2c [1], the "educated guess" is around 8byte (to start using dma safe buffers).
[1]: https://elixir.bootlin.com/linux/latest/source/Documentation/i2c/dma-considerations.rst#L15
- Nuno Sá
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-07-05 10:18 ` Sa, Nuno
@ 2021-07-06 9:04 ` Jonathan Cameron
2021-07-06 10:22 ` Sa, Nuno
0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Cameron @ 2021-07-06 9:04 UTC (permalink / raw)
To: Sa, Nuno
Cc: Jonathan Cameron, Miclaus, Antoniu, linux-iio, linux-kernel,
devicetree, robh+dt
On Mon, 5 Jul 2021 10:18:51 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, July 3, 2021 6:57 PM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org; robh+dt@kernel.org
> > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for
> > ADRF6780
> >
> > On Fri, 2 Jul 2021 14:12:38 +0300
> > Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> >
> > > The ADRF6780 is a silicon germanium (SiGe) design, wideband,
> > > microwave upconverter optimized for point to point microwave
> > > radio designs operating in the 5.9 GHz to 23.6 GHz frequency
> > > range.
> > >
> > > Datasheet:
> > > https://www.analog.com/media/en/technical-documentation/data-
> > sheets/ADRF6780.pdf
> > >
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> >
> > Hi Antoniu,
> >
> > Frequency drivers are fairly unusual so if you could add a listing of
> > the attributes in sysfs that would be great (it's nice practice anyway but
> > I don't insist on it!)
> >
> > Various fairly minor comments inline.
> >
> > Thanks,
> >
> > Jonathan
> >
> >
> > > ---
> > > changes in v4:
> > > - change license to: GPL-2.0-only
> > > drivers/iio/frequency/Kconfig | 13 +
> > > drivers/iio/frequency/Makefile | 1 +
> > > drivers/iio/frequency/adrf6780.c | 498
> > +++++++++++++++++++++++++++++++
> > > 3 files changed, 512 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
> >
> > Why? Pretty much everything seems to have defaults if not provided
> > via OF.
> > I've asked for the generic firmware functions anyway, so you can drop
> > this
> > for that reason if nothing else!
> >
> > > + 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..472a66f90c7f
> > > --- /dev/null
> > > +++ b/drivers/iio/frequency/adrf6780.c
> > > @@ -0,0 +1,498 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * ADRF6780 driver
> > > + *
> > > + * Copyright 2021 Analog Devices Inc.
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bits.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/clkdev.h>
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/module.h>
> >
> > #include <linux/mod_devicetable.h>
> >
> > > +#include <linux/spi/spi.h>
> > > +
> > > +/* 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)
> >
> > Not used. In general, just use FIELD_PREP / FIELD_GET inline rather
> > than having extra
> > macros like these. That approach is simpler for reviewers to follow.
> >
> > > +
> > > +struct adrf6780_dev {
> > > + struct spi_device *spi;
> > > + struct clk *clkin;
> > > + /* Protect against concurrent accesses to the device */
> > > + struct mutex lock;
> > > + bool vga_buff_en;
> > > + bool lo_buff_en;
> > > + bool if_mode_en;
> > > + bool iq_mode_en;
> > > + bool lo_x2_en;
> > > + bool lo_ppf_en;
> > > + bool lo_en;
> > > + bool uc_bias_en;
> > > + bool lo_sideband;
> > > + bool vdet_out_en;
> > > +};
> > > +
> > > +static int adrf6780_spi_read(struct adrf6780_dev *dev, unsigned int
> > reg,
> > > + unsigned int *val)
> > > +{
> > > + int ret;
> > > + unsigned int 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);
> >
> > data needs to be dma safe.
> >
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + temp = ((data[0] | 0x80 | (reg << 1)) << 16) |
> > > + (data[1] << 8) | data[2];
> >
> > Ouch. That's a bit nasty, but why are you writing the reg into
> > it? Looks like a get_unaligned_be24() >> 1 and a 16bit mask.
> > (use GENMASK(15, 0) for that to make it apparent what is happening.
> >
> > > +
> > > + *val = (temp >> 1) & 0xFFFF;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int adrf6780_spi_write(struct adrf6780_dev *dev,
> > > + unsigned int reg,
> > > + unsigned int val)
> > > +{
> > > + u8 data[3];
> > > +
> > > + val = (val << 1);
> > > +
> > > + data[0] = (reg << 1) | (val >> 16);
> > > + data[1] = val >> 8;
> > > + data[2] = val;
> >
> > An opportunity for
> > put_unaligned_be24() with a value of (I think)
> >
> > (val << 1) | (reg << 17)
> >
> >
> > > +
> > > + return spi_write(dev->spi, &data[0], 3);
> >
> > Needs a dma safe buffer, which basically means it can't be on the
> > stack.
> > Lots of ways of handling that, but look for __cacheline_aligned in IIO
> > drivers
> > to see the one we probably use mostly commonly in IIO drivers.
>
> Hi Jonathan,
>
> This is something I wanted to ask for some time so I will take the opportunity here :).
> Is this something you prefer just not to risk at all and make it an hard requirement
> (which is fair)? ...
Yes, I think we need to keep this as a hard requirement.
There are drivers out there which we missed this on in the past, and I'm not necessarily
going to take the time to go through them all as this can be hard to spot, but lets not
introduce any more potential problems.
>
> I'm asking this because, tbh, I would be very surprised if any spi/i2c controller out there
> is using dma for a 3byte transfer. I guess the overhead of setting it up is probably not
> worth it...
There are (I believe) a few i2c and spi controllers out there that don't do anything other
than DMA. Wolfram mentioned one of those in his talk on adding DMA support to i2c.
Also, the reference in the file below to the wonderful case of USB to i2c bridges that always
require DMA safe buffers.
>
> For instance, in i2c [1], the "educated guess" is around 8byte (to start using dma safe buffers).
>
> [1]: https://elixir.bootlin.com/linux/latest/source/Documentation/i2c/dma-considerations.rst#L15
>
> - Nuno Sá
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-07-06 9:04 ` Jonathan Cameron
@ 2021-07-06 10:22 ` Sa, Nuno
2021-07-07 8:26 ` Sa, Nuno
0 siblings, 1 reply; 12+ messages in thread
From: Sa, Nuno @ 2021-07-06 10:22 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, Miclaus, Antoniu, linux-iio, linux-kernel,
devicetree, robh+dt
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Tuesday, July 6, 2021 11:04 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org;
> robh+dt@kernel.org
> Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
>
> On Mon, 5 Jul 2021 10:18:51 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
>
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Saturday, July 3, 2021 6:57 PM
> > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > devicetree@vger.kernel.org; robh+dt@kernel.org
> > > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support
> for
> > > ADRF6780
> > >
> > > On Fri, 2 Jul 2021 14:12:38 +0300
> > > Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> > >
> > > > The ADRF6780 is a silicon germanium (SiGe) design, wideband,
> > > > microwave upconverter optimized for point to point microwave
> > > > radio designs operating in the 5.9 GHz to 23.6 GHz frequency
> > > > range.
> > > >
> > > > Datasheet:
> > > > https://www.analog.com/media/en/technical-
> documentation/data-
> > > sheets/ADRF6780.pdf
> > > >
> > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > >
> > > Hi Antoniu,
> > >
> > > Frequency drivers are fairly unusual so if you could add a listing of
> > > the attributes in sysfs that would be great (it's nice practice anyway
> but
> > > I don't insist on it!)
> > >
> > > Various fairly minor comments inline.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > >
> > > > ---
> > > > changes in v4:
> > > > - change license to: GPL-2.0-only
> > > > drivers/iio/frequency/Kconfig | 13 +
> > > > drivers/iio/frequency/Makefile | 1 +
> > > > drivers/iio/frequency/adrf6780.c | 498
> > > +++++++++++++++++++++++++++++++
> > > > 3 files changed, 512 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
> > >
> > > Why? Pretty much everything seems to have defaults if not
> provided
> > > via OF.
> > > I've asked for the generic firmware functions anyway, so you can
> drop
> > > this
> > > for that reason if nothing else!
> > >
> > > > + 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..472a66f90c7f
> > > > --- /dev/null
> > > > +++ b/drivers/iio/frequency/adrf6780.c
> > > > @@ -0,0 +1,498 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * ADRF6780 driver
> > > > + *
> > > > + * Copyright 2021 Analog Devices Inc.
> > > > + */
> > > > +
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/bits.h>
> > > > +#include <linux/clk.h>
> > > > +#include <linux/clkdev.h>
> > > > +#include <linux/clk-provider.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/module.h>
> > >
> > > #include <linux/mod_devicetable.h>
> > >
> > > > +#include <linux/spi/spi.h>
> > > > +
> > > > +/* 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)
> > >
> > > Not used. In general, just use FIELD_PREP / FIELD_GET inline
> rather
> > > than having extra
> > > macros like these. That approach is simpler for reviewers to follow.
> > >
> > > > +
> > > > +struct adrf6780_dev {
> > > > + struct spi_device *spi;
> > > > + struct clk *clkin;
> > > > + /* Protect against concurrent accesses to the device */
> > > > + struct mutex lock;
> > > > + bool vga_buff_en;
> > > > + bool lo_buff_en;
> > > > + bool if_mode_en;
> > > > + bool iq_mode_en;
> > > > + bool lo_x2_en;
> > > > + bool lo_ppf_en;
> > > > + bool lo_en;
> > > > + bool uc_bias_en;
> > > > + bool lo_sideband;
> > > > + bool vdet_out_en;
> > > > +};
> > > > +
> > > > +static int adrf6780_spi_read(struct adrf6780_dev *dev,
> unsigned int
> > > reg,
> > > > + unsigned int *val)
> > > > +{
> > > > + int ret;
> > > > + unsigned int 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);
> > >
> > > data needs to be dma safe.
> > >
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + temp = ((data[0] | 0x80 | (reg << 1)) << 16) |
> > > > + (data[1] << 8) | data[2];
> > >
> > > Ouch. That's a bit nasty, but why are you writing the reg into
> > > it? Looks like a get_unaligned_be24() >> 1 and a 16bit mask.
> > > (use GENMASK(15, 0) for that to make it apparent what is
> happening.
> > >
> > > > +
> > > > + *val = (temp >> 1) & 0xFFFF;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int adrf6780_spi_write(struct adrf6780_dev *dev,
> > > > + unsigned int reg,
> > > > + unsigned int val)
> > > > +{
> > > > + u8 data[3];
> > > > +
> > > > + val = (val << 1);
> > > > +
> > > > + data[0] = (reg << 1) | (val >> 16);
> > > > + data[1] = val >> 8;
> > > > + data[2] = val;
> > >
> > > An opportunity for
> > > put_unaligned_be24() with a value of (I think)
> > >
> > > (val << 1) | (reg << 17)
> > >
> > >
> > > > +
> > > > + return spi_write(dev->spi, &data[0], 3);
> > >
> > > Needs a dma safe buffer, which basically means it can't be on the
> > > stack.
> > > Lots of ways of handling that, but look for __cacheline_aligned in
> IIO
> > > drivers
> > > to see the one we probably use mostly commonly in IIO drivers.
> >
> > Hi Jonathan,
> >
> > This is something I wanted to ask for some time so I will take the
> opportunity here :).
> > Is this something you prefer just not to risk at all and make it an hard
> requirement
> > (which is fair)? ...
>
> Yes, I think we need to keep this as a hard requirement.
> There are drivers out there which we missed this on in the past, and
> I'm not necessarily
> going to take the time to go through them all as this can be hard to
> spot, but lets not
> introduce any more potential problems.
>
I see. That makes sense and it's fair :). The only annoying (but not too annoying :)) is that
making the data/buffer global forces you to use a lock in cases you potentially would
not have too (just using local buffers). But that's life, better play safe :)
> >
> > I'm asking this because, tbh, I would be very surprised if any spi/i2c
> controller out there
> > is using dma for a 3byte transfer. I guess the overhead of setting it up
> is probably not
> > worth it...
>
> There are (I believe) a few i2c and spi controllers out there that don't
> do anything other
> than DMA. Wolfram mentioned one of those in his talk on adding
> DMA support to i2c.
Hmm, I see...
> Also, the reference in the file below to the wonderful case of USB to
> i2c bridges that always
> require DMA safe buffers.
Indeed it does.
- Nuno Sá
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-07-06 10:22 ` Sa, Nuno
@ 2021-07-07 8:26 ` Sa, Nuno
2021-07-07 8:56 ` Jonathan Cameron
0 siblings, 1 reply; 12+ messages in thread
From: Sa, Nuno @ 2021-07-07 8:26 UTC (permalink / raw)
To: Sa, Nuno, Jonathan Cameron
Cc: Jonathan Cameron, Miclaus, Antoniu, linux-iio, linux-kernel,
devicetree, robh+dt
> From: Sa, Nuno <Nuno.Sa@analog.com>
> Sent: Tuesday, July 6, 2021 12:23 PM
> To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org;
> robh+dt@kernel.org
> Subject: RE: [PATCH v4 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
>
>
>
> > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Sent: Tuesday, July 6, 2021 11:04 AM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> > <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org; linux-
> > kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > robh+dt@kernel.org
> > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for
> > ADRF6780
> >
> > On Mon, 5 Jul 2021 10:18:51 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Saturday, July 3, 2021 6:57 PM
> > > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > > Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > devicetree@vger.kernel.org; robh+dt@kernel.org
> > > > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support
> > for
> > > > ADRF6780
> > > >
> > > > On Fri, 2 Jul 2021 14:12:38 +0300
> > > > Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> > > >
> > > > > The ADRF6780 is a silicon germanium (SiGe) design, wideband,
> > > > > microwave upconverter optimized for point to point microwave
> > > > > radio designs operating in the 5.9 GHz to 23.6 GHz frequency
> > > > > range.
> > > > >
> > > > > Datasheet:
> > > > > https://www.analog.com/media/en/technical-
> > documentation/data-
> > > > sheets/ADRF6780.pdf
> > > > >
> > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > >
> > > > Hi Antoniu,
> > > >
> > > > Frequency drivers are fairly unusual so if you could add a listing of
> > > > the attributes in sysfs that would be great (it's nice practice
> anyway
> > but
> > > > I don't insist on it!)
> > > >
> > > > Various fairly minor comments inline.
> > > >
> > > > Thanks,
> > > >
> > > > Jonathan
> > > >
> > > >
> > > > > ---
> > > > > changes in v4:
> > > > > - change license to: GPL-2.0-only
> > > > > drivers/iio/frequency/Kconfig | 13 +
> > > > > drivers/iio/frequency/Makefile | 1 +
> > > > > drivers/iio/frequency/adrf6780.c | 498
> > > > +++++++++++++++++++++++++++++++
> > > > > 3 files changed, 512 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
> > > >
> > > > Why? Pretty much everything seems to have defaults if not
> > provided
> > > > via OF.
> > > > I've asked for the generic firmware functions anyway, so you can
> > drop
> > > > this
> > > > for that reason if nothing else!
> > > >
> > > > > + 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..472a66f90c7f
> > > > > --- /dev/null
> > > > > +++ b/drivers/iio/frequency/adrf6780.c
> > > > > @@ -0,0 +1,498 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > +/*
> > > > > + * ADRF6780 driver
> > > > > + *
> > > > > + * Copyright 2021 Analog Devices Inc.
> > > > > + */
> > > > > +
> > > > > +#include <linux/bitfield.h>
> > > > > +#include <linux/bits.h>
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/clkdev.h>
> > > > > +#include <linux/clk-provider.h>
> > > > > +#include <linux/delay.h>
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/iio/iio.h>
> > > > > +#include <linux/module.h>
> > > >
> > > > #include <linux/mod_devicetable.h>
> > > >
> > > > > +#include <linux/spi/spi.h>
> > > > > +
> > > > > +/* 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)
> > > >
> > > > Not used. In general, just use FIELD_PREP / FIELD_GET inline
> > rather
> > > > than having extra
> > > > macros like these. That approach is simpler for reviewers to
> follow.
> > > >
> > > > > +
> > > > > +struct adrf6780_dev {
> > > > > + struct spi_device *spi;
> > > > > + struct clk *clkin;
> > > > > + /* Protect against concurrent accesses to the device */
> > > > > + struct mutex lock;
> > > > > + bool vga_buff_en;
> > > > > + bool lo_buff_en;
> > > > > + bool if_mode_en;
> > > > > + bool iq_mode_en;
> > > > > + bool lo_x2_en;
> > > > > + bool lo_ppf_en;
> > > > > + bool lo_en;
> > > > > + bool uc_bias_en;
> > > > > + bool lo_sideband;
> > > > > + bool vdet_out_en;
> > > > > +};
> > > > > +
> > > > > +static int adrf6780_spi_read(struct adrf6780_dev *dev,
> > unsigned int
> > > > reg,
> > > > > + unsigned int *val)
> > > > > +{
> > > > > + int ret;
> > > > > + unsigned int 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);
> > > >
> > > > data needs to be dma safe.
> > > >
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > +
> > > > > + temp = ((data[0] | 0x80 | (reg << 1)) << 16) |
> > > > > + (data[1] << 8) | data[2];
> > > >
> > > > Ouch. That's a bit nasty, but why are you writing the reg into
> > > > it? Looks like a get_unaligned_be24() >> 1 and a 16bit mask.
> > > > (use GENMASK(15, 0) for that to make it apparent what is
> > happening.
> > > >
> > > > > +
> > > > > + *val = (temp >> 1) & 0xFFFF;
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static int adrf6780_spi_write(struct adrf6780_dev *dev,
> > > > > + unsigned int reg,
> > > > > + unsigned int val)
> > > > > +{
> > > > > + u8 data[3];
> > > > > +
> > > > > + val = (val << 1);
> > > > > +
> > > > > + data[0] = (reg << 1) | (val >> 16);
> > > > > + data[1] = val >> 8;
> > > > > + data[2] = val;
> > > >
> > > > An opportunity for
> > > > put_unaligned_be24() with a value of (I think)
> > > >
> > > > (val << 1) | (reg << 17)
> > > >
> > > >
> > > > > +
> > > > > + return spi_write(dev->spi, &data[0], 3);
> > > >
> > > > Needs a dma safe buffer, which basically means it can't be on the
> > > > stack.
> > > > Lots of ways of handling that, but look for __cacheline_aligned in
> > IIO
> > > > drivers
> > > > to see the one we probably use mostly commonly in IIO drivers.
> > >
> > > Hi Jonathan,
> > >
> > > This is something I wanted to ask for some time so I will take the
> > opportunity here :).
> > > Is this something you prefer just not to risk at all and make it an
> hard
> > requirement
> > > (which is fair)? ...
> >
> > Yes, I think we need to keep this as a hard requirement.
> > There are drivers out there which we missed this on in the past, and
> > I'm not necessarily
> > going to take the time to go through them all as this can be hard to
> > spot, but lets not
> > introduce any more potential problems.
> >
>
> I see. That makes sense and it's fair :). The only annoying (but not too
> annoying :)) is that
> making the data/buffer global forces you to use a lock in cases you
> potentially would
> not have too (just using local buffers). But that's life, better play safe :)
>
> > >
> > > I'm asking this because, tbh, I would be very surprised if any spi/i2c
> > controller out there
> > > is using dma for a 3byte transfer. I guess the overhead of setting it
> up
> > is probably not
> > > worth it...
> >
> > There are (I believe) a few i2c and spi controllers out there that don't
> > do anything other
> > than DMA. Wolfram mentioned one of those in his talk on adding
> > DMA support to i2c.
>
> Hmm, I see...
>
> > Also, the reference in the file below to the wonderful case of USB to
> > i2c bridges that always
> > require DMA safe buffers.
>
> Indeed it does.
>
Hi Jonathan,
Just for closure, I also realized that the pattern in IIO looks to be to use
DMA safe buffers only on the tx side. For instance in the IMU lib [1],
only the tx buffer is safe (well, I think there's problem with this as
I believe all spi transfers buffers should be properly aligned which won't
be the case in the IMU lib). Is there any reason for this? AFAICT, we should
also take care with rx buffers or am I missing something?
[1]: https://elixir.bootlin.com/linux/latest/source/include/linux/iio/imu/adis.h#L129
- Nuno Sá
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-07-07 8:26 ` Sa, Nuno
@ 2021-07-07 8:56 ` Jonathan Cameron
2021-07-07 11:11 ` Sa, Nuno
0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Cameron @ 2021-07-07 8:56 UTC (permalink / raw)
To: Sa, Nuno
Cc: Jonathan Cameron, Miclaus, Antoniu, linux-iio, linux-kernel,
devicetree, robh+dt
On Wed, 7 Jul 2021 08:26:59 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > From: Sa, Nuno <Nuno.Sa@analog.com>
> > Sent: Tuesday, July 6, 2021 12:23 PM
> > To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> > <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org; linux-
> > kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > robh+dt@kernel.org
> > Subject: RE: [PATCH v4 1/2] iio: frequency: adrf6780: add support for
> > ADRF6780
> >
> >
> >
> > > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > Sent: Tuesday, July 6, 2021 11:04 AM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> > > <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > > robh+dt@kernel.org
> > > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for
> > > ADRF6780
> > >
> > > On Mon, 5 Jul 2021 10:18:51 +0000
> > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > Sent: Saturday, July 3, 2021 6:57 PM
> > > > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > > > Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > > devicetree@vger.kernel.org; robh+dt@kernel.org
> > > > > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support
> > > for
> > > > > ADRF6780
> > > > >
> > > > > On Fri, 2 Jul 2021 14:12:38 +0300
> > > > > Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> > > > >
> > > > > > The ADRF6780 is a silicon germanium (SiGe) design, wideband,
> > > > > > microwave upconverter optimized for point to point microwave
> > > > > > radio designs operating in the 5.9 GHz to 23.6 GHz frequency
> > > > > > range.
> > > > > >
> > > > > > Datasheet:
> > > > > > https://www.analog.com/media/en/technical-
> > > documentation/data-
> > > > > sheets/ADRF6780.pdf
> > > > > >
> > > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > > >
> > > > > Hi Antoniu,
> > > > >
> > > > > Frequency drivers are fairly unusual so if you could add a listing of
> > > > > the attributes in sysfs that would be great (it's nice practice
> > anyway
> > > but
> > > > > I don't insist on it!)
> > > > >
> > > > > Various fairly minor comments inline.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Jonathan
> > > > >
> > > > >
> > > > > > ---
> > > > > > changes in v4:
> > > > > > - change license to: GPL-2.0-only
> > > > > > drivers/iio/frequency/Kconfig | 13 +
> > > > > > drivers/iio/frequency/Makefile | 1 +
> > > > > > drivers/iio/frequency/adrf6780.c | 498
> > > > > +++++++++++++++++++++++++++++++
> > > > > > 3 files changed, 512 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
> > > > >
> > > > > Why? Pretty much everything seems to have defaults if not
> > > provided
> > > > > via OF.
> > > > > I've asked for the generic firmware functions anyway, so you can
> > > drop
> > > > > this
> > > > > for that reason if nothing else!
> > > > >
> > > > > > + 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..472a66f90c7f
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/iio/frequency/adrf6780.c
> > > > > > @@ -0,0 +1,498 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > +/*
> > > > > > + * ADRF6780 driver
> > > > > > + *
> > > > > > + * Copyright 2021 Analog Devices Inc.
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/bitfield.h>
> > > > > > +#include <linux/bits.h>
> > > > > > +#include <linux/clk.h>
> > > > > > +#include <linux/clkdev.h>
> > > > > > +#include <linux/clk-provider.h>
> > > > > > +#include <linux/delay.h>
> > > > > > +#include <linux/device.h>
> > > > > > +#include <linux/iio/iio.h>
> > > > > > +#include <linux/module.h>
> > > > >
> > > > > #include <linux/mod_devicetable.h>
> > > > >
> > > > > > +#include <linux/spi/spi.h>
> > > > > > +
> > > > > > +/* 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)
> > > > >
> > > > > Not used. In general, just use FIELD_PREP / FIELD_GET inline
> > > rather
> > > > > than having extra
> > > > > macros like these. That approach is simpler for reviewers to
> > follow.
> > > > >
> > > > > > +
> > > > > > +struct adrf6780_dev {
> > > > > > + struct spi_device *spi;
> > > > > > + struct clk *clkin;
> > > > > > + /* Protect against concurrent accesses to the device */
> > > > > > + struct mutex lock;
> > > > > > + bool vga_buff_en;
> > > > > > + bool lo_buff_en;
> > > > > > + bool if_mode_en;
> > > > > > + bool iq_mode_en;
> > > > > > + bool lo_x2_en;
> > > > > > + bool lo_ppf_en;
> > > > > > + bool lo_en;
> > > > > > + bool uc_bias_en;
> > > > > > + bool lo_sideband;
> > > > > > + bool vdet_out_en;
> > > > > > +};
> > > > > > +
> > > > > > +static int adrf6780_spi_read(struct adrf6780_dev *dev,
> > > unsigned int
> > > > > reg,
> > > > > > + unsigned int *val)
> > > > > > +{
> > > > > > + int ret;
> > > > > > + unsigned int 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);
> > > > >
> > > > > data needs to be dma safe.
> > > > >
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > +
> > > > > > + temp = ((data[0] | 0x80 | (reg << 1)) << 16) |
> > > > > > + (data[1] << 8) | data[2];
> > > > >
> > > > > Ouch. That's a bit nasty, but why are you writing the reg into
> > > > > it? Looks like a get_unaligned_be24() >> 1 and a 16bit mask.
> > > > > (use GENMASK(15, 0) for that to make it apparent what is
> > > happening.
> > > > >
> > > > > > +
> > > > > > + *val = (temp >> 1) & 0xFFFF;
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static int adrf6780_spi_write(struct adrf6780_dev *dev,
> > > > > > + unsigned int reg,
> > > > > > + unsigned int val)
> > > > > > +{
> > > > > > + u8 data[3];
> > > > > > +
> > > > > > + val = (val << 1);
> > > > > > +
> > > > > > + data[0] = (reg << 1) | (val >> 16);
> > > > > > + data[1] = val >> 8;
> > > > > > + data[2] = val;
> > > > >
> > > > > An opportunity for
> > > > > put_unaligned_be24() with a value of (I think)
> > > > >
> > > > > (val << 1) | (reg << 17)
> > > > >
> > > > >
> > > > > > +
> > > > > > + return spi_write(dev->spi, &data[0], 3);
> > > > >
> > > > > Needs a dma safe buffer, which basically means it can't be on the
> > > > > stack.
> > > > > Lots of ways of handling that, but look for __cacheline_aligned in
> > > IIO
> > > > > drivers
> > > > > to see the one we probably use mostly commonly in IIO drivers.
> > > >
> > > > Hi Jonathan,
> > > >
> > > > This is something I wanted to ask for some time so I will take the
> > > opportunity here :).
> > > > Is this something you prefer just not to risk at all and make it an
> > hard
> > > requirement
> > > > (which is fair)? ...
> > >
> > > Yes, I think we need to keep this as a hard requirement.
> > > There are drivers out there which we missed this on in the past, and
> > > I'm not necessarily
> > > going to take the time to go through them all as this can be hard to
> > > spot, but lets not
> > > introduce any more potential problems.
> > >
> >
> > I see. That makes sense and it's fair :). The only annoying (but not too
> > annoying :)) is that
> > making the data/buffer global forces you to use a lock in cases you
> > potentially would
> > not have too (just using local buffers). But that's life, better play safe :)
> >
> > > >
> > > > I'm asking this because, tbh, I would be very surprised if any spi/i2c
> > > controller out there
> > > > is using dma for a 3byte transfer. I guess the overhead of setting it
> > up
> > > is probably not
> > > > worth it...
> > >
> > > There are (I believe) a few i2c and spi controllers out there that don't
> > > do anything other
> > > than DMA. Wolfram mentioned one of those in his talk on adding
> > > DMA support to i2c.
> >
> > Hmm, I see...
> >
> > > Also, the reference in the file below to the wonderful case of USB to
> > > i2c bridges that always
> > > require DMA safe buffers.
> >
> > Indeed it does.
> >
>
> Hi Jonathan,
>
> Just for closure, I also realized that the pattern in IIO looks to be to use
> DMA safe buffers only on the tx side. For instance in the IMU lib [1],
> only the tx buffer is safe (well, I think there's problem with this as
> I believe all spi transfers buffers should be properly aligned which won't
> be the case in the IMU lib). Is there any reason for this? AFAICT, we should
> also take care with rx buffers or am I missing something?
Ah. So this is a fun corner :)
The reason cache line corruption can occur is as follows.
1. DMA starts, typically involving some tx and rx usage by the device.
This flushes the CPU caches for the relevant lines.
... whilst DMA is not completed ...
2. The host software pulls the line into it's cache and updates something (say a flag
elsewhere in that cacheline).
3. Cacheline is evicted from the CPU cache causing a write back.
4. Device then writes back the stuff it had cached locally which is allowed to include data
it wasn't accessing in the same cache line. Boom, it just overwrote the flag we updated
with an older value. Basically this is a performance optimization / simplification
the DMA engine is allowed to make. Note I believe they are 'technically' allowed to
write back to the RX buffers as well, though not sure what devices do this for i2c/spi.
So, why do we only need to force one of the buffers to the start of a cacheline?
What we are actually doing, is not keeping the buffer in it's own cacheline, but rather
making sure nothing else is in the same cacheline (so there is no race as above).
(it's easier to move the buffer, than to ensure everything else is moved out of the cache
line it happens to be in!)
There is a safe assumption here that the DMA device can't corrupt it's own data as that
would be crazy :)
Hence, pushing the first buffer to the start of a line, allows the second one to be
after it in the same line (it's not a problem if it takes multiple lines)
One more subtlety is why we can be sure nothing else ends up after the buffers.
That's by construction. The allocations IIO does for those iio_priv structures should
always get padded out to at least the end of the cacheline.
(IIRC, not looked at this code for many years!)
>
> [1]: https://elixir.bootlin.com/linux/latest/source/include/linux/iio/imu/adis.h#L129
>
> - Nuno Sá
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-07-07 8:56 ` Jonathan Cameron
@ 2021-07-07 11:11 ` Sa, Nuno
2021-07-10 17:57 ` Jonathan Cameron
0 siblings, 1 reply; 12+ messages in thread
From: Sa, Nuno @ 2021-07-07 11:11 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, Miclaus, Antoniu, linux-iio, linux-kernel,
devicetree, robh+dt
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Wednesday, July 7, 2021 10:56 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org;
> robh+dt@kernel.org
> Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
>
> On Wed, 7 Jul 2021 08:26:59 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
>
> > > From: Sa, Nuno <Nuno.Sa@analog.com>
> > > Sent: Tuesday, July 6, 2021 12:23 PM
> > > To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> > > <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > > robh+dt@kernel.org
> > > Subject: RE: [PATCH v4 1/2] iio: frequency: adrf6780: add support
> for
> > > ADRF6780
> > >
> > >
> > >
> > > > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > > Sent: Tuesday, July 6, 2021 11:04 AM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> > > > <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org;
> linux-
> > > > kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > > > robh+dt@kernel.org
> > > > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support
> for
> > > > ADRF6780
> > > >
> > > > On Mon, 5 Jul 2021 10:18:51 +0000
> > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > > >
> > > > > > -----Original Message-----
> > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > Sent: Saturday, July 3, 2021 6:57 PM
> > > > > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > > > > Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > > > devicetree@vger.kernel.org; robh+dt@kernel.org
> > > > > > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add
> support
> > > > for
> > > > > > ADRF6780
> > > > > >
> > > > > > On Fri, 2 Jul 2021 14:12:38 +0300
> > > > > > Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> > > > > >
> > > > > > > The ADRF6780 is a silicon germanium (SiGe) design,
> wideband,
> > > > > > > microwave upconverter optimized for point to point
> microwave
> > > > > > > radio designs operating in the 5.9 GHz to 23.6 GHz
> frequency
> > > > > > > range.
> > > > > > >
> > > > > > > Datasheet:
> > > > > > > https://www.analog.com/media/en/technical-
> > > > documentation/data-
> > > > > > sheets/ADRF6780.pdf
> > > > > > >
> > > > > > > Signed-off-by: Antoniu Miclaus
> <antoniu.miclaus@analog.com>
> > > > > >
> > > > > > Hi Antoniu,
> > > > > >
> > > > > > Frequency drivers are fairly unusual so if you could add a
> listing of
> > > > > > the attributes in sysfs that would be great (it's nice practice
> > > anyway
> > > > but
> > > > > > I don't insist on it!)
> > > > > >
> > > > > > Various fairly minor comments inline.
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Jonathan
> > > > > >
> > > > > >
> > > > > > > ---
> > > > > > > changes in v4:
> > > > > > > - change license to: GPL-2.0-only
> > > > > > > drivers/iio/frequency/Kconfig | 13 +
> > > > > > > drivers/iio/frequency/Makefile | 1 +
> > > > > > > drivers/iio/frequency/adrf6780.c | 498
> > > > > > +++++++++++++++++++++++++++++++
> > > > > > > 3 files changed, 512 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
> > > > > >
> > > > > > Why? Pretty much everything seems to have defaults if not
> > > > provided
> > > > > > via OF.
> > > > > > I've asked for the generic firmware functions anyway, so you
> can
> > > > drop
> > > > > > this
> > > > > > for that reason if nothing else!
> > > > > >
> > > > > > > + 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..472a66f90c7f
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/iio/frequency/adrf6780.c
> > > > > > > @@ -0,0 +1,498 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > > +/*
> > > > > > > + * ADRF6780 driver
> > > > > > > + *
> > > > > > > + * Copyright 2021 Analog Devices Inc.
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/bitfield.h>
> > > > > > > +#include <linux/bits.h>
> > > > > > > +#include <linux/clk.h>
> > > > > > > +#include <linux/clkdev.h>
> > > > > > > +#include <linux/clk-provider.h>
> > > > > > > +#include <linux/delay.h>
> > > > > > > +#include <linux/device.h>
> > > > > > > +#include <linux/iio/iio.h>
> > > > > > > +#include <linux/module.h>
> > > > > >
> > > > > > #include <linux/mod_devicetable.h>
> > > > > >
> > > > > > > +#include <linux/spi/spi.h>
> > > > > > > +
> > > > > > > +/* 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)
> > > > > >
> > > > > > Not used. In general, just use FIELD_PREP / FIELD_GET inline
> > > > rather
> > > > > > than having extra
> > > > > > macros like these. That approach is simpler for reviewers to
> > > follow.
> > > > > >
> > > > > > > +
> > > > > > > +struct adrf6780_dev {
> > > > > > > + struct spi_device *spi;
> > > > > > > + struct clk *clkin;
> > > > > > > + /* Protect against concurrent accesses to the device */
> > > > > > > + struct mutex lock;
> > > > > > > + bool vga_buff_en;
> > > > > > > + bool lo_buff_en;
> > > > > > > + bool if_mode_en;
> > > > > > > + bool iq_mode_en;
> > > > > > > + bool lo_x2_en;
> > > > > > > + bool lo_ppf_en;
> > > > > > > + bool lo_en;
> > > > > > > + bool uc_bias_en;
> > > > > > > + bool lo_sideband;
> > > > > > > + bool vdet_out_en;
> > > > > > > +};
> > > > > > > +
> > > > > > > +static int adrf6780_spi_read(struct adrf6780_dev *dev,
> > > > unsigned int
> > > > > > reg,
> > > > > > > + unsigned int *val)
> > > > > > > +{
> > > > > > > + int ret;
> > > > > > > + unsigned int 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);
> > > > > >
> > > > > > data needs to be dma safe.
> > > > > >
> > > > > > > + if (ret < 0)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + temp = ((data[0] | 0x80 | (reg << 1)) << 16) |
> > > > > > > + (data[1] << 8) | data[2];
> > > > > >
> > > > > > Ouch. That's a bit nasty, but why are you writing the reg into
> > > > > > it? Looks like a get_unaligned_be24() >> 1 and a 16bit mask.
> > > > > > (use GENMASK(15, 0) for that to make it apparent what is
> > > > happening.
> > > > > >
> > > > > > > +
> > > > > > > + *val = (temp >> 1) & 0xFFFF;
> > > > > > > +
> > > > > > > + return ret;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int adrf6780_spi_write(struct adrf6780_dev *dev,
> > > > > > > + unsigned int reg,
> > > > > > > + unsigned int val)
> > > > > > > +{
> > > > > > > + u8 data[3];
> > > > > > > +
> > > > > > > + val = (val << 1);
> > > > > > > +
> > > > > > > + data[0] = (reg << 1) | (val >> 16);
> > > > > > > + data[1] = val >> 8;
> > > > > > > + data[2] = val;
> > > > > >
> > > > > > An opportunity for
> > > > > > put_unaligned_be24() with a value of (I think)
> > > > > >
> > > > > > (val << 1) | (reg << 17)
> > > > > >
> > > > > >
> > > > > > > +
> > > > > > > + return spi_write(dev->spi, &data[0], 3);
> > > > > >
> > > > > > Needs a dma safe buffer, which basically means it can't be on
> the
> > > > > > stack.
> > > > > > Lots of ways of handling that, but look for
> __cacheline_aligned in
> > > > IIO
> > > > > > drivers
> > > > > > to see the one we probably use mostly commonly in IIO
> drivers.
> > > > >
> > > > > Hi Jonathan,
> > > > >
> > > > > This is something I wanted to ask for some time so I will take
> the
> > > > opportunity here :).
> > > > > Is this something you prefer just not to risk at all and make it an
> > > hard
> > > > requirement
> > > > > (which is fair)? ...
> > > >
> > > > Yes, I think we need to keep this as a hard requirement.
> > > > There are drivers out there which we missed this on in the past,
> and
> > > > I'm not necessarily
> > > > going to take the time to go through them all as this can be hard
> to
> > > > spot, but lets not
> > > > introduce any more potential problems.
> > > >
> > >
> > > I see. That makes sense and it's fair :). The only annoying (but not
> too
> > > annoying :)) is that
> > > making the data/buffer global forces you to use a lock in cases you
> > > potentially would
> > > not have too (just using local buffers). But that's life, better play
> safe :)
> > >
> > > > >
> > > > > I'm asking this because, tbh, I would be very surprised if any
> spi/i2c
> > > > controller out there
> > > > > is using dma for a 3byte transfer. I guess the overhead of
> setting it
> > > up
> > > > is probably not
> > > > > worth it...
> > > >
> > > > There are (I believe) a few i2c and spi controllers out there that
> don't
> > > > do anything other
> > > > than DMA. Wolfram mentioned one of those in his talk on adding
> > > > DMA support to i2c.
> > >
> > > Hmm, I see...
> > >
> > > > Also, the reference in the file below to the wonderful case of
> USB to
> > > > i2c bridges that always
> > > > require DMA safe buffers.
> > >
> > > Indeed it does.
> > >
> >
> > Hi Jonathan,
> >
> > Just for closure, I also realized that the pattern in IIO looks to be to
> use
> > DMA safe buffers only on the tx side. For instance in the IMU lib [1],
> > only the tx buffer is safe (well, I think there's problem with this as
> > I believe all spi transfers buffers should be properly aligned which
> won't
> > be the case in the IMU lib). Is there any reason for this? AFAICT, we
> should
> > also take care with rx buffers or am I missing something?
> Ah. So this is a fun corner :)
>
> The reason cache line corruption can occur is as follows.
> 1. DMA starts, typically involving some tx and rx usage by the device.
> This flushes the CPU caches for the relevant lines.
> ... whilst DMA is not completed ...
> 2. The host software pulls the line into it's cache and updates
> something (say a flag
> elsewhere in that cacheline).
> 3. Cacheline is evicted from the CPU cache causing a write back.
> 4. Device then writes back the stuff it had cached locally which is
> allowed to include data
> it wasn't accessing in the same cache line. Boom, it just overwrote
> the flag we updated
> with an older value. Basically this is a performance optimization /
> simplification
> the DMA engine is allowed to make. Note I believe they are
> 'technically' allowed to
> write back to the RX buffers as well, though not sure what devices do
> this for i2c/spi.
Yes, got it...
> So, why do we only need to force one of the buffers to the start of a
> cacheline?
>
> What we are actually doing, is not keeping the buffer in it's own
> cacheline, but rather
> making sure nothing else is in the same cacheline (so there is no race
> as above).
> (it's easier to move the buffer, than to ensure everything else is
> moved out of the cache
> line it happens to be in!)
> There is a safe assumption here that the DMA device can't corrupt it's
> own data as that
> would be crazy :)
My confusion here was looking at this [1] and somehow thinking that
DMA mappings require that both 'tx_buf' and 'rx_buf' to be on their own
cache lines... But I guess that since both buffers are guaranteed to be alone
(in our case) in their cache line (or span across multiple lines) we are fine... Though
I didn't really looked into the details on the DMA subsystem... I'm just aware that
for instance 'dma_map_single()' states that the addr to map must begin on a cache
line boundary.
> Hence, pushing the first buffer to the start of a line, allows the second
> one to be
> after it in the same line (it's not a problem if it takes multiple lines)
>
> One more subtlety is why we can be sure nothing else ends up after
> the buffers.
> That's by construction. The allocations IIO does for those iio_priv
> structures should
> always get padded out to at least the end of the cacheline.
> (IIRC, not looked at this code for many years!)
>
Looking at [2], I think that holds correct as long as private structures have
their buffer properly aligned in the end of the struct. Hence, the natural padding
of the structure ensures us that nothing else gets into the same cache
line as our buffer.
Which means that, if I'm not missing nothing obvious, all the adis IMUS are not
complying with this [3]... I think the 'struct adis adis' field has to be in the end of the
adis16480 structure so that our tx and rx buffers in the adis struct are on their own
line...
Thanks for your inputs!
- Nuno Sá
[1]: https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi.c#L976
[2]: https://elixir.bootlin.com/linux/v5.13/source/drivers/iio/industrialio-core.c#L1605
[3]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/imu/adis16480.c#L159
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-07-07 11:11 ` Sa, Nuno
@ 2021-07-10 17:57 ` Jonathan Cameron
2021-07-12 7:17 ` Sa, Nuno
0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Cameron @ 2021-07-10 17:57 UTC (permalink / raw)
To: Sa, Nuno
Cc: Jonathan Cameron, Miclaus, Antoniu, linux-iio, linux-kernel,
devicetree, robh+dt
On Wed, 7 Jul 2021 11:11:20 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Sent: Wednesday, July 7, 2021 10:56 AM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> > <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org; linux-
> > kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > robh+dt@kernel.org
> > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for
> > ADRF6780
> >
> > On Wed, 7 Jul 2021 08:26:59 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >
> > > > From: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Sent: Tuesday, July 6, 2021 12:23 PM
> > > > To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > > Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> > > > <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > > > robh+dt@kernel.org
> > > > Subject: RE: [PATCH v4 1/2] iio: frequency: adrf6780: add support
> > for
> > > > ADRF6780
> > > >
> > > >
> > > >
> > > > > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > > > Sent: Tuesday, July 6, 2021 11:04 AM
> > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> > > > > <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org;
> > linux-
> > > > > kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > > > > robh+dt@kernel.org
> > > > > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support
> > for
> > > > > ADRF6780
> > > > >
> > > > > On Mon, 5 Jul 2021 10:18:51 +0000
> > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > > Sent: Saturday, July 3, 2021 6:57 PM
> > > > > > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > > > > > Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > > > > devicetree@vger.kernel.org; robh+dt@kernel.org
> > > > > > > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add
> > support
> > > > > for
> > > > > > > ADRF6780
> > > > > > >
> > > > > > > On Fri, 2 Jul 2021 14:12:38 +0300
> > > > > > > Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> > > > > > >
> > > > > > > > The ADRF6780 is a silicon germanium (SiGe) design,
> > wideband,
> > > > > > > > microwave upconverter optimized for point to point
> > microwave
> > > > > > > > radio designs operating in the 5.9 GHz to 23.6 GHz
> > frequency
> > > > > > > > range.
> > > > > > > >
> > > > > > > > Datasheet:
> > > > > > > > https://www.analog.com/media/en/technical-
> > > > > documentation/data-
> > > > > > > sheets/ADRF6780.pdf
> > > > > > > >
> > > > > > > > Signed-off-by: Antoniu Miclaus
> > <antoniu.miclaus@analog.com>
> > > > > > >
> > > > > > > Hi Antoniu,
> > > > > > >
> > > > > > > Frequency drivers are fairly unusual so if you could add a
> > listing of
> > > > > > > the attributes in sysfs that would be great (it's nice practice
> > > > anyway
> > > > > but
> > > > > > > I don't insist on it!)
> > > > > > >
> > > > > > > Various fairly minor comments inline.
> > > > > > >
> > > > > > > Thanks,
> > > > > > >
> > > > > > > Jonathan
> > > > > > >
> > > > > > >
> > > > > > > > ---
> > > > > > > > changes in v4:
> > > > > > > > - change license to: GPL-2.0-only
> > > > > > > > drivers/iio/frequency/Kconfig | 13 +
> > > > > > > > drivers/iio/frequency/Makefile | 1 +
> > > > > > > > drivers/iio/frequency/adrf6780.c | 498
> > > > > > > +++++++++++++++++++++++++++++++
> > > > > > > > 3 files changed, 512 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
> > > > > > >
> > > > > > > Why? Pretty much everything seems to have defaults if not
> > > > > provided
> > > > > > > via OF.
> > > > > > > I've asked for the generic firmware functions anyway, so you
> > can
> > > > > drop
> > > > > > > this
> > > > > > > for that reason if nothing else!
> > > > > > >
> > > > > > > > + 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..472a66f90c7f
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/iio/frequency/adrf6780.c
> > > > > > > > @@ -0,0 +1,498 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > > > +/*
> > > > > > > > + * ADRF6780 driver
> > > > > > > > + *
> > > > > > > > + * Copyright 2021 Analog Devices Inc.
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include <linux/bitfield.h>
> > > > > > > > +#include <linux/bits.h>
> > > > > > > > +#include <linux/clk.h>
> > > > > > > > +#include <linux/clkdev.h>
> > > > > > > > +#include <linux/clk-provider.h>
> > > > > > > > +#include <linux/delay.h>
> > > > > > > > +#include <linux/device.h>
> > > > > > > > +#include <linux/iio/iio.h>
> > > > > > > > +#include <linux/module.h>
> > > > > > >
> > > > > > > #include <linux/mod_devicetable.h>
> > > > > > >
> > > > > > > > +#include <linux/spi/spi.h>
> > > > > > > > +
> > > > > > > > +/* 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)
> > > > > > >
> > > > > > > Not used. In general, just use FIELD_PREP / FIELD_GET inline
> > > > > rather
> > > > > > > than having extra
> > > > > > > macros like these. That approach is simpler for reviewers to
> > > > follow.
> > > > > > >
> > > > > > > > +
> > > > > > > > +struct adrf6780_dev {
> > > > > > > > + struct spi_device *spi;
> > > > > > > > + struct clk *clkin;
> > > > > > > > + /* Protect against concurrent accesses to the device */
> > > > > > > > + struct mutex lock;
> > > > > > > > + bool vga_buff_en;
> > > > > > > > + bool lo_buff_en;
> > > > > > > > + bool if_mode_en;
> > > > > > > > + bool iq_mode_en;
> > > > > > > > + bool lo_x2_en;
> > > > > > > > + bool lo_ppf_en;
> > > > > > > > + bool lo_en;
> > > > > > > > + bool uc_bias_en;
> > > > > > > > + bool lo_sideband;
> > > > > > > > + bool vdet_out_en;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static int adrf6780_spi_read(struct adrf6780_dev *dev,
> > > > > unsigned int
> > > > > > > reg,
> > > > > > > > + unsigned int *val)
> > > > > > > > +{
> > > > > > > > + int ret;
> > > > > > > > + unsigned int 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);
> > > > > > >
> > > > > > > data needs to be dma safe.
> > > > > > >
> > > > > > > > + if (ret < 0)
> > > > > > > > + return ret;
> > > > > > > > +
> > > > > > > > + temp = ((data[0] | 0x80 | (reg << 1)) << 16) |
> > > > > > > > + (data[1] << 8) | data[2];
> > > > > > >
> > > > > > > Ouch. That's a bit nasty, but why are you writing the reg into
> > > > > > > it? Looks like a get_unaligned_be24() >> 1 and a 16bit mask.
> > > > > > > (use GENMASK(15, 0) for that to make it apparent what is
> > > > > happening.
> > > > > > >
> > > > > > > > +
> > > > > > > > + *val = (temp >> 1) & 0xFFFF;
> > > > > > > > +
> > > > > > > > + return ret;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int adrf6780_spi_write(struct adrf6780_dev *dev,
> > > > > > > > + unsigned int reg,
> > > > > > > > + unsigned int val)
> > > > > > > > +{
> > > > > > > > + u8 data[3];
> > > > > > > > +
> > > > > > > > + val = (val << 1);
> > > > > > > > +
> > > > > > > > + data[0] = (reg << 1) | (val >> 16);
> > > > > > > > + data[1] = val >> 8;
> > > > > > > > + data[2] = val;
> > > > > > >
> > > > > > > An opportunity for
> > > > > > > put_unaligned_be24() with a value of (I think)
> > > > > > >
> > > > > > > (val << 1) | (reg << 17)
> > > > > > >
> > > > > > >
> > > > > > > > +
> > > > > > > > + return spi_write(dev->spi, &data[0], 3);
> > > > > > >
> > > > > > > Needs a dma safe buffer, which basically means it can't be on
> > the
> > > > > > > stack.
> > > > > > > Lots of ways of handling that, but look for
> > __cacheline_aligned in
> > > > > IIO
> > > > > > > drivers
> > > > > > > to see the one we probably use mostly commonly in IIO
> > drivers.
> > > > > >
> > > > > > Hi Jonathan,
> > > > > >
> > > > > > This is something I wanted to ask for some time so I will take
> > the
> > > > > opportunity here :).
> > > > > > Is this something you prefer just not to risk at all and make it an
> > > > hard
> > > > > requirement
> > > > > > (which is fair)? ...
> > > > >
> > > > > Yes, I think we need to keep this as a hard requirement.
> > > > > There are drivers out there which we missed this on in the past,
> > and
> > > > > I'm not necessarily
> > > > > going to take the time to go through them all as this can be hard
> > to
> > > > > spot, but lets not
> > > > > introduce any more potential problems.
> > > > >
> > > >
> > > > I see. That makes sense and it's fair :). The only annoying (but not
> > too
> > > > annoying :)) is that
> > > > making the data/buffer global forces you to use a lock in cases you
> > > > potentially would
> > > > not have too (just using local buffers). But that's life, better play
> > safe :)
> > > >
> > > > > >
> > > > > > I'm asking this because, tbh, I would be very surprised if any
> > spi/i2c
> > > > > controller out there
> > > > > > is using dma for a 3byte transfer. I guess the overhead of
> > setting it
> > > > up
> > > > > is probably not
> > > > > > worth it...
> > > > >
> > > > > There are (I believe) a few i2c and spi controllers out there that
> > don't
> > > > > do anything other
> > > > > than DMA. Wolfram mentioned one of those in his talk on adding
> > > > > DMA support to i2c.
> > > >
> > > > Hmm, I see...
> > > >
> > > > > Also, the reference in the file below to the wonderful case of
> > USB to
> > > > > i2c bridges that always
> > > > > require DMA safe buffers.
> > > >
> > > > Indeed it does.
> > > >
> > >
> > > Hi Jonathan,
> > >
> > > Just for closure, I also realized that the pattern in IIO looks to be to
> > use
> > > DMA safe buffers only on the tx side. For instance in the IMU lib [1],
> > > only the tx buffer is safe (well, I think there's problem with this as
> > > I believe all spi transfers buffers should be properly aligned which
> > won't
> > > be the case in the IMU lib). Is there any reason for this? AFAICT, we
> > should
> > > also take care with rx buffers or am I missing something?
> > Ah. So this is a fun corner :)
> >
> > The reason cache line corruption can occur is as follows.
> > 1. DMA starts, typically involving some tx and rx usage by the device.
> > This flushes the CPU caches for the relevant lines.
> > ... whilst DMA is not completed ...
> > 2. The host software pulls the line into it's cache and updates
> > something (say a flag
> > elsewhere in that cacheline).
> > 3. Cacheline is evicted from the CPU cache causing a write back.
> > 4. Device then writes back the stuff it had cached locally which is
> > allowed to include data
> > it wasn't accessing in the same cache line. Boom, it just overwrote
> > the flag we updated
> > with an older value. Basically this is a performance optimization /
> > simplification
> > the DMA engine is allowed to make. Note I believe they are
> > 'technically' allowed to
> > write back to the RX buffers as well, though not sure what devices do
> > this for i2c/spi.
>
> Yes, got it...
>
> > So, why do we only need to force one of the buffers to the start of a
> > cacheline?
> >
> > What we are actually doing, is not keeping the buffer in it's own
> > cacheline, but rather
> > making sure nothing else is in the same cacheline (so there is no race
> > as above).
> > (it's easier to move the buffer, than to ensure everything else is
> > moved out of the cache
> > line it happens to be in!)
> > There is a safe assumption here that the DMA device can't corrupt it's
> > own data as that
> > would be crazy :)
>
> My confusion here was looking at this [1] and somehow thinking that
> DMA mappings require that both 'tx_buf' and 'rx_buf' to be on their own
> cache lines... But I guess that since both buffers are guaranteed to be alone
> (in our case) in their cache line (or span across multiple lines) we are fine... Though
> I didn't really looked into the details on the DMA subsystem... I'm just aware that
> for instance 'dma_map_single()' states that the addr to map must begin on a cache
> line boundary.
>
> > Hence, pushing the first buffer to the start of a line, allows the second
> > one to be
> > after it in the same line (it's not a problem if it takes multiple lines)
> >
> > One more subtlety is why we can be sure nothing else ends up after
> > the buffers.
> > That's by construction. The allocations IIO does for those iio_priv
> > structures should
> > always get padded out to at least the end of the cacheline.
> > (IIRC, not looked at this code for many years!)
> >
>
> Looking at [2], I think that holds correct as long as private structures have
> their buffer properly aligned in the end of the struct. Hence, the natural padding
> of the structure ensures us that nothing else gets into the same cache
> line as our buffer.
>
> Which means that, if I'm not missing nothing obvious, all the adis IMUS are not
> complying with this [3]... I think the 'struct adis adis' field has to be in the end of the
> adis16480 structure so that our tx and rx buffers in the adis struct are on their own
> line...
Hmm. Not quite because another subtlety of c slips in. Hohum - finding the reference
for this always takes me a while...
I can't find it in the c spec today for some reason (we tracked it down when fixing
the timestamp alignment issues last year)... But the gcc docs refer to the correct
requirements https://gcc.gnu.org/onlinedocs/gcc-3.3/gcc/Type-Attributes.html
Because we have the buffer embedded in the adis structure, the question becomes how
is that aligned and how is the data after it aligned.
Now, c structures are aligned to at least the alignment requirement of the element which has
the largest alignment requirements. Now the next bit I'm dodgier on proving, but
my understanding is the following elements of the outer structure are guaranteed
to not be pushed into the padding at the end of the adis structure. It's documented
in that gcc thing that an array would be appropriately spaced, so I'd assume that
also applies to the struct in struct case (or things would be really odd!)
If anyone else digs up the c spec reference that would be great. I remember it took
quite some finding last time...
Jonathan
>
> Thanks for your inputs!
> - Nuno Sá
>
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi.c#L976
> [2]: https://elixir.bootlin.com/linux/v5.13/source/drivers/iio/industrialio-core.c#L1605
> [3]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/imu/adis16480.c#L159
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780
2021-07-10 17:57 ` Jonathan Cameron
@ 2021-07-12 7:17 ` Sa, Nuno
0 siblings, 0 replies; 12+ messages in thread
From: Sa, Nuno @ 2021-07-12 7:17 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, Miclaus, Antoniu, linux-iio, linux-kernel,
devicetree, robh+dt
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, July 10, 2021 7:57 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>; Miclaus,
> Antoniu <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> robh+dt@kernel.org
> Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
>
> On Wed, 7 Jul 2021 11:11:20 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
>
> > > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > Sent: Wednesday, July 7, 2021 10:56 AM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> > > <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > > robh+dt@kernel.org
> > > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add support
> for
> > > ADRF6780
> > >
> > > On Wed, 7 Jul 2021 08:26:59 +0000
> > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > >
> > > > > From: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Sent: Tuesday, July 6, 2021 12:23 PM
> > > > > To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > > > Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> > > > > <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org;
> linux-
> > > > > kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > > > > robh+dt@kernel.org
> > > > > Subject: RE: [PATCH v4 1/2] iio: frequency: adrf6780: add
> support
> > > for
> > > > > ADRF6780
> > > > >
> > > > >
> > > > >
> > > > > > From: Jonathan Cameron
> <Jonathan.Cameron@Huawei.com>
> > > > > > Sent: Tuesday, July 6, 2021 11:04 AM
> > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > Cc: Jonathan Cameron <jic23@kernel.org>; Miclaus, Antoniu
> > > > > > <Antoniu.Miclaus@analog.com>; linux-iio@vger.kernel.org;
> > > linux-
> > > > > > kernel@vger.kernel.org; devicetree@vger.kernel.org;
> > > > > > robh+dt@kernel.org
> > > > > > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add
> support
> > > for
> > > > > > ADRF6780
> > > > > >
> > > > > > On Mon, 5 Jul 2021 10:18:51 +0000
> > > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > > > Sent: Saturday, July 3, 2021 6:57 PM
> > > > > > > > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > > > > > > > Cc: linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org;
> > > > > > > > devicetree@vger.kernel.org; robh+dt@kernel.org
> > > > > > > > Subject: Re: [PATCH v4 1/2] iio: frequency: adrf6780: add
> > > support
> > > > > > for
> > > > > > > > ADRF6780
> > > > > > > >
> > > > > > > > On Fri, 2 Jul 2021 14:12:38 +0300
> > > > > > > > Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> > > > > > > >
> > > > > > > > > The ADRF6780 is a silicon germanium (SiGe) design,
> > > wideband,
> > > > > > > > > microwave upconverter optimized for point to point
> > > microwave
> > > > > > > > > radio designs operating in the 5.9 GHz to 23.6 GHz
> > > frequency
> > > > > > > > > range.
> > > > > > > > >
> > > > > > > > > Datasheet:
> > > > > > > > > https://www.analog.com/media/en/technical-
> > > > > > documentation/data-
> > > > > > > > sheets/ADRF6780.pdf
> > > > > > > > >
> > > > > > > > > Signed-off-by: Antoniu Miclaus
> > > <antoniu.miclaus@analog.com>
> > > > > > > >
> > > > > > > > Hi Antoniu,
> > > > > > > >
> > > > > > > > Frequency drivers are fairly unusual so if you could add a
> > > listing of
> > > > > > > > the attributes in sysfs that would be great (it's nice
> practice
> > > > > anyway
> > > > > > but
> > > > > > > > I don't insist on it!)
> > > > > > > >
> > > > > > > > Various fairly minor comments inline.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > >
> > > > > > > > Jonathan
> > > > > > > >
> > > > > > > >
> > > > > > > > > ---
> > > > > > > > > changes in v4:
> > > > > > > > > - change license to: GPL-2.0-only
> > > > > > > > > drivers/iio/frequency/Kconfig | 13 +
> > > > > > > > > drivers/iio/frequency/Makefile | 1 +
> > > > > > > > > drivers/iio/frequency/adrf6780.c | 498
> > > > > > > > +++++++++++++++++++++++++++++++
> > > > > > > > > 3 files changed, 512 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
> > > > > > > >
> > > > > > > > Why? Pretty much everything seems to have defaults if
> not
> > > > > > provided
> > > > > > > > via OF.
> > > > > > > > I've asked for the generic firmware functions anyway, so
> you
> > > can
> > > > > > drop
> > > > > > > > this
> > > > > > > > for that reason if nothing else!
> > > > > > > >
> > > > > > > > > + 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..472a66f90c7f
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/drivers/iio/frequency/adrf6780.c
> > > > > > > > > @@ -0,0 +1,498 @@
> > > > > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > > > > +/*
> > > > > > > > > + * ADRF6780 driver
> > > > > > > > > + *
> > > > > > > > > + * Copyright 2021 Analog Devices Inc.
> > > > > > > > > + */
> > > > > > > > > +
> > > > > > > > > +#include <linux/bitfield.h>
> > > > > > > > > +#include <linux/bits.h>
> > > > > > > > > +#include <linux/clk.h>
> > > > > > > > > +#include <linux/clkdev.h>
> > > > > > > > > +#include <linux/clk-provider.h>
> > > > > > > > > +#include <linux/delay.h>
> > > > > > > > > +#include <linux/device.h>
> > > > > > > > > +#include <linux/iio/iio.h>
> > > > > > > > > +#include <linux/module.h>
> > > > > > > >
> > > > > > > > #include <linux/mod_devicetable.h>
> > > > > > > >
> > > > > > > > > +#include <linux/spi/spi.h>
> > > > > > > > > +
> > > > > > > > > +/* 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)
> > > > > > > >
> > > > > > > > Not used. In general, just use FIELD_PREP / FIELD_GET
> inline
> > > > > > rather
> > > > > > > > than having extra
> > > > > > > > macros like these. That approach is simpler for reviewers
> to
> > > > > follow.
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > +struct adrf6780_dev {
> > > > > > > > > + struct spi_device *spi;
> > > > > > > > > + struct clk *clkin;
> > > > > > > > > + /* Protect against concurrent accesses to the
> device */
> > > > > > > > > + struct mutex lock;
> > > > > > > > > + bool vga_buff_en;
> > > > > > > > > + bool lo_buff_en;
> > > > > > > > > + bool if_mode_en;
> > > > > > > > > + bool iq_mode_en;
> > > > > > > > > + bool lo_x2_en;
> > > > > > > > > + bool lo_ppf_en;
> > > > > > > > > + bool lo_en;
> > > > > > > > > + bool uc_bias_en;
> > > > > > > > > + bool lo_sideband;
> > > > > > > > > + bool vdet_out_en;
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > +static int adrf6780_spi_read(struct adrf6780_dev *dev,
> > > > > > unsigned int
> > > > > > > > reg,
> > > > > > > > > + unsigned int *val)
> > > > > > > > > +{
> > > > > > > > > + int ret;
> > > > > > > > > + unsigned int 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);
> > > > > > > >
> > > > > > > > data needs to be dma safe.
> > > > > > > >
> > > > > > > > > + if (ret < 0)
> > > > > > > > > + return ret;
> > > > > > > > > +
> > > > > > > > > + temp = ((data[0] | 0x80 | (reg << 1)) << 16) |
> > > > > > > > > + (data[1] << 8) | data[2];
> > > > > > > >
> > > > > > > > Ouch. That's a bit nasty, but why are you writing the reg
> into
> > > > > > > > it? Looks like a get_unaligned_be24() >> 1 and a 16bit
> mask.
> > > > > > > > (use GENMASK(15, 0) for that to make it apparent what is
> > > > > > happening.
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > + *val = (temp >> 1) & 0xFFFF;
> > > > > > > > > +
> > > > > > > > > + return ret;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static int adrf6780_spi_write(struct adrf6780_dev
> *dev,
> > > > > > > > > + unsigned int reg,
> > > > > > > > > + unsigned int val)
> > > > > > > > > +{
> > > > > > > > > + u8 data[3];
> > > > > > > > > +
> > > > > > > > > + val = (val << 1);
> > > > > > > > > +
> > > > > > > > > + data[0] = (reg << 1) | (val >> 16);
> > > > > > > > > + data[1] = val >> 8;
> > > > > > > > > + data[2] = val;
> > > > > > > >
> > > > > > > > An opportunity for
> > > > > > > > put_unaligned_be24() with a value of (I think)
> > > > > > > >
> > > > > > > > (val << 1) | (reg << 17)
> > > > > > > >
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > + return spi_write(dev->spi, &data[0], 3);
> > > > > > > >
> > > > > > > > Needs a dma safe buffer, which basically means it can't be
> on
> > > the
> > > > > > > > stack.
> > > > > > > > Lots of ways of handling that, but look for
> > > __cacheline_aligned in
> > > > > > IIO
> > > > > > > > drivers
> > > > > > > > to see the one we probably use mostly commonly in IIO
> > > drivers.
> > > > > > >
> > > > > > > Hi Jonathan,
> > > > > > >
> > > > > > > This is something I wanted to ask for some time so I will take
> > > the
> > > > > > opportunity here :).
> > > > > > > Is this something you prefer just not to risk at all and make it
> an
> > > > > hard
> > > > > > requirement
> > > > > > > (which is fair)? ...
> > > > > >
> > > > > > Yes, I think we need to keep this as a hard requirement.
> > > > > > There are drivers out there which we missed this on in the
> past,
> > > and
> > > > > > I'm not necessarily
> > > > > > going to take the time to go through them all as this can be
> hard
> > > to
> > > > > > spot, but lets not
> > > > > > introduce any more potential problems.
> > > > > >
> > > > >
> > > > > I see. That makes sense and it's fair :). The only annoying (but
> not
> > > too
> > > > > annoying :)) is that
> > > > > making the data/buffer global forces you to use a lock in cases
> you
> > > > > potentially would
> > > > > not have too (just using local buffers). But that's life, better play
> > > safe :)
> > > > >
> > > > > > >
> > > > > > > I'm asking this because, tbh, I would be very surprised if any
> > > spi/i2c
> > > > > > controller out there
> > > > > > > is using dma for a 3byte transfer. I guess the overhead of
> > > setting it
> > > > > up
> > > > > > is probably not
> > > > > > > worth it...
> > > > > >
> > > > > > There are (I believe) a few i2c and spi controllers out there
> that
> > > don't
> > > > > > do anything other
> > > > > > than DMA. Wolfram mentioned one of those in his talk on
> adding
> > > > > > DMA support to i2c.
> > > > >
> > > > > Hmm, I see...
> > > > >
> > > > > > Also, the reference in the file below to the wonderful case of
> > > USB to
> > > > > > i2c bridges that always
> > > > > > require DMA safe buffers.
> > > > >
> > > > > Indeed it does.
> > > > >
> > > >
> > > > Hi Jonathan,
> > > >
> > > > Just for closure, I also realized that the pattern in IIO looks to be
> to
> > > use
> > > > DMA safe buffers only on the tx side. For instance in the IMU lib
> [1],
> > > > only the tx buffer is safe (well, I think there's problem with this as
> > > > I believe all spi transfers buffers should be properly aligned which
> > > won't
> > > > be the case in the IMU lib). Is there any reason for this? AFAICT,
> we
> > > should
> > > > also take care with rx buffers or am I missing something?
> > > Ah. So this is a fun corner :)
> > >
> > > The reason cache line corruption can occur is as follows.
> > > 1. DMA starts, typically involving some tx and rx usage by the
> device.
> > > This flushes the CPU caches for the relevant lines.
> > > ... whilst DMA is not completed ...
> > > 2. The host software pulls the line into it's cache and updates
> > > something (say a flag
> > > elsewhere in that cacheline).
> > > 3. Cacheline is evicted from the CPU cache causing a write back.
> > > 4. Device then writes back the stuff it had cached locally which is
> > > allowed to include data
> > > it wasn't accessing in the same cache line. Boom, it just
> overwrote
> > > the flag we updated
> > > with an older value. Basically this is a performance optimization /
> > > simplification
> > > the DMA engine is allowed to make. Note I believe they are
> > > 'technically' allowed to
> > > write back to the RX buffers as well, though not sure what
> devices do
> > > this for i2c/spi.
> >
> > Yes, got it...
> >
> > > So, why do we only need to force one of the buffers to the start of
> a
> > > cacheline?
> > >
> > > What we are actually doing, is not keeping the buffer in it's own
> > > cacheline, but rather
> > > making sure nothing else is in the same cacheline (so there is no
> race
> > > as above).
> > > (it's easier to move the buffer, than to ensure everything else is
> > > moved out of the cache
> > > line it happens to be in!)
> > > There is a safe assumption here that the DMA device can't corrupt
> it's
> > > own data as that
> > > would be crazy :)
> >
> > My confusion here was looking at this [1] and somehow thinking that
> > DMA mappings require that both 'tx_buf' and 'rx_buf' to be on their
> own
> > cache lines... But I guess that since both buffers are guaranteed to be
> alone
> > (in our case) in their cache line (or span across multiple lines) we are
> fine... Though
> > I didn't really looked into the details on the DMA subsystem... I'm
> just aware that
> > for instance 'dma_map_single()' states that the addr to map must
> begin on a cache
> > line boundary.
> >
> > > Hence, pushing the first buffer to the start of a line, allows the
> second
> > > one to be
> > > after it in the same line (it's not a problem if it takes multiple lines)
> > >
> > > One more subtlety is why we can be sure nothing else ends up
> after
> > > the buffers.
> > > That's by construction. The allocations IIO does for those iio_priv
> > > structures should
> > > always get padded out to at least the end of the cacheline.
> > > (IIRC, not looked at this code for many years!)
> > >
> >
> > Looking at [2], I think that holds correct as long as private structures
> have
> > their buffer properly aligned in the end of the struct. Hence, the
> natural padding
> > of the structure ensures us that nothing else gets into the same
> cache
> > line as our buffer.
> >
> > Which means that, if I'm not missing nothing obvious, all the adis
> IMUS are not
> > complying with this [3]... I think the 'struct adis adis' field has to be in
> the end of the
> > adis16480 structure so that our tx and rx buffers in the adis struct are
> on their own
> > line...
>
> Hmm. Not quite because another subtlety of c slips in. Hohum -
> finding the reference
> for this always takes me a while...
Hmm, I should have took 5mins more and thought about this one
before rushing in thinking we had problems :facepalm...
> I can't find it in the c spec today for some reason (we tracked it down
> when fixing
> the timestamp alignment issues last year)... But the gcc docs refer to
> the correct
> requirements
> https://urldefense.com/v3/__https://gcc.gnu.org/onlinedocs/gcc-
> 3.3/gcc/Type-Attributes.html__;!!A3Ni8CS0y2Y!rEUxApIDgq-
> JGdeWUGFacwOFhHME2nhCkCUQ9WymNCIZgf0zSPjH5OWkuj07uQ$
>
> Because we have the buffer embedded in the adis structure, the
> question becomes how
> is that aligned and how is the data after it aligned.
>
> Now, c structures are aligned to at least the alignment requirement of
> the element which has
> the largest alignment requirements. Now the next bit I'm dodgier on
> proving, but
> my understanding is the following elements of the outer structure are
> guaranteed
> to not be pushed into the padding at the end of the adis structure. It's
> documented
Yes, you're right. I would be very surprise if this was not the case. It would
be very unsettling and disturbing if the 'sizeof(adis)' was not respected.
I wonder how it would work if we wanted to embed an array of these
structures. And if the answer was, well for this case the compiler will do fine,
that would not be less disturbing...
- Nuno Sá
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] dt-bindings: iio: frequency: add adrf6780 doc
2021-07-02 11:12 ` [PATCH v4 2/2] dt-bindings: iio: frequency: add adrf6780 doc Antoniu Miclaus
@ 2021-07-14 22:56 ` Rob Herring
0 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2021-07-14 22:56 UTC (permalink / raw)
To: Antoniu Miclaus; +Cc: linux-iio, jic23, linux-kernel, robh+dt, devicetree
On Fri, 02 Jul 2021 14:12:39 +0300, Antoniu Miclaus wrote:
> Add device tree bindings for the ADRF6780 Upconverter.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> changes in v4:
> - fix dt_binding_check detected issues
> - update license to GPL-2.0-only OR BSD-2-Clause
> - add dependencies schema
> - other minor fixes based on the review received
>
> .../bindings/iio/frequency/adi,adrf6780.yaml | 119 ++++++++++++++++++
> 1 file changed, 119 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-07-14 22:56 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-02 11:12 [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780 Antoniu Miclaus
2021-07-02 11:12 ` [PATCH v4 2/2] dt-bindings: iio: frequency: add adrf6780 doc Antoniu Miclaus
2021-07-14 22:56 ` Rob Herring
2021-07-03 16:57 ` [PATCH v4 1/2] iio: frequency: adrf6780: add support for ADRF6780 Jonathan Cameron
2021-07-05 10:18 ` Sa, Nuno
2021-07-06 9:04 ` Jonathan Cameron
2021-07-06 10:22 ` Sa, Nuno
2021-07-07 8:26 ` Sa, Nuno
2021-07-07 8:56 ` Jonathan Cameron
2021-07-07 11:11 ` Sa, Nuno
2021-07-10 17:57 ` Jonathan Cameron
2021-07-12 7:17 ` Sa, Nuno
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).