On 11/21/2014 10:15 PM, Hartmut Knaack wrote:
> Ezequiel Garcia schrieb am 13.11.2014 15:13:
>> From: Phani Movva <Phani.Movva@xxxxxxxxxx>
>>
>> This commit adds support for Cosmic Circuits 10001 10-bit ADC device.
> A few more issues - some got newly introduced, some have been overseen earlier (and some are just matter of taste ;-)
OK, let's see then.
>>
>> Signed-off-by: Phani Movva <Phani.Movva@xxxxxxxxxx>
>> Signed-off-by: Naidu Tellapati <Naidu.Tellapati@xxxxxxxxxx>
>> [Ezequiel: code style cleaning]
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@xxxxxxxxxx>
>> ---
>> drivers/iio/adc/Kconfig | 11 ++
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/cc10001_adc.c | 425 ++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 437 insertions(+)
>> create mode 100644 drivers/iio/adc/cc10001_adc.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 88bdc8f..09e2975 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -127,6 +127,17 @@ config AT91_ADC
>> help
>> Say yes here to build support for Atmel AT91 ADC.
>>
>> +config CC10001_ADC
>> + tristate "Cosmic Circuits 10001 ADC driver"
>> + depends on HAS_IOMEM
>> + select IIO_BUFFER
>> + select IIO_TRIGGERED_BUFFER
>> + help
>> + Say yes here to build support for Cosmic Circuits 10001 ADC.
>> +
>> + This driver can also be built as a module. If so, the module will be
>> + called cc10001_adc.
>> +
>> config EXYNOS_ADC
>> tristate "Exynos ADC driver support"
>> depends on ARCH_EXYNOS || ARCH_S3C24XX || ARCH_S3C64XX || (OF && COMPILE_TEST)
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index cb88a6a..9286c59 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
>> obj-$(CONFIG_AD7887) += ad7887.o
>> obj-$(CONFIG_AD799X) += ad799x.o
>> obj-$(CONFIG_AT91_ADC) += at91_adc.o
>> +obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>> obj-$(CONFIG_MAX1027) += max1027.o
>> diff --git a/drivers/iio/adc/cc10001_adc.c b/drivers/iio/adc/cc10001_adc.c
>> new file mode 100644
>> index 0000000..0b74838
>> --- /dev/null
>> +++ b/drivers/iio/adc/cc10001_adc.c
>> @@ -0,0 +1,425 @@
>> +/*
>> + * Copyright (c) 2014 Imagination Technologies Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/slab.h>
>> +
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +
>> +/* Registers */
>> +#define CC10001_ADC_CONFIG 0x00
>> +#define CC10001_ADC_START_CONV BIT(4)
>> +#define CC10001_ADC_MODE_SINGLE_CONV BIT(5)
>> +
>> +#define CC10001_ADC_DDATA_OUT 0x04
>> +#define CC10001_ADC_EOC 0x08
>> +#define CC10001_ADC_EOC_SET BIT(0)
>> +
>> +#define CC10001_ADC_CHSEL_SAMPLED 0x0c
>> +#define CC10001_ADC_POWER_UP 0x10
>> +#define CC10001_ADC_POWER_UP_SET BIT(0)
>> +#define CC10001_ADC_DEBUG 0x14
>> +#define CC10001_ADC_DATA_COUNT 0x20
>> +
>> +#define CC10001_ADC_DATA_MASK GENMASK(9, 0)
>> +#define CC10001_ADC_NUM_CHANNELS 8
>> +#define CC10001_ADC_CH_MASK (CC10001_ADC_NUM_CHANNELS - 1)
> Don't define CC10001_ADC_CH_MASK this way, as it implies a simple relation to CC10001_ADC_NUM_CHANNELS, but it actually requires it to be base 2 (which it happens to be in this case). Preferably use GENMASK(2, 0).
Right.
>> +
>> +#define CC10001_INVALID_SAMPLED_OUTPUT 0xFFFF
> After changing your hex values to lower case, this one is still left.
Right.
>> +#define CC10001_MAX_POLL_COUNT 20
>> +
>> +/*
>> + * As per device specification, wait six clock cycles after power-up to
>> + * activate START. Since adding two more clock cycles delay does not
>> + * impact the performance too much, we are adding two additional cycles delay
>> + * intentionally here.
>> + */
>> +#define CC10001_WAIT_CYCLES 8
>> +
>> +struct cc10001_adc_device {
>> + void __iomem *reg_base;
>> + struct iio_dev *indio_dev;
> This element is never used.
Right.
>> + struct clk *adc_clk;
>> + struct regulator *reg;
>> + u16 *buf;
>> +
>> + struct mutex lock;
>> + unsigned long channel_map;
>> + unsigned int start_delay_ns;
>> + unsigned int eoc_delay_ns;
>> +};
>> +
>> +static inline void cc10001_adc_write_reg(struct cc10001_adc_device *adc_dev,
>> + u32 reg, u32 val)
>> +{
>> + writel(val, adc_dev->reg_base + reg);
>> +}
>> +
>> +static inline u32 cc10001_adc_read_reg(struct cc10001_adc_device *adc_dev,
>> + u32 reg)
>> +{
>> + return readl(adc_dev->reg_base + reg);
>> +}
>> +
>> +static void cc10001_adc_start(struct cc10001_adc_device *adc_dev,
>> + unsigned int channel)
>> +{
>> + u32 val;
>> +
>> + /* Channel selection and mode of operation */
>> + val = (channel & CC10001_ADC_CH_MASK) | CC10001_ADC_MODE_SINGLE_CONV;
>> + cc10001_adc_write_reg(adc_dev, CC10001_ADC_CONFIG, val);
>> +
>> + val = cc10001_adc_read_reg(adc_dev, CC10001_ADC_CONFIG);
>> + val = val | CC10001_ADC_START_CONV;
>> + cc10001_adc_write_reg(adc_dev, CC10001_ADC_CONFIG, val);
>> +}
>> +
>> +static u16 cc10001_adc_poll_done(struct iio_dev *indio_dev,
>> + unsigned int channel,
>> + unsigned int delay)
>> +{
>> + struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
>> + unsigned int poll_count = 0;
>> +
>> + while (!(cc10001_adc_read_reg(adc_dev, CC10001_ADC_EOC) &
>> + CC10001_ADC_EOC_SET)) {
> Matter of taste case here, and I understand that indenting it to align with cc10001_adc_read_reg would look like the indentation of the following subroutine. Then again, you left an empty line to show a certain distinction. Up to you, I just slightly prefer to align blocks with equal hierarchy levels.
>> +
>> + ndelay(delay);
>> + if (poll_count++ == CC10001_MAX_POLL_COUNT)
>> + return CC10001_INVALID_SAMPLED_OUTPUT;
>> + }
>> +
>> + poll_count = 0;
>> + while ((cc10001_adc_read_reg(adc_dev, CC10001_ADC_CHSEL_SAMPLED) &
>> + CC10001_ADC_CH_MASK) != channel) {
> Same here.
>> +
>> + ndelay(delay);
>> + if (poll_count++ == CC10001_MAX_POLL_COUNT)
>> + return CC10001_INVALID_SAMPLED_OUTPUT;
>> + }
>> +
>> + /* Read the 10 bit output register */
>> + return cc10001_adc_read_reg(adc_dev, CC10001_ADC_DDATA_OUT) &
>> + CC10001_ADC_DATA_MASK;
> Here I feel stronger about alignment.
>> +}
>> +
>> +static irqreturn_t cc10001_adc_trigger_h(int irq, void *p)
>> +{
>> + struct cc10001_adc_device *adc_dev;
>> + struct iio_poll_func *pf = p;
>> + struct iio_dev *indio_dev;
>> + unsigned int delay_ns;
>> + unsigned int channel;
>> + bool sample_invalid;
>> + u16 *data;
>> + int i;
>> +
>> + indio_dev = pf->indio_dev;
>> + adc_dev = iio_priv(indio_dev);
>> + data = adc_dev->buf;
>> +
>> + mutex_lock(&adc_dev->lock);
>> +
>> + cc10001_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP,
>> + CC10001_ADC_POWER_UP_SET);
>> +
>> + /* Wait for 8 (6+2) clock cycles before activating START */
>> + ndelay(adc_dev->start_delay_ns);
>> +
>> + /* Calculate delay step for eoc and sampled data */
>> + delay_ns = adc_dev->eoc_delay_ns / CC10001_MAX_POLL_COUNT;
>> +
>> + i = 0;
>> + sample_invalid = false;
> These definitions could be done already during declaration.
Yeah, I guess I felt it was more readable this way. But I don't care much.
>> + for_each_set_bit(channel, indio_dev->active_scan_mask,
>> + indio_dev->masklength) {
> Too much indentation, it should be aligned with channel.
Ditto.
>> +
>> + cc10001_adc_start(adc_dev, channel);
>> +
>> + data[i] = cc10001_adc_poll_done(indio_dev, channel, delay_ns);
>> + if (data[i] == CC10001_INVALID_SAMPLED_OUTPUT) {
>> + dev_warn(&indio_dev->dev,
>> + "invalid sample on channel %d\n", channel);
>> + sample_invalid = true;
>> + goto done;
>> + }
>> + i++;
>> + }
>> +
>> +done:
>> + cc10001_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP, 0);
>> +
>> + mutex_unlock(&adc_dev->lock);
>> +
>> + if (!sample_invalid)
>> + iio_push_to_buffers_with_timestamp(indio_dev, data,
>> + iio_get_time_ns());
>> + iio_trigger_notify_done(indio_dev->trig);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static u16 cc10001_adc_read_raw_voltage(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan)
>> +{
>> + struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
>> + unsigned int delay_ns;
>> + u16 val;
>> +
>> + cc10001_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP,
>> + CC10001_ADC_POWER_UP_SET);
>> +
>> + /* Wait for 8 (6+2) clock cycles before activating START */
>> + ndelay(adc_dev->start_delay_ns);
>> +
>> + /* Calculate delay step for eoc and sampled data */
>> + delay_ns = adc_dev->eoc_delay_ns / CC10001_MAX_POLL_COUNT;
>> +
>> + cc10001_adc_start(adc_dev, chan->channel);
>> +
>> + val = cc10001_adc_poll_done(indio_dev, chan->channel, delay_ns);
>> +
>> + cc10001_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP, 0);
>> +
>> + return val;
>> +}
>> +
>> +static int cc10001_adc_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + if (iio_buffer_enabled(indio_dev))
>> + return -EBUSY;
>> + mutex_lock(&adc_dev->lock);
>> + *val = cc10001_adc_read_raw_voltage(indio_dev, chan);
>> + mutex_unlock(&adc_dev->lock);
>> +
>> + if (*val == CC10001_INVALID_SAMPLED_OUTPUT)
>> + return -EIO;
>> + return IIO_VAL_INT;
> Since C offers it:
> return (*val == CC10001_INVALID_SAMPLED_OUTPUT) ? -EIO : IIO_VAL_INT;
Hm, don't you find this a bit eyesore?
> It would fit in one line if CC10001_INVALID_SAMPLED_OUTPUT was called CC10001_INVALID_SAMPLE ;-)
Right, that's a better name indeed.
>> +
>> + case IIO_CHAN_INFO_SCALE:
>> + ret = regulator_get_voltage(adc_dev->reg);
>> + if (ret)
>> + return ret;
>> +
>> + *val = ret / 1000;
>> + *val2 = chan->scan_type.realbits;
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static int cc10001_update_scan_mode(struct iio_dev *indio_dev,
>> + const unsigned long *scan_mask)
> Reduce indentation by one space.
>> +{
>> + struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
>> +
>> + kfree(adc_dev->buf);
>> + adc_dev->buf = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
>> + if (!adc_dev->buf)
>> + return -ENOMEM;
>> +
>> + return 0;
> Save some lines:
> return (!adc_dev->buf) ? -ENOMEM : 0;
Hm... that's uglier to me. Lines are free, no need to save them :)
>> +}
>> +
>> +static const struct iio_info cc10001_adc_info = {
>> + .driver_module = THIS_MODULE,
>> + .read_raw = &cc10001_adc_read_raw,
>> + .update_scan_mode = &cc10001_update_scan_mode,
>> +};
>> +
>> +static int cc10001_adc_channel_init(struct iio_dev *indio_dev)
>> +{
>> + struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
>> + struct iio_chan_spec *chan_array, *timestamp;
>> + unsigned int bit, idx = 0;
>> +
>> + indio_dev->num_channels = bitmap_weight(&adc_dev->channel_map,
>> + CC10001_ADC_NUM_CHANNELS);
>> +
>> + chan_array = devm_kcalloc(&indio_dev->dev, indio_dev->num_channels + 1,
>> + sizeof(struct iio_chan_spec),
>> + GFP_KERNEL);
>> + if (!chan_array)
>> + return -ENOMEM;
>> +
>> + for_each_set_bit(bit, &adc_dev->channel_map,
>> + CC10001_ADC_NUM_CHANNELS) {
> No need to wrap, this fits exactly 80 chars.
Right.
>> + struct iio_chan_spec *chan = &chan_array[idx];
>> +
>> + chan->type = IIO_VOLTAGE;
>> + chan->indexed = 1;
>> + chan->channel = bit;
>> + chan->scan_index = idx;
>> + chan->scan_type.sign = 'u';
>> + chan->scan_type.realbits = 10;
>> + chan->scan_type.storagebits = 16;
>> + chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
>> + chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> + idx++;
>> + }
>> +
>> + timestamp = &chan_array[idx];
>> + timestamp->type = IIO_TIMESTAMP;
>> + timestamp->channel = -1;
>> + timestamp->scan_index = idx;
>> + timestamp->scan_type.sign = 's';
>> + timestamp->scan_type.realbits = 64;
>> + timestamp->scan_type.storagebits = 64;
>> +
>> + indio_dev->channels = chan_array;
>> +
>> + return 0;
>> +}
>> +
>> +static int cc10001_adc_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *node = pdev->dev.of_node;
>> + struct cc10001_adc_device *adc_dev;
>> + unsigned long adc_clk_rate;
>> + struct resource *res;
>> + struct iio_dev *indio_dev;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
>> + if (indio_dev == NULL)
>> + return -ENOMEM;
>> +
>> + adc_dev = iio_priv(indio_dev);
>> +
>> + adc_dev->channel_map = CC10001_ADC_CH_MASK;
> You certainly want adc_dev->channel_map to be something like GENMASK(CC10001_ADC_NUM_CHANNELS -1, 0).
>> + if (!of_property_read_u32(node, "cosmic,reserved-adc-channels", &ret))
>> + adc_dev->channel_map ^= ret;
> Correct way is to mask out, not to XOR.
Unless I'm missing something, that's exactly what XOR does.
Example:
reserved_channels = 0x2;
channel_map = 0x7 ^ reserved_channels -> 0x5
>> +
>> + adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
>> + if (IS_ERR_OR_NULL(adc_dev->reg))
>> + return -EINVAL;
> if (IS_ERR(adc_dev->reg))
> return PTR_ERR(adc_dev->reg);
Are you sure? What if devm_regulator_get() returns NULL?
>> +
>> + ret = regulator_enable(adc_dev->reg);
>> + if (ret)
>> + return ret;
>> +
>> + indio_dev->dev.parent = &pdev->dev;
>> + indio_dev->name = dev_name(&pdev->dev);
>> + indio_dev->info = &cc10001_adc_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + adc_dev->reg_base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(adc_dev->reg_base))
> Need to put error code into ret.
Right.
>> + goto err_disable_reg;
>> +
>> + adc_dev->adc_clk = devm_clk_get(&pdev->dev, "adc");
>> + if (IS_ERR(adc_dev->adc_clk)) {
>> + dev_err(&pdev->dev, "Failed to get the clock\n");
> Need to put error code into ret.
>> + goto err_disable_reg;
>> + }
>> +
>> + ret = clk_prepare_enable(adc_dev->adc_clk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to enable the clock\n");
>> + goto err_disable_reg;
>> + }
>> +
>> + adc_clk_rate = clk_get_rate(adc_dev->adc_clk);
>> + if (!adc_clk_rate) {
>> + ret = -EINVAL;
>> + dev_err(&pdev->dev, "null clock rate!\n");
> Start message with upper case?
Actually, I'd rather make the others start with lower case.
>> + goto err_disable_clk;
>> + }
>> +
>> + adc_dev->eoc_delay_ns = NSEC_PER_SEC / adc_clk_rate;
>> + adc_dev->start_delay_ns = adc_dev->eoc_delay_ns * CC10001_WAIT_CYCLES;
>> +
>> + /* Setup the ADC channels available on the device */
>> + ret = cc10001_adc_channel_init(indio_dev);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "Could not initialize the channels.\n");
>> + goto err_disable_clk;
>> + }
>> +
>> + mutex_init(&adc_dev->lock);
>> +
>> + ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> + &cc10001_adc_trigger_h, NULL);
>> + if (ret < 0)
>> + goto err_disable_clk;
>> +
>> + ret = iio_device_register(indio_dev);
>> + if (ret < 0)
>> + goto err_cleanup_buffer;
>> +
>> + platform_set_drvdata(pdev, indio_dev);
> Move this above iio_device_register.
What for?
>> +
>> + return 0;
>> +
>> +err_cleanup_buffer:
>> + iio_triggered_buffer_cleanup(indio_dev);
>> +err_disable_clk:
>> + clk_disable_unprepare(adc_dev->adc_clk);
>> +err_disable_reg:
>> + regulator_disable(adc_dev->reg);
>> + return ret;
>> +}
>> +
>> +static int cc10001_adc_remove(struct platform_device *pdev)
>> +{
>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> + struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
>> +
>> + iio_device_unregister(indio_dev);
>> + iio_triggered_buffer_cleanup(indio_dev);
>> + clk_disable_unprepare(adc_dev->adc_clk);
>> + regulator_disable(adc_dev->reg);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id cc10001_adc_dt_ids[] = {
>> + { .compatible = "cosmic,10001-adc", },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, cc10001_adc_dt_ids);
>> +
>> +static struct platform_driver cc10001_adc_driver = {
>> + .driver = {
>> + .name = "cc10001-adc",
>> + .owner = THIS_MODULE,
>> + .of_match_table = cc10001_adc_dt_ids,
>> + },
>> + .probe = cc10001_adc_probe,
>> + .remove = cc10001_adc_remove,
>> +};
>> +module_platform_driver(cc10001_adc_driver);
>> +
>> +MODULE_AUTHOR("Phani Movva <Phani.Movva@xxxxxxxxxx>");
>> +MODULE_DESCRIPTION("Cosmic Circuits ADC driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
Thanks for the review!
--
Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
