2019-02-15 00:56:05

by Beniamin Bia

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

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. Ad9834 will diverge from ad9833 in the
future, that is why we have two identical arrays for ad9834 and 9833.

Signed-off-by: Beniamin Bia <[email protected]>
---
Changes in v2:
-the personal email address was replaced by the work email
-separate define for frequency channel
-address field from channel specification was removed
-frequency variables were replaced by an array
-specified in comment why we have differente chan_spec for ad9834 and ad9833
-enum used for write_frequency function

drivers/staging/iio/frequency/ad9834.c | 110 ++++++++++++++++++++-----
1 file changed, 91 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
index f036f75d1f22..561617046c20 100644
--- a/drivers/staging/iio/frequency/ad9834.c
+++ b/drivers/staging/iio/frequency/ad9834.c
@@ -81,6 +81,8 @@ struct ad9834_state {
struct spi_message freq_msg;
struct mutex lock; /* protect sensor state */

+ unsigned long frequency[2];
+
/*
* DMA (thus cache coherency maintenance) requires the
* transfer buffers to live in their own cache lines.
@@ -89,6 +91,11 @@ struct ad9834_state {
__be16 freq_data[2];
};

+enum ad9834_ch_addr {
+ AD9834_CHANNEL_ADDRESS0,
+ AD9834_CHANNEL_ADDRESS1,
+};
+
/**
* ad9834_supported_device_ids:
*/
@@ -100,6 +107,24 @@ enum ad9834_supported_device_ids {
ID_AD9838,
};

+#define AD9833_CHANNEL(chan) { \
+ .type = IIO_ALTVOLTAGE, \
+ .indexed = 1, \
+ .output = 1, \
+ .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);
@@ -109,10 +134,12 @@ static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned long fout)
}

static int ad9834_write_frequency(struct ad9834_state *st,
- unsigned long addr, unsigned long fout)
+ enum ad9834_ch_addr addr,
+ unsigned long fout)
{
unsigned long clk_freq;
unsigned long regval;
+ int ret;

clk_freq = clk_get_rate(st->mclk);

@@ -121,13 +148,27 @@ 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 &
- RES_MASK(AD9834_FREQ_BITS / 2)));
- st->freq_data[1] = cpu_to_be16(addr | ((regval >>
- (AD9834_FREQ_BITS / 2)) &
- RES_MASK(AD9834_FREQ_BITS / 2)));
+ if (addr == AD9834_CHANNEL_ADDRESS0) {
+ st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ0 | (regval &
+ RES_MASK(AD9834_FREQ_BITS / 2)));
+ st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ0 | ((regval >>
+ (AD9834_FREQ_BITS / 2)) &
+ RES_MASK(AD9834_FREQ_BITS / 2)));
+ } else {
+ st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ1 | (regval &
+ RES_MASK(AD9834_FREQ_BITS / 2)));
+ st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ1 | ((regval >>
+ (AD9834_FREQ_BITS / 2)) &
+ RES_MASK(AD9834_FREQ_BITS / 2)));
+ }
+
+ ret = spi_sync(st->spi, &st->freq_msg);
+ if (ret)
+ return ret;
+
+ st->frequency[(int)addr] = fout;

- return spi_sync(st->spi, &st->freq_msg);
+ return 0;
}

static int ad9834_write_phase(struct ad9834_state *st,
@@ -140,6 +181,39 @@ 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:
+ *val = st->frequency[chan->channel];
+ 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,
+ (enum ad9834_ch_addr)chan->channel,
+ val);
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static ssize_t ad9834_write(struct device *dev,
struct device_attribute *attr,
const char *buf,
@@ -157,10 +231,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 +393,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 +410,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 +427,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 +448,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 +503,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 +546,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, AD9834_CHANNEL_ADDRESS0, 1000000);
if (ret)
goto error_clock_unprepare;

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

--
2.17.1

2019-02-15 00:56:22

by Beniamin Bia

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

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

