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

2026-04-26 Thread Jonathan Cameron
On Fri, 17 Apr 2026 09:17:32 +0100
Rodrigo Alencar via B4 Relay  
wrote:

> From: Rodrigo Alencar 
> 
> 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 
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;
>   }
> 




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

2026-04-17 Thread Rodrigo Alencar via B4 Relay
From: Rodrigo Alencar 

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 
---
 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
@@ -114,9 +114,13 @@
 /* Auxiliary DAC Control Register Bits */
 #define AD9910_AUX_DAC_FSC_MSK GENMASK(7, 0)
 
+/* POW Register Bits */
+#define AD9910_POW_PP_LSB_MSK  GENMASK(7, 0)
+
 /* ASF Register Bits */
 #define AD9910_ASF_RAMP_RATE_MSK   GENMASK(31, 16)
 #define AD9910_ASF_SCALE_FACTOR_MSKGENMASK(15, 2)
+#define AD9910_ASF_SCALE_FACTOR_PP_LSB_MSK GENMASK(7, 2)
 #define AD9910_ASF_STEP_SIZE_MSK   GENMASK(1, 0)
 
 /* Multichip Sync Register Bits */
@@ -140,7 +144,9 @@
 #define AD9910_MAX_PHASE_MICRORAD  (AD9910_PI_NANORAD / 500)
 
 #define AD9910_ASF_MAX GENMASK(13, 0)
+#define AD9910_ASF_PP_LSB_MAX  GENMASK(5, 0)
 #define AD9910_POW_MAX GENMASK(15, 0)
+#define AD9910_POW_PP_LSB_MAX  GENMASK(7, 0)
 #define AD9910_NUM_PROFILES8
 
 /* PLL constants */
@@ -191,6 +197,7 @@
  * @AD9910_CHANNEL_PROFILE_5: Profile 5 output channel
  * @AD9910_CHANNEL_PROFILE_6: Profile 6 output channel
  * @AD9910_CHANNEL_PROFILE_7: Profile 7 output channel
+ * @AD9910_CHANNEL_PARALLEL_PORT: Parallel port output channel
  */
 enum ad9910_channel {
AD9910_CHANNEL_PHY = 100,
@@ -202,6 +209,7 @@ enum ad9910_channel {
AD9910_CHANNEL_PROFILE_5 = 106,
AD9910_CHANNEL_PROFILE_6 = 107,
AD9910_CHANNEL_PROFILE_7 = 108,
+   AD9910_CHANNEL_PARALLEL_PORT = 110,
 };
 
 enum {
@@ -224,6 +232,10 @@ enum {
 
 enum {
AD9910_POWERDOWN,
+   AD9910_PP_FREQ_SCALE,
+   AD9910_PP_FREQ_OFFSET,
+   AD9910_PP_PHASE_OFFSET,
+   AD9910_PP_AMP_OFFSET,
 };
 
 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);
+   break;
+   default:
+   return -EINVAL;
+   }
+
+   return ret ?: len;
+}
+
+static ssize_t ad9910_pp_attrs_read(struct iio_dev *indio_dev,
+   uintptr_t private,
+   const struct iio_chan_spec *chan,
+   char *buf)
+{
+   struct ad9910_state *st = iio_priv(indio_dev);
+   int vals[2];
+   u32 tmp32;
+   u64 tmp64;
+
+   guard(mutex)(&st->lock);
+
+   switch (private) {
+   case AD9910_PP_FREQ_OFFSET:
+   tmp64 = (u64)st->reg[AD9910_REG_FTW].val32 * 
st->data.sysclk_freq_hz;
+   vals[0] = tmp64 >> 32;
+   vals[1] = ((tmp64 & GENMASK_ULL(31, 0)) * MICRO) >> 32;
+   break;
+   case AD9910_PP_PHASE_OFFSET:
+   tmp32 = FIELD_GET(AD9910_POW_PP_LSB_MSK,
+ st->reg[AD9910_REG_POW].val16);
+   tmp32 = (tmp32 * AD9910_MAX_PHASE_MICRORAD) >> 16;
+   vals[0] = tmp32 / MICRO;
+   vals[1] = tmp32 % MICRO;
+   break;
+   case AD9910_PP_AMP_OFFSET:
+   tmp32 = FIELD_GET(AD9910_ASF_SCALE_FACTOR_PP_LSB_MSK,
+ st->reg[AD9910_REG_ASF].val32);
+   vals[0] = 0;
+   vals[1] = (u64)tmp32 * MICRO >> 14;
+   break;
+   default:
+   return -EINVAL;
+   }
+
+   return iio_format_value(buf, IIO_VAL_INT_PLUS_MICRO, ARRAY_SIZE(vals), 
vals);
+}
+
+static ssize_t ad9910_pp_attrs_write(struct iio_dev *indio_dev,
+uintptr_t private,
+