Re: [PATCH v2 1/2] staging: iio: frequency: ad9834: Move frequency to standard iio types
From: Bia, Beniamin
Date: Fri Feb 22 2019 - 08:10:44 EST
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 <beniamin.bia@xxxxxxxxxx> 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 <beniamin.bia@xxxxxxxxxx>
>
> 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;
> >
>
>