Hello Jonathan,

-----Original Message-----
From: Jonathan Cameron <jic23@kernel.org>
Sent: Saturday, July 17, 2021 4:26 PM
To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org
Subject: Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for
ADRF6780

[External]

On Fri, 16 Jul 2021 14:42:09 +0300
Antoniu Miclaus [off-list ref] wrote:

quoted

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

quoted

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

Hi Antoniu

As it looks like we'll have a v7, a few trivial comments from me.

...

quoted

+
+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)
+		return ret;
+
+	ret = adrf6780_spi_read(dev, ADRF6780_REG_CONTROL, &chip_id);
+	if (ret)
+		return ret;
+
+	chip_id = FIELD_GET(ADRF6780_CHIP_ID_MSK, chip_id);
+	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 = FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, dev-
vga_buff_en) |
+			FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, 1) |
+			FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, dev-
lo_buff_en) |
+			FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, dev-
if_mode_en) |
+			FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, dev-
iq_mode_en) |
+			FIELD_PREP(ADRF6780_LO_X2_EN_MSK, dev-
lo_x2_en) |
+			FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, dev-
lo_ppf_en) |
+			FIELD_PREP(ADRF6780_LO_EN_MSK, dev->lo_en) |
+			FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, dev-
uc_bias_en);

Here we are probably turning on a bunch of stuff which will result in power
usage.
Would it be sensible to turn it off again in remove path?  (devm managed
should be fine).

Most of these attributes are enabled by default after device reset.
Taking into account this statement, is it still worth adding a 'custom' remove path?

quoted

+
+	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ENABLE,

enable_reg_msk, enable_reg);

quoted

+	if (ret)
+		return ret;
+
+	ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
+

	ADRF6780_LO_SIDEBAND_MSK,

quoted

+

	FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, dev->lo_sideband));

quoted

+	if (ret)
+		return ret;
+
+	return adrf6780_spi_update_bits(dev,

ADRF6780_REG_ADC_CONTROL,

quoted

+

	ADRF6780_VDET_OUTPUT_SELECT_MSK,

quoted

+

	FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, dev-

quoted

vdet_out_en));
+}
+

quoted

+static void adrf6780_dt_parse(struct adrf6780_dev *dev)

Trivial nitpick, but given this is all device property based (great)
the dt naming is now misleading.  Perhaps _properties_parse() ?

quoted

+{
+	struct spi_device *spi = dev->spi;
+
+	dev->vga_buff_en = device_property_read_bool(&spi->dev,

"adi,vga-buff-en");

quoted

+	dev->lo_buff_en = device_property_read_bool(&spi->dev, "adi,lo-

buff-en");

quoted

+	dev->if_mode_en = device_property_read_bool(&spi->dev, "adi,if-

mode-en");

quoted

+	dev->iq_mode_en = device_property_read_bool(&spi->dev, "adi,iq-

mode-en");

quoted

+	dev->lo_x2_en = device_property_read_bool(&spi->dev, "adi,lo-x2-

en");

quoted

+	dev->lo_ppf_en = device_property_read_bool(&spi->dev, "adi,lo-

ppf-en");

quoted

+	dev->lo_en = device_property_read_bool(&spi->dev, "adi,lo-en");
+	dev->uc_bias_en = device_property_read_bool(&spi->dev, "adi,uc-

bias-en");

quoted

+	dev->lo_sideband = device_property_read_bool(&spi->dev, "adi,lo-

sideband");

quoted

+	dev->vdet_out_en = device_property_read_bool(&spi->dev,

"adi,vdet-out-en");

quoted

+}

--
Antoniu Miclăuş