Re: [PATCH] IIO: ADC: New driver for AD7606/AD7606-6/AD7606-4

[Jonathan Cameron <jic23@cam.ac.uk>]

On 02/04/11 14:27, Hennerich, Michael wrote:
>> From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
>> On 02/03/11 14:51, michael.hennerich@analog.com wrote:
>>> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> [--snip--]
> 
>>> +
>>> +What:              /sys/bus/iio/devices/deviceX/range_available
>>> +KernelVersion:     2.6.38
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Hardware dependent supported vales for ADC Full Scale
>> Range.
>>> +
>>> +What:              /sys/bus/iio/devices/deviceX/oversampling
>>> +KernelVersion:     2.6.38
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Hardware dependent ADC oversamplimg.
>> Typo.
>>> Controls the sampling ratio
>>> +           of the digital filter if available.
>> What are the units?  The obvious choice is a multiplier, but could be
>> frequency.
>> Either way, needs to be specified here.
> 
> 'Ratio' or 'rate' is the best you can use here.
> http://mathworld.wolfram.com/Oversampling.html
Then lets call it oversampling_ratio and make this very obvious.
> 
>>> +
>>> +What:              /sys/bus/iio/devices/deviceX/oversampling_available
>>> +KernelVersion:     2.6.38
>>> +Contact:   linux-iio@vger.kernel.org
>>> +Description:
>>> +           Hardware dependent supported vales
>> values (and should probably be Hardware dependent values supported by
>> the
>> oversampling filter.)
>>> by the oversampling filter.
>>> +
<snip>

>>> index 0000000..21e5af8
>>> --- /dev/null
>>> +++ b/drivers/staging/iio/adc/ad7606_core.c
>>> @@ -0,0 +1,560 @@
>>> +/*
>>> + * AD7606 SPI ADC driver
>>> + *
>>> + * Copyright 2011 Analog Devices Inc.
>>> + *
>>> + * Licensed under the GPL-2.
>>> + */
>>> +
>>> +#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/regulator/consumer.h>
>>> +#include <linux/err.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/delay.h>
>>> +
>>> +#include "../iio.h"
>>> +#include "../sysfs.h"
>>> +#include "../ring_generic.h"
>>> +#include "adc.h"
>>> +
>>> +#include "ad7606.h"
>>> +
>>> +int ad7606_reset(struct ad7606_state *st)
>>> +{
>>> +   if (st->have_reset) {
>>> +           gpio_set_value(st->pdata->gpio_reset, 1);
>>> +           ndelay(100); /* t_reset >= 100ns */
>> could it be a sleep?
> 
> Only 100ns are required!
> Why would I go to sleep?
Fair point.
> 
> 
>>> +           gpio_set_value(st->pdata->gpio_reset, 0);
>>> +           return 0;
>>> +   }
>>> +
>>> +   return -ENODEV;
>>> +}
>>> +
>>> +static int ad7606_scan_direct(struct ad7606_state *st, unsigned ch)
>>> +{
>>> +   int ret;
>>> +
>>> +   gpio_set_value(st->pdata->gpio_convst, 1);
>>> +   st->done = false;
>>> +
>>> +   ret = wait_event_interruptible(st->wq_data_avail, st->done);
>>> +   if (ret)
>>> +           goto error_ret;
>>> +
>>> +   if (st->have_frstdata) {
>>> +           st->bops->read_block(st->dev, 1, st->data);
>>> +           if (!gpio_get_value(st->pdata->gpio_frstdata)) {
>>> +                   /* This should never happen */
>>                       As below.... does it and if so why?
> 
> In case it happens we're out of sync.
> 
> Most people won't use FRSTDATA checks at all.
> 
> However some signal glitch caused perhaps by some electrostatic
> discharge, could cause an extra read or clock.
> This allows recovery.
Can you add that to the comment please for future readers of the code.
> 
>>> +                   ad7606_reset(st);
>>> +                   ret = -EIO;
>>> +                   goto error_ret;
>>> +           }
<snip>