Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 6 Feb 2022 15:22:37 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Andy Shevchenko <andriy.shevchenko@intel.com>
Cc:     Antoniu Miclaus <antoniu.miclaus@analog.com>, robh+dt@kernel.org,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 1/4] iio:frequency:admv1014: add support for ADMV1014
Message-ID: <20220206152237.03f15456@jic23-huawei>
In-Reply-To: <Yf7AjXsRuhFeFTpD@smile.fi.intel.com>
References: <20220131100102.15372-1-antoniu.miclaus@analog.com>
        <Yf7AjXsRuhFeFTpD@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sat, 5 Feb 2022 20:23:09 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Jan 31, 2022 at 12:00:59PM +0200, Antoniu Miclaus wrote:
> > The ADMV1014 is a silicon germanium (SiGe), wideband,
> > microwave downconverter optimized for point to point microwave
> > radio designs operating in the 24 GHz to 44 GHz frequency range.  
> 
> Excellent job!
> A few comments / questions below.
Emails crossed. Given outstanding comments here I've dropped the
series again for now until these are dealt with.

Thanks,

Jonathan

> 
> ...
> 
> > +config ADMV1014
> > +	tristate "Analog Devices ADMV1014 Microwave Downconverter"
> > +	depends on SPI && COMMON_CLK && 64BIT  
> 
> Why 64BIT only?
> 
> ..
> 
> > +enum {
> > +	ADMV1014_IQ_MODE,
> > +	ADMV1014_IF_MODE  
> 
> + Comma.
> 
> > +};
> > +
> > +enum {
> > +	ADMV1014_SE_MODE_POS = 6,
> > +	ADMV1014_SE_MODE_NEG = 9,
> > +	ADMV1014_SE_MODE_DIFF = 12  
> 
> + Comma.
> 
> > +};  
> 
> > +
> > +enum {
> > +	ADMV1014_CALIBSCALE_COARSE,
> > +	ADMV1014_CALIBSCALE_FINE,
> > +};  
> 
> ...
> 
> > +	int ret;
> > +	struct spi_transfer t = {0};  
> 
> {} would suffice.
> Also, can we here and everywhere else use reversed xmas tree order?
> 
> 	struct spi_transfer t = {};
> 	int ret;
> 
> ...
> 
> > +	st->data[0] = ADMV1014_READ | FIELD_PREP(ADMV1014_REG_ADDR_READ_MSK, reg);
> > +	st->data[1] = 0x0;
> > +	st->data[2] = 0x0;  
> 
> 0x0 --> 0
> 
> Or I would rather put it cleaner with
> 
> 	put_unaligned_be16(...);
> 
> (or is it le16?)
> 
> ...
> 
> > +	t.len = 3;  
> 
> sizeof()?
> 
> ...
> 
> > +	put_unaligned_be24(FIELD_PREP(ADMV1014_REG_DATA_MSK, val) |
> > +			   FIELD_PREP(ADMV1014_REG_ADDR_WRITE_MSK, reg), &st->data[0]);  
> 
> Looking at the above I think the proper one will be byte assignment + put_unaligned_be16().
> 
> ...
> 
> > +	return spi_write(st->spi, &st->data[0], 3);  
> 
> sizeof() ?
> 
> ...
> 
> > +	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))  
> 
> What's the point to have 5400 here? Shouldn't be 7000?
> 
> > +		filt_raw = 10;
> > +	else if (rate >= (6600 * HZ_PER_MHZ) && rate <= (9200 * HZ_PER_MHZ))
> > +		filt_raw = 5;
> > +	else
> > +		filt_raw = 0;  
> 
> ...
> 
> > +		vcm_comp = 1050 + (i * 50) + (i / 8 * 50);  
> 
> 		vcm_comp = 1050 + mult_frac(i, 450, 8);
> 		// alternatively mult_frac(i * 50, 9, 8)
> 
> ?
> 
> > +		if (vcm_mv != vcm_comp)
> > +			continue;  
> 
> ...
> 
> > +	ret = kstrtou32(buf, 10, &data);  
> 
> Should be kstrtouint(). Please check all such calls to be in align with the
> type of variables that are used as a receptor.
> 
> > +	if (ret)
> > +		return ret;  
> 
> ...
> 
> > +	switch ((u32)private) {  
> 
> Why casting? Switch-case with castings is quite unusual.
> 
> > +	}  
> 
> ...
> 
> > +	return ret ? ret : len;  
> 
> 	return ret ?: len; ?
> 
> ...
> 
> > +static const char * const admv1014_reg_name[] = {
> > +	 "vcm", "vcc-if-bb", "vcc-vga", "vcc-vva", "vcc-lna-3p3", "vcc-lna-1p5",
> > +	 "vcc-bg", "vcc-quad", "vcc-mixer"  
> 
> 4 or 5 on one line is easier to read and follow the index (if needed).
> Also add a comma to the end.
> 
> > +};  
> 
> ...
> 
> > +#define ADMV1014_CHAN_IQ(_channel, rf_comp) {				\
> > +	.type = IIO_ALTVOLTAGE,						\
> > +	.modified = 1,							\
> > +	.output = 0,							\
> > +	.indexed = 1,							\
> > +	.channel2 = IIO_MOD_##rf_comp,					\
> > +	.channel = _channel,						\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE) |		\
> > +		BIT(IIO_CHAN_INFO_OFFSET),				\
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBSCALE)	\  
> 
> + Comma, same for the rest similar cases below.
> 
> > +	}  
> 
> ...
> 
> > +#define ADMV1014_CHAN_CALIBSCALE(_channel, rf_comp, _admv1014_ext_info) {	\
> > +	.type = IIO_ALTVOLTAGE,							\
> > +	.modified = 1,								\
> > +	.output = 0,								\
> > +	.indexed = 1,								\
> > +	.channel2 = IIO_MOD_##rf_comp,						\
> > +	.channel = _channel,							\
> > +	.ext_info = _admv1014_ext_info						\  
> 
> + Comma.
> 
> > +	}
> > +
> > +static const struct iio_chan_spec admv1014_channels_iq[] = {
> > +	ADMV1014_CHAN_IQ(0, I),
> > +	ADMV1014_CHAN_IQ(0, Q),
> > +	ADMV1014_CHAN_POWER(0)  
> 
> + Comma.
> 
> > +};
> > +
> > +static const struct iio_chan_spec admv1014_channels_if[] = {
> > +	ADMV1014_CHAN_IF(0, I),
> > +	ADMV1014_CHAN_IF(0, Q),
> > +	ADMV1014_CHAN_CALIBSCALE(0, I, admv1014_ext_info),
> > +	ADMV1014_CHAN_CALIBSCALE(0, Q, admv1014_ext_info),
> > +	ADMV1014_CHAN_POWER(0)  
> 
> + Comma.
> 
> > +};  
> 
> ...
> 
> > +	enable_reg = FIELD_PREP(ADMV1014_P1DB_COMPENSATION_MSK, st->p1db_comp ? 3 : 0) |  
> 
> 3 --> GENMASK() ?
> 
> > +		     FIELD_PREP(ADMV1014_IF_AMP_PD_MSK, !(st->input_mode)) |
> > +		     FIELD_PREP(ADMV1014_BB_AMP_PD_MSK, st->input_mode) |
> > +		     FIELD_PREP(ADMV1014_DET_EN_MSK, st->det_en);
> > +
> > +	return __admv1014_spi_update_bits(st, ADMV1014_REG_ENABLE, enable_reg_msk, enable_reg);
> > +}  
> 
> ...
> 
> > +	str = "iq";
> > +	device_property_read_string(&spi->dev, "adi,input-mode", &str);
> > +
> > +	if (!strcmp(str, "iq"))
> > +		st->input_mode = ADMV1014_IQ_MODE;
> > +	else if (!strcmp(str, "if"))
> > +		st->input_mode = ADMV1014_IF_MODE;
> > +	else
> > +		return -EINVAL;  
> 
> Please, put the array of strings near to the enums and use match-string here.
> Something like:
> 
> static const char const *mode_names = { "iq", "if" };
> 
> 	ret = device_property_read_string(&spi->dev, "adi,input-mode", &str);
> 	if (ret)
> 		input_mode = ...
> 	else {
> 		ret = match_string();
> 		if (ret < 0)
> 			return ret;
> 		input_mode = ret;
> 	}
> 
> ...
> 
> > +	str = "diff";
> > +	device_property_read_string(&spi->dev, "adi,quad-se-mode", &str);
> > +
> > +	if (!strcmp(str, "diff"))
> > +		st->quad_se_mode = ADMV1014_SE_MODE_DIFF;
> > +	else if (!strcmp(str, "se-pos"))
> > +		st->quad_se_mode = ADMV1014_SE_MODE_POS;
> > +	else if (!strcmp(str, "se-neg"))
> > +		st->quad_se_mode = ADMV1014_SE_MODE_NEG;
> > +	else
> > +		return -EINVAL;  
> 
> Ditto.
> 
> While in the above case no win in LOCs, here already a little win.
> The most important not that, the possibility to extend without changing the
> code much.
> 
> ...
> 
> > +	st->clkin = devm_clk_get(&spi->dev, "lo_in");  
> 
> Perhaps _optional?
> 
> > +	if (IS_ERR(st->clkin))
> > +		return dev_err_probe(&spi->dev, PTR_ERR(st->clkin),
> > +				     "failed to get the LO input clock\n");  
>