Re: [PATCH 1/3] iio: frequency: adf41513: driver implementation

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

Hi Rodrigo,

A couple of comments inline since this is on the mailing list.
As mentioned in the other thread, we ought to continue the review of this internally.

On 11/10, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> - ADF41513: 1 GHz to 26.5 GHz frequency range
> - ADF41510: 1 GHz to 10 GHz frequency range
> - Integer-N and fractional-N operation modes
> - Ultra-low phase noise (-235 dBc/Hz integer-N, -231 dBc/Hz fractional-N)
> - High maximum PFD frequency (250 MHz integer-N, 125 MHz fractional-N)
> - 25-bit fixed modulus or 49-bit variable modulus fractional modes
> - Programmable charge pump currents with 16x range
> - Digital lock detect functionality
> - Phase resync capability for consistent output phase
> - Clock framework integration for system clock generation
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> ---
...
> +
> +static int adf41513_parse_uhz(const char *str, u64 *freq_uhz)
> +{
> +	u64 uhz = 0;
> +	int f_count = ADF41513_HZ_DECIMAL_PRECISION;
> +	bool frac_part = false;
> +
> +	if (str[0] == '+')
> +		str++;
> +
> +	while (*str && f_count > 0) {
> +		if ('0' <= *str && *str <= '9') {
> +			uhz = uhz * 10 + *str - '0';
> +			if (frac_part)
> +				f_count--;
> +		} else if (*str == '\n') {
> +			if (*(str + 1) == '\0')
> +				break;
> +			return -EINVAL;
> +		} else if (*str == '.' && !frac_part) {
> +			frac_part = true;
> +		} else {
> +			return -EINVAL;
> +		}
> +		str++;
> +	}
> +
> +	for (; f_count > 0; f_count--)
> +		uhz *= 10;
> +
> +	*freq_uhz = uhz;
> +
> +	return 0;
> +}
didn't check the details, but can't the sub-Hz resolution be supported with
.write_raw_get_fmt()?
e.g.

static int adf41513_write_raw_get_fmt(struct iio_dev *indio_dev,
				    struct iio_chan_spec const *chan, long mask)
{
	switch (mask) {
	case IIO_CHAN_INFO_FREQUENCY:
		return IIO_VAL_INT_64;
	default:
		return IIO_VAL_INT_PLUS_MICRO;
	}
}

static const struct iio_info adf41513_info = {
...
	.write_raw_get_fmt = adf41513_write_raw_get_fmt(),
};

...
> +
> +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 long readin;
> +	unsigned long tmp;
> +	u64 freq_uhz;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch ((u32)private) {
> +	case ADF41513_FREQ:
> +		ret = adf41513_parse_uhz(buf, &freq_uhz);
> +		if (ret)
> +			return ret;
> +		ret = adf41513_set_frequency(st, freq_uhz, ADF41513_SYNC_DIFF);
> +		break;
> +	case ADF41513_FREQ_REFIN:
> +		ret = kstrtoull(buf, 10, &readin);
> +		if (ret)
> +			return ret;
> +
> +		if (readin < ADF41513_MIN_REF_FREQ || readin > ADF41513_MAX_REF_FREQ) {
Can, alternatively, this check be made with in_range() macro?
If so, then
#include <linux/minmax.h>

Same question/suggestion to other similar value bounds checks throughout the driver.

> +			ret = -EINVAL;
> +			break;
> +		}
> +

With best regards,
Marcelo