Re: [PATCH RFC v3 3/9] iio: frequency: ad9910: add simple parallel port mode support

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 17 Apr 2026 09:17:32 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Add parallel port channel with frequency scale, frequency offset, phase
> offset, and amplitude offset extended attributes for configuring the
> parallel data path.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
Really minor stuff - mostly follow on from review of previous patch.

> ---
>  drivers/iio/frequency/ad9910.c | 152 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 152 insertions(+)
> 
> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index e9005037db1a..5b4076028a29 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c

>  struct ad9910_data {
> @@ -478,6 +490,10 @@ static ssize_t ad9910_ext_info_read(struct iio_dev *indio_dev,
>  		val = !!FIELD_GET(AD9910_CFR1_SOFT_POWER_DOWN_MSK,
>  				  st->reg[AD9910_REG_CFR1].val32);
>  		break;
> +	case AD9910_PP_FREQ_SCALE:
> +		val = BIT(FIELD_GET(AD9910_CFR2_FM_GAIN_MSK,
> +				    st->reg[AD9910_REG_CFR2].val32));
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -508,6 +524,113 @@ static ssize_t ad9910_ext_info_write(struct iio_dev *indio_dev,
>  					  AD9910_CFR1_SOFT_POWER_DOWN_MSK,
>  					  val32, true);
>  		break;
> +	case AD9910_PP_FREQ_SCALE:
> +		if (val32 > BIT(15) || !is_power_of_2(val32))
> +			return -EINVAL;
> +
> +		val32 = FIELD_PREP(AD9910_CFR2_FM_GAIN_MSK, ilog2(val32));
> +		ret = ad9910_reg32_update(st, AD9910_REG_CFR2,
> +					  AD9910_CFR2_FM_GAIN_MSK,
> +					  val32, true);
As in previous, I'd prefer the more verbose
		if (ret)
			return ret;

		break;

Same for all the similar cases.


> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret ?: len;
> +}


> @@ -661,6 +808,11 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  			}
>  
>  			return ad9910_profile_set(st, tmp32);
> +		case AD9910_CHANNEL_PARALLEL_PORT:
> +			tmp32 = FIELD_PREP(AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK, !!val);
> +			return ad9910_reg32_update(st, AD9910_REG_CFR2,
> +						   AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> +						   tmp32, true);
Ah. So tmp32 isn't always an index.  Maybe just use local clearer named variables?
>  		default:
>  			return -EINVAL;
>  		}
>