Signed-off-by: Beniamin Bia <[email protected]>
---
Changes in v2:
-the personal email address was replaced by the work email
-separate define for every phase channel
-enum used for write_phase functions
-phase variables were replaced by an array

drivers/staging/iio/frequency/ad9834.c | 53 ++++++++++++++++----------
1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
index 561617046c20..4366b6121154 100644
--- a/drivers/staging/iio/frequency/ad9834.c
+++ b/drivers/staging/iio/frequency/ad9834.c
@@ -82,6 +82,7 @@ struct ad9834_state {
struct mutex lock; /* protect sensor state */

unsigned long frequency[2];
+ unsigned long phase[2];

/*
* DMA (thus cache coherency maintenance) requires the
@@ -113,6 +114,8 @@ enum ad9834_supported_device_ids {
.output = 1, \
.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[] = {
@@ -172,13 +175,26 @@ static int ad9834_write_frequency(struct ad9834_state *st,
}

static int ad9834_write_phase(struct ad9834_state *st,
- unsigned long addr, unsigned long phase)
+ enum ad9834_ch_addr addr,
+ unsigned long phase)
{
+ int ret;
+
if (phase > BIT(AD9834_PHASE_BITS))
return -EINVAL;
- st->data = cpu_to_be16(addr | phase);

- return spi_sync(st->spi, &st->msg);
+ if (addr == AD9834_CHANNEL_ADDRESS0)
+ st->data = cpu_to_be16(AD9834_REG_PHASE0 | phase);
+ else
+ st->data = cpu_to_be16(AD9834_REG_PHASE1 | phase);
+
+ ret = spi_sync(st->spi, &st->msg);
+ if (ret)
+ return ret;
+
+ st->phase[(int)addr] = phase;
+
+ return 0;
}

static int ad9834_read_raw(struct iio_dev *indio_dev,
@@ -191,6 +207,13 @@ static int ad9834_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_FREQUENCY:
*val = st->frequency[chan->channel];
return IIO_VAL_INT;
+ case IIO_CHAN_INFO_PHASE:
+ *val = st->phase[chan->channel];
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ /*1 hz */
+ *val = 1;
+ return IIO_VAL_INT;
}

return -EINVAL;
@@ -207,6 +230,10 @@ static int ad9834_write_raw(struct iio_dev *indio_dev,
return ad9834_write_frequency(st,
(enum ad9834_ch_addr)chan->channel,
val);
+ case IIO_CHAN_INFO_PHASE:
+ return ad9834_write_phase(st,
+ (enum ad9834_ch_addr)chan->channel,
+ val);
default:
return -EINVAL;
}
@@ -231,10 +258,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 */
@@ -394,12 +417,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);
@@ -410,10 +429,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,
@@ -427,10 +442,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,
@@ -554,11 +565,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, AD9834_CHANNEL_ADDRESS0, 512);
if (ret)
goto error_clock_unprepare;

- ret = ad9834_write_phase(st, AD9834_REG_PHASE1, 1024);
+ ret = ad9834_write_phase(st, AD9834_CHANNEL_ADDRESS1, 1024);
if (ret)
goto error_clock_unprepare;

--
2.17.1

2019-02-20 11:52:32

by Jonathan Cameron

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

On Thu, 14 Feb 2019 18:41:29 +0200
Beniamin Bia <[email protected]> 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. Ad9834 will diverge from ad9833 in the
> future, that is why we have two identical arrays for ad9834 and 9833.
>
> Signed-off-by: Beniamin Bia <[email protected]>
Hi Beniamin

When you make a change like this, please explain in detail how the ABI
changes. Here I'm not sure we can actually map it to channels like this.
We are controlling the two frequencies of FSK not independent channels.

The ABI around this needs very careful thinking out. I would document
your proposed ABI first and share that. Once we reach agreement on the
ABI, the actual code should be more straight forward!

Thanks,

Jonathan

> ---
> Changes in v2:
> -the personal email address was replaced by the work email
> -separate define for frequency channel
> -address field from channel specification was removed
> -frequency variables were replaced by an array
> -specified in comment why we have differente chan_spec for ad9834 and ad9833
> -enum used for write_frequency function
>
> drivers/staging/iio/frequency/ad9834.c | 110 ++++++++++++++++++++-----
> 1 file changed, 91 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
> index f036f75d1f22..561617046c20 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -81,6 +81,8 @@ struct ad9834_state {
> struct spi_message freq_msg;
> struct mutex lock; /* protect sensor state */
>
> + unsigned long frequency[2];
> +
> /*
> * DMA (thus cache coherency maintenance) requires the
> * transfer buffers to live in their own cache lines.
> @@ -89,6 +91,11 @@ struct ad9834_state {
> __be16 freq_data[2];
> };
>
> +enum ad9834_ch_addr {
> + AD9834_CHANNEL_ADDRESS0,
> + AD9834_CHANNEL_ADDRESS1,
> +};
> +
> /**
> * ad9834_supported_device_ids:
> */
> @@ -100,6 +107,24 @@ enum ad9834_supported_device_ids {
> ID_AD9838,
> };
>
> +#define AD9833_CHANNEL(chan) { \
> + .type = IIO_ALTVOLTAGE, \
> + .indexed = 1, \
> + .output = 1, \
> + .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);
> @@ -109,10 +134,12 @@ static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned long fout)
> }
>
> static int ad9834_write_frequency(struct ad9834_state *st,
> - unsigned long addr, unsigned long fout)
> + enum ad9834_ch_addr addr,
> + unsigned long fout)
> {
> unsigned long clk_freq;
> unsigned long regval;
> + int ret;
>
> clk_freq = clk_get_rate(st->mclk);
>
> @@ -121,13 +148,27 @@ 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 &
> - RES_MASK(AD9834_FREQ_BITS / 2)));
> - st->freq_data[1] = cpu_to_be16(addr | ((regval >>
> - (AD9834_FREQ_BITS / 2)) &
> - RES_MASK(AD9834_FREQ_BITS / 2)));
> + if (addr == AD9834_CHANNEL_ADDRESS0) {

So this if statement picks between the two addresses with no other differences?
That's fine, but use a local variable for the register address to simplify
the code.

> + st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ0 | (regval &
> + RES_MASK(AD9834_FREQ_BITS / 2)));
> + st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ0 | ((regval >>
> + (AD9834_FREQ_BITS / 2)) &
> + RES_MASK(AD9834_FREQ_BITS / 2)));
> + } else {
> + st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ1 | (regval &
> + RES_MASK(AD9834_FREQ_BITS / 2)));
> + st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ1 | ((regval >>
> + (AD9834_FREQ_BITS / 2)) &
> + RES_MASK(AD9834_FREQ_BITS / 2)));
> + }
> +
> + ret = spi_sync(st->spi, &st->freq_msg);
> + if (ret)
> + return ret;
> +
> + st->frequency[(int)addr] = fout;
>
> - return spi_sync(st->spi, &st->freq_msg);
> + return 0;
> }
>
> static int ad9834_write_phase(struct ad9834_state *st,
> @@ -140,6 +181,39 @@ 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:
> + *val = st->frequency[chan->channel];
> + 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,
> + (enum ad9834_ch_addr)chan->channel,
> + val);
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static ssize_t ad9834_write(struct device *dev,
> struct device_attribute *attr,
> const char *buf,
> @@ -157,10 +231,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 +393,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 +410,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 +427,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,

This may be a problem... These don't fit in the existing ABI in that they
are (IIRC) two different frequencies for the same channel, not two different
channels... This is meant for simple FSK modulation.

> &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 +448,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 +503,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:

For future reference, I would definitely like to see the options that
lead to here explicitly listed. Semantically it's not a 'default'
but rather an alternative equally valid choice.
People also tend to grep for their devid ;)

> + indio_dev->channels = ad9834_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ad9834_channels);
> indio_dev->info = &ad9834_info;
> break;
> }
> @@ -474,11 +546,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, AD9834_CHANNEL_ADDRESS0, 1000000);
> if (ret)
> goto error_clock_unprepare;
>
> - ret = ad9834_write_frequency(st, AD9834_REG_FREQ1, 5000000);
> + ret = ad9834_write_frequency(st, AD9834_CHANNEL_ADDRESS1, 5000000);
> if (ret)
> goto error_clock_unprepare;
>

2019-02-20 11:54:14

by Jonathan Cameron

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

On Thu, 14 Feb 2019 18:41:30 +0200
Beniamin Bia <[email protected]> wrote:

> The custom phase and scale attributes were moved to standard iio types.
>
> Signed-off-by: Beniamin Bia <[email protected]>
Hi,

This hits the same fundamental questions of ABI as the FSK
elements in the previous patch, just now for Phase Shift Keying.

Unfortunately this ABI element is a strong part of the reason
these DDS chips have languished in staging a long time.
It is not easy to generalize. On that note, we also want to
take into account any other chips that will want to share ABI with
this one as we define it.

Jonathan

> ---
> Changes in v2:
> -the personal email address was replaced by the work email
> -separate define for every phase channel
> -enum used for write_phase functions
> -phase variables were replaced by an array
>
> drivers/staging/iio/frequency/ad9834.c | 53 ++++++++++++++++----------
> 1 file changed, 32 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
> index 561617046c20..4366b6121154 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -82,6 +82,7 @@ struct ad9834_state {
> struct mutex lock; /* protect sensor state */
>
> unsigned long frequency[2];
> + unsigned long phase[2];
>
> /*
> * DMA (thus cache coherency maintenance) requires the
> @@ -113,6 +114,8 @@ enum ad9834_supported_device_ids {
> .output = 1, \
> .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[] = {
> @@ -172,13 +175,26 @@ static int ad9834_write_frequency(struct ad9834_state *st,
> }
>
> static int ad9834_write_phase(struct ad9834_state *st,
> - unsigned long addr, unsigned long phase)
> + enum ad9834_ch_addr addr,
> + unsigned long phase)
> {
> + int ret;
> +
> if (phase > BIT(AD9834_PHASE_BITS))
> return -EINVAL;
> - st->data = cpu_to_be16(addr | phase);
>
> - return spi_sync(st->spi, &st->msg);
> + if (addr == AD9834_CHANNEL_ADDRESS0)
> + st->data = cpu_to_be16(AD9834_REG_PHASE0 | phase);
> + else
> + st->data = cpu_to_be16(AD9834_REG_PHASE1 | phase);
> +
> + ret = spi_sync(st->spi, &st->msg);
> + if (ret)
> + return ret;
> +
> + st->phase[(int)addr] = phase;
> +
> + return 0;
> }
>
> static int ad9834_read_raw(struct iio_dev *indio_dev,
> @@ -191,6 +207,13 @@ static int ad9834_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_FREQUENCY:
> *val = st->frequency[chan->channel];
> return IIO_VAL_INT;
> + case IIO_CHAN_INFO_PHASE:
> + *val = st->phase[chan->channel];
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + /*1 hz */
> + *val = 1;
> + return IIO_VAL_INT;
> }
>
> return -EINVAL;
> @@ -207,6 +230,10 @@ static int ad9834_write_raw(struct iio_dev *indio_dev,
> return ad9834_write_frequency(st,
> (enum ad9834_ch_addr)chan->channel,
> val);
> + case IIO_CHAN_INFO_PHASE:
> + return ad9834_write_phase(st,
> + (enum ad9834_ch_addr)chan->channel,
> + val);
> default:
> return -EINVAL;
> }
> @@ -231,10 +258,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 */
> @@ -394,12 +417,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);
> @@ -410,10 +429,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,
> @@ -427,10 +442,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,
> @@ -554,11 +565,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, AD9834_CHANNEL_ADDRESS0, 512);
> if (ret)
> goto error_clock_unprepare;
>
> - ret = ad9834_write_phase(st, AD9834_REG_PHASE1, 1024);
> + ret = ad9834_write_phase(st, AD9834_CHANNEL_ADDRESS1, 1024);
> if (ret)
> goto error_clock_unprepare;
>

2019-02-22 13:11:20

by Beniamin Bia

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

Thank you for taking some time reviewing my code.
Ad9833/ad9834 has two frequency and phase registers and only one
output. The user can select which register is selected as input with a
mux.Because I wanted to reduce as much as possible the custom
attributes, I mapped the frequency 0 from register to channel 0 and
frequency 1 to channel 1. Same rule applies for phase.

In order to keep the same structure as before: out_altvoltage0_frequency0 and out_altvoltage1_frequency1, we would have to create custom attributes for every frequency and phase value. This is the only way as far as I know, but I don't have a lot of experience with iio. What is your opinion? Do you have a better solution?

Thanks,
Ben
> [External]
>
>
> On Thu, 14 Feb 2019 18:41:29 +0200
> Beniamin Bia <[email protected]> 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. Ad9834 will diverge from ad9833 in
> > the
> > future, that is why we have two identical arrays for ad9834 and
> > 9833.
> >
> > Signed-off-by: Beniamin Bia <[email protected]>
>
> Hi Beniamin
>
> When you make a change like this, please explain in detail how the
> ABI
> changes. Here I'm not sure we can actually map it to channels like
> this.
> We are controlling the two frequencies of FSK not independent
> channels.
>
> The ABI around this needs very careful thinking out. I would
> document
> your proposed ABI first and share that. Once we reach agreement on
> the
> ABI, the actual code should be more straight forward!
>
> Thanks,
>
> Jonathan
>
> > ---
> > Changes in v2:
> > -the personal email address was replaced by the work email
> > -separate define for frequency channel
> > -address field from channel specification was removed
> > -frequency variables were replaced by an array
> > -specified in comment why we have differente chan_spec for
> > ad9834 and ad9833
> > -enum used for write_frequency function
> >
> > drivers/staging/iio/frequency/ad9834.c | 110 ++++++++++++++++++++-
> > ----
> > 1 file changed, 91 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/staging/iio/frequency/ad9834.c
> > b/drivers/staging/iio/frequency/ad9834.c
> > index f036f75d1f22..561617046c20 100644
> > --- a/drivers/staging/iio/frequency/ad9834.c
> > +++ b/drivers/staging/iio/frequency/ad9834.c
> > @@ -81,6 +81,8 @@ struct ad9834_state {
> > struct spi_message freq_msg;
> > struct mutex lock; /* protect sensor
> > state */
> >
> > + unsigned long frequency[2];
> > +
> > /*
> > * DMA (thus cache coherency maintenance) requires the
> > * transfer buffers to live in their own cache lines.
> > @@ -89,6 +91,11 @@ struct ad9834_state {
> > __be16 freq_data[2];
> > };
> >
> > +enum ad9834_ch_addr {
> > + AD9834_CHANNEL_ADDRESS0,
> > + AD9834_CHANNEL_ADDRESS1,
> > +};
> > +
> > /**
> > * ad9834_supported_device_ids:
> > */
> > @@ -100,6 +107,24 @@ enum ad9834_supported_device_ids {
> > ID_AD9838,
> > };
> >
> > +#define AD9833_CHANNEL(chan)
> > { \
> > + .type =
> > IIO_ALTVOLTAGE, \
> > + .indexed =
> > 1, \
> > + .output =
> > 1, \
> > + .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);
> > @@ -109,10 +134,12 @@ static unsigned int
> > ad9834_calc_freqreg(unsigned long mclk, unsigned long fout)
> > }
> >
> > static int ad9834_write_frequency(struct ad9834_state *st,
> > - unsigned long addr, unsigned long
> > fout)
> > + enum ad9834_ch_addr addr,
> > + unsigned long fout)
> > {
> > unsigned long clk_freq;
> > unsigned long regval;
> > + int ret;
> >
> > clk_freq = clk_get_rate(st->mclk);
> >
> > @@ -121,13 +148,27 @@ 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 &
> > - RES_MASK(AD9834_FREQ_BITS /
> > 2)));
> > - st->freq_data[1] = cpu_to_be16(addr | ((regval >>
> > - (AD9834_FREQ_BITS / 2)) &
> > - RES_MASK(AD9834_FREQ_BITS /
> > 2)));
> > + if (addr == AD9834_CHANNEL_ADDRESS0) {
>
> So this if statement picks between the two addresses with no other
> differences?
> That's fine, but use a local variable for the register address to
> simplify
> the code.
>
> > + st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ0 |
> > (regval &
> > + RES_MASK(AD9834_FREQ_B
> > ITS / 2)));
> > + st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ0 |
> > ((regval >>
> > + (AD9834_FREQ_BITS /
> > 2)) &
> > + RES_MASK(AD9834_FREQ_B
> > ITS / 2)));
> > + } else {
> > + st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ1 |
> > (regval &
> > + RES_MASK(AD9834_FREQ_B
> > ITS / 2)));
> > + st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ1 |
> > ((regval >>
> > + (AD9834_FREQ_BITS /
> > 2)) &
> > + RES_MASK(AD9834_FREQ_B
> > ITS / 2)));
> > + }
> > +
> > + ret = spi_sync(st->spi, &st->freq_msg);
> > + if (ret)
> > + return ret;
> > +
> > + st->frequency[(int)addr] = fout;
> >
> > - return spi_sync(st->spi, &st->freq_msg);
> > + return 0;
> > }
> >
> > static int ad9834_write_phase(struct ad9834_state *st,
> > @@ -140,6 +181,39 @@ 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:
> > + *val = st->frequency[chan->channel];
> > + 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,
> > + (enum
> > ad9834_ch_addr)chan->channel,
> > + val);
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static ssize_t ad9834_write(struct device *dev,
> > struct device_attribute *attr,
> > const char *buf,
> > @@ -157,10 +231,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 +393,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 +410,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 +427,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,
>
> This may be a problem... These don't fit in the existing ABI in that
> they
> are (IIRC) two different frequencies for the same channel, not two
> different
> channels... This is meant for simple FSK modulation.
>
> > &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 +448,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 +503,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:
>
> For future reference, I would definitely like to see the options that
> lead to here explicitly listed. Semantically it's not a 'default'
> but rather an alternative equally valid choice.
> People also tend to grep for their devid ;)
>
> > + indio_dev->channels = ad9834_channels;
> > + indio_dev->num_channels =
> > ARRAY_SIZE(ad9834_channels);
> > indio_dev->info = &ad9834_info;
> > break;
> > }
> > @@ -474,11 +546,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, AD9834_CHANNEL_ADDRESS0,
> > 1000000);
> > if (ret)
> > goto error_clock_unprepare;
> >
> > - ret = ad9834_write_frequency(st, AD9834_REG_FREQ1, 5000000);
> > + ret = ad9834_write_frequency(st, AD9834_CHANNEL_ADDRESS1,
> > 5000000);
> > if (ret)
> > goto error_clock_unprepare;
> >
>
>

2019-03-03 17:03:25

by Jonathan Cameron

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

On Fri, 22 Feb 2019 13:10:26 +0000
"Bia, Beniamin" <[email protected]> wrote:

> Thank you for taking some time reviewing my code.
> Ad9833/ad9834 has two frequency and phase registers and only one
> output. The user can select which register is selected as input with a
> mux.Because I wanted to reduce as much as possible the custom
> attributes, I mapped the frequency 0 from register to channel 0 and
> frequency 1 to channel 1. Same rule applies for phase.
I can see the argument, but really don't think this ends up as an
intuitive mapping. I think we need something new to handle
values that change with 'symbol'.

>
> In order to keep the same structure as before:
> out_altvoltage0_frequency0 and out_altvoltage1_frequency1,
> we would have to create custom attributes for every frequency and phase value.
> This is the only way as far as I know, but I don't have a lot of experience with iio. What is your opinion? Do you have a better solution?

Sorry it took me so long to reply to this.

So, we have had similar issues before, particularly around power
meters where we need a 'broader' concept of index to allow for multiple
related measurements.

However, in this case I think we can sort of think of the values being
aspects controlled by a symbol, which might map onto an 'axis' for
example (stretching the definition).

So maybe define some new modifiers, symbolA, symbolB for example giving
us
out_altvoltage0_symbolA_frequency
out_altvoltage0_symbolB_frequency.

This is a discussion I'd like others to contribute to though as
interface design is always a pain to get right!



>
> Thanks,
> Ben
> > [External]
> >
> >
> > On Thu, 14 Feb 2019 18:41:29 +0200
> > Beniamin Bia <[email protected]> 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. Ad9834 will diverge from ad9833 in
> > > the
> > > future, that is why we have two identical arrays for ad9834 and
> > > 9833.
> > >
> > > Signed-off-by: Beniamin Bia <[email protected]>
> >
> > Hi Beniamin
> >
> > When you make a change like this, please explain in detail how the
> > ABI
> > changes. Here I'm not sure we can actually map it to channels like
> > this.
> > We are controlling the two frequencies of FSK not independent
> > channels.
> >
> > The ABI around this needs very careful thinking out. I would
> > document
> > your proposed ABI first and share that. Once we reach agreement on
> > the
> > ABI, the actual code should be more straight forward!
> >
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > Changes in v2:
> > > -the personal email address was replaced by the work email
> > > -separate define for frequency channel
> > > -address field from channel specification was removed
> > > -frequency variables were replaced by an array
> > > -specified in comment why we have differente chan_spec for
> > > ad9834 and ad9833
> > > -enum used for write_frequency function
> > >
> > > drivers/staging/iio/frequency/ad9834.c | 110 ++++++++++++++++++++-
> > > ----
> > > 1 file changed, 91 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/frequency/ad9834.c
> > > b/drivers/staging/iio/frequency/ad9834.c
> > > index f036f75d1f22..561617046c20 100644
> > > --- a/drivers/staging/iio/frequency/ad9834.c
> > > +++ b/drivers/staging/iio/frequency/ad9834.c
> > > @@ -81,6 +81,8 @@ struct ad9834_state {
> > > struct spi_message freq_msg;
> > > struct mutex lock; /* protect sensor
> > > state */
> > >
> > > + unsigned long frequency[2];
> > > +
> > > /*
> > > * DMA (thus cache coherency maintenance) requires the
> > > * transfer buffers to live in their own cache lines.
> > > @@ -89,6 +91,11 @@ struct ad9834_state {
> > > __be16 freq_data[2];
> > > };
> > >
> > > +enum ad9834_ch_addr {
> > > + AD9834_CHANNEL_ADDRESS0,
> > > + AD9834_CHANNEL_ADDRESS1,
> > > +};
> > > +
> > > /**
> > > * ad9834_supported_device_ids:
> > > */
> > > @@ -100,6 +107,24 @@ enum ad9834_supported_device_ids {
> > > ID_AD9838,
> > > };
> > >
> > > +#define AD9833_CHANNEL(chan)
> > > { \
> > > + .type =
> > > IIO_ALTVOLTAGE, \
> > > + .indexed =
> > > 1, \
> > > + .output =
> > > 1, \
> > > + .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);
> > > @@ -109,10 +134,12 @@ static unsigned int
> > > ad9834_calc_freqreg(unsigned long mclk, unsigned long fout)
> > > }
> > >
> > > static int ad9834_write_frequency(struct ad9834_state *st,
> > > - unsigned long addr, unsigned long
> > > fout)
> > > + enum ad9834_ch_addr addr,
> > > + unsigned long fout)
> > > {
> > > unsigned long clk_freq;
> > > unsigned long regval;
> > > + int ret;
> > >
> > > clk_freq = clk_get_rate(st->mclk);
> > >
> > > @@ -121,13 +148,27 @@ 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 &
> > > - RES_MASK(AD9834_FREQ_BITS /
> > > 2)));
> > > - st->freq_data[1] = cpu_to_be16(addr | ((regval >>
> > > - (AD9834_FREQ_BITS / 2)) &
> > > - RES_MASK(AD9834_FREQ_BITS /
> > > 2)));
> > > + if (addr == AD9834_CHANNEL_ADDRESS0) {
> >
> > So this if statement picks between the two addresses with no other
> > differences?
> > That's fine, but use a local variable for the register address to
> > simplify
> > the code.
> >
> > > + st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ0 |
> > > (regval &
> > > + RES_MASK(AD9834_FREQ_B
> > > ITS / 2)));
> > > + st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ0 |
> > > ((regval >>
> > > + (AD9834_FREQ_BITS /
> > > 2)) &
> > > + RES_MASK(AD9834_FREQ_B
> > > ITS / 2)));
> > > + } else {
> > > + st->freq_data[0] = cpu_to_be16(AD9834_REG_FREQ1 |
> > > (regval &
> > > + RES_MASK(AD9834_FREQ_B
> > > ITS / 2)));
> > > + st->freq_data[1] = cpu_to_be16(AD9834_REG_FREQ1 |
> > > ((regval >>
> > > + (AD9834_FREQ_BITS /
> > > 2)) &
> > > + RES_MASK(AD9834_FREQ_B
> > > ITS / 2)));
> > > + }
> > > +
> > > + ret = spi_sync(st->spi, &st->freq_msg);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + st->frequency[(int)addr] = fout;
> > >
> > > - return spi_sync(st->spi, &st->freq_msg);
> > > + return 0;
> > > }
> > >
> > > static int ad9834_write_phase(struct ad9834_state *st,
> > > @@ -140,6 +181,39 @@ 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:
> > > + *val = st->frequency[chan->channel];
> > > + 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,
> > > + (enum
> > > ad9834_ch_addr)chan->channel,
> > > + val);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static ssize_t ad9834_write(struct device *dev,
> > > struct device_attribute *attr,
> > > const char *buf,
> > > @@ -157,10 +231,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 +393,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 +410,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 +427,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,
> >
> > This may be a problem... These don't fit in the existing ABI in that
> > they
> > are (IIRC) two different frequencies for the same channel, not two
> > different
> > channels... This is meant for simple FSK modulation.
> >
> > > &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 +448,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 +503,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:
> >
> > For future reference, I would definitely like to see the options that
> > lead to here explicitly listed. Semantically it's not a 'default'
> > but rather an alternative equally valid choice.
> > People also tend to grep for their devid ;)
> >
> > > + indio_dev->channels = ad9834_channels;
> > > + indio_dev->num_channels =
> > > ARRAY_SIZE(ad9834_channels);
> > > indio_dev->info = &ad9834_info;
> > > break;
> > > }
> > > @@ -474,11 +546,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, AD9834_CHANNEL_ADDRESS0,
> > > 1000000);
> > > if (ret)
> > > goto error_clock_unprepare;
> > >
> > > - ret = ad9834_write_frequency(st, AD9834_REG_FREQ1, 5000000);
> > > + ret = ad9834_write_frequency(st, AD9834_CHANNEL_ADDRESS1,
> > > 5000000);
> > > if (ret)
> > > goto error_clock_unprepare;
> > >
> >
> >