Date:   Tue, 2 Nov 2021 10:09:15 +0000
From:   "Sa, Nuno" <Nuno.Sa@...log.com>
To:     "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>
Subject: RE: [PATCH v3 1/2] iio: frequency: admv1013: add support for ADMV1013

> From: Antoniu Miclaus <antoniu.miclaus@...log.com>
> Sent: Monday, November 1, 2021 11:04 AM
> To: jic23@...nel.org; robh+dt@...nel.org; linux-iio@...r.kernel.org;
> devicetree@...r.kernel.org; linux-kernel@...r.kernel.org
> Cc: Sa, Nuno <Nuno.Sa@...log.com>; Miclaus, Antoniu
> <Antoniu.Miclaus@...log.com>
> Subject: [PATCH v3 1/2] iio: frequency: admv1013: add support for
> ADMV1013
> 
> The ADMV1013 is a wideband, microwave upconverter optimized
> for point to point microwave radio designs operating in the
> 24 GHz to 44 GHz radio frequency (RF) range.
> 
> Datasheet:
> https://www.analog.com/media/en/technical-documentation/data-
> sheets/ADMV1013.pdf
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> changes in v3:
>  - fix includes
>  - make masks more generic where possible
>  - improve mutex attribute description
>  - rework IIO channels and remove IIO_VAL_INT_MULTIPLE return
>  - use HZ_PER_MHZ macro
>  - improve readability in the `int admv1013_reg_access` function
>  - use `devm_clk_notifier_registers()`
> ---
>  drivers/iio/frequency/Kconfig    |  11 +
>  drivers/iio/frequency/Makefile   |   1 +
>  drivers/iio/frequency/admv1013.c | 571
> +++++++++++++++++++++++++++++++
>  3 files changed, 583 insertions(+)
>  create mode 100644 drivers/iio/frequency/admv1013.c
> 
> diff --git a/drivers/iio/frequency/Kconfig
> b/drivers/iio/frequency/Kconfig
> index 240b81502512..411b3b961e46 100644
> --- a/drivers/iio/frequency/Kconfig
> +++ b/drivers/iio/frequency/Kconfig
> @@ -49,5 +49,16 @@ config ADF4371
> 
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called adf4371.
> +
> +config ADMV1013
> +	tristate "Analog Devices ADMV1013 Microwave Upconverter"
> +	depends on SPI && COMMON_CLK
> +	help
> +	  Say yes here to build support for Analog Devices ADMV1013
> +	  24 GHz to 44 GHz, Wideband, Microwave Upconverter.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called admv1013.
> +
>  endmenu
>  endmenu
> diff --git a/drivers/iio/frequency/Makefile
> b/drivers/iio/frequency/Makefile
> index 518b1e50caef..559922a8196e 100644
> --- a/drivers/iio/frequency/Makefile
> +++ b/drivers/iio/frequency/Makefile
> @@ -7,3 +7,4 @@
>  obj-$(CONFIG_AD9523) += ad9523.o
>  obj-$(CONFIG_ADF4350) += adf4350.o
>  obj-$(CONFIG_ADF4371) += adf4371.o
> +obj-$(CONFIG_ADMV1013) += admv1013.o
> diff --git a/drivers/iio/frequency/admv1013.c
> b/drivers/iio/frequency/admv1013.c
> new file mode 100644
> index 000000000000..6451a2cc7c52
> --- /dev/null
> +++ b/drivers/iio/frequency/admv1013.c
> @@ -0,0 +1,571 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ADMV1013 driver
> + *
> + * Copyright 2021 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/notifier.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
> +
> +#include <asm/unaligned.h>
> +
> +/* ADMV1013 Register Map */
> +#define ADMV1013_REG_SPI_CONTROL		0x00
> +#define ADMV1013_REG_ALARM			0x01
> +#define ADMV1013_REG_ALARM_MASKS		0x02
> +#define ADMV1013_REG_ENABLE			0x03
> +#define ADMV1013_REG_LO_AMP_I			0x05
> +#define ADMV1013_REG_LO_AMP_Q			0x06
> +#define ADMV1013_REG_OFFSET_ADJUST_I		0x07
> +#define ADMV1013_REG_OFFSET_ADJUST_Q		0x08
> +#define ADMV1013_REG_QUAD			0x09
> +#define ADMV1013_REG_VVA_TEMP_COMP		0x0A
> +
> +/* ADMV1013_REG_SPI_CONTROL Map */
> +#define ADMV1013_PARITY_EN_MSK			BIT(15)
> +#define ADMV1013_SPI_SOFT_RESET_MSK		BIT(14)
> +#define ADMV1013_CHIP_ID_MSK			GENMASK(11, 4)
> +#define ADMV1013_CHIP_ID			0xA
> +#define ADMV1013_REVISION_ID_MSK		GENMASK(3, 0)
> +
> +/* ADMV1013_REG_ALARM Map */
> +#define ADMV1013_PARITY_ERROR_MSK		BIT(15)
> +#define ADMV1013_TOO_FEW_ERRORS_MSK		BIT(14)
> +#define ADMV1013_TOO_MANY_ERRORS_MSK		BIT(13)
> +#define ADMV1013_ADDRESS_RANGE_ERROR_MSK	BIT(12)
> +
> +/* ADMV1013_REG_ENABLE Map */
> +#define ADMV1013_VGA_PD_MSK			BIT(15)
> +#define ADMV1013_MIXER_PD_MSK			BIT(14)
> +#define ADMV1013_QUAD_PD_MSK
> 	GENMASK(13, 11)
> +#define ADMV1013_BG_PD_MSK			BIT(10)
> +#define ADMV1013_MIXER_IF_EN_MSK		BIT(7)
> +#define ADMV1013_DET_EN_MSK			BIT(5)
> +
> +/* ADMV1013_REG_LO_AMP Map */
> +#define ADMV1013_LOAMP_PH_ADJ_FINE_MSK
> 	GENMASK(13, 7)
> +#define ADMV1013_MIXER_VGATE_MSK		GENMASK(6, 0)
> +
> +/* ADMV1013_REG_OFFSET_ADJUST Map */
> +#define ADMV1013_MIXER_OFF_ADJ_P_MSK
> 	GENMASK(15, 9)
> +#define ADMV1013_MIXER_OFF_ADJ_N_MSK
> 	GENMASK(8, 2)
> +
> +/* ADMV1013_REG_QUAD Map */
> +#define ADMV1013_QUAD_SE_MODE_MSK		GENMASK(9, 6)
> +#define ADMV1013_QUAD_FILTERS_MSK		GENMASK(3, 0)
> +
> +/* ADMV1013_REG_VVA_TEMP_COMP Map */
> +#define ADMV1013_VVA_TEMP_COMP_MSK
> 	GENMASK(15, 0)
> +
> +struct admv1013_state {
> +	struct spi_device	*spi;
> +	struct clk		*clkin;
> +	/* Protect against concurrent accesses to the device and to
> data */
> +	struct mutex		lock;
> +	struct regulator	*reg;
> +	struct notifier_block	nb;
> +	unsigned int		quad_se_mode;
> +	bool			vga_pd;
> +	bool			mixer_pd;
> +	bool			quad_pd;
> +	bool			bg_pd;
> +	bool			mixer_if_en;
> +	bool			det_en;
> +	u8			data[3] ____cacheline_aligned;
> +};
> +
> +static int __admv1013_spi_read(struct admv1013_state *st, unsigned
> int reg,
> +			       unsigned int *val)
> +{
> +	int ret;
> +	struct spi_transfer t = {0};
> +
> +	st->data[0] = 0x80 | (reg << 1);
> +	st->data[1] = 0x0;
> +	st->data[2] = 0x0;
> +
> +	t.rx_buf = &st->data[0];
> +	t.tx_buf = &st->data[0];
> +	t.len = 3;
> +
> +	ret = spi_sync_transfer(st->spi, &t, 1);
> +	if (ret)
> +		return ret;
> +
> +	*val = (get_unaligned_be24(&st->data[0]) >> 1) &
> GENMASK(15, 0);
> +
> +	return ret;
> +}
> +
> +static int admv1013_spi_read(struct admv1013_state *st, unsigned
> int reg,
> +			     unsigned int *val)
> +{
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = __admv1013_spi_read(st, reg, val);
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int __admv1013_spi_write(struct admv1013_state *st,
> +				unsigned int reg,
> +				unsigned int val)
> +{
> +	put_unaligned_be24((val << 1) | (reg << 17), &st->data[0]);
> +
> +	return spi_write(st->spi, &st->data[0], 3);
> +}
> +
> +static int admv1013_spi_write(struct admv1013_state *st, unsigned
> int reg,
> +			      unsigned int val)
> +{
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = __admv1013_spi_write(st, reg, val);
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int __admv1013_spi_update_bits(struct admv1013_state *st,
> unsigned int reg,
> +				      unsigned int mask, unsigned int val)
> +{
> +	int ret;
> +	unsigned int data, temp;
> +
> +	ret = __admv1013_spi_read(st, reg, &data);
> +	if (ret)
> +		return ret;
> +
> +	temp = (data & ~mask) | (val & mask);
> +
> +	return __admv1013_spi_write(st, reg, temp);
> +}
> +
> +static int admv1013_spi_update_bits(struct admv1013_state *st,
> unsigned int reg,
> +				    unsigned int mask, unsigned int val)
> +{
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = __admv1013_spi_update_bits(st, reg, mask, val);
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int admv1013_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long info)
> +{
> +	struct admv1013_state *st = iio_priv(indio_dev);
> +	unsigned int data, addr;
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		if (chan->channel2 == IIO_MOD_I)
> +			addr = ADMV1013_REG_OFFSET_ADJUST_I;
> +		else
> +			addr = ADMV1013_REG_OFFSET_ADJUST_Q;
> +
> +		ret = admv1013_spi_read(st, addr, &data);
> +		if (ret)
> +			return ret;
> +
> +		if (!(chan->channel))
> +			*val =
> FIELD_GET(ADMV1013_MIXER_OFF_ADJ_P_MSK, data);
> +		else
> +			*val =
> FIELD_GET(ADMV1013_MIXER_OFF_ADJ_N_MSK, data);
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_PHASE:
> +		if (chan->channel2 == IIO_MOD_I)
> +			addr = ADMV1013_REG_LO_AMP_I;
> +		else
> +			addr = ADMV1013_REG_LO_AMP_Q;
> +
> +		ret = admv1013_spi_read(st, addr, &data);
> +		if (ret)
> +			return ret;
> +
> +		*val =
> FIELD_GET(ADMV1013_LOAMP_PH_ADJ_FINE_MSK, data);
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int admv1013_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long info)
> +{
> +	struct admv1013_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		if (chan->channel2 == IIO_MOD_I) {
> +			if (!(chan->channel))
> +				ret = admv1013_spi_update_bits(st,
> ADMV1013_REG_OFFSET_ADJUST_I,
> +
> ADMV1013_MIXER_OFF_ADJ_P_MSK,
> +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_P_MSK, val));
> +			else
> +				ret = admv1013_spi_update_bits(st,
> ADMV1013_REG_OFFSET_ADJUST_I,
> +
> ADMV1013_MIXER_OFF_ADJ_N_MSK,
> +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_N_MSK, val));
> +		} else {
> +			if (!(chan->channel))
> +				ret = admv1013_spi_update_bits(st,
> ADMV1013_REG_OFFSET_ADJUST_Q,
> +
> ADMV1013_MIXER_OFF_ADJ_P_MSK,
> +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_P_MSK, val));
> +			else
> +				ret = admv1013_spi_update_bits(st,
> ADMV1013_REG_OFFSET_ADJUST_Q,
> +
> ADMV1013_MIXER_OFF_ADJ_N_MSK,
> +
> FIELD_PREP(ADMV1013_MIXER_OFF_ADJ_N_MSK, val));
> +		}
> +
> +		return ret;
> +	case IIO_CHAN_INFO_PHASE:
> +		if (chan->channel2 == IIO_MOD_I)
> +			return admv1013_spi_update_bits(st,
> ADMV1013_REG_LO_AMP_I,
> +
> 	ADMV1013_LOAMP_PH_ADJ_FINE_MSK,
> +
> 	FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_FINE_MSK, val));
> +		else
> +			return admv1013_spi_update_bits(st,
> ADMV1013_REG_LO_AMP_Q,
> +
> 	ADMV1013_LOAMP_PH_ADJ_FINE_MSK,
> +
> 	FIELD_PREP(ADMV1013_LOAMP_PH_ADJ_FINE_MSK, val));
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int admv1013_update_quad_filters(struct admv1013_state *st)
> +{
> +	unsigned int filt_raw;
> +	u64 rate = clk_get_rate(st->clkin);
> +
> +	if (rate >= (5400 * HZ_PER_MHZ) && rate <= (7000 *
> HZ_PER_MHZ))
> +		filt_raw = 15;
> +	else if (rate >= (5400 * HZ_PER_MHZ) && rate <= (8000 *
> HZ_PER_MHZ))
> +		filt_raw = 10;
> +	else if (rate >= (6600 * HZ_PER_MHZ) && rate <= (9200 *
> HZ_PER_MHZ))
> +		filt_raw = 5;
> +	else
> +		filt_raw = 0;
> +
> +	return __admv1013_spi_update_bits(st,
> ADMV1013_REG_QUAD,
> +
> 	ADMV1013_QUAD_FILTERS_MSK,
> +
> 	FIELD_PREP(ADMV1013_QUAD_FILTERS_MSK, filt_raw));
> +}
> +
> +static int admv1013_update_mixer_vgate(struct admv1013_state *st)
> +{
> +	unsigned int vcm, mixer_vgate;
> +
> +	vcm = regulator_get_voltage(st->reg);
> +
> +	if (vcm >= 0 && vcm < 1800000)
> +		mixer_vgate = (2389 * vcm / 1000000 + 8100) / 100;
> +	else if (vcm > 1800000 && vcm < 2600000)
> +		mixer_vgate = (2375 * vcm / 1000000 + 125) / 100;
> +	else
> +		return -EINVAL;
> +
> +	return __admv1013_spi_update_bits(st,
> ADMV1013_REG_LO_AMP_I,
> +				 ADMV1013_MIXER_VGATE_MSK,
> +
> FIELD_PREP(ADMV1013_MIXER_VGATE_MSK, mixer_vgate));
> +}
> +
> +static int admv1013_reg_access(struct iio_dev *indio_dev,
> +			       unsigned int reg,
> +			       unsigned int write_val,
> +			       unsigned int *read_val)
> +{
> +	struct admv1013_state *st = iio_priv(indio_dev);
> +
> +	if (read_val)
> +		return admv1013_spi_read(st, reg, read_val);
> +	else
> +		return admv1013_spi_write(st, reg, write_val);
> +}
> +
> +static const struct iio_info admv1013_info = {
> +	.read_raw = admv1013_read_raw,
> +	.write_raw = admv1013_write_raw,
> +	.debugfs_reg_access = &admv1013_reg_access,
> +};
> +
> +static int admv1013_freq_change(struct notifier_block *nb, unsigned
> long action, void *data)
> +{
> +	struct admv1013_state *st = container_of(nb, struct
> admv1013_state, nb);
> +	int ret;
> +
> +	if (action == POST_RATE_CHANGE) {
> +		mutex_lock(&st->lock);
> +		ret =
> notifier_from_errno(admv1013_update_quad_filters(st));
> +		mutex_unlock(&st->lock);
> +		return ret;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +#define ADMV1013_CHAN_PHASE(_channel, rf_comp) {		\
> +	.type = IIO_ALTVOLTAGE,					\
> +	.modified = 1,						\
> +	.output = 1,						\
> +	.indexed = 1,						\
> +	.channel2 = IIO_MOD_##rf_comp,				\
> +	.channel = _channel,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE)		\
> +	}
> +
> +#define ADMV1013_CHAN_CALIB(_channel, rf_comp) {		\
> +	.type = IIO_ALTVOLTAGE,					\
> +	.modified = 1,						\
> +	.output = 1,						\
> +	.indexed = 1,						\
> +	.channel2 = IIO_MOD_##rf_comp,				\
> +	.channel = _channel,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBBIAS)	\
> +	}
> +
> +static const struct iio_chan_spec admv1013_channels[] = {
> +	ADMV1013_CHAN_PHASE(0, I),
> +	ADMV1013_CHAN_PHASE(0, Q),
> +	ADMV1013_CHAN_CALIB(0, I),
> +	ADMV1013_CHAN_CALIB(0, Q),
> +	ADMV1013_CHAN_CALIB(1, I),
> +	ADMV1013_CHAN_CALIB(1, Q),
> +};
> +

Hmm, If I'm not missing nothing this leads to something like:

out_altvoltage0_i_phase
out_altvoltage0_q_phase
out_altvoltage0_i_calibbias
out_altvoltage0_q_calibbias
out_altvoltage1_i_calibbias
out_altvoltage1_q_calibbias

To me it is really non obvious that index 1 also applies to the same
channel. I see that we have this like this probably because we
can't use modified and differential at the same time, right?

Jonathan, I'm not sure what should be the done here... From the top of my
head  I guess we can either:

* drop the modifiers (not perfect but also not that bad IMO),
* tweak something adding extended info (not really neat)
* or handling this in the core with a new variable. Of course, we would need
to be careful to not break existing drivers...

Not sure if labels could also be something to use... Any suggestion from your
side?

- Nuno Sá

Powered by blists - more mailing lists