RE: [RFC 3/3] IIO: DDS: AD9833 / AD9834 driver

[Datta, Shubhrajyoti]

Michel,
Just a thought not a comment. However I think error handling could be dealt later.

> -----Original Message-----
> From: Hennerich, Michael [mailto:Michael.Hennerich@xxxxxxxxxx]
> Sent: Friday, December 03, 2010 4:13 PM
> To: Datta, Shubhrajyoti; linux-iio@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Cc: Drivers; jic23@xxxxxxxxx; device-drivers-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: RE: [RFC 3/3] IIO: DDS: AD9833 / AD9834 driver
> 
> > Datta, Shubhrajyoti wrote on 2010-12-02:
> > > -----Original Message-----
> > > From: linux-iio-owner@xxxxxxxxxxxxxxx [mailto:linux-iio-
> > > owner@xxxxxxxxxxxxxxx] On Behalf Of michael.hennerich@xxxxxxxxxx
> > > Sent: Thursday, December 02, 2010 5:51 PM
> > > To: linux-iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > Cc: drivers@xxxxxxxxxx; jic23@xxxxxxxxx; device-drivers-
> > > devel@xxxxxxxxxxxxxxxxxxxx; Michael Hennerich
> > > Subject: [RFC 3/3] IIO: DDS: AD9833 / AD9834 driver
> > >
> > > From: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
> > >
> > > Example driver using the proposed ABI
> > >
> > > Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
> > > ---
> > >  drivers/staging/iio/dds/Kconfig  |    7 +
> > >  drivers/staging/iio/dds/Makefile |    1 +
> > >  drivers/staging/iio/dds/ad9834.c |  483
> > > ++++++++++++++++++++++++++++++++++++++
> > >  drivers/staging/iio/dds/ad9834.h |  112 +++++++++
> > >  4 files changed, 603 insertions(+), 0 deletions(-)
> > >  create mode 100644 drivers/staging/iio/dds/ad9834.c
> > >  create mode 100644 drivers/staging/iio/dds/ad9834.h
> > >
> > > diff --git a/drivers/staging/iio/dds/Kconfig
> > > b/drivers/staging/iio/dds/Kconfig
> > > index 7969be2..4c9cce3 100644
> > > --- a/drivers/staging/iio/dds/Kconfig
> > > +++ b/drivers/staging/iio/dds/Kconfig
> > > @@ -17,6 +17,13 @@ config AD9832
> > >         Say yes here to build support for Analog Devices DDS chip
> > >         ad9832 and ad9835, provides direct access via sysfs.
> > >
> > > +config AD9834
> > > +     tristate "Analog Devices ad9833/4/ driver"
> > > +     depends on SPI
> > > +     help
> > > +       Say yes here to build support for Analog Devices DDS chip
> > > +       AD9833 and AD9834, provides direct access via sysfs.
> > > +
> > >  config AD9850
> > >       tristate "Analog Devices ad9850/1 driver"
> > >       depends on SPI
> > > diff --git a/drivers/staging/iio/dds/Makefile
> > > b/drivers/staging/iio/dds/Makefile
> > > index 6f274ac..1477461 100644
> > > --- a/drivers/staging/iio/dds/Makefile
> > > +++ b/drivers/staging/iio/dds/Makefile
> > > @@ -4,6 +4,7 @@
> > >
> > >  obj-$(CONFIG_AD5930) += ad5930.o
> > >  obj-$(CONFIG_AD9832) += ad9832.o
> > > +obj-$(CONFIG_AD9834) += ad9834.o
> > >  obj-$(CONFIG_AD9850) += ad9850.o
> > >  obj-$(CONFIG_AD9852) += ad9852.o
> > >  obj-$(CONFIG_AD9910) += ad9910.o
> > > diff --git a/drivers/staging/iio/dds/ad9834.c
> > > b/drivers/staging/iio/dds/ad9834.c
> > > new file mode 100644
> > > index 0000000..eb1fabf
> > > --- /dev/null
> > > +++ b/drivers/staging/iio/dds/ad9834.c
> > > @@ -0,0 +1,483 @@
> > > +/*
> > > + * AD9834 SPI DAC driver
> > > + *
> > > + * Copyright 2010 Analog Devices Inc.
> > > + *
> > > + * Licensed under the GPL-2 or later.
> > > + */
> > > +
> > > +#include <linux/interrupt.h>
> > > +#include <linux/workqueue.h>
> > > +#include <linux/device.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/sysfs.h>
> > > +#include <linux/list.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/err.h>
> > > +#include <asm/div64.h>
> > > +
> > > +#include "../iio.h"
> > > +#include "../sysfs.h"
> > > +#include "dds.h"
> > > +
> > > +#include "ad9834.h"
> > > +
> > > +static unsigned int ad9834_calc_freqreg(unsigned long mclk, unsigned
> > long
> > > fout)
> > > +{
> > > +     unsigned long long freqreg = (u64) fout * (u64) (1 <<
> > > AD9834_FREQ_BITS);
> > > +     do_div(freqreg, mclk);
> > > +     return freqreg;
> > > +}
> > > +
> > > +static int ad9834_write_frequency(struct ad9834_state *st,
> > > +                               unsigned long addr, unsigned long
> > fout)
> > > +{
> > > +     unsigned long regval;
> > > +
> > > +     if (fout > (st->mclk / 2))
> > > +             return -EINVAL;
> > > +
> > > +     regval = ad9834_calc_freqreg(st->mclk, 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)));
> > > +
> > > +     return spi_sync(st->spi, &st->freq_msg);;
> > > +}
> > > +
> > > +static int ad9834_write_phase(struct ad9834_state *st,
> > > +                               unsigned long addr, unsigned long
> > phase)
> > > +{
> > > +     if (phase > (1 << AD9834_PHASE_BITS))
> > > +             return -EINVAL;
> > > +     st->data = cpu_to_be16(addr | phase);
> > > +
> > > +     return spi_sync(st->spi, &st->msg);
> > > +}
> > > +
> > > +static ssize_t ad9834_write(struct device *dev,
> > > +             struct device_attribute *attr,
> > > +             const char *buf,
> > > +             size_t len)
> > > +{
> > > +     struct iio_dev *dev_info = dev_get_drvdata(dev);
> > > +     struct ad9834_state *st = dev_info->dev_data;
> > > +     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > > +     int ret;
> > > +     long val;
> > > +
> > > +     ret = strict_strtol(buf, 10, &val);
> > > +     if (ret)
> > > +             goto error_ret;
> > Could we do some bounds check.
> The functions called with val as argument from the switch cases below, do
> some bound checking, where necessary.
> It's questionable whether these simple enable switch cases, should return
> with
> error if someone writes something other than 0 or 1.

In that case we could we at least consider ret = strict_strtoul(buf, 10, &val);

> > > +
> > > +     mutex_lock(&dev_info->mlock);
> > > +     switch (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);
> > > +             break;
> > > +
> > > +     case AD9834_OPBITEN:
> > > +             if (st->control & AD9834_MODE) {
> > > +                     ret = -EINVAL;  /* AD9843 reserved mode */
> > > +                     break;
> > > +             }
> > > +
> > > +             if (val)
> > > +                     st->control &= ~AD9834_OPBITEN;
> > > +             else
> > > +                     st->control |= AD9834_OPBITEN;
Here you check for something other than 0 if the user passes -1 then it may go through.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/