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

["Nuno Sá" <noname.nuno@gmail.com>]

On Thu, 2025-11-13 at 10:13 -0300, Marcelo Schmitt wrote:
> 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;

I think the above is already the default anyways... But the key here is that (I think) the goal
is to be able to do things like:

echo integer.fractional > out_altvoltage0_frequency

where integer can be u64. If I'm not missing anything, we cannot do that through the standard
interfaces.

- Nuno Sá

> 	}
> }
> 
> 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