Message-ID: <3agb73fmwhcoho4uowhwh3tchux5wb5amgzrmr2fj66uiw4grg@oddcbaeqmneu>
Date: Mon, 22 Dec 2025 09:45:30 +0000
From: Rodrigo Alencar <455.rodrigo.alencar@...il.com>
To: Jonathan Cameron <jic23@...nel.org>, 
	Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@...nel.org>
Cc: rodrigo.alencar@...log.com, linux-kernel@...r.kernel.org, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org, linux-doc@...r.kernel.org, 
	David Lechner <dlechner@...libre.com>, Andy Shevchenko <andy@...nel.org>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH v2 2/6] iio: frequency: adf41513: driver implementation

On 25/12/21 05:49PM, Jonathan Cameron wrote:
> On Fri, 19 Dec 2025 12:34:49 +0000
> Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@...nel.org> wrote:
> 
> > From: Rodrigo Alencar <rodrigo.alencar@...log.com>
> > 
> > The driver is based on existing PLL drivers in the IIO subsystem and
> > implements the following key features:
> > 
> > - Integer-N and fractional-N (fixed/variable modulus) synthesis modes
> > - High-resolution frequency calculations using microhertz (µHz) precision
> >   to handle sub-Hz resolution across multi-GHz frequency ranges
> > - IIO debugfs interface for direct register access
> > - FW property parsing from devicetree including charge pump settings,
> >   reference path configuration and muxout options
> > - Power management support with suspend/resume callbacks
> > - Lock detect GPIO monitoring
> > 
> > The driver uses 64-bit microhertz values throughout PLL calculations to
> > maintain precision when working with frequencies that exceed 32-bit Hz
> > representation while requiring fractional Hz resolution.
> > 
> > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@...log.com>
> Hi Rodrigo,
> 
> Various comments inline.
> 
> Thanks,
> 
> Jonathan

Hi Jonathan,

valuable review, thanks!

kind regards,

Rodrigo Alencar
 
> > diff --git a/drivers/iio/frequency/adf41513.c b/drivers/iio/frequency/adf41513.c
> > new file mode 100644
> > index 000000000000..a967dc4479e7
> > --- /dev/null
> > +++ b/drivers/iio/frequency/adf41513.c
> 
> 
> > +
> > +static int adf41513_sync_config(struct adf41513_state *st, u16 sync_mask)
> > +{
> > +	int ret;
> > +	int i;
> > +
> > +	/* write registers in reverse order (R13 to R0)*/
> > +	for (i = ADF41513_REG13; i >= ADF41513_REG0; i--) {
> > +		if (st->regs_hw[i] != st->regs[i] || sync_mask & BIT(i)) {
> For code cases like this where you want to only do something if a condition is matched
> it can be neater to invert the condition.  e.g.
> 
> 		if (st->regs_hw[i] == st->regs[i] && !(sync_mask & BIT(i)))
> 			continue;
> 
> 		st->buf = cpu...
> 
> > +			st->buf = cpu_to_be32(st->regs[i] | i);
> > +			ret = spi_write(st->spi, &st->buf, sizeof(st->buf));
> > +			if (ret < 0)
> > +				return ret;
> > +			st->regs_hw[i] = st->regs[i];
> > +			dev_dbg(&st->spi->dev, "REG%d <= 0x%08X\n", i, st->regs[i] | i);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static u64 adf41513_pll_get_rate(struct adf41513_state *st)
> > +{
> > +	struct adf41513_pll_settings *cfg = &st->settings;
> > +
> > +	if (cfg->mode != ADF41513_MODE_INVALID)
> > +		return cfg->actual_frequency_uhz;
> > +
> > +	/* get pll settings from regs_hw */
> > +	cfg->int_value = FIELD_GET(ADF41513_REG0_INT_MSK,
> > +				   st->regs_hw[ADF41513_REG0]);
> > +	cfg->frac1 = FIELD_GET(ADF41513_REG1_FRAC1_MSK,
> > +			       st->regs_hw[ADF41513_REG1]);
> > +	cfg->frac2 = FIELD_GET(ADF41513_REG3_FRAC2_MSK,
> > +			       st->regs_hw[ADF41513_REG3]);
> > +	cfg->mod2 = FIELD_GET(ADF41513_REG4_MOD2_MSK,
> > +			      st->regs_hw[ADF41513_REG4]);
> > +	cfg->r_counter = FIELD_GET(ADF41513_REG5_R_CNT_MSK,
> > +				   st->regs_hw[ADF41513_REG5]);
> > +	cfg->ref_doubler = FIELD_GET(ADF41513_REG5_REF_DOUBLER_MSK,
> > +				     st->regs_hw[ADF41513_REG5]);
> > +	cfg->ref_div2 = FIELD_GET(ADF41513_REG5_RDIV2_MSK,
> > +				  st->regs_hw[ADF41513_REG5]);
> > +	cfg->prescaler = FIELD_GET(ADF41513_REG5_PRESCALER_MSK,
> > +				   st->regs_hw[ADF41513_REG5]);
> For cases like this I think keeping to 80 chars is hurting readability
> and so it is fine to go a little higher. 
> 	cfg->int_value = FIELD_GET(ADF41513_REG0_INT_MSK, st->regs_hw[ADF41513_REG0]);
> 	cfg->frac1 = FIELD_GET(ADF41513_REG1_FRAC1_MSK, st->regs_hw[ADF41513_REG1]);
> 	cfg->frac2 = FIELD_GET(ADF41513_REG3_FRAC2_MSK, st->regs_hw[ADF41513_REG3]);
> 	cfg->mod2 = FIELD_GET(ADF41513_REG4_MOD2_MSK, st->regs_hw[ADF41513_REG4]);
> 	cfg->r_counter = FIELD_GET(ADF41513_REG5_R_CNT_MSK, st->regs_hw[ADF41513_REG5]);
> 	cfg->ref_doubler = FIELD_GET(ADF41513_REG5_REF_DOUBLER_MSK, st->regs_hw[ADF41513_REG5]);
> 	cfg->ref_div2 = FIELD_GET(ADF41513_REG5_RDIV2_MSK, st->regs_hw[ADF41513_REG5]);
> 	cfg->prescaler = FIELD_GET(ADF41513_REG5_PRESCALER_MSK,st->regs_hw[ADF41513_REG5]);
> Is fine here. I'd also be fine with wrapping the ref_doubler line as it's rather
> longer than the others.

ack
 
> > +
> > +	/* calculate pfd frequency */
> > +	cfg->pfd_frequency_uhz = st->ref_freq_hz * MICROHZ_PER_HZ;
> > +	if (cfg->ref_doubler)
> > +		cfg->pfd_frequency_uhz <<= 1;
> > +	if (cfg->ref_div2)
> > +		cfg->pfd_frequency_uhz >>= 1;
> > +	cfg->pfd_frequency_uhz = div_u64(cfg->pfd_frequency_uhz,
> > +					 cfg->r_counter);
> > +	cfg->actual_frequency_uhz = (u64)cfg->int_value * cfg->pfd_frequency_uhz;
> > +
> > +	/* check if int mode is selected */
> > +	if (FIELD_GET(ADF41513_REG6_INT_MODE_MSK, st->regs_hw[ADF41513_REG6])) {
> > +		cfg->mode = ADF41513_MODE_INTEGER_N;
> > +	} else {
> > +		cfg->actual_frequency_uhz += mul_u64_u64_div_u64(cfg->frac1,
> > +								 cfg->pfd_frequency_uhz,
> > +								 ADF41513_FIXED_MODULUS);
> > +
> > +		/* check if variable modulus is selected */
> > +		if (FIELD_GET(ADF41513_REG0_VAR_MOD_MSK, st->regs_hw[ADF41513_REG0])) {
> > +			cfg->actual_frequency_uhz +=
> > +				mul_u64_u64_div_u64(cfg->frac2,
> > +						    cfg->pfd_frequency_uhz,
> > +						    ADF41513_FIXED_MODULUS * cfg->mod2);
> > +
> > +			cfg->mode = ADF41513_MODE_VARIABLE_MODULUS;
> > +		} else {
> > +			/* LSB_P1 offset */
> > +			if (!FIELD_GET(ADF41513_REG5_LSB_P1_MSK, st->regs_hw[ADF41513_REG5]))
> > +				cfg->actual_frequency_uhz +=
> > +					div_u64(cfg->pfd_frequency_uhz,
> > +						ADF41513_FIXED_MODULUS * 2);
> > +			cfg->mode = ADF41513_MODE_FIXED_MODULUS;
> > +		}
> > +	}
> > +
> > +	cfg->target_frequency_uhz = cfg->actual_frequency_uhz;
> > +
> > +	return cfg->actual_frequency_uhz;
> > +}
> 
> 
> > +static int adf41513_calc_pll_settings(struct adf41513_state *st,
> > +				      struct adf41513_pll_settings *result,
> > +				      u64 rf_out_uhz)
> > +{
> > +	u64 max_rf_freq_uhz = st->chip_info->max_rf_freq_hz * MICROHZ_PER_HZ;
> > +	u64 min_rf_freq_uhz = ADF41513_MIN_RF_FREQ * MICROHZ_PER_HZ;
> > +	u64 pfd_freq_limit_uhz;
> > +	int ret;
> > +
> > +	/* input validation */
> 
> That's obvious.  No need to have the comment.

ack

> > +	if (rf_out_uhz < min_rf_freq_uhz || rf_out_uhz > max_rf_freq_uhz) {
> > +		dev_err(&st->spi->dev, "RF frequency %llu uHz out of range [%llu, %llu] uHz\n",
> > +			rf_out_uhz, min_rf_freq_uhz, max_rf_freq_uhz);
> > +		return -EINVAL;
> > +	}
> > +
> > +	result->target_frequency_uhz = rf_out_uhz;
> > +
> > +	/* try integer-N first (best phase noise performance) */
> > +	pfd_freq_limit_uhz = min(div_u64(rf_out_uhz, ADF41513_MIN_INT_4_5),
> > +				 ADF41513_MAX_PFD_FREQ_INT_N_UHZ);
> > +	ret = adf41513_calc_pfd_frequency(st, result, pfd_freq_limit_uhz);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = adf41513_calc_integer_n(st, result);
> > +	if (ret < 0) {
> > +		/* try fractional-N: recompute pfd frequency if necessary */
> > +		pfd_freq_limit_uhz = min(div_u64(rf_out_uhz, ADF41513_MIN_INT_FRAC_4_5),
> > +					 ADF41513_MAX_PFD_FREQ_FRAC_N_UHZ);
> > +		if (pfd_freq_limit_uhz < result->pfd_frequency_uhz) {
> > +			ret = adf41513_calc_pfd_frequency(st, result, pfd_freq_limit_uhz);
> > +			if (ret < 0)
> > +				return ret;
> > +		}
> > +
> > +		/* fixed-modulus attempt */
> > +		ret = adf41513_calc_fixed_mod(st, result);
> > +		if (ret < 0) {
> > +			/* variable-modulus attempt */
> > +			ret = adf41513_calc_variable_mod(st, result);
> > +			if (ret < 0) {
> > +				dev_err(&st->spi->dev,
> > +					"no valid PLL configuration found for %llu uHz\n",
> > +					rf_out_uhz);
> > +				return -EINVAL;
> > +			}
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int adf41513_set_frequency(struct adf41513_state *st, u64 freq_uhz, u16 sync_mask)
> > +{
> > +	struct adf41513_pll_settings result;
> > +	int ret;
> > +
> > +	/* calculate pll settings candidate */
> > +	ret = adf41513_calc_pll_settings(st, &result, freq_uhz);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* apply computed results to pll settings */
> > +	memcpy(&st->settings, &result, sizeof(struct adf41513_pll_settings));
> 
> sizeof(st->settings)
> 
> > +
> > +	dev_dbg(&st->spi->dev,
> > +		"%s mode: int=%u, frac1=%u, frac2=%u, mod2=%u, fpdf=%llu Hz, prescaler=%s\n",
> > +		(result.mode == ADF41513_MODE_INTEGER_N) ? "integer-n" :
> > +		(result.mode == ADF41513_MODE_FIXED_MODULUS) ? "fixed-modulus" : "variable-modulus",
> > +		result.int_value, result.frac1, result.frac2, result.mod2,
> > +		div64_u64(result.pfd_frequency_uhz, MICROHZ_PER_HZ),
> > +		result.prescaler ? "8/9" : "4/5");
> > +
> > +	/* int */
> > +	st->regs[ADF41513_REG0] = FIELD_PREP(ADF41513_REG0_INT_MSK,
> > +					     st->settings.int_value);
> > +	if (st->settings.mode == ADF41513_MODE_VARIABLE_MODULUS)
> > +		st->regs[ADF41513_REG0] |= ADF41513_REG0_VAR_MOD_MSK;
> > +	/* frac1 */
> > +	st->regs[ADF41513_REG1] = FIELD_PREP(ADF41513_REG1_FRAC1_MSK,
> > +					     st->settings.frac1);
> > +	if (st->settings.mode != ADF41513_MODE_INTEGER_N)
> > +		st->regs[ADF41513_REG1] |= ADF41513_REG1_DITHER2_MSK;
> > +
> > +	/* frac2 */
> 
> Where the field name makes it obvious there is little point in
> adding a comment to say the same thing. I'd clear out most if not all
> of these. Stick to comments that add significant value.

ack

> > +	st->regs[ADF41513_REG3] = FIELD_PREP(ADF41513_REG3_FRAC2_MSK,
> > +					     st->settings.frac2);
> > +	/* mod2 */
> > +	st->regs[ADF41513_REG4] &= ADF41513_REG4_MOD2_MSK;
> > +	st->regs[ADF41513_REG4] |= FIELD_PREP(ADF41513_REG4_MOD2_MSK,
> > +					      st->settings.mod2);
> > +
> > +	/* r-cnt | doubler | rdiv2 | prescaler */
> > +	st->regs[ADF41513_REG5] &= ~(ADF41513_REG5_R_CNT_MSK |
> > +				     ADF41513_REG5_REF_DOUBLER_MSK |
> > +				     ADF41513_REG5_RDIV2_MSK |
> > +				     ADF41513_REG5_PRESCALER_MSK);
> > +	st->regs[ADF41513_REG5] |= FIELD_PREP(ADF41513_REG5_R_CNT_MSK,
> > +					      st->settings.r_counter);
> > +	st->regs[ADF41513_REG5] |= FIELD_PREP(ADF41513_REG5_REF_DOUBLER_MSK,
> > +					      st->settings.ref_doubler);
> > +	st->regs[ADF41513_REG5] |= FIELD_PREP(ADF41513_REG5_RDIV2_MSK,
> > +					      st->settings.ref_div2);
> > +	st->regs[ADF41513_REG5] |= FIELD_PREP(ADF41513_REG5_PRESCALER_MSK,
> > +					      st->settings.prescaler);
> 
> Probably better to use FIELD_MODIFY for all of these and let the compiler
> figure out it can mask them all in one go.

ack

> > +
> > +	if (st->settings.mode == ADF41513_MODE_INTEGER_N) {
> > +		st->regs[ADF41513_REG6] |= ADF41513_REG6_INT_MODE_MSK;
> > +		st->regs[ADF41513_REG6] &= ~ADF41513_REG6_BLEED_ENABLE_MSK;
> > +	} else {
> > +		st->regs[ADF41513_REG6] &= ~ADF41513_REG6_INT_MODE_MSK;
> > +		st->regs[ADF41513_REG6] |= ADF41513_REG6_BLEED_ENABLE_MSK;
> > +	}
> > +
> > +	return adf41513_sync_config(st, sync_mask | ADF41513_SYNC_REG0);
> > +}
> 
> > +static ssize_t adf41513_read_uhz(struct iio_dev *indio_dev,
> > +				 uintptr_t private,
> > +				 const struct iio_chan_spec *chan,
> > +				 char *buf)
> > +{
> > +	struct adf41513_state *st = iio_priv(indio_dev);
> > +	u64 freq_uhz;
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	switch ((u32)private) {
> > +	case ADF41513_FREQ:
> > +		freq_uhz = adf41513_pll_get_rate(st);
> > +		if (st->lock_detect)
> > +			if (!gpiod_get_value_cansleep(st->lock_detect)) {
> > +				dev_dbg(&st->spi->dev, "PLL un-locked\n");
> > +				return -EBUSY;
> > +			}
> > +		break;
> > +	case ADF41513_FREQ_RESOLUTION:
> > +		freq_uhz = st->data.freq_resolution_uhz;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return adf41513_uhz_to_str(freq_uhz, buf);
> This is a more marginal case than the ones below wrt to a common
> function making sense as there is more overlap. I''m not sure it
> is worth doing even so (rather than separate callbacks).

ack. will split them.

> > +}
> > +
> > +static ssize_t adf41513_read(struct iio_dev *indio_dev,
> > +			     uintptr_t private,
> > +			     const struct iio_chan_spec *chan,
> > +			     char *buf)
> > +{
> > +	struct adf41513_state *st = iio_priv(indio_dev);
> > +	u32 val;
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	switch ((u32)private) {
> > +	case ADF41513_FREQ_REFIN:
> > +		st->ref_freq_hz = clk_get_rate(st->ref_clk);
> > +		return sysfs_emit(buf, "%llu\n", st->ref_freq_hz);
> 
> Not much sharing here either (see below). I'd be tempted to just spit this
> into specific callbacks.

ack.

> > +	case ADF41513_POWER_DOWN:
> > +		val = FIELD_GET(ADF41513_REG6_POWER_DOWN_MSK,
> > +				st->regs_hw[ADF41513_REG6]);
> > +		return sysfs_emit(buf, "%u\n", val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> 
> > +
> > +static ssize_t adf41513_write(struct iio_dev *indio_dev,
> > +			      uintptr_t private,
> > +			      const struct iio_chan_spec *chan,
> > +			      const char *buf, size_t len)
> > +{
> > +	struct adf41513_state *st = iio_priv(indio_dev);
> > +	unsigned long readin, tmp;
> > +	int ret;
> > +
> > +	ret = kstrtoul(buf, 10, &readin);
> > +	if (ret)
> > +		return ret;
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	switch ((u32)private) {
> > +	case ADF41513_FREQ_REFIN:
> 
> There isn't a lot of shared code between different calls of this.
> Perhaps just have separate callbacks for each one.

ack.

> > +		if (readin < ADF41513_MIN_REF_FREQ || readin > ADF41513_MAX_REF_FREQ)
> > +			return -EINVAL;
> > +
> > +		tmp = clk_round_rate(st->ref_clk, readin);
> > +		if (tmp != readin)
> > +			return -EINVAL;
> > +
> > +		ret = clk_set_rate(st->ref_clk, tmp);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		st->ref_freq_hz = readin;
> > +		ret = adf41513_set_frequency(st, st->settings.target_frequency_uhz,
> > +					     ADF41513_SYNC_DIFF);
> > +		break;
> > +	case ADF41513_POWER_DOWN:
> > +		if (readin)
> > +			ret = adf41513_suspend(st);
> > +		else
> > +			ret = adf41513_resume(st);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return ret ? ret : len;
> > +}
> > +
> > +#define _ADF41513_EXT_INFO(_name, _ident) { \
> > +	.name = _name, \
> > +	.read = adf41513_read, \
> > +	.write = adf41513_write, \
> > +	.private = _ident, \
> > +	.shared = IIO_SEPARATE, \
> > +}
> > +
> > +#define _ADF41513_EXT_UHZ_INFO(_name, _ident) { \
> > +	.name = _name, \
> > +	.read = adf41513_read_uhz, \
> > +	.write = adf41513_write_uhz, \
> > +	.private = _ident, \
> > +	.shared = IIO_SEPARATE, \
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info adf41513_ext_info[] = {
> > +	/*
> > +	 * Ideally we would use IIO_CHAN_INFO_FREQUENCY, but the device supports
> > +	 * frequency values greater 2^32 with sub-Hz resolution, i.e. 64-bit
> > +	 * fixed point with 6 decimal places values are used to represent
> > +	 * frequencies.
> > +	 */
> > +	_ADF41513_EXT_UHZ_INFO("frequency", ADF41513_FREQ),
> > +	_ADF41513_EXT_UHZ_INFO("frequency_resolution", ADF41513_FREQ_RESOLUTION),
> > +	_ADF41513_EXT_INFO("refin_frequency", ADF41513_FREQ_REFIN),
> Some of these are not things I recall as being standard ABI.
> This one is in one other driver but to make it generic you need to promote
> the ABI documentation to a shared file.

ack. will create the shared ABI file.

> > +	_ADF41513_EXT_INFO("powerdown", ADF41513_POWER_DOWN),
> > +	{ },
> 
> No comma on terminating entries like this.

ack

> > +};
> > +
> > +static const struct iio_chan_spec adf41513_chan = {
> > +	.type = IIO_ALTVOLTAGE,
> > +	.indexed = 1,
> > +	.output = 1,
> > +	.channel = 0,
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE),
> > +	.ext_info = adf41513_ext_info,
> > +};
> > +
> > +static int adf41513_read_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int *val, int *val2, long info)
> > +{
> > +	struct adf41513_state *st = iio_priv(indio_dev);
> > +	u32 phase_mdeg;
> > +	u16 phase_val;
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_PHASE:
> > +		phase_val = FIELD_GET(ADF41513_REG2_PHASE_VAL_MSK,
> > +				      st->regs_hw[ADF41513_REG2]);
> > +		phase_mdeg = DIV_ROUND_CLOSEST(360 * MILLI * phase_val, BIT(12));
> > +		*val = phase_mdeg / MILLI;
> > +		*val2 = (phase_mdeg % MILLI) * 1000;
> 
> This sounds like it is in degrees. Note _phase attributes are in the documented
> ABI Documentation/ABI/testing/sysfs-bus-iio as in radians.

ack. will use radians conversion.

> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int adf41513_write_raw(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan,
> > +			      int val, int val2, long info)
> > +{
> > +	struct adf41513_state *st = iio_priv(indio_dev);
> > +	u32 phase_mdeg;
> > +	u16 phase_val;
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_PHASE:
> > +		val %= 360;
> > +		if (val < 0)
> > +			val += 360;
> > +		phase_mdeg = val * MILLI + val2 / 1000;
> > +		phase_val = DIV_ROUND_CLOSEST(phase_mdeg << 12, 360 * MILLI);
> > +
> > +		st->regs[ADF41513_REG2] |= ADF41513_REG2_PHASE_ADJ_MSK;
> > +		st->regs[ADF41513_REG2] &= ~ADF41513_REG2_PHASE_VAL_MSK;
> 
> FIELD_MODIFY() can save doing the clear and fill as separate calls.

ack.

> > +		st->regs[ADF41513_REG2] |= FIELD_PREP(ADF41513_REG2_PHASE_VAL_MSK, phase_val);
> > +		return adf41513_sync_config(st, ADF41513_SYNC_REG0);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> 
> 
> > +static int adf41513_parse_fw(struct adf41513_state *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	int ret;
> > +	u32 tmp;
> > +	u32 cp_resistance;
> > +	u32 cp_current;
> Where you have set of variables of same type and grouping doesn't hurt
> readability, declare them all on one line.
> 
> 	u32 tmp, cp_resistance, cp_current;

ack

> I'll not repeat comments I made on the dt-binding in here but I'd expect
> this code to change somewhat in response to those.

understood. for now, will use -mhz for the power-up frequency, but I will
see how the discussion follows.

> > +
> 
> ...
> 
> 
> > +static int adf41513_setup(struct adf41513_state *st)
> > +{
> > +	u32 tmp;
> > +
> > +	memset(st->regs_hw, 0xFF, sizeof(st->regs_hw));
> > +
> > +	/* assume DLD pin is used for digital lock detect */
> > +	st->regs[ADF41513_REG5] = FIELD_PREP(ADF41513_REG5_DLD_MODES_MSK,
> > +					     ADF41513_DLD_DIG_LD);
> > +
> > +	/* configure charge pump current settings */
> > +	tmp = DIV_ROUND_CLOSEST(st->data.charge_pump_voltage_mv, ADF41513_MIN_CP_VOLTAGE_mV);
> > +	st->regs[ADF41513_REG5] |= FIELD_PREP(ADF41513_REG5_CP_CURRENT_MSK, tmp - 1);
> > +
> > +	/* narrow ABP | loss of lock detect enable | SD reset | LDP from data */
> 
> I'm not sure what LPD from data means. Can't correlate that with the datasheet.
> Perhaps add more info or reword.

will remove, this is not applicable anymore.

> > +	st->regs[ADF41513_REG6] = ADF41513_REG6_ABP_MSK |
> > +				  ADF41513_REG6_LOL_ENABLE_MSK |
> > +				  ADF41513_REG6_SD_RESET_MSK;
> > +	if (st->data.phase_detector_polarity)
> > +		st->regs[ADF41513_REG6] |= ADF41513_REG6_PD_POLARITY_MSK;
> > +
> > +	/* PS bias | lock detect count */
> Confusing comment as covering multiple bits of code. I'd just drop
> it on basis the field names in the code are less confusing than the comment.

ack. will drop.

> > +	st->regs[ADF41513_REG7] = FIELD_PREP(ADF41513_REG7_PS_BIAS_MSK, 2);
> 
> That magic 2 is interesting as it is truely magic with no explanation on
> the datasheet beyond 'program this value'. Even better, on the datasheet I'm
> looking at the Prescaler (PS) bias section says set it to 3 and figure 30
> say set it to 2.  Maybe add a commeon on this.

understood. 2 should be the correct vallue, but I will ask around.

> > +	tmp = ilog2(st->data.lock_detect_count);
> > +	if (st->data.lock_detect_count < ADF41513_LD_COUNT_FAST_LIMIT) {
> > +		tmp -= const_ilog2(ADF41513_LD_COUNT_FAST_MIN);
> > +		st->regs[ADF41513_REG7] |= ADF41513_REG7_LD_CLK_SEL_MSK;
> > +	} else {
> > +		tmp -= const_ilog2(ADF41513_LD_COUNT_MIN);
> > +	}
> > +	st->regs[ADF41513_REG7] |= FIELD_PREP(ADF41513_REG7_LD_COUNT_MSK, tmp);
> > +
> > +	/* power down select */
> > +	st->regs[ADF41513_REG11] = ADF41513_REG11_POWER_DOWN_SEL_MSK;
> > +
> > +	/* muxout */
> > +	st->regs[ADF41513_REG12] = FIELD_PREP(ADF41513_REG12_MUXOUT_MSK,
> > +					      st->data.muxout_select);
> > +	st->regs[ADF41513_REG12] |= FIELD_PREP(ADF41513_REG12_LOGIC_LEVEL_MSK,
> > +					       st->data.muxout_1v8_en ? 0 : 1);
> > +
> > +	/* perform initialization sequence with power-up frequency */
> > +	return adf41513_set_frequency(st, (u64)st->data.power_up_frequency_hz * MICROHZ_PER_HZ,
> > +				      ADF41513_SYNC_ALL);
> > +}
> 
> ...
> 
> > +
> > +static int adf41513_pm_suspend(struct device *dev)
> > +{
> > +	struct adf41513_state *st = dev_get_drvdata(dev);
> > +
> > +	return adf41513_suspend(st);
> 
> Not at lot in point in the local variable
> 
> 	return adf41513_suspend(dev_get_drvdata(dev));

ack

> > +}
> > +
> > +static int adf41513_pm_resume(struct device *dev)
> > +{
> > +	struct adf41513_state *st = dev_get_drvdata(dev);
> > +
> > +	return adf41513_resume(st);
> As above.
> > +}
> > +
> > +static const struct adf41513_chip_info adf41513_chip_info = {
> > +	.name = "adf41513",
> > +	.has_prescaler_8_9 = true,
> > +	.max_rf_freq_hz = ADF41513_MAX_RF_FREQ,
> > +};
> > +
> > +static const struct adf41513_chip_info adf41510_chip_info = {
> 
> Just for long term organization when many devices are supported:
> keep these structures in alphanumeric order.

ack

> > +	.name = "adf41510",
> > +	.has_prescaler_8_9 = false,
> > +	.max_rf_freq_hz = ADF41510_MAX_RF_FREQ,
> > +};
> > +
> > +static int adf41513_probe(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct adf41513_state *st;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> 
> I'd use a 
> 	struct device *dev = &spi->dev;
> so that you can shorten all the lines where spi->dev is used in here.

ack

> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +	st->spi = spi;
> > +	st->chip_info = spi_get_device_match_data(spi);
> > +	if (!st->chip_info)
> > +		return -EINVAL;
> > +
> > +	spi_set_drvdata(spi, st);
> > +
> > +	st->ref_clk = devm_clk_get_enabled(&spi->dev, NULL);
> > +	if (IS_ERR(st->ref_clk))
> > +		return PTR_ERR(st->ref_clk);
> > +
> > +	st->ref_freq_hz = clk_get_rate(st->ref_clk);
> > +	if (st->ref_freq_hz < ADF41513_MIN_REF_FREQ || st->ref_freq_hz > ADF41513_MAX_REF_FREQ)
> > +		return dev_err_probe(&spi->dev, -ERANGE,
> > +				     "reference frequency %llu Hz out of range\n",
> > +				     st->ref_freq_hz);
> > +
> > +	ret = adf41513_parse_fw(st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_regulator_bulk_get_enable(&spi->dev,
> > +					     ARRAY_SIZE(adf41513_power_supplies),
> > +					     adf41513_power_supplies);
> > +	if (ret)
> > +		return dev_err_probe(&spi->dev, ret,
> > +				     "failed to get and enable regulators\n");
> > +
> > +	st->chip_enable = devm_gpiod_get_optional(&spi->dev, "enable", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(st->chip_enable))
> > +		return dev_err_probe(&spi->dev, PTR_ERR(st->chip_enable),
> > +				     "fail to request chip enable GPIO\n");
> > +
> > +	st->lock_detect = devm_gpiod_get_optional(&spi->dev, "lock-detect", GPIOD_IN);
> > +	if (IS_ERR(st->lock_detect))
> > +		return dev_err_probe(&spi->dev, PTR_ERR(st->lock_detect),
> > +				     "fail to request lock detect GPIO\n");
> > +
> > +	ret = devm_mutex_init(&spi->dev, &st->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->name = st->chip_info->name;
> > +	indio_dev->info = &adf41513_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->channels = &adf41513_chan;
> > +	indio_dev->num_channels = 1;
> > +
> > +	ret = adf41513_setup(st);
> > +	if (ret < 0)
> > +		return dev_err_probe(&spi->dev, ret, "failed to setup device: %d\n", ret);
> Look at what dev_err_probe() prints.  (short answer, it includes a much nicer print
> of the error value than the one you have here).  So dev_err_probe() should never
> include the error value itself as that is duplicating the info.

yes, that looks bad. will adjust.

> > +
> > +	ret = devm_add_action_or_reset(&spi->dev, adf41513_power_down, st);
> > +	if (ret)
> > +		return dev_err_probe(&spi->dev, ret, "Failed to add power down action: %d\n", ret);
> 
> As above.  No printing ret by hand in dev_err_probe() calls.

ack

> > +
> > +	return devm_iio_device_register(&spi->dev, indio_dev);
> > +}

Powered by blists - more mailing lists