On Mon, 17 Jan 2022 18:52:47 +0200 Cristian Pop <cristian.pop@xxxxxxxxxx> wrote: > Add support for K Band Downconverter with Integrated > Fractional-N PLL and VCO. > More info: > https://www.analog.com/en/products/admv4420.html > > Signed-off-by: Cristian Pop <cristian.pop@xxxxxxxxxx> Hi Cristian, A few comments inline from a very quick look. Thanks, Jonathan > --- > drivers/iio/frequency/Kconfig | 10 + > drivers/iio/frequency/Makefile | 1 + > drivers/iio/frequency/admv4420.c | 412 +++++++++++++++++++++++++++++++ > 3 files changed, 423 insertions(+) > create mode 100644 drivers/iio/frequency/admv4420.c > > diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig > index b44036f843af..55aa63548c00 100644 > --- a/drivers/iio/frequency/Kconfig > +++ b/drivers/iio/frequency/Kconfig > @@ -60,6 +60,16 @@ config ADMV1013 > To compile this driver as a module, choose M here: the > module will be called admv1013. > > +config ADMV4420 > + tristate "Analog Devices ADMV4420 K Band Downconverter" > + depends on SPI && COMMON_CLK Why COMMON_CLK? I can't see any usage. However, I'm not sure just providing an input frequency is that sensible as it is likely to be connect to something variable (to some degree at least). > + help > + Say yes here to build support for Analog Devices K Band > + Downconverter with integrated Fractional-N PLL and VCO. > + > + To compile this driver as a module, choose M here: the > + module will be called admv4420. > + > config ADRF6780 > tristate "Analog Devices ADRF6780 Microwave Upconverter" > depends on SPI > diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile > index ae6899856c99..782e5baa1630 100644 > --- a/drivers/iio/frequency/Makefile > +++ b/drivers/iio/frequency/Makefile > @@ -8,4 +8,5 @@ obj-$(CONFIG_AD9523) += ad9523.o > obj-$(CONFIG_ADF4350) += adf4350.o > obj-$(CONFIG_ADF4371) += adf4371.o > obj-$(CONFIG_ADMV1013) += admv1013.o > +obj-$(CONFIG_ADMV4420) += admv4420.o > obj-$(CONFIG_ADRF6780) += adrf6780.o > diff --git a/drivers/iio/frequency/admv4420.c b/drivers/iio/frequency/admv4420.c > new file mode 100644 > index 000000000000..f32039ab2cd3 > --- /dev/null > +++ b/drivers/iio/frequency/admv4420.c > @@ -0,0 +1,412 @@ > +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause > +/* > + * ADMV4420 > + * > + * Copyright 2021 Analog Devices Inc. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/spi/spi.h> > + > +/* ADMV4420 Register Map */ > +#define ADMV4420_SPI_CONFIG_1 0x00 > +#define ADMV4420_SPI_CONFIG_2 0x01 > +#define ADMV4420_CHIPTYPE 0x03 > +#define ADMV4420_PRODUCT_ID_L 0x04 > +#define ADMV4420_PRODUCT_ID_H 0x05 > +#define ADMV4420_SCRATCHPAD 0x0A > +#define ADMV4420_SPI_REV 0x0B > +#define ADMV4420_ENABLES 0x103 > +#define ADMV4420_SDO_LEVEL 0x108 > +#define ADMV4420_INT_L 0x200 > +#define ADMV4420_INT_H 0x201 > +#define ADMV4420_FRAC_L 0x202 > +#define ADMV4420_FRAC_M 0x203 > +#define ADMV4420_FRAC_H 0x204 > +#define ADMV4420_MOD_L 0x208 > +#define ADMV4420_MOD_M 0x209 > +#define ADMV4420_MOD_H 0x20A > +#define ADMV4420_R_DIV_L 0x20C > +#define ADMV4420_R_DIV_H 0x20D > +#define ADMV4420_REFERENCE 0x20E > +#define ADMV4420_VCO_DATA_READBACK1 0x211 > +#define ADMV4420_VCO_DATA_READBACK2 0x212 > +#define ADMV4420_PLL_MUX_SEL 0x213 > +#define ADMV4420_LOCK_DETECT 0x214 > +#define ADMV4420_BAND_SELECT 0x215 > +#define ADMV4420_VCO_ALC_TIMEOUT 0x216 > +#define ADMV4420_VCO_MANUAL 0x217 > +#define ADMV4420_ALC 0x219 > +#define ADMV4420_VCO_TIMEOUT1 0x21C > +#define ADMV4420_VCO_TIMEOUT2 0x21D > +#define ADMV4420_VCO_BAND_DIV 0x21E > +#define ADMV4420_VCO_READBACK_SEL 0x21F > +#define ADMV4420_AUTOCAL 0x226 > +#define ADMV4420_CP_STATE 0x22C > +#define ADMV4420_CP_BLEED_EN 0x22D > +#define ADMV4420_CP_CURRENT 0x22E > +#define ADMV4420_CP_BLEED 0x22F > + > + > +#define ADMV4420_SPI_CONFIG_1_SOFTRESET_ BIT(7) > +#define ADMV4420_SPI_CONFIG_1_SDOACTIVE_ BIT(4) > +#define ADMV4420_SPI_CONFIG_1_SDOACTIVE BIT(3) > +#define ADMV4420_SPI_CONFIG_1_SOFTRESET BIT(1) > + > +#define ADMV4420_REFERENCE_IN_MODE(x) (x << 1) > +#define ADMV4420_REFERENCE_DOUBLER(x) (x << 2) > +#define ADMV4420_REFERENCE_DIVIDE_BY_2_MASK BIT(0) > +#define ADMV4420_REFERENCE_MODE_MASK BIT(1) > +#define ADMV4420_REFERENCE_DOUBLER_MASK BIT(2) > +#define ADMV4420_REF_DIVIDER_MAX_VAL GENMASK(9, 0) > +#define ADMV4420_N_COUNTER_INT_MAX GENMASK(15, 0) > +#define ADMV4420_N_COUNTER_FRAC_MAX GENMASK(23, 0) > +#define ADMV4420_N_COUNTER_MOD_MAX GENMASK(23, 0) > + > +#define ADMV4420_INT_L_MASK GENMASK(7, 0) > +#define ADMV4420_INT_H_MASK GENMASK(15, 8) > +#define ADMV4420_FRAC_L_MASK GENMASK(7, 0) > +#define ADMV4420_FRAC_M_MASK GENMASK(15, 8) > +#define ADMV4420_FRAC_H_MASK GENMASK(23, 16) > +#define ADMV4420_MOD_L_MASK GENMASK(7, 0) > +#define ADMV4420_MOD_M_MASK GENMASK(15, 8) > +#define ADMV4420_MOD_H_MASK GENMASK(23, 16) > + > +#define ENABLE_PLL BIT(6) > +#define ENABLE_LO BIT(5) > +#define ENABLE_VCO BIT(3) > +#define ENABLE_IFAMP BIT(2) > +#define ENABLE_MIXER BIT(1) > +#define ENABLE_LNA BIT(0) > + > +#define ADMV4420_SCRATCH_PAD_VAL_1 0xAD > +#define ADMV4420_SCRATCH_PAD_VAL_2 0xEA > + > +#define ADMV4420_REF_FREQ_HZ 50000000 > +#define MAX_N_COUNTER 655360UL > +#define MAX_R_DIVIDER 1024 > +#define ADMV4420_DEFAULT_LO_FREQ_HZ 16750000000ULL > + > +enum admv4420_mux_sel { > + ADMV4420_LOW = 0, > + ADMV4420_LOCK_DTCT = 1, > + ADMV4420_R_COUNTER_PER_2 = 4, > + ADMV4420_N_CONUTER_PER_2 = 5, > + ADMV4420_HIGH = 8, > +}; > + > +struct admv4420_reference_block { > + bool doubler_en; > + bool divide_by_2_en; > + bool ref_single_ended; > + u32 divider; > +}; > + > +struct admv4420_n_counter { > + u32 int_val; > + u32 frac_val; > + u32 mod_val; > + u32 n_counter; > +}; > + > +struct admv4420_state { > + struct spi_device *spi; > + struct regmap *regmap; > + u64 pfd_freq_hz; > + u64 vco_freq_hz; > + u64 lo_freq_hz; > + struct admv4420_reference_block ref_block; > + struct admv4420_n_counter n_counter; > + enum admv4420_mux_sel mux_sel; > + struct mutex lock; All locks need a comment saying what exactly their intended scope is. > +}; > + > +static const struct regmap_config admv4420_regmap_config = { > + .reg_bits = 16, > + .val_bits = 8, > + .read_flag_mask = BIT(7), > +}; > + > +static int admv4420_reg_access(struct iio_dev *indio_dev, > + u32 reg, u32 writeval, > + u32 *readval) > +{ > + struct admv4420_state *st = iio_priv(indio_dev); > + > + if (readval) > + return regmap_read(st->regmap, reg, readval); > + else > + return regmap_write(st->regmap, reg, writeval); > +} > + > +static int admv4420_set_n_counter(struct admv4420_state *st, u32 int_val, u32 frac_val, u32 mod_val) Shorter lines preferred. Under 80 chars unless it significantly affects readability or breaks up a string we might want to grep for. > +{ > + int ret; > + > + ret = regmap_write(st->regmap, ADMV4420_FRAC_H, FIELD_GET(ADMV4420_FRAC_H_MASK, frac_val)); I'm not that keen on doing FIELD_GET for a simple breaking up into 8 bit chunks. It is a rare occasion where it makes it harder to see what is going on. Can't you do a bulk write to set 3 registers in one function call? Will require a DMA safe buffer but it's probably easier to read than this repetition > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_FRAC_M, FIELD_GET(ADMV4420_FRAC_M_MASK, frac_val)); > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_FRAC_L, FIELD_GET(ADMV4420_FRAC_L_MASK, frac_val)); > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_MOD_H, FIELD_GET(ADMV4420_MOD_H_MASK, mod_val)); > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_MOD_M, FIELD_GET(ADMV4420_MOD_M_MASK, mod_val)); > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_MOD_L, FIELD_GET(ADMV4420_MOD_L_MASK, mod_val)); > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_INT_H, FIELD_GET(ADMV4420_INT_H_MASK, int_val)); > + if (ret) > + return ret; > + > + return regmap_write(st->regmap, ADMV4420_INT_L, FIELD_GET(ADMV4420_INT_L_MASK, int_val)); > +} > + > +static int admv4420_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long info) > +{ > + struct admv4420_state *st = iio_priv(indio_dev); > + > + switch (info) { > + case IIO_CHAN_INFO_FREQUENCY: > + *val = div_u64(st->lo_freq_hz, 1000000); > + div_u64_rem(st->lo_freq_hz, 1000000, val2); > + > + return IIO_VAL_INT_PLUS_MICRO; > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_info admv4420_info = { > + .read_raw = admv4420_read_raw, > + .debugfs_reg_access = &admv4420_reg_access, > +}; > + > +static const struct iio_chan_spec admv4420_channels[] = { > + { > + .type = IIO_ALTVOLTAGE, > + .output = 0, > + .indexed = 1, > + .channel = 0, > + .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY), > + }, > +}; > + > +static void admv4420_dt_parse(struct admv4420_state *st) > +{ > + struct spi_device *spi = st->spi; > + > + device_property_read_u64(&spi->dev, "adi,lo_freq_hz", &st->lo_freq_hz); > + st->ref_block.ref_single_ended = of_property_read_bool(spi->dev.of_node, > + "adi,ref_ext_single_ended_en"); Use generic fw properties from include/linux/property.h as then we get support for other firmware types for free. > +} > + > +static inline uint64_t admv4420_calc_pfd_vco(struct admv4420_state *st) > +{ > + return div_u64(st->vco_freq_hz * 10, st->n_counter.n_counter); > +} > + > +static inline uint64_t admv4420_calc_pfd_ref(struct admv4420_state *st) > +{ > + uint64_t tmp; > + > + tmp = ADMV4420_REF_FREQ_HZ * (st->ref_block.doubler_en ? 2 : 1); > + return div_u64(tmp, st->ref_block.divider * (st->ref_block.divide_by_2_en ? 2 : 1)); Pull that ternary operating out on it's own line. There is too much going on in one statement to make it easy to read! > +} > + > +static int admv4420_calc_parameters(struct admv4420_state *st) > +{ > + u64 pfd_ref, pfd_vco; > + bool sol_found = false; > + > + st->ref_block.doubler_en = false; > + st->ref_block.divide_by_2_en = false; > + st->vco_freq_hz = div_u64(st->lo_freq_hz, 2); > + > + for (st->ref_block.divider = 1; st->ref_block.divider < MAX_R_DIVIDER; > + st->ref_block.divider++) { > + pfd_ref = admv4420_calc_pfd_ref(st); > + for (st->n_counter.n_counter = 1; st->n_counter.n_counter < MAX_N_COUNTER; > + st->n_counter.n_counter++) { > + pfd_vco = admv4420_calc_pfd_vco(st); > + if (pfd_ref == pfd_vco) { > + sol_found = true; > + break; > + } > + } > + > + if (sol_found) > + break; > + > + st->n_counter.n_counter = 1; > + } > + if (!sol_found) > + return -1; > + > + st->n_counter.int_val = div_u64_rem(st->n_counter.n_counter, 10, &st->n_counter.frac_val); > + st->n_counter.mod_val = 1; > + > + return 0; > +} > + > +static int admv4420_setup(struct iio_dev *indio_dev) > +{ > + struct admv4420_state *st = iio_priv(indio_dev); > + u32 val = 0; This will always be set before use. If you are seeing a compiler warning then fine to leave it here but perhaps add a comment so no one 'cleans it up'. > + int ret; > + > + ret = regmap_write(st->regmap, ADMV4420_SPI_CONFIG_1, > + ADMV4420_SPI_CONFIG_1_SOFTRESET_ | ADMV4420_SPI_CONFIG_1_SOFTRESET); In general try to keep below 80 chars where it is easy to do like here. > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_SPI_CONFIG_1, > + ADMV4420_SPI_CONFIG_1_SDOACTIVE_ | ADMV4420_SPI_CONFIG_1_SDOACTIVE); > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_SCRATCHPAD, ADMV4420_SCRATCH_PAD_VAL_1); > + if (ret) > + return ret; > + > + ret = regmap_read(st->regmap, ADMV4420_SCRATCHPAD, &val); > + if (ret) > + return ret; > + > + if (val != ADMV4420_SCRATCH_PAD_VAL_1) { > + dev_err(indio_dev->dev.parent, "Failed ADMV4420 to read/write scratchpad %x ", val); > + return -EIO; > + } > + > + ret = regmap_write(st->regmap, ADMV4420_SCRATCHPAD, ADMV4420_SCRATCH_PAD_VAL_2); > + if (ret) > + return ret; > + > + ret = regmap_read(st->regmap, ADMV4420_SCRATCHPAD, &val); > + if (ret) > + return ret; > + > + if (val != ADMV4420_SCRATCH_PAD_VAL_2) { > + dev_err(indio_dev->dev.parent, "Failed to read/write scratchpad %x ", val); > + return -EIO; > + } > + > + st->mux_sel = ADMV4420_LOCK_DTCT; > + st->lo_freq_hz = ADMV4420_DEFAULT_LO_FREQ_HZ; > + > + admv4420_dt_parse(st); I doubt this needs to be dt specific so _fw_parse() > + > + ret = admv4420_calc_parameters(st); > + if (ret) { > + dev_err(indio_dev->dev.parent, "Failed calc parameters for %lld ", st->vco_freq_hz); > + return ret; > + } > + > + ret = regmap_write(st->regmap, ADMV4420_R_DIV_L, > + FIELD_GET(0xFF, st->ref_block.divider)); > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_R_DIV_H, > + FIELD_GET(0xFF00, st->ref_block.divider)); > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_REFERENCE, > + st->ref_block.divide_by_2_en | > + ADMV4420_REFERENCE_IN_MODE(st->ref_block.ref_single_ended) | > + ADMV4420_REFERENCE_DOUBLER(st->ref_block.doubler_en)); > + if (ret) > + return ret; > + > + ret = admv4420_set_n_counter(st, st->n_counter.int_val, st->n_counter.frac_val, st->n_counter.mod_val); > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_PLL_MUX_SEL, st->mux_sel); > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADMV4420_ENABLES, ENABLE_PLL | ENABLE_LO | ENABLE_VCO | > + ENABLE_IFAMP | ENABLE_MIXER | ENABLE_LNA); return regmap_write() > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int admv4420_probe(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev; > + struct admv4420_state *st; > + struct regmap *regmap; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); > + if (!indio_dev) > + return -ENOMEM; > + > + regmap = devm_regmap_init_spi(spi, &admv4420_regmap_config); > + if (IS_ERR(regmap)) { > + dev_err(&spi->dev, "Error ADMV4420 initializing spi regmap: %ld\n", > + PTR_ERR(regmap)); > + return PTR_ERR(regmap); For probe errors, it is often easier to just use dev_err_probe() for all of them whether or not they can defer. > + } > + > + st = iio_priv(indio_dev); > + st->spi = spi; > + st->regmap = regmap; > + mutex_init(&st->lock); > + > + indio_dev->dev.parent = &spi->dev; We set that automatically in devm_iio_device_alloc() as it's almost always the same value as passed in as dev to that function and in rare case where it is different we can override. Here they are the same. > + indio_dev->name = "admv4420"; > + indio_dev->info = &admv4420_info; > + indio_dev->channels = admv4420_channels; > + indio_dev->num_channels = ARRAY_SIZE(admv4420_channels); > + > + ret = admv4420_setup(indio_dev); > + if (ret) { > + dev_err(&spi->dev, "Setup ADMV4420 failed (%d)\n", ret); > + return ret; > + } > + > + return devm_iio_device_register(&spi->dev, indio_dev); > +} > +
