[PATCH 1/2] staging: iio: frequency: ad9834: Move frequency to standard iio types

[PATCH 1/2] staging: iio: frequency: ad9834: Move frequency to standard iio types

[Beniamin Bia]

From: Beniamin Bia <biabeniamin@outlook.com>

Frequency attribute is added with a standard type from iio framework
instead of custom attribute. This is a small step towards removing any
unnecessary custom attribute.

Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
---
 drivers/staging/iio/frequency/ad9834.c | 97 +++++++++++++++++++++-----
 1 file changed, 80 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
index f036f75d1f22..370e8263899e 100644
--- a/drivers/staging/iio/frequency/ad9834.c
+++ b/drivers/staging/iio/frequency/ad9834.c
@@ -29,8 +29,7 @@
 /* Registers */
 
 #define AD9834_REG_CMD		0
-#define AD9834_REG_FREQ0	BIT(14)
-#define AD9834_REG_FREQ1	BIT(15)
+#define AD9834_REG_FREQ(chann)	(BIT(14) << (chann))
 #define AD9834_REG_PHASE0	(BIT(15) | BIT(14))
 #define AD9834_REG_PHASE1	(BIT(15) | BIT(14) | BIT(13))
 
@@ -81,6 +80,9 @@ struct ad9834_state {
 	struct spi_message		freq_msg;
 	struct mutex                    lock;   /* protect sensor state */
 
+	unsigned long			frequency0;
+	unsigned long			frequency1;
+
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache lines.
@@ -100,6 +102,25 @@ enum ad9834_supported_device_ids {
 	ID_AD9838,
 };
 
+#define AD9833_CHANNEL(_chan) {						\
+		.type = IIO_ALTVOLTAGE,					\
+		.indexed = 1,						\
+		.output = 1,						\
+		.address = (_chan),					\
+		.channel = (_chan),					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY)	\
+}
+
+static const struct iio_chan_spec ad9833_channels[] = {
+	AD9833_CHANNEL(0),
+	AD9833_CHANNEL(1),
+};
+
+static const struct iio_chan_spec ad9834_channels[] = {
+	AD9833_CHANNEL(0),
+	AD9833_CHANNEL(1),
+};
+
 static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned long fout)
 {
 	unsigned long long freqreg = (u64)fout * (u64)BIT(AD9834_FREQ_BITS);
@@ -113,6 +134,7 @@ static int ad9834_write_frequency(struct ad9834_state *st,
 {
 	unsigned long clk_freq;
 	unsigned long regval;
+	int ret;
 
 	clk_freq = clk_get_rate(st->mclk);
 
@@ -121,13 +143,22 @@ static int ad9834_write_frequency(struct ad9834_state *st,
 
 	regval = ad9834_calc_freqreg(clk_freq, fout);
 
-	st->freq_data[0] = cpu_to_be16(addr | (regval &
+	st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ(addr) | (regval &
 				       RES_MASK(AD9834_FREQ_BITS / 2)));
-	st->freq_data[1] = cpu_to_be16(addr | ((regval >>
+	st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ(addr) | ((regval >>
 				       (AD9834_FREQ_BITS / 2)) &
 				       RES_MASK(AD9834_FREQ_BITS / 2)));
 
-	return spi_sync(st->spi, &st->freq_msg);
+	ret = spi_sync(st->spi, &st->freq_msg);
+	if (ret)
+		return ret;
+
+	if (addr == 0)
+		st->frequency0 = fout;
+	else
+		st->frequency1 = fout;
+
+	return 0;
 }
 
 static int ad9834_write_phase(struct ad9834_state *st,
@@ -140,6 +171,40 @@ static int ad9834_write_phase(struct ad9834_state *st,
 	return spi_sync(st->spi, &st->msg);
 }
 
+static int ad9834_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct ad9834_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_FREQUENCY:
+		if (chan->address == 0)
+			*val = st->frequency0;
+		else
+			*val = st->frequency1;
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static int ad9834_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct ad9834_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_FREQUENCY:
+		return ad9834_write_frequency(st, chan->address, val);
+	default:
+		return  -EINVAL;
+	}
+
+	return 0;
+}
+
 static ssize_t ad9834_write(struct device *dev,
 			    struct device_attribute *attr,
 			    const char *buf,
@@ -157,10 +222,6 @@ static ssize_t ad9834_write(struct device *dev,
 
 	mutex_lock(&st->lock);
 	switch ((u32)this_attr->address) {
-	case AD9834_REG_FREQ0:
-	case AD9834_REG_FREQ1:
-		ret = ad9834_write_frequency(st, this_attr->address, val);
-		break;
 	case AD9834_REG_PHASE0:
 	case AD9834_REG_PHASE1:
 		ret = ad9834_write_phase(st, this_attr->address, val);
@@ -323,8 +384,6 @@ static IIO_DEVICE_ATTR(out_altvoltage0_out1_wavetype_available, 0444,
  * see dds.h for further information
  */
 
-static IIO_DEV_ATTR_FREQ(0, 0, 0200, NULL, ad9834_write, AD9834_REG_FREQ0);
-static IIO_DEV_ATTR_FREQ(0, 1, 0200, NULL, ad9834_write, AD9834_REG_FREQ1);
 static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9834_write, AD9834_FSEL);
 static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
 
@@ -342,8 +401,6 @@ static IIO_DEV_ATTR_OUT_WAVETYPE(0, 0, ad9834_store_wavetype, 0);
 static IIO_DEV_ATTR_OUT_WAVETYPE(0, 1, ad9834_store_wavetype, 1);
 
 static struct attribute *ad9834_attributes[] = {
-	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_frequency1.dev_attr.attr,
 	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
@@ -361,8 +418,6 @@ static struct attribute *ad9834_attributes[] = {
 };
 
 static struct attribute *ad9833_attributes[] = {
-	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_frequency1.dev_attr.attr,
 	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
@@ -384,11 +439,15 @@ static const struct attribute_group ad9833_attribute_group = {
 };
 
 static const struct iio_info ad9834_info = {
+	.write_raw = &ad9834_write_raw,
+	.read_raw = &ad9834_read_raw,
 	.attrs = &ad9834_attribute_group,
 	.driver_module = THIS_MODULE,
 };
 
 static const struct iio_info ad9833_info = {
+	.write_raw = &ad9834_write_raw,
+	.read_raw = &ad9834_read_raw,
 	.attrs = &ad9833_attribute_group,
 	.driver_module = THIS_MODULE,
 };
@@ -435,9 +494,13 @@ static int ad9834_probe(struct spi_device *spi)
 	switch (st->devid) {
 	case ID_AD9833:
 	case ID_AD9837:
+		indio_dev->channels = ad9833_channels;
+		indio_dev->num_channels = ARRAY_SIZE(ad9833_channels);
 		indio_dev->info = &ad9833_info;
 		break;
 	default:
+		indio_dev->channels = ad9834_channels;
+		indio_dev->num_channels = ARRAY_SIZE(ad9834_channels);
 		indio_dev->info = &ad9834_info;
 		break;
 	}
@@ -474,11 +537,11 @@ static int ad9834_probe(struct spi_device *spi)
 		goto error_clock_unprepare;
 	}
 
-	ret = ad9834_write_frequency(st, AD9834_REG_FREQ0, 1000000);
+	ret = ad9834_write_frequency(st, 0, 1000000);
 	if (ret)
 		goto error_clock_unprepare;
 
-	ret = ad9834_write_frequency(st, AD9834_REG_FREQ1, 5000000);
+	ret = ad9834_write_frequency(st, 1, 5000000);
 	if (ret)
 		goto error_clock_unprepare;
 
-- 
2.17.1

[PATCH 2/2] staging: iio: frequency: ad9834: Move phase and scale to standard iio attribute

[Beniamin Bia]

From: Beniamin Bia <biabeniamin@outlook.com>

The custom phase and scale attributes were moved to standard iio types.

Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
---
 drivers/staging/iio/frequency/ad9834.c | 54 +++++++++++++++-----------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
index 370e8263899e..3ecf976ddefe 100644
--- a/drivers/staging/iio/frequency/ad9834.c
+++ b/drivers/staging/iio/frequency/ad9834.c
@@ -30,8 +30,7 @@
 
 #define AD9834_REG_CMD		0
 #define AD9834_REG_FREQ(chann)	(BIT(14) << (chann))
-#define AD9834_REG_PHASE0	(BIT(15) | BIT(14))
-#define AD9834_REG_PHASE1	(BIT(15) | BIT(14) | BIT(13))
+#define AD9834_REG_PHASE(chann)	(BIT(15) | BIT(14) | ((chann) << 13UL))
 
 /* Command Control Bits */
 
@@ -82,6 +81,8 @@ struct ad9834_state {
 
 	unsigned long			frequency0;
 	unsigned long			frequency1;
+	unsigned long			phase0;
+	unsigned long			phase1;
 
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
@@ -109,6 +110,8 @@ enum ad9834_supported_device_ids {
 		.address = (_chan),					\
 		.channel = (_chan),					\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY)	\
+						| BIT(IIO_CHAN_INFO_PHASE),\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
 }
 
 static const struct iio_chan_spec ad9833_channels[] = {
@@ -164,11 +167,22 @@ static int ad9834_write_frequency(struct ad9834_state *st,
 static int ad9834_write_phase(struct ad9834_state *st,
 			      unsigned long addr, unsigned long phase)
 {
+	int ret;
+
 	if (phase > BIT(AD9834_PHASE_BITS))
 		return -EINVAL;
-	st->data = cpu_to_be16(addr | phase);
+	st->data = cpu_to_be16(AD9834_REG_PHASE(addr) | phase);
+
+	ret = spi_sync(st->spi, &st->msg);
+	if (ret)
+		return ret;
 
-	return spi_sync(st->spi, &st->msg);
+	if (addr == 0)
+		st->phase0 = phase;
+	else
+		st->phase1 = phase;
+
+	return 0;
 }
 
 static int ad9834_read_raw(struct iio_dev *indio_dev,
@@ -184,6 +198,16 @@ static int ad9834_read_raw(struct iio_dev *indio_dev,
 		else
 			*val = st->frequency1;
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_PHASE:
+		if (chan->address == 0)
+			*val = st->phase0;
+		else
+			*val = st->phase1;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		/*1 hz */
+		*val = 1;
+		return IIO_VAL_INT;
 	}
 
 	return -EINVAL;
@@ -198,6 +222,8 @@ static int ad9834_write_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_FREQUENCY:
 		return ad9834_write_frequency(st, chan->address, val);
+	case IIO_CHAN_INFO_PHASE:
+		return ad9834_write_phase(st, chan->address, val);
 	default:
 		return  -EINVAL;
 	}
@@ -222,10 +248,6 @@ static ssize_t ad9834_write(struct device *dev,
 
 	mutex_lock(&st->lock);
 	switch ((u32)this_attr->address) {
-	case AD9834_REG_PHASE0:
-	case AD9834_REG_PHASE1:
-		ret = ad9834_write_phase(st, this_attr->address, val);
-		break;
 	case AD9834_OPBITEN:
 		if (st->control & AD9834_MODE) {
 			ret = -EINVAL;  /* AD9843 reserved mode */
@@ -385,12 +407,8 @@ static IIO_DEVICE_ATTR(out_altvoltage0_out1_wavetype_available, 0444,
  */
 
 static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9834_write, AD9834_FSEL);
-static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
 
-static IIO_DEV_ATTR_PHASE(0, 0, 0200, NULL, ad9834_write, AD9834_REG_PHASE0);
-static IIO_DEV_ATTR_PHASE(0, 1, 0200, NULL, ad9834_write, AD9834_REG_PHASE1);
 static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL, ad9834_write, AD9834_PSEL);
-static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
 
 static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
 	ad9834_write, AD9834_PIN_SW);
@@ -401,10 +419,6 @@ static IIO_DEV_ATTR_OUT_WAVETYPE(0, 0, ad9834_store_wavetype, 0);
 static IIO_DEV_ATTR_OUT_WAVETYPE(0, 1, ad9834_store_wavetype, 1);
 
 static struct attribute *ad9834_attributes[] = {
-	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
-	&iio_const_attr_out_altvoltage0_phase_scale.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_pincontrol_en.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_frequencysymbol.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_phasesymbol.dev_attr.attr,
@@ -418,10 +432,6 @@ static struct attribute *ad9834_attributes[] = {
 };
 
 static struct attribute *ad9833_attributes[] = {
-	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
-	&iio_const_attr_out_altvoltage0_phase_scale.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_frequencysymbol.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_phasesymbol.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_out_enable.dev_attr.attr,
@@ -545,11 +555,11 @@ static int ad9834_probe(struct spi_device *spi)
 	if (ret)
 		goto error_clock_unprepare;
 
-	ret = ad9834_write_phase(st, AD9834_REG_PHASE0, 512);
+	ret = ad9834_write_phase(st, 0, 512);
 	if (ret)
 		goto error_clock_unprepare;
 
-	ret = ad9834_write_phase(st, AD9834_REG_PHASE1, 1024);
+	ret = ad9834_write_phase(st, 1, 1024);
 	if (ret)
 		goto error_clock_unprepare;
 
-- 
2.17.1

Re: [PATCH 1/2] staging: iio: frequency: ad9834: Move frequency to standard iio types

[Jonathan Cameron]

On Wed, 6 Feb 2019 14:25:41 +0200
Beniamin Bia <beniamin.bia@analog.com> wrote:

> From: Beniamin Bia <biabeniamin@outlook.com>
> 
> Frequency attribute is added with a standard type from iio framework
> instead of custom attribute. This is a small step towards removing any
> unnecessary custom attribute.
> 
> Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>

Hi,

How is this related to the one a few hours earlier?
V2?

If you do accidentally send twice (and realise it obviously) please
reply to say what you want ignored. For now I'll ignore this version.

I'm guessing it was fighting with your work email system.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/frequency/ad9834.c | 97 +++++++++++++++++++++-----
>  1 file changed, 80 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
> index f036f75d1f22..370e8263899e 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -29,8 +29,7 @@
>  /* Registers */
>  
>  #define AD9834_REG_CMD		0
> -#define AD9834_REG_FREQ0	BIT(14)
> -#define AD9834_REG_FREQ1	BIT(15)
> +#define AD9834_REG_FREQ(chann)	(BIT(14) << (chann))
>  #define AD9834_REG_PHASE0	(BIT(15) | BIT(14))
>  #define AD9834_REG_PHASE1	(BIT(15) | BIT(14) | BIT(13))
>  
> @@ -81,6 +80,9 @@ struct ad9834_state {
>  	struct spi_message		freq_msg;
>  	struct mutex                    lock;   /* protect sensor state */
>  
> +	unsigned long			frequency0;
> +	unsigned long			frequency1;
> +
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> @@ -100,6 +102,25 @@ enum ad9834_supported_device_ids {
>  	ID_AD9838,
>  };
>  
> +#define AD9833_CHANNEL(_chan) {						\
> +		.type = IIO_ALTVOLTAGE,					\
> +		.indexed = 1,						\
> +		.output = 1,						\
> +		.address = (_chan),					\
> +		.channel = (_chan),					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY)	\
> +}
> +
> +static const struct iio_chan_spec ad9833_channels[] = {
> +	AD9833_CHANNEL(0),
> +	AD9833_CHANNEL(1),
> +};
> +
> +static const struct iio_chan_spec ad9834_channels[] = {
> +	AD9833_CHANNEL(0),
> +	AD9833_CHANNEL(1),
> +};
> +
>  static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned long fout)
>  {
>  	unsigned long long freqreg = (u64)fout * (u64)BIT(AD9834_FREQ_BITS);
> @@ -113,6 +134,7 @@ static int ad9834_write_frequency(struct ad9834_state *st,
>  {
>  	unsigned long clk_freq;
>  	unsigned long regval;
> +	int ret;
>  
>  	clk_freq = clk_get_rate(st->mclk);
>  
> @@ -121,13 +143,22 @@ static int ad9834_write_frequency(struct ad9834_state *st,
>  
>  	regval = ad9834_calc_freqreg(clk_freq, fout);
>  
> -	st->freq_data[0] = cpu_to_be16(addr | (regval &
> +	st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ(addr) | (regval &
>  				       RES_MASK(AD9834_FREQ_BITS / 2)));
> -	st->freq_data[1] = cpu_to_be16(addr | ((regval >>
> +	st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ(addr) | ((regval >>
>  				       (AD9834_FREQ_BITS / 2)) &
>  				       RES_MASK(AD9834_FREQ_BITS / 2)));
>  
> -	return spi_sync(st->spi, &st->freq_msg);
> +	ret = spi_sync(st->spi, &st->freq_msg);
> +	if (ret)
> +		return ret;
> +
> +	if (addr == 0)
> +		st->frequency0 = fout;
> +	else
> +		st->frequency1 = fout;
> +
> +	return 0;
>  }
>  
>  static int ad9834_write_phase(struct ad9834_state *st,
> @@ -140,6 +171,40 @@ static int ad9834_write_phase(struct ad9834_state *st,
>  	return spi_sync(st->spi, &st->msg);
>  }
>  
> +static int ad9834_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad9834_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_FREQUENCY:
> +		if (chan->address == 0)
> +			*val = st->frequency0;
> +		else
> +			*val = st->frequency1;
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad9834_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ad9834_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_FREQUENCY:
> +		return ad9834_write_frequency(st, chan->address, val);
> +	default:
> +		return  -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static ssize_t ad9834_write(struct device *dev,
>  			    struct device_attribute *attr,
>  			    const char *buf,
> @@ -157,10 +222,6 @@ static ssize_t ad9834_write(struct device *dev,
>  
>  	mutex_lock(&st->lock);
>  	switch ((u32)this_attr->address) {
> -	case AD9834_REG_FREQ0:
> -	case AD9834_REG_FREQ1:
> -		ret = ad9834_write_frequency(st, this_attr->address, val);
> -		break;
>  	case AD9834_REG_PHASE0:
>  	case AD9834_REG_PHASE1:
>  		ret = ad9834_write_phase(st, this_attr->address, val);
> @@ -323,8 +384,6 @@ static IIO_DEVICE_ATTR(out_altvoltage0_out1_wavetype_available, 0444,
>   * see dds.h for further information
>   */
>  
> -static IIO_DEV_ATTR_FREQ(0, 0, 0200, NULL, ad9834_write, AD9834_REG_FREQ0);
> -static IIO_DEV_ATTR_FREQ(0, 1, 0200, NULL, ad9834_write, AD9834_REG_FREQ1);
>  static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9834_write, AD9834_FSEL);
>  static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
>  
> @@ -342,8 +401,6 @@ static IIO_DEV_ATTR_OUT_WAVETYPE(0, 0, ad9834_store_wavetype, 0);
>  static IIO_DEV_ATTR_OUT_WAVETYPE(0, 1, ad9834_store_wavetype, 1);
>  
>  static struct attribute *ad9834_attributes[] = {
> -	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_frequency1.dev_attr.attr,
>  	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
> @@ -361,8 +418,6 @@ static struct attribute *ad9834_attributes[] = {
>  };
>  
>  static struct attribute *ad9833_attributes[] = {
> -	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_frequency1.dev_attr.attr,
>  	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
> @@ -384,11 +439,15 @@ static const struct attribute_group ad9833_attribute_group = {
>  };
>  
>  static const struct iio_info ad9834_info = {
> +	.write_raw = &ad9834_write_raw,
> +	.read_raw = &ad9834_read_raw,
>  	.attrs = &ad9834_attribute_group,
>  	.driver_module = THIS_MODULE,
>  };
>  
>  static const struct iio_info ad9833_info = {
> +	.write_raw = &ad9834_write_raw,
> +	.read_raw = &ad9834_read_raw,
>  	.attrs = &ad9833_attribute_group,
>  	.driver_module = THIS_MODULE,
>  };
> @@ -435,9 +494,13 @@ static int ad9834_probe(struct spi_device *spi)
>  	switch (st->devid) {
>  	case ID_AD9833:
>  	case ID_AD9837:
> +		indio_dev->channels = ad9833_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(ad9833_channels);
>  		indio_dev->info = &ad9833_info;
>  		break;
>  	default:
> +		indio_dev->channels = ad9834_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(ad9834_channels);
>  		indio_dev->info = &ad9834_info;
>  		break;
>  	}
> @@ -474,11 +537,11 @@ static int ad9834_probe(struct spi_device *spi)
>  		goto error_clock_unprepare;
>  	}
>  
> -	ret = ad9834_write_frequency(st, AD9834_REG_FREQ0, 1000000);
> +	ret = ad9834_write_frequency(st, 0, 1000000);
>  	if (ret)
>  		goto error_clock_unprepare;
>  
> -	ret = ad9834_write_frequency(st, AD9834_REG_FREQ1, 5000000);
> +	ret = ad9834_write_frequency(st, 1, 5000000);
>  	if (ret)
>  		goto error_clock_unprepare;
>  

Re: [PATCH 1/2] staging: iio: frequency: ad9834: Move frequency to standard iio types

[Jonathan Cameron]

On Wed, 6 Feb 2019 14:05:41 +0200
Beniamin Bia <biabeniamin@outlook.com> wrote:

> Frequency attribute is added with a standard type from iio framework
> instead of custom attribute. This is a small step towards removing any
> unnecessary custom attribute.
> 
> Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
> ---
>  drivers/staging/iio/frequency/ad9834.c | 97 +++++++++++++++++++++-----
>  1 file changed, 80 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
> index f036f75d1f22..370e8263899e 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -29,8 +29,7 @@
>  /* Registers */
>  
>  #define AD9834_REG_CMD		0
> -#define AD9834_REG_FREQ0	BIT(14)
> -#define AD9834_REG_FREQ1	BIT(15)
> +#define AD9834_REG_FREQ(chann)	(BIT(14) << (chann))

That is rather confusing... 

Perhaps we can represent this as a variable that is 3 bits wide, with values that
correspond to the different settings.


So define
#define AD7834_REG_ADDR_MASK GENMASK(15,13)
#define AD7834_REG_FREQ0 2
#define AD7834_REG_FREQ1 4
#define AD7834_REG_PHASE0 6
#define AD7834_REG_PHASE0 7

And use FIELD_SET(reg, AD7834_REG_ADDR_MASK, AD7834_REG_FREQ0) etc.

or something along those lines. It's a bit nasty because in the FREQ
case the bottom bit is actually part of the value.
Hmm. May just need two definitions of the mask. how about.
#define AD7834_REG_FREQ_MASK GENMASK(15,14)
#define AD7834_REG_FREQ0 1
#define AD7934_REG_FREQ1 2
 
#define AD7834_REG_PHASE_MASK GENMASK(15,13)
#define AD7834_REG_PHASE0 6
#define AD7834_REG_PHASE1 7
?

>  #define AD9834_REG_PHASE0	(BIT(15) | BIT(14))
>  #define AD9834_REG_PHASE1	(BIT(15) | BIT(14) | BIT(13))
>  
> @@ -81,6 +80,9 @@ struct ad9834_state {
>  	struct spi_message		freq_msg;
>  	struct mutex                    lock;   /* protect sensor state */
>  
> +	unsigned long			frequency0;
> +	unsigned long			frequency1;
> +
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> @@ -100,6 +102,25 @@ enum ad9834_supported_device_ids {
>  	ID_AD9838,
>  };
>  
> +#define AD9833_CHANNEL(_chan) {						\
> +		.type = IIO_ALTVOLTAGE,					\
> +		.indexed = 1,						\
> +		.output = 1,						\
> +		.address = (_chan),					\

Not a lot of point in setting them both if they are always the same.

> +		.channel = (_chan),					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY)	\
> +}
> +
> +static const struct iio_chan_spec ad9833_channels[] = {
> +	AD9833_CHANNEL(0),
> +	AD9833_CHANNEL(1),
> +};
> +
> +static const struct iio_chan_spec ad9834_channels[] = {
> +	AD9833_CHANNEL(0),
> +	AD9833_CHANNEL(1),
> +};

Umm. That needs some explaining.  Why have two identical arrays?
I'll guess that later patches will make them different?
That is fine, but you need a comment in the patch description to give
the reasoning.

> +
>  static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned long fout)
>  {
>  	unsigned long long freqreg = (u64)fout * (u64)BIT(AD9834_FREQ_BITS);
> @@ -113,6 +134,7 @@ static int ad9834_write_frequency(struct ad9834_state *st,
>  {
>  	unsigned long clk_freq;
>  	unsigned long regval;
> +	int ret;
>  
>  	clk_freq = clk_get_rate(st->mclk);
>  
> @@ -121,13 +143,22 @@ static int ad9834_write_frequency(struct ad9834_state *st,
>  
>  	regval = ad9834_calc_freqreg(clk_freq, fout);
>  
> -	st->freq_data[0] = cpu_to_be16(addr | (regval &
> +	st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ(addr) | (regval &
>  				       RES_MASK(AD9834_FREQ_BITS / 2)));
> -	st->freq_data[1] = cpu_to_be16(addr | ((regval >>
> +	st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ(addr) | ((regval >>
>  				       (AD9834_FREQ_BITS / 2)) &
>  				       RES_MASK(AD9834_FREQ_BITS / 2)));
>  
> -	return spi_sync(st->spi, &st->freq_msg);
> +	ret = spi_sync(st->spi, &st->freq_msg);
> +	if (ret)
> +		return ret;
> +
> +	if (addr == 0)
> +		st->frequency0 = fout;

How about moving to an array you can just index with addr?
(or the enum I suggest below).

> +	else
> +		st->frequency1 = fout;
> +
> +	return 0;
>  }
>  
>  static int ad9834_write_phase(struct ad9834_state *st,
> @@ -140,6 +171,40 @@ static int ad9834_write_phase(struct ad9834_state *st,
>  	return spi_sync(st->spi, &st->msg);
>  }
>  
> +static int ad9834_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad9834_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_FREQUENCY:
> +		if (chan->address == 0)
> +			*val = st->frequency0;

As above, an array for frequency would tidy this up.

> +		else
> +			*val = st->frequency1;
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad9834_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ad9834_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_FREQUENCY:
> +		return ad9834_write_frequency(st, chan->address, val);
> +	default:
> +		return  -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static ssize_t ad9834_write(struct device *dev,
>  			    struct device_attribute *attr,
>  			    const char *buf,
> @@ -157,10 +222,6 @@ static ssize_t ad9834_write(struct device *dev,
>  
>  	mutex_lock(&st->lock);
>  	switch ((u32)this_attr->address) {
> -	case AD9834_REG_FREQ0:
> -	case AD9834_REG_FREQ1:
> -		ret = ad9834_write_frequency(st, this_attr->address, val);
> -		break;
>  	case AD9834_REG_PHASE0:
>  	case AD9834_REG_PHASE1:
>  		ret = ad9834_write_phase(st, this_attr->address, val);
> @@ -323,8 +384,6 @@ static IIO_DEVICE_ATTR(out_altvoltage0_out1_wavetype_available, 0444,
>   * see dds.h for further information
>   */
>  
> -static IIO_DEV_ATTR_FREQ(0, 0, 0200, NULL, ad9834_write, AD9834_REG_FREQ0);
> -static IIO_DEV_ATTR_FREQ(0, 1, 0200, NULL, ad9834_write, AD9834_REG_FREQ1);
>  static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9834_write, AD9834_FSEL);
>  static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
>  
> @@ -342,8 +401,6 @@ static IIO_DEV_ATTR_OUT_WAVETYPE(0, 0, ad9834_store_wavetype, 0);
>  static IIO_DEV_ATTR_OUT_WAVETYPE(0, 1, ad9834_store_wavetype, 1);
>  
>  static struct attribute *ad9834_attributes[] = {
> -	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_frequency1.dev_attr.attr,
>  	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
> @@ -361,8 +418,6 @@ static struct attribute *ad9834_attributes[] = {
>  };
>  
>  static struct attribute *ad9833_attributes[] = {
> -	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_frequency1.dev_attr.attr,
>  	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
> @@ -384,11 +439,15 @@ static const struct attribute_group ad9833_attribute_group = {
>  };
>  
>  static const struct iio_info ad9834_info = {
> +	.write_raw = &ad9834_write_raw,
> +	.read_raw = &ad9834_read_raw,
>  	.attrs = &ad9834_attribute_group,
>  	.driver_module = THIS_MODULE,
>  };
>  
>  static const struct iio_info ad9833_info = {
> +	.write_raw = &ad9834_write_raw,
> +	.read_raw = &ad9834_read_raw,
>  	.attrs = &ad9833_attribute_group,
>  	.driver_module = THIS_MODULE,
>  };
> @@ -435,9 +494,13 @@ static int ad9834_probe(struct spi_device *spi)
>  	switch (st->devid) {
>  	case ID_AD9833:
>  	case ID_AD9837:
> +		indio_dev->channels = ad9833_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(ad9833_channels);
>  		indio_dev->info = &ad9833_info;
>  		break;
>  	default:
> +		indio_dev->channels = ad9834_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(ad9834_channels);
>  		indio_dev->info = &ad9834_info;
>  		break;
>  	}
> @@ -474,11 +537,11 @@ static int ad9834_probe(struct spi_device *spi)
>  		goto error_clock_unprepare;
>  	}
>  
> -	ret = ad9834_write_frequency(st, AD9834_REG_FREQ0, 1000000);
> +	ret = ad9834_write_frequency(st, 0, 1000000);
Use an enum for that second variable, to make it clear that we are
picking which frequency value is being set and that there are only two
possible.

>  	if (ret)
>  		goto error_clock_unprepare;
>  
> -	ret = ad9834_write_frequency(st, AD9834_REG_FREQ1, 5000000);
> +	ret = ad9834_write_frequency(st, 1, 5000000);
>  	if (ret)
>  		goto error_clock_unprepare;
>  

Re: [PATCH 1/2] staging: iio: frequency: ad9834: Move frequency to standard iio types

[Dan Carpenter]

On Wed, Feb 06, 2019 at 02:05:41PM +0200, Beniamin Bia wrote:
> Frequency attribute is added with a standard type from iio framework
> instead of custom attribute. This is a small step towards removing any
> unnecessary custom attribute.
> 
> Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>


This doesn't match your email.  You should probably add a From: to
give your @analog.com address authorship credit.

> ---
>  drivers/staging/iio/frequency/ad9834.c | 97 +++++++++++++++++++++-----
>  1 file changed, 80 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
> index f036f75d1f22..370e8263899e 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -29,8 +29,7 @@
>  /* Registers */
>  
>  #define AD9834_REG_CMD		0
> -#define AD9834_REG_FREQ0	BIT(14)
> -#define AD9834_REG_FREQ1	BIT(15)
> +#define AD9834_REG_FREQ(chann)	(BIT(14) << (chann))

Only one 'n' in "chan", please.  But we actually pass the address not
the channel here so that was unexpected to me.  I think the channel is
always the same as the address?  So maybe it doesn't matter.

>  #define AD9834_REG_PHASE0	(BIT(15) | BIT(14))
>  #define AD9834_REG_PHASE1	(BIT(15) | BIT(14) | BIT(13))
>  
> @@ -81,6 +80,9 @@ struct ad9834_state {
>  	struct spi_message		freq_msg;
>  	struct mutex                    lock;   /* protect sensor state */
>  
> +	unsigned long			frequency0;
> +	unsigned long			frequency1;

I don't understand why we have two frequencies when we only ever use
one.

> +
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> @@ -100,6 +102,25 @@ enum ad9834_supported_device_ids {
>  	ID_AD9838,
>  };
>  
> +#define AD9833_CHANNEL(_chan) {						\

No need for the underscore.  (I think?)

> +		.type = IIO_ALTVOLTAGE,					\
> +		.indexed = 1,						\
> +		.output = 1,						\
> +		.address = (_chan),					\
> +		.channel = (_chan),					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY)	\
> +}

regards,
dan carpenter