* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-21 9:24 ` [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver Ludovic Desroches
@ 2015-12-21 9:38 ` Peter Meerwald-Stadler
2015-12-21 10:16 ` Ludovic Desroches
2015-12-22 18:34 ` Jonathan Cameron
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Peter Meerwald-Stadler @ 2015-12-21 9:38 UTC (permalink / raw)
To: Ludovic Desroches; +Cc: jic23, nicolas.ferre, alexandre.belloni, linux-iio
> This driver supports the new version of the Atmel ADC device introduced
> with the SAMA5D2 SoC family.
some minor, quick comments below...
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
> .../devicetree/bindings/iio/adc/at91_adc8xx.txt | 27 ++
> drivers/iio/adc/Kconfig | 11 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/at91_adc8xx.c | 417 +++++++++++++++++++++
> 4 files changed, 456 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> create mode 100644 drivers/iio/adc/at91_adc8xx.c
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> new file mode 100644
> index 0000000..64ad6a5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> @@ -0,0 +1,27 @@
> +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> +
> +Required properties:
> + - compatible: Should be "atmel,sama5d2-adc".
> + - reg: Should contain ADC registers location and length.
> + - interrupts: Should contain the IRQ line for the ADC.
> + - clocks: phandles to clocks.
> + - clock-names: tuple listing clock names.
> + Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> + clock, "adc_clk" is the sampling clock.
> + - vref-supply: Supply used as reference for conversions.
> +
> +Optional properties:
> + - vddana-supply: Supply for the adc device.
> +
> +
> +Example:
> +
> +adc: adc@fc030000 {
> + compatible = "atmel,sama5d2-adc";
> + reg = <0xfc030000 0x100>;
> + interrupts = <40 IRQ_TYPE_LEVEL_HIGH 7>;
> + clocks = <&adc_clk>, <&adc_op_clk>;
> + clock-names = "adc_clk", "adc_op_clk";
> + vddana-supply = <&vdd_3v3_lp_reg>;
> + vref-supply = <&vdd_3v3_lp_reg>;
> +}
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 9162dfe..5819e41 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -131,6 +131,17 @@ config AT91_ADC
> To compile this driver as a module, choose M here: the module will be
> called at91_adc.
>
> +config AT91_ADC8xx
> + tristate "Atmel AT91 ADC 8xx"
> + depends on ARCH_AT91
> + depends on INPUT
> + help
> + Say yes here to build support for Atmel ADC 8xx which is available
> + from SAMA5D2 SoC family.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called at91_adc8xx.
> +
> config AXP288_ADC
> tristate "X-Powers AXP288 ADC driver"
> depends on MFD_AXP20X
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 91a65bf..d684a52 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_AT91_ADC8xx) += at91_adc8xx.o
> obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
> obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
> diff --git a/drivers/iio/adc/at91_adc8xx.c b/drivers/iio/adc/at91_adc8xx.c
> new file mode 100644
> index 0000000..8b4a6e7
> --- /dev/null
> +++ b/drivers/iio/adc/at91_adc8xx.c
> @@ -0,0 +1,417 @@
> +/*
> + * Atmel ADC driver for SAMA5D2 devices and later.
> + *
> + * Copyright (C) 2015 Atmel,
> + * 2015 Ludovic Desroches <ludovic.desroches@atmel.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched.h>
> +#include <linux/wait.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define ADC_CR 0x00 /* Control Register */
please use a prefix such as AT91_ADC8XX_
> +#define ADC_CR_SWRST BIT(0) /* Software Reset */
> +#define ADC_CR_START BIT(1) /* Start Conversion */
> +#define ADC_CR_TSCALIB BIT(2) /* Touchscreen Calibration */
> +#define ADC_CR_CMPRST BIT(4) /* Comparison Restart */
> +#define ADC_MR 0x04 /* Mode Register */
> +#define ADC_MR_TRGSEL(v) (v << 1) /* Trigger Selection */
> +#define ADC_MR_TRGSEL_TRIG0 0 /* ADTRG */
> +#define ADC_MR_TRGSEL_TRIG1 1 /* TIOA0 */
> +#define ADC_MR_TRGSEL_TRIG2 2 /* TIOA1 */
> +#define ADC_MR_TRGSEL_TRIG3 3 /* TIOA2 */
> +#define ADC_MR_TRGSEL_TRIG4 4 /* PWM event line 0 */
> +#define ADC_MR_TRGSEL_TRIG5 5 /* PWM event line 1 */
> +#define ADC_MR_TRGSEL_TRIG6 6 /* TIOA3 */
> +#define ADC_MR_TRGSEL_TRIG7 7 /* RTCOUT0 */
> +#define ADC_MR_SLEEP BIT(5) /* Sleep Mode */
> +#define ADC_MR_FWUP BIT(6) /* Fast Wake Up */
> +#define ADC_MR_PRESCAL(v) (v << 8) /* Prescaler Rate Selection */
> +#define ADC_MR_PRESCAL_MAX 0xffff
> +#define ADC_MR_STARTUP(v) (v << 16) /* Startup Time */
> +#define ADC_MR_STARTUP_SUT0 0 /* 0 period of ADCCLK */
> +#define ADC_MR_STARTUP_SUT8 1 /* 8 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT16 2 /* 16 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT24 3 /* 24 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT64 4 /* 64 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT80 5 /* 80 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT96 6 /* 96 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT112 7 /* 112 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT512 8 /* 512 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT576 9 /* 576 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT640 10 /* 640 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT704 11 /* 704 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT768 12 /* 768 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT832 13 /* 832 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT896 14 /* 896 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT960 15 /* 960 periods of ADCCLK */
> +#define ADC_MR_ANACH BIT(23) /* Analog Change */
> +#define ADC_MR_TRACKTIM(v) (v << 24) /* Tracking Time */
> +#define ADC_MR_TRACKTIM_MAX 0xff
> +#define ADC_MR_TRANSFER(v) (v << 28) /* Transfer Time */
> +#define ADC_MR_TRANSFER_MAX 0x3
> +#define ADC_MR_USEQ BIT(31) /* Use Sequence Enable */
> +#define ADC_SEQR1 0x08 /* Channel Sequence Register 1 */
> +#define ADC_SEQR2 0x0c /* Channel Sequence Register 2 */
> +#define ADC_CHER 0x10 /* Channel Enable Register */
> +#define ADC_CHDR 0x14 /* Channel Disable Register */
> +#define ADC_CHSR 0x18 /* Channel Status Register */
> +#define ADC_LCDR 0x20 /* Last Converted Data Register */
> +#define ADC_IER 0x24 /* Interrupt Enable Register */
> +#define ADC_IDR 0x28 /* Interrupt Disable Register */
> +#define ADC_IMR 0x2c /* Interrupt Mask Register */
> +#define ADC_ISR 0x30 /* Interrupt Status Register */
> +#define ADC_LCTMR 0x34 /* Last Channel Trigger Mode Register */
> +#define ADC_LCCWR 0x38 /* Last Channel Compare Window Register */
> +#define ADC_OVER 0x3c /* Overrun Status Register */
> +#define ADC_EMR 0x40 /* Extended Mode Register */
> +#define ADC_CWR 0x44 /* Compare Window Register */
> +#define ADC_CGR 0x48 /* Channel Gain Register */
> +#define ADC_COR 0x4c /* Channel Offset Register */
> +#define ADC_CDR0 0x50 /* Channel Data Register 0 */
> +#define ADC_ACR 0x94 /* Analog Control Register */
> +#define ADC_TSMR 0xb0 /* Touchscreen Mode Register */
> +#define ADC_XPOSR 0xb4 /* Touchscreen X Position Register */
> +#define ADC_YPOSR 0xb8 /* Touchscreen Y Position Register */
> +#define ADC_PRESSR 0xbc /* Touchscreen Pressure Register */
> +#define ADC_TRGR 0xc0 /* Trigger Register */
> +#define ADC_COSR 0xd0 /* Correction Select Register */
> +#define ADC_CVR 0xd4 /* Correction Value Register */
> +#define ADC_CECR 0xd8 /* Channel Error Correction Register */
> +#define ADC_WPMR 0xe4 /* Write Protection Mode Register */
> +#define ADC_WPSR 0xe8 /* Write Protection Status Register */
> +#define ADC_VERSION 0xfc /* Version Register */
> +
> +#define AT91_ADC_CHAN(num, addr) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .channel = num, \
> + .address = addr, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 12, \
> + .storagebits = 14, \
storagebits should be a multiple of 8
> + }, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .datasheet_name = "CH"#num, \
> + .indexed = 1, \
> + }
> +
> +#define at91_adc_readl(st, reg) readl_relaxed(st->base + reg)
> +#define at91_adc_writel(st, reg, val) writel_relaxed(val, st->base + reg)
> +
> +struct at91_adc_soc_info {
> + unsigned startup_time;
> + unsigned min_f_adc;
> + unsigned max_f_adc;
> + const struct iio_chan_spec *channels;
> + int num_channels;
> +};
> +
> +struct at91_adc_state {
> + void __iomem *base;
> + struct clk *per_clk;
> + struct clk *adc_clk;
> + int irq;
> + struct regulator *reg;
> + struct regulator *vref;
> + u32 vref_uv;
> + struct at91_adc_soc_info *soc_info;
> + wait_queue_head_t wq_data_available;
> + bool done;
> + const struct iio_chan_spec *chan;
> + u32 last_value;
> + struct mutex lock;
> +};
> +
> +static struct at91_adc_soc_info at91_adc_sama5d2_soc_info = {
> + .startup_time = 4,
> + .min_f_adc = 200000,
> + .max_f_adc = 20000000,
> +};
> +
> +static const struct iio_chan_spec at91_adc_channels[] = {
> + AT91_ADC_CHAN(0, 0x50),
> + AT91_ADC_CHAN(1, 0x54),
> + AT91_ADC_CHAN(2, 0x58),
> + AT91_ADC_CHAN(3, 0x5c),
> + AT91_ADC_CHAN(4, 0x60),
> + AT91_ADC_CHAN(5, 0x64),
> + AT91_ADC_CHAN(6, 0x68),
> + AT91_ADC_CHAN(7, 0x6c),
> + AT91_ADC_CHAN(8, 0x70),
> + AT91_ADC_CHAN(9, 0x74),
> + AT91_ADC_CHAN(10, 0x78),
> + AT91_ADC_CHAN(11, 0x7c),
> +};
> +
> +static irqreturn_t at91_adc_interrupt(int irq, void *private)
> +{
> + struct iio_dev *indio = private;
> + struct at91_adc_state *st = iio_priv(indio);
> + u32 status = at91_adc_readl(st, ADC_ISR);
> +
> + status &= at91_adc_readl(st, ADC_IMR);
> + if (status & 0xFFF) {
you are mixing uppercase and lowercase hex constants, i.e. 0xfff vs. 0xFFF
> + st->last_value = at91_adc_readl(st, st->chan->address);
> + st->done = true;
> + wake_up_interruptible(&st->wq_data_available);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static bool at91_adc_freq_supported(struct at91_adc_state *st, unsigned freq)
> +{
> + return ((freq >= st->soc_info->min_f_adc)
> + && (freq <= st->soc_info->max_f_adc));
> +}
> +
> +static unsigned at91_adc_startup_time(unsigned startup_time_min,
> + unsigned adc_clk_khz)
> +{
> + const unsigned startup_lookup[] = {
> + 0, 8, 16, 24,
> + 64, 80, 96, 112,
> + 512, 576, 640, 704,
> + 768, 832, 896, 960
> + };
do we need the ADC_MR_STARTUP_SUT #defines then?
> + unsigned ticks_min, i;
> +
> + /*
> + * Since the adc frequency is checked before, there is no reason
> + * to not meet the startup time constraint.
> + */
> +
> + ticks_min = startup_time_min * adc_clk_khz / 1000;
> + for (i = 0; i < ARRAY_SIZE(startup_lookup); i++)
> + if (startup_lookup[i] > ticks_min)
> + break;
> +
> + return i;
> +}
> +
> +static int at91_adc_init(struct at91_adc_state *st)
> +{
> + struct iio_dev *indio_dev = iio_priv_to_dev(st);
> + unsigned f_adc, f_per, prescal, startup;
> +
> + at91_adc_writel(st, ADC_CR, ADC_CR_SWRST);
> + at91_adc_writel(st, ADC_IDR, 0xffffffff);
> +
> + f_adc = clk_get_rate(st->adc_clk);
> + if (!at91_adc_freq_supported(st, f_adc)) {
> + dev_err(&indio_dev->dev, "unsupported adc clock frequency\n");
> + return -EINVAL;
> + }
> +
> + f_per = clk_get_rate(st->per_clk);
> + prescal = (f_per / (2 * f_adc)) - 1;
> +
> + startup = at91_adc_startup_time(st->soc_info->startup_time,
> + f_adc / 1000);
> +
> + at91_adc_writel(st, ADC_MR,
> + ADC_MR_TRANSFER(2)
> + | ADC_MR_STARTUP(startup)
> + | ADC_MR_PRESCAL(prescal));
> +
> + dev_dbg(&indio_dev->dev, "startup: %u, prescal: %u\n",
> + startup, prescal);
> +
> + return 0;
> +}
> +
> +static int at91_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct at91_adc_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + st->chan = chan;
why? isn't this potentially dangerous outside the mutex lock?
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&st->lock);
> +
> + at91_adc_writel(st, ADC_CHER, BIT(chan->channel));
> + at91_adc_writel(st, ADC_IER, BIT(chan->channel));
> + at91_adc_writel(st, ADC_CR, ADC_CR_START);
> +
> + ret = wait_event_interruptible_timeout(st->wq_data_available,
> + st->done,
> + msecs_to_jiffies(1000));
> + if (ret == 0)
> + ret = -ETIMEDOUT;
> +
> + if (ret <= 0) {
probably ret < 0
> + at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> + at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> + mutex_unlock(&st->lock);
> + return ret;
> + }
> +
> + if (chan->scan_type.sign == 's')
only 'u' is used so far
> + *val = sign_extend32(st->last_value,
> + chan->scan_type.realbits - 1);
> + else
> + *val = st->last_value;
> + at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> + at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> + st->done = false;
> +
> + mutex_unlock(&st->lock);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->vref_uv / 1000;
> + *val2 = chan->scan_type.realbits;
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info at91_adc_info = {
> + .read_raw = &at91_adc_read_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int at91_adc_probe(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev;
> + struct at91_adc_state *st;
> + struct resource *res;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev,
> + sizeof(struct at91_adc_state));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->name = dev_name(&pdev->dev);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &at91_adc_info;
> + indio_dev->channels = at91_adc_channels;
> + indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
> +
> + st = iio_priv(indio_dev);
> + st->soc_info = (struct at91_adc_soc_info *)
> + of_device_get_match_data(&pdev->dev);
> + init_waitqueue_head(&st->wq_data_available);
> + mutex_init(&st->lock);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + st->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(st->base))
> + return PTR_ERR(st->base);
> +
> + st->irq = platform_get_irq(pdev, 0);
> + if (st->irq < 0)
> + return st->irq;
> +
> + st->per_clk = devm_clk_get(&pdev->dev, "adc_clk");
> + if (IS_ERR(st->per_clk))
> + return PTR_ERR(st->per_clk);
> +
> + st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
> + if (IS_ERR(st->adc_clk))
> + return PTR_ERR(st->adc_clk);
> +
> + st->reg = devm_regulator_get(&pdev->dev, "vddana");
> + if (IS_ERR(st->reg))
> + return PTR_ERR(st->reg);
> +
> + st->vref = devm_regulator_get(&pdev->dev, "vref");
> + if (IS_ERR(st->vref))
> + return PTR_ERR(st->vref);
> +
> + ret = devm_request_irq(&pdev->dev, st->irq, at91_adc_interrupt, 0,
> + pdev->dev.driver->name, indio_dev);
> + if (ret)
> + return ret;
> +
> + st->vref_uv = regulator_get_voltage(st->vref);
> + if (st->vref_uv <= 0) {
> + ret = -EINVAL;
> + goto error;
> + }
> +
> + ret = at91_adc_init(st);
> + if (ret)
> + goto error;
> +
> + ret = clk_prepare_enable(st->adc_clk);
> + ret = clk_prepare_enable(st->per_clk);
> +
> + ret = iio_device_register(indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + dev_info(&pdev->dev, "version: %x\n",
> + readl_relaxed(st->base + ADC_VERSION));
> +
> +error:
> + return ret;
> +}
> +
> +static int at91_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct at91_adc_state *st = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> +
> + clk_disable_unprepare(st->per_clk);
> + clk_disable_unprepare(st->adc_clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id at91_adc_dt_match[] = {
> + {
> + .compatible = "atmel,sama5d2-adc",
> + .data = &at91_adc_sama5d2_soc_info
> + }, {
> + /* sentinel */
> + }
> +};
> +MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
> +
> +static struct platform_driver at91_adc_driver = {
> + .probe = at91_adc_probe,
> + .remove = at91_adc_remove,
> + .driver = {
> + .name = "at91_adc8xx",
> + .of_match_table = at91_adc_dt_match,
> + },
> +};
> +module_platform_driver(at91_adc_driver)
> +
> +MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches@atmel.com>");
> +MODULE_DESCRIPTION("Atmel AT91 ADC 8xx");
> +MODULE_LICENSE("GPL v2");
>
--
Peter Meerwald-Stadler
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-21 9:38 ` Peter Meerwald-Stadler
@ 2015-12-21 10:16 ` Ludovic Desroches
2015-12-21 10:25 ` Ludovic Desroches
0 siblings, 1 reply; 18+ messages in thread
From: Ludovic Desroches @ 2015-12-21 10:16 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: Ludovic Desroches, jic23, nicolas.ferre, alexandre.belloni,
linux-iio
On Mon, Dec 21, 2015 at 10:38:11AM +0100, Peter Meerwald-Stadler wrote:
>
> > This driver supports the new version of the Atmel ADC device introduced
> > with the SAMA5D2 SoC family.
>
> some minor, quick comments below...
Thanks for your review
>
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > ---
> > .../devicetree/bindings/iio/adc/at91_adc8xx.txt | 27 ++
> > drivers/iio/adc/Kconfig | 11 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/at91_adc8xx.c | 417 +++++++++++++++++++++
> > 4 files changed, 456 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > create mode 100644 drivers/iio/adc/at91_adc8xx.c
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > new file mode 100644
> > index 0000000..64ad6a5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > @@ -0,0 +1,27 @@
> > +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> > +
> > +Required properties:
> > + - compatible: Should be "atmel,sama5d2-adc".
> > + - reg: Should contain ADC registers location and length.
> > + - interrupts: Should contain the IRQ line for the ADC.
> > + - clocks: phandles to clocks.
> > + - clock-names: tuple listing clock names.
> > + Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> > + clock, "adc_clk" is the sampling clock.
> > + - vref-supply: Supply used as reference for conversions.
> > +
> > +Optional properties:
> > + - vddana-supply: Supply for the adc device.
> > +
> > +
> > +Example:
> > +
> > +adc: adc@fc030000 {
> > + compatible = "atmel,sama5d2-adc";
> > + reg = <0xfc030000 0x100>;
> > + interrupts = <40 IRQ_TYPE_LEVEL_HIGH 7>;
> > + clocks = <&adc_clk>, <&adc_op_clk>;
> > + clock-names = "adc_clk", "adc_op_clk";
> > + vddana-supply = <&vdd_3v3_lp_reg>;
> > + vref-supply = <&vdd_3v3_lp_reg>;
> > +}
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 9162dfe..5819e41 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -131,6 +131,17 @@ config AT91_ADC
> > To compile this driver as a module, choose M here: the module will be
> > called at91_adc.
> >
> > +config AT91_ADC8xx
> > + tristate "Atmel AT91 ADC 8xx"
> > + depends on ARCH_AT91
> > + depends on INPUT
> > + help
> > + Say yes here to build support for Atmel ADC 8xx which is available
> > + from SAMA5D2 SoC family.
> > +
> > + To compile this driver as a module, choose M here: the module will be
> > + called at91_adc8xx.
> > +
> > config AXP288_ADC
> > tristate "X-Powers AXP288 ADC driver"
> > depends on MFD_AXP20X
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 91a65bf..d684a52 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_AT91_ADC8xx) += at91_adc8xx.o
> > obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> > obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
> > obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
> > diff --git a/drivers/iio/adc/at91_adc8xx.c b/drivers/iio/adc/at91_adc8xx.c
> > new file mode 100644
> > index 0000000..8b4a6e7
> > --- /dev/null
> > +++ b/drivers/iio/adc/at91_adc8xx.c
> > @@ -0,0 +1,417 @@
> > +/*
> > + * Atmel ADC driver for SAMA5D2 devices and later.
> > + *
> > + * Copyright (C) 2015 Atmel,
> > + * 2015 Ludovic Desroches <ludovic.desroches@atmel.com>
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/sched.h>
> > +#include <linux/wait.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define ADC_CR 0x00 /* Control Register */
>
> please use a prefix such as AT91_ADC8XX_
>
Ok, I'll add it.
> > +#define ADC_CR_SWRST BIT(0) /* Software Reset */
> > +#define ADC_CR_START BIT(1) /* Start Conversion */
> > +#define ADC_CR_TSCALIB BIT(2) /* Touchscreen Calibration */
> > +#define ADC_CR_CMPRST BIT(4) /* Comparison Restart */
> > +#define ADC_MR 0x04 /* Mode Register */
> > +#define ADC_MR_TRGSEL(v) (v << 1) /* Trigger Selection */
> > +#define ADC_MR_TRGSEL_TRIG0 0 /* ADTRG */
> > +#define ADC_MR_TRGSEL_TRIG1 1 /* TIOA0 */
> > +#define ADC_MR_TRGSEL_TRIG2 2 /* TIOA1 */
> > +#define ADC_MR_TRGSEL_TRIG3 3 /* TIOA2 */
> > +#define ADC_MR_TRGSEL_TRIG4 4 /* PWM event line 0 */
> > +#define ADC_MR_TRGSEL_TRIG5 5 /* PWM event line 1 */
> > +#define ADC_MR_TRGSEL_TRIG6 6 /* TIOA3 */
> > +#define ADC_MR_TRGSEL_TRIG7 7 /* RTCOUT0 */
> > +#define ADC_MR_SLEEP BIT(5) /* Sleep Mode */
> > +#define ADC_MR_FWUP BIT(6) /* Fast Wake Up */
> > +#define ADC_MR_PRESCAL(v) (v << 8) /* Prescaler Rate Selection */
> > +#define ADC_MR_PRESCAL_MAX 0xffff
> > +#define ADC_MR_STARTUP(v) (v << 16) /* Startup Time */
> > +#define ADC_MR_STARTUP_SUT0 0 /* 0 period of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT8 1 /* 8 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT16 2 /* 16 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT24 3 /* 24 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT64 4 /* 64 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT80 5 /* 80 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT96 6 /* 96 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT112 7 /* 112 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT512 8 /* 512 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT576 9 /* 576 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT640 10 /* 640 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT704 11 /* 704 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT768 12 /* 768 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT832 13 /* 832 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT896 14 /* 896 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT960 15 /* 960 periods of ADCCLK */
> > +#define ADC_MR_ANACH BIT(23) /* Analog Change */
> > +#define ADC_MR_TRACKTIM(v) (v << 24) /* Tracking Time */
> > +#define ADC_MR_TRACKTIM_MAX 0xff
> > +#define ADC_MR_TRANSFER(v) (v << 28) /* Transfer Time */
> > +#define ADC_MR_TRANSFER_MAX 0x3
> > +#define ADC_MR_USEQ BIT(31) /* Use Sequence Enable */
> > +#define ADC_SEQR1 0x08 /* Channel Sequence Register 1 */
> > +#define ADC_SEQR2 0x0c /* Channel Sequence Register 2 */
> > +#define ADC_CHER 0x10 /* Channel Enable Register */
> > +#define ADC_CHDR 0x14 /* Channel Disable Register */
> > +#define ADC_CHSR 0x18 /* Channel Status Register */
> > +#define ADC_LCDR 0x20 /* Last Converted Data Register */
> > +#define ADC_IER 0x24 /* Interrupt Enable Register */
> > +#define ADC_IDR 0x28 /* Interrupt Disable Register */
> > +#define ADC_IMR 0x2c /* Interrupt Mask Register */
> > +#define ADC_ISR 0x30 /* Interrupt Status Register */
> > +#define ADC_LCTMR 0x34 /* Last Channel Trigger Mode Register */
> > +#define ADC_LCCWR 0x38 /* Last Channel Compare Window Register */
> > +#define ADC_OVER 0x3c /* Overrun Status Register */
> > +#define ADC_EMR 0x40 /* Extended Mode Register */
> > +#define ADC_CWR 0x44 /* Compare Window Register */
> > +#define ADC_CGR 0x48 /* Channel Gain Register */
> > +#define ADC_COR 0x4c /* Channel Offset Register */
> > +#define ADC_CDR0 0x50 /* Channel Data Register 0 */
> > +#define ADC_ACR 0x94 /* Analog Control Register */
> > +#define ADC_TSMR 0xb0 /* Touchscreen Mode Register */
> > +#define ADC_XPOSR 0xb4 /* Touchscreen X Position Register */
> > +#define ADC_YPOSR 0xb8 /* Touchscreen Y Position Register */
> > +#define ADC_PRESSR 0xbc /* Touchscreen Pressure Register */
> > +#define ADC_TRGR 0xc0 /* Trigger Register */
> > +#define ADC_COSR 0xd0 /* Correction Select Register */
> > +#define ADC_CVR 0xd4 /* Correction Value Register */
> > +#define ADC_CECR 0xd8 /* Channel Error Correction Register */
> > +#define ADC_WPMR 0xe4 /* Write Protection Mode Register */
> > +#define ADC_WPSR 0xe8 /* Write Protection Status Register */
> > +#define ADC_VERSION 0xfc /* Version Register */
> > +
> > +#define AT91_ADC_CHAN(num, addr) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .channel = num, \
> > + .address = addr, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 12, \
> > + .storagebits = 14, \
>
> storagebits should be a multiple of 8
>
I'll remove it since I am not using buffer at the moment. I'll keep in
mind this alignement constraint.
> > + }, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> > + .datasheet_name = "CH"#num, \
> > + .indexed = 1, \
> > + }
> > +
> > +#define at91_adc_readl(st, reg) readl_relaxed(st->base + reg)
> > +#define at91_adc_writel(st, reg, val) writel_relaxed(val, st->base + reg)
> > +
> > +struct at91_adc_soc_info {
> > + unsigned startup_time;
> > + unsigned min_f_adc;
> > + unsigned max_f_adc;
> > + const struct iio_chan_spec *channels;
> > + int num_channels;
> > +};
> > +
> > +struct at91_adc_state {
> > + void __iomem *base;
> > + struct clk *per_clk;
> > + struct clk *adc_clk;
> > + int irq;
> > + struct regulator *reg;
> > + struct regulator *vref;
> > + u32 vref_uv;
> > + struct at91_adc_soc_info *soc_info;
> > + wait_queue_head_t wq_data_available;
> > + bool done;
> > + const struct iio_chan_spec *chan;
> > + u32 last_value;
> > + struct mutex lock;
> > +};
> > +
> > +static struct at91_adc_soc_info at91_adc_sama5d2_soc_info = {
> > + .startup_time = 4,
> > + .min_f_adc = 200000,
> > + .max_f_adc = 20000000,
> > +};
> > +
> > +static const struct iio_chan_spec at91_adc_channels[] = {
> > + AT91_ADC_CHAN(0, 0x50),
> > + AT91_ADC_CHAN(1, 0x54),
> > + AT91_ADC_CHAN(2, 0x58),
> > + AT91_ADC_CHAN(3, 0x5c),
> > + AT91_ADC_CHAN(4, 0x60),
> > + AT91_ADC_CHAN(5, 0x64),
> > + AT91_ADC_CHAN(6, 0x68),
> > + AT91_ADC_CHAN(7, 0x6c),
> > + AT91_ADC_CHAN(8, 0x70),
> > + AT91_ADC_CHAN(9, 0x74),
> > + AT91_ADC_CHAN(10, 0x78),
> > + AT91_ADC_CHAN(11, 0x7c),
> > +};
> > +
> > +static irqreturn_t at91_adc_interrupt(int irq, void *private)
> > +{
> > + struct iio_dev *indio = private;
> > + struct at91_adc_state *st = iio_priv(indio);
> > + u32 status = at91_adc_readl(st, ADC_ISR);
> > +
> > + status &= at91_adc_readl(st, ADC_IMR);
> > + if (status & 0xFFF) {
>
> you are mixing uppercase and lowercase hex constants, i.e. 0xfff vs. 0xFFF
>
I'll fix it.
> > + st->last_value = at91_adc_readl(st, st->chan->address);
> > + st->done = true;
> > + wake_up_interruptible(&st->wq_data_available);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static bool at91_adc_freq_supported(struct at91_adc_state *st, unsigned freq)
> > +{
> > + return ((freq >= st->soc_info->min_f_adc)
> > + && (freq <= st->soc_info->max_f_adc));
> > +}
> > +
> > +static unsigned at91_adc_startup_time(unsigned startup_time_min,
> > + unsigned adc_clk_khz)
> > +{
> > + const unsigned startup_lookup[] = {
> > + 0, 8, 16, 24,
> > + 64, 80, 96, 112,
> > + 512, 576, 640, 704,
> > + 768, 832, 896, 960
> > + };
>
> do we need the ADC_MR_STARTUP_SUT #defines then?
>
Why not.
> > + unsigned ticks_min, i;
> > +
> > + /*
> > + * Since the adc frequency is checked before, there is no reason
> > + * to not meet the startup time constraint.
> > + */
> > +
> > + ticks_min = startup_time_min * adc_clk_khz / 1000;
> > + for (i = 0; i < ARRAY_SIZE(startup_lookup); i++)
> > + if (startup_lookup[i] > ticks_min)
> > + break;
> > +
> > + return i;
> > +}
> > +
> > +static int at91_adc_init(struct at91_adc_state *st)
> > +{
> > + struct iio_dev *indio_dev = iio_priv_to_dev(st);
> > + unsigned f_adc, f_per, prescal, startup;
> > +
> > + at91_adc_writel(st, ADC_CR, ADC_CR_SWRST);
> > + at91_adc_writel(st, ADC_IDR, 0xffffffff);
> > +
> > + f_adc = clk_get_rate(st->adc_clk);
> > + if (!at91_adc_freq_supported(st, f_adc)) {
> > + dev_err(&indio_dev->dev, "unsupported adc clock frequency\n");
> > + return -EINVAL;
> > + }
> > +
> > + f_per = clk_get_rate(st->per_clk);
> > + prescal = (f_per / (2 * f_adc)) - 1;
> > +
> > + startup = at91_adc_startup_time(st->soc_info->startup_time,
> > + f_adc / 1000);
> > +
> > + at91_adc_writel(st, ADC_MR,
> > + ADC_MR_TRANSFER(2)
> > + | ADC_MR_STARTUP(startup)
> > + | ADC_MR_PRESCAL(prescal));
> > +
> > + dev_dbg(&indio_dev->dev, "startup: %u, prescal: %u\n",
> > + startup, prescal);
> > +
> > + return 0;
> > +}
> > +
> > +static int at91_adc_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct at91_adc_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + st->chan = chan;
>
> why? isn't this potentially dangerous outside the mutex lock?
>
Sorry, remainder of a debug session.
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + mutex_lock(&st->lock);
> > +
> > + at91_adc_writel(st, ADC_CHER, BIT(chan->channel));
> > + at91_adc_writel(st, ADC_IER, BIT(chan->channel));
> > + at91_adc_writel(st, ADC_CR, ADC_CR_START);
> > +
> > + ret = wait_event_interruptible_timeout(st->wq_data_available,
> > + st->done,
> > + msecs_to_jiffies(1000));
> > + if (ret == 0)
> > + ret = -ETIMEDOUT;
> > +
> > + if (ret <= 0) {
>
> probably ret < 0
>
yes
> > + at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> > + at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> > + mutex_unlock(&st->lock);
> > + return ret;
> > + }
> > +
> > + if (chan->scan_type.sign == 's')
>
> only 'u' is used so far
>
It was for future signed conversions support. I'll remove it.
> > + *val = sign_extend32(st->last_value,
> > + chan->scan_type.realbits - 1);
> > + else
> > + *val = st->last_value;
> > + at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> > + at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> > + st->done = false;
> > +
> > + mutex_unlock(&st->lock);
> > + return IIO_VAL_INT;
> > +
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = st->vref_uv / 1000;
> > + *val2 = chan->scan_type.realbits;
> > + return IIO_VAL_FRACTIONAL_LOG2;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct iio_info at91_adc_info = {
> > + .read_raw = &at91_adc_read_raw,
> > + .driver_module = THIS_MODULE,
> > +};
> > +
> > +static int at91_adc_probe(struct platform_device *pdev)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct at91_adc_state *st;
> > + struct resource *res;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&pdev->dev,
> > + sizeof(struct at91_adc_state));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + indio_dev->dev.parent = &pdev->dev;
> > + indio_dev->name = dev_name(&pdev->dev);
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &at91_adc_info;
> > + indio_dev->channels = at91_adc_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
> > +
> > + st = iio_priv(indio_dev);
> > + st->soc_info = (struct at91_adc_soc_info *)
> > + of_device_get_match_data(&pdev->dev);
> > + init_waitqueue_head(&st->wq_data_available);
> > + mutex_init(&st->lock);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -EINVAL;
> > +
> > + st->base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(st->base))
> > + return PTR_ERR(st->base);
> > +
> > + st->irq = platform_get_irq(pdev, 0);
> > + if (st->irq < 0)
> > + return st->irq;
> > +
> > + st->per_clk = devm_clk_get(&pdev->dev, "adc_clk");
> > + if (IS_ERR(st->per_clk))
> > + return PTR_ERR(st->per_clk);
> > +
> > + st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
> > + if (IS_ERR(st->adc_clk))
> > + return PTR_ERR(st->adc_clk);
> > +
> > + st->reg = devm_regulator_get(&pdev->dev, "vddana");
> > + if (IS_ERR(st->reg))
> > + return PTR_ERR(st->reg);
> > +
> > + st->vref = devm_regulator_get(&pdev->dev, "vref");
> > + if (IS_ERR(st->vref))
> > + return PTR_ERR(st->vref);
> > +
> > + ret = devm_request_irq(&pdev->dev, st->irq, at91_adc_interrupt, 0,
> > + pdev->dev.driver->name, indio_dev);
> > + if (ret)
> > + return ret;
> > +
> > + st->vref_uv = regulator_get_voltage(st->vref);
> > + if (st->vref_uv <= 0) {
> > + ret = -EINVAL;
> > + goto error;
> > + }
> > +
> > + ret = at91_adc_init(st);
> > + if (ret)
> > + goto error;
> > +
> > + ret = clk_prepare_enable(st->adc_clk);
> > + ret = clk_prepare_enable(st->per_clk);
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + dev_info(&pdev->dev, "version: %x\n",
> > + readl_relaxed(st->base + ADC_VERSION));
> > +
> > +error:
> > + return ret;
> > +}
> > +
> > +static int at91_adc_remove(struct platform_device *pdev)
> > +{
> > + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > + struct at91_adc_state *st = iio_priv(indio_dev);
> > +
> > + iio_device_unregister(indio_dev);
> > +
> > + clk_disable_unprepare(st->per_clk);
> > + clk_disable_unprepare(st->adc_clk);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id at91_adc_dt_match[] = {
> > + {
> > + .compatible = "atmel,sama5d2-adc",
> > + .data = &at91_adc_sama5d2_soc_info
> > + }, {
> > + /* sentinel */
> > + }
> > +};
> > +MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
> > +
> > +static struct platform_driver at91_adc_driver = {
> > + .probe = at91_adc_probe,
> > + .remove = at91_adc_remove,
> > + .driver = {
> > + .name = "at91_adc8xx",
> > + .of_match_table = at91_adc_dt_match,
> > + },
> > +};
> > +module_platform_driver(at91_adc_driver)
> > +
> > +MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches@atmel.com>");
> > +MODULE_DESCRIPTION("Atmel AT91 ADC 8xx");
> > +MODULE_LICENSE("GPL v2");
> >
>
> --
>
> Peter Meerwald-Stadler
> +43-664-2444418 (mobile)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-21 10:16 ` Ludovic Desroches
@ 2015-12-21 10:25 ` Ludovic Desroches
0 siblings, 0 replies; 18+ messages in thread
From: Ludovic Desroches @ 2015-12-21 10:25 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: Ludovic Desroches, jic23, nicolas.ferre, alexandre.belloni,
linux-iio
On Mon, Dec 21, 2015 at 11:16:11AM +0100, Ludovic Desroches wrote:
> On Mon, Dec 21, 2015 at 10:38:11AM +0100, Peter Meerwald-Stadler wrote:
> >
> > > This driver supports the new version of the Atmel ADC device introduced
> > > with the SAMA5D2 SoC family.
> >
> > some minor, quick comments below...
>
> Thanks for your review
>
[...]
> > > +static unsigned at91_adc_startup_time(unsigned startup_time_min,
> > > + unsigned adc_clk_khz)
> > > +{
> > > + const unsigned startup_lookup[] = {
> > > + 0, 8, 16, 24,
> > > + 64, 80, 96, 112,
> > > + 512, 576, 640, 704,
> > > + 768, 832, 896, 960
> > > + };
> >
> > do we need the ADC_MR_STARTUP_SUT #defines then?
> >
>
> Why not.
>
Sorry my answer is not clear! I mean, yes these defines are probably
useless.
[...]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-21 9:24 ` [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver Ludovic Desroches
2015-12-21 9:38 ` Peter Meerwald-Stadler
@ 2015-12-22 18:34 ` Jonathan Cameron
2015-12-23 10:27 ` Ludovic Desroches
2015-12-23 0:51 ` Rob Herring
2015-12-23 10:27 ` Alexandre Belloni
3 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2015-12-22 18:34 UTC (permalink / raw)
To: Ludovic Desroches, nicolas.ferre, alexandre.belloni
Cc: devicetree, linux-kernel, linux-iio, plagnioj, linux-arm-kernel
On 21/12/15 09:24, Ludovic Desroches wrote:
> This driver supports the new version of the Atmel ADC device introduced
> with the SAMA5D2 SoC family.
>
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
A few more bits and bobs from me. Mostly looking good.
Jonathan
> ---
> .../devicetree/bindings/iio/adc/at91_adc8xx.txt | 27 ++
> drivers/iio/adc/Kconfig | 11 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/at91_adc8xx.c | 417 +++++++++++++++++++++
> 4 files changed, 456 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> create mode 100644 drivers/iio/adc/at91_adc8xx.c
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> new file mode 100644
> index 0000000..64ad6a5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> @@ -0,0 +1,27 @@
> +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> +
> +Required properties:
> + - compatible: Should be "atmel,sama5d2-adc".
> + - reg: Should contain ADC registers location and length.
> + - interrupts: Should contain the IRQ line for the ADC.
> + - clocks: phandles to clocks.
> + - clock-names: tuple listing clock names.
> + Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> + clock, "adc_clk" is the sampling clock.
> + - vref-supply: Supply used as reference for conversions.
> +
> +Optional properties:
> + - vddana-supply: Supply for the adc device.
> +
> +
> +Example:
> +
> +adc: adc@fc030000 {
> + compatible = "atmel,sama5d2-adc";
> + reg = <0xfc030000 0x100>;
> + interrupts = <40 IRQ_TYPE_LEVEL_HIGH 7>;
> + clocks = <&adc_clk>, <&adc_op_clk>;
> + clock-names = "adc_clk", "adc_op_clk";
> + vddana-supply = <&vdd_3v3_lp_reg>;
> + vref-supply = <&vdd_3v3_lp_reg>;
> +}
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 9162dfe..5819e41 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -131,6 +131,17 @@ config AT91_ADC
> To compile this driver as a module, choose M here: the module will be
> called at91_adc.
>
> +config AT91_ADC8xx
> + tristate "Atmel AT91 ADC 8xx"
> + depends on ARCH_AT91
> + depends on INPUT
> + help
> + Say yes here to build support for Atmel ADC 8xx which is available
> + from SAMA5D2 SoC family.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called at91_adc8xx.
> +
> config AXP288_ADC
> tristate "X-Powers AXP288 ADC driver"
> depends on MFD_AXP20X
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 91a65bf..d684a52 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_AT91_ADC8xx) += at91_adc8xx.o
> obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
> obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
> diff --git a/drivers/iio/adc/at91_adc8xx.c b/drivers/iio/adc/at91_adc8xx.c
> new file mode 100644
> index 0000000..8b4a6e7
> --- /dev/null
> +++ b/drivers/iio/adc/at91_adc8xx.c
> @@ -0,0 +1,417 @@
> +/*
> + * Atmel ADC driver for SAMA5D2 devices and later.
> + *
> + * Copyright (C) 2015 Atmel,
> + * 2015 Ludovic Desroches <ludovic.desroches@atmel.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched.h>
> +#include <linux/wait.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define ADC_CR 0x00 /* Control Register */
> +#define ADC_CR_SWRST BIT(0) /* Software Reset */
> +#define ADC_CR_START BIT(1) /* Start Conversion */
> +#define ADC_CR_TSCALIB BIT(2) /* Touchscreen Calibration */
> +#define ADC_CR_CMPRST BIT(4) /* Comparison Restart */
> +#define ADC_MR 0x04 /* Mode Register */
> +#define ADC_MR_TRGSEL(v) (v << 1) /* Trigger Selection */
> +#define ADC_MR_TRGSEL_TRIG0 0 /* ADTRG */
> +#define ADC_MR_TRGSEL_TRIG1 1 /* TIOA0 */
> +#define ADC_MR_TRGSEL_TRIG2 2 /* TIOA1 */
> +#define ADC_MR_TRGSEL_TRIG3 3 /* TIOA2 */
> +#define ADC_MR_TRGSEL_TRIG4 4 /* PWM event line 0 */
> +#define ADC_MR_TRGSEL_TRIG5 5 /* PWM event line 1 */
> +#define ADC_MR_TRGSEL_TRIG6 6 /* TIOA3 */
> +#define ADC_MR_TRGSEL_TRIG7 7 /* RTCOUT0 */
> +#define ADC_MR_SLEEP BIT(5) /* Sleep Mode */
> +#define ADC_MR_FWUP BIT(6) /* Fast Wake Up */
> +#define ADC_MR_PRESCAL(v) (v << 8) /* Prescaler Rate Selection */
> +#define ADC_MR_PRESCAL_MAX 0xffff
> +#define ADC_MR_STARTUP(v) (v << 16) /* Startup Time */
> +#define ADC_MR_STARTUP_SUT0 0 /* 0 period of ADCCLK */
> +#define ADC_MR_STARTUP_SUT8 1 /* 8 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT16 2 /* 16 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT24 3 /* 24 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT64 4 /* 64 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT80 5 /* 80 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT96 6 /* 96 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT112 7 /* 112 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT512 8 /* 512 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT576 9 /* 576 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT640 10 /* 640 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT704 11 /* 704 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT768 12 /* 768 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT832 13 /* 832 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT896 14 /* 896 periods of ADCCLK */
> +#define ADC_MR_STARTUP_SUT960 15 /* 960 periods of ADCCLK */
> +#define ADC_MR_ANACH BIT(23) /* Analog Change */
> +#define ADC_MR_TRACKTIM(v) (v << 24) /* Tracking Time */
> +#define ADC_MR_TRACKTIM_MAX 0xff
> +#define ADC_MR_TRANSFER(v) (v << 28) /* Transfer Time */
> +#define ADC_MR_TRANSFER_MAX 0x3
> +#define ADC_MR_USEQ BIT(31) /* Use Sequence Enable */
> +#define ADC_SEQR1 0x08 /* Channel Sequence Register 1 */
> +#define ADC_SEQR2 0x0c /* Channel Sequence Register 2 */
> +#define ADC_CHER 0x10 /* Channel Enable Register */
> +#define ADC_CHDR 0x14 /* Channel Disable Register */
> +#define ADC_CHSR 0x18 /* Channel Status Register */
> +#define ADC_LCDR 0x20 /* Last Converted Data Register */
> +#define ADC_IER 0x24 /* Interrupt Enable Register */
> +#define ADC_IDR 0x28 /* Interrupt Disable Register */
> +#define ADC_IMR 0x2c /* Interrupt Mask Register */
> +#define ADC_ISR 0x30 /* Interrupt Status Register */
> +#define ADC_LCTMR 0x34 /* Last Channel Trigger Mode Register */
> +#define ADC_LCCWR 0x38 /* Last Channel Compare Window Register */
> +#define ADC_OVER 0x3c /* Overrun Status Register */
> +#define ADC_EMR 0x40 /* Extended Mode Register */
> +#define ADC_CWR 0x44 /* Compare Window Register */
> +#define ADC_CGR 0x48 /* Channel Gain Register */
> +#define ADC_COR 0x4c /* Channel Offset Register */
> +#define ADC_CDR0 0x50 /* Channel Data Register 0 */
> +#define ADC_ACR 0x94 /* Analog Control Register */
> +#define ADC_TSMR 0xb0 /* Touchscreen Mode Register */
> +#define ADC_XPOSR 0xb4 /* Touchscreen X Position Register */
> +#define ADC_YPOSR 0xb8 /* Touchscreen Y Position Register */
> +#define ADC_PRESSR 0xbc /* Touchscreen Pressure Register */
> +#define ADC_TRGR 0xc0 /* Trigger Register */
> +#define ADC_COSR 0xd0 /* Correction Select Register */
> +#define ADC_CVR 0xd4 /* Correction Value Register */
> +#define ADC_CECR 0xd8 /* Channel Error Correction Register */
> +#define ADC_WPMR 0xe4 /* Write Protection Mode Register */
> +#define ADC_WPSR 0xe8 /* Write Protection Status Register */
> +#define ADC_VERSION 0xfc /* Version Register */
> +
> +#define AT91_ADC_CHAN(num, addr) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .channel = num, \
> + .address = addr, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 12, \
> + .storagebits = 14, \
> + }, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .datasheet_name = "CH"#num, \
> + .indexed = 1, \
> + }
> +
> +#define at91_adc_readl(st, reg) readl_relaxed(st->base + reg)
> +#define at91_adc_writel(st, reg, val) writel_relaxed(val, st->base + reg)
> +
> +struct at91_adc_soc_info {
> + unsigned startup_time;
> + unsigned min_f_adc;
> + unsigned max_f_adc;
> + const struct iio_chan_spec *channels;
> + int num_channels;
> +};
> +
> +struct at91_adc_state {
> + void __iomem *base;
> + struct clk *per_clk;
> + struct clk *adc_clk;
> + int irq;
> + struct regulator *reg;
> + struct regulator *vref;
> + u32 vref_uv;
> + struct at91_adc_soc_info *soc_info;
> + wait_queue_head_t wq_data_available;
> + bool done;
> + const struct iio_chan_spec *chan;
> + u32 last_value;
> + struct mutex lock;
> +};
> +
> +static struct at91_adc_soc_info at91_adc_sama5d2_soc_info = {
> + .startup_time = 4,
> + .min_f_adc = 200000,
> + .max_f_adc = 20000000,
These rather look like things that should be in the device tree if they
are going to change between SoC varients.
> +};
> +
> +static const struct iio_chan_spec at91_adc_channels[] = {
> + AT91_ADC_CHAN(0, 0x50),
> + AT91_ADC_CHAN(1, 0x54),
> + AT91_ADC_CHAN(2, 0x58),
> + AT91_ADC_CHAN(3, 0x5c),
> + AT91_ADC_CHAN(4, 0x60),
> + AT91_ADC_CHAN(5, 0x64),
> + AT91_ADC_CHAN(6, 0x68),
> + AT91_ADC_CHAN(7, 0x6c),
> + AT91_ADC_CHAN(8, 0x70),
> + AT91_ADC_CHAN(9, 0x74),
> + AT91_ADC_CHAN(10, 0x78),
> + AT91_ADC_CHAN(11, 0x7c),
> +};
> +
> +static irqreturn_t at91_adc_interrupt(int irq, void *private)
> +{
> + struct iio_dev *indio = private;
> + struct at91_adc_state *st = iio_priv(indio);
> + u32 status = at91_adc_readl(st, ADC_ISR);
> +
> + status &= at91_adc_readl(st, ADC_IMR);
> + if (status & 0xFFF) {
> + st->last_value = at91_adc_readl(st, st->chan->address);
If this is a polled read - is there any reason to read this value here
rather than outside the interrupt?
> + st->done = true;
> + wake_up_interruptible(&st->wq_data_available);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static bool at91_adc_freq_supported(struct at91_adc_state *st, unsigned freq)
> +{
> + return ((freq >= st->soc_info->min_f_adc)
> + && (freq <= st->soc_info->max_f_adc));
> +}
> +
> +static unsigned at91_adc_startup_time(unsigned startup_time_min,
> + unsigned adc_clk_khz)
> +{
> + const unsigned startup_lookup[] = {
> + 0, 8, 16, 24,
> + 64, 80, 96, 112,
> + 512, 576, 640, 704,
> + 768, 832, 896, 960
> + };
> + unsigned ticks_min, i;
> +
> + /*
> + * Since the adc frequency is checked before, there is no reason
> + * to not meet the startup time constraint.
> + */
> +
> + ticks_min = startup_time_min * adc_clk_khz / 1000;
> + for (i = 0; i < ARRAY_SIZE(startup_lookup); i++)
> + if (startup_lookup[i] > ticks_min)
> + break;
> +
> + return i;
> +}
> +
> +static int at91_adc_init(struct at91_adc_state *st)
> +{
> + struct iio_dev *indio_dev = iio_priv_to_dev(st);
> + unsigned f_adc, f_per, prescal, startup;
> +
> + at91_adc_writel(st, ADC_CR, ADC_CR_SWRST);
> + at91_adc_writel(st, ADC_IDR, 0xffffffff);
> +
> + f_adc = clk_get_rate(st->adc_clk);
> + if (!at91_adc_freq_supported(st, f_adc)) {
> + dev_err(&indio_dev->dev, "unsupported adc clock frequency\n");
> + return -EINVAL;
> + }
> +
> + f_per = clk_get_rate(st->per_clk);
> + prescal = (f_per / (2 * f_adc)) - 1;
> +
> + startup = at91_adc_startup_time(st->soc_info->startup_time,
> + f_adc / 1000);
> +
> + at91_adc_writel(st, ADC_MR,
> + ADC_MR_TRANSFER(2)
> + | ADC_MR_STARTUP(startup)
> + | ADC_MR_PRESCAL(prescal));
> +
> + dev_dbg(&indio_dev->dev, "startup: %u, prescal: %u\n",
> + startup, prescal);
> +
> + return 0;
> +}
> +
> +static int at91_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct at91_adc_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + st->chan = chan;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&st->lock);
> +
> + at91_adc_writel(st, ADC_CHER, BIT(chan->channel));
> + at91_adc_writel(st, ADC_IER, BIT(chan->channel));
> + at91_adc_writel(st, ADC_CR, ADC_CR_START);
> +
> + ret = wait_event_interruptible_timeout(st->wq_data_available,
> + st->done,
> + msecs_to_jiffies(1000));
> + if (ret == 0)
> + ret = -ETIMEDOUT;
> +
> + if (ret <= 0) {
> + at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> + at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> + mutex_unlock(&st->lock);
> + return ret;
> + }
> +
> + if (chan->scan_type.sign == 's')
> + *val = sign_extend32(st->last_value,
> + chan->scan_type.realbits - 1);
> + else
> + *val = st->last_value;
> + at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> + at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> + st->done = false;
> +
> + mutex_unlock(&st->lock);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->vref_uv / 1000;
> + *val2 = chan->scan_type.realbits;
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info at91_adc_info = {
> + .read_raw = &at91_adc_read_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int at91_adc_probe(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev;
> + struct at91_adc_state *st;
> + struct resource *res;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev,
> + sizeof(struct at91_adc_state));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->name = dev_name(&pdev->dev);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &at91_adc_info;
> + indio_dev->channels = at91_adc_channels;
> + indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
> +
> + st = iio_priv(indio_dev);
> + st->soc_info = (struct at91_adc_soc_info *)
> + of_device_get_match_data(&pdev->dev);
> + init_waitqueue_head(&st->wq_data_available);
> + mutex_init(&st->lock);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + st->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(st->base))
> + return PTR_ERR(st->base);
> +
> + st->irq = platform_get_irq(pdev, 0);
> + if (st->irq < 0)
could be be 0 (effectively marked as not present) - probably want to check
for that and fault out if so.
> + return st->irq;
> +
> + st->per_clk = devm_clk_get(&pdev->dev, "adc_clk");
> + if (IS_ERR(st->per_clk))
> + return PTR_ERR(st->per_clk);
> +
> + st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
> + if (IS_ERR(st->adc_clk))
> + return PTR_ERR(st->adc_clk);
> +
> + st->reg = devm_regulator_get(&pdev->dev, "vddana");
> + if (IS_ERR(st->reg))
> + return PTR_ERR(st->reg);
You get this regulator but never explicitly request that it is on...
I can understand that for a reference like below (though probably want
to turn that on as well really).
> +
> + st->vref = devm_regulator_get(&pdev->dev, "vref");
> + if (IS_ERR(st->vref))
> + return PTR_ERR(st->vref);
> +
> + ret = devm_request_irq(&pdev->dev, st->irq, at91_adc_interrupt, 0,
> + pdev->dev.driver->name, indio_dev);
> + if (ret)
> + return ret;
> +
> + st->vref_uv = regulator_get_voltage(st->vref);
> + if (st->vref_uv <= 0) {
> + ret = -EINVAL;
> + goto error;
> + }
> +
> + ret = at91_adc_init(st);
> + if (ret)
> + goto error;
> +
> + ret = clk_prepare_enable(st->adc_clk);
Handle these possible errors.
> + ret = clk_prepare_enable(st->per_clk);
> +
> + ret = iio_device_register(indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + dev_info(&pdev->dev, "version: %x\n",
> + readl_relaxed(st->base + ADC_VERSION));
> +
> +error:
> + return ret;
> +}
> +
> +static int at91_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct at91_adc_state *st = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> +
> + clk_disable_unprepare(st->per_clk);
> + clk_disable_unprepare(st->adc_clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id at91_adc_dt_match[] = {
> + {
> + .compatible = "atmel,sama5d2-adc",
> + .data = &at91_adc_sama5d2_soc_info
> + }, {
> + /* sentinel */
> + }
> +};
> +MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
> +
> +static struct platform_driver at91_adc_driver = {
> + .probe = at91_adc_probe,
> + .remove = at91_adc_remove,
> + .driver = {
> + .name = "at91_adc8xx",
> + .of_match_table = at91_adc_dt_match,
> + },
> +};
> +module_platform_driver(at91_adc_driver)
> +
> +MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches@atmel.com>");
> +MODULE_DESCRIPTION("Atmel AT91 ADC 8xx");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-22 18:34 ` Jonathan Cameron
@ 2015-12-23 10:27 ` Ludovic Desroches
2015-12-23 10:48 ` Ludovic Desroches
2015-12-31 19:23 ` Jonathan Cameron
0 siblings, 2 replies; 18+ messages in thread
From: Ludovic Desroches @ 2015-12-23 10:27 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Ludovic Desroches, nicolas.ferre, alexandre.belloni, devicetree,
linux-kernel, linux-iio, plagnioj, linux-arm-kernel
On Tue, Dec 22, 2015 at 06:34:00PM +0000, Jonathan Cameron wrote:
> On 21/12/15 09:24, Ludovic Desroches wrote:
> > This driver supports the new version of the Atmel ADC device introduced
> > with the SAMA5D2 SoC family.
> >
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> A few more bits and bobs from me. Mostly looking good.
>
> Jonathan
> > ---
> > .../devicetree/bindings/iio/adc/at91_adc8xx.txt | 27 ++
> > drivers/iio/adc/Kconfig | 11 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/at91_adc8xx.c | 417 +++++++++++++++++++++
> > 4 files changed, 456 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > create mode 100644 drivers/iio/adc/at91_adc8xx.c
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > new file mode 100644
> > index 0000000..64ad6a5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > @@ -0,0 +1,27 @@
> > +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> > +
> > +Required properties:
> > + - compatible: Should be "atmel,sama5d2-adc".
> > + - reg: Should contain ADC registers location and length.
> > + - interrupts: Should contain the IRQ line for the ADC.
> > + - clocks: phandles to clocks.
> > + - clock-names: tuple listing clock names.
> > + Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> > + clock, "adc_clk" is the sampling clock.
> > + - vref-supply: Supply used as reference for conversions.
> > +
> > +Optional properties:
> > + - vddana-supply: Supply for the adc device.
> > +
> > +
> > +Example:
> > +
> > +adc: adc@fc030000 {
> > + compatible = "atmel,sama5d2-adc";
> > + reg = <0xfc030000 0x100>;
> > + interrupts = <40 IRQ_TYPE_LEVEL_HIGH 7>;
> > + clocks = <&adc_clk>, <&adc_op_clk>;
> > + clock-names = "adc_clk", "adc_op_clk";
> > + vddana-supply = <&vdd_3v3_lp_reg>;
> > + vref-supply = <&vdd_3v3_lp_reg>;
> > +}
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 9162dfe..5819e41 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -131,6 +131,17 @@ config AT91_ADC
> > To compile this driver as a module, choose M here: the module will be
> > called at91_adc.
> >
> > +config AT91_ADC8xx
> > + tristate "Atmel AT91 ADC 8xx"
> > + depends on ARCH_AT91
> > + depends on INPUT
> > + help
> > + Say yes here to build support for Atmel ADC 8xx which is available
> > + from SAMA5D2 SoC family.
> > +
> > + To compile this driver as a module, choose M here: the module will be
> > + called at91_adc8xx.
> > +
> > config AXP288_ADC
> > tristate "X-Powers AXP288 ADC driver"
> > depends on MFD_AXP20X
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 91a65bf..d684a52 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_AT91_ADC8xx) += at91_adc8xx.o
> > obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> > obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
> > obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
> > diff --git a/drivers/iio/adc/at91_adc8xx.c b/drivers/iio/adc/at91_adc8xx.c
> > new file mode 100644
> > index 0000000..8b4a6e7
> > --- /dev/null
> > +++ b/drivers/iio/adc/at91_adc8xx.c
> > @@ -0,0 +1,417 @@
> > +/*
> > + * Atmel ADC driver for SAMA5D2 devices and later.
> > + *
> > + * Copyright (C) 2015 Atmel,
> > + * 2015 Ludovic Desroches <ludovic.desroches@atmel.com>
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/sched.h>
> > +#include <linux/wait.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define ADC_CR 0x00 /* Control Register */
> > +#define ADC_CR_SWRST BIT(0) /* Software Reset */
> > +#define ADC_CR_START BIT(1) /* Start Conversion */
> > +#define ADC_CR_TSCALIB BIT(2) /* Touchscreen Calibration */
> > +#define ADC_CR_CMPRST BIT(4) /* Comparison Restart */
> > +#define ADC_MR 0x04 /* Mode Register */
> > +#define ADC_MR_TRGSEL(v) (v << 1) /* Trigger Selection */
> > +#define ADC_MR_TRGSEL_TRIG0 0 /* ADTRG */
> > +#define ADC_MR_TRGSEL_TRIG1 1 /* TIOA0 */
> > +#define ADC_MR_TRGSEL_TRIG2 2 /* TIOA1 */
> > +#define ADC_MR_TRGSEL_TRIG3 3 /* TIOA2 */
> > +#define ADC_MR_TRGSEL_TRIG4 4 /* PWM event line 0 */
> > +#define ADC_MR_TRGSEL_TRIG5 5 /* PWM event line 1 */
> > +#define ADC_MR_TRGSEL_TRIG6 6 /* TIOA3 */
> > +#define ADC_MR_TRGSEL_TRIG7 7 /* RTCOUT0 */
> > +#define ADC_MR_SLEEP BIT(5) /* Sleep Mode */
> > +#define ADC_MR_FWUP BIT(6) /* Fast Wake Up */
> > +#define ADC_MR_PRESCAL(v) (v << 8) /* Prescaler Rate Selection */
> > +#define ADC_MR_PRESCAL_MAX 0xffff
> > +#define ADC_MR_STARTUP(v) (v << 16) /* Startup Time */
> > +#define ADC_MR_STARTUP_SUT0 0 /* 0 period of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT8 1 /* 8 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT16 2 /* 16 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT24 3 /* 24 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT64 4 /* 64 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT80 5 /* 80 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT96 6 /* 96 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT112 7 /* 112 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT512 8 /* 512 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT576 9 /* 576 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT640 10 /* 640 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT704 11 /* 704 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT768 12 /* 768 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT832 13 /* 832 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT896 14 /* 896 periods of ADCCLK */
> > +#define ADC_MR_STARTUP_SUT960 15 /* 960 periods of ADCCLK */
> > +#define ADC_MR_ANACH BIT(23) /* Analog Change */
> > +#define ADC_MR_TRACKTIM(v) (v << 24) /* Tracking Time */
> > +#define ADC_MR_TRACKTIM_MAX 0xff
> > +#define ADC_MR_TRANSFER(v) (v << 28) /* Transfer Time */
> > +#define ADC_MR_TRANSFER_MAX 0x3
> > +#define ADC_MR_USEQ BIT(31) /* Use Sequence Enable */
> > +#define ADC_SEQR1 0x08 /* Channel Sequence Register 1 */
> > +#define ADC_SEQR2 0x0c /* Channel Sequence Register 2 */
> > +#define ADC_CHER 0x10 /* Channel Enable Register */
> > +#define ADC_CHDR 0x14 /* Channel Disable Register */
> > +#define ADC_CHSR 0x18 /* Channel Status Register */
> > +#define ADC_LCDR 0x20 /* Last Converted Data Register */
> > +#define ADC_IER 0x24 /* Interrupt Enable Register */
> > +#define ADC_IDR 0x28 /* Interrupt Disable Register */
> > +#define ADC_IMR 0x2c /* Interrupt Mask Register */
> > +#define ADC_ISR 0x30 /* Interrupt Status Register */
> > +#define ADC_LCTMR 0x34 /* Last Channel Trigger Mode Register */
> > +#define ADC_LCCWR 0x38 /* Last Channel Compare Window Register */
> > +#define ADC_OVER 0x3c /* Overrun Status Register */
> > +#define ADC_EMR 0x40 /* Extended Mode Register */
> > +#define ADC_CWR 0x44 /* Compare Window Register */
> > +#define ADC_CGR 0x48 /* Channel Gain Register */
> > +#define ADC_COR 0x4c /* Channel Offset Register */
> > +#define ADC_CDR0 0x50 /* Channel Data Register 0 */
> > +#define ADC_ACR 0x94 /* Analog Control Register */
> > +#define ADC_TSMR 0xb0 /* Touchscreen Mode Register */
> > +#define ADC_XPOSR 0xb4 /* Touchscreen X Position Register */
> > +#define ADC_YPOSR 0xb8 /* Touchscreen Y Position Register */
> > +#define ADC_PRESSR 0xbc /* Touchscreen Pressure Register */
> > +#define ADC_TRGR 0xc0 /* Trigger Register */
> > +#define ADC_COSR 0xd0 /* Correction Select Register */
> > +#define ADC_CVR 0xd4 /* Correction Value Register */
> > +#define ADC_CECR 0xd8 /* Channel Error Correction Register */
> > +#define ADC_WPMR 0xe4 /* Write Protection Mode Register */
> > +#define ADC_WPSR 0xe8 /* Write Protection Status Register */
> > +#define ADC_VERSION 0xfc /* Version Register */
> > +
> > +#define AT91_ADC_CHAN(num, addr) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .channel = num, \
> > + .address = addr, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 12, \
> > + .storagebits = 14, \
> > + }, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> > + .datasheet_name = "CH"#num, \
> > + .indexed = 1, \
> > + }
> > +
> > +#define at91_adc_readl(st, reg) readl_relaxed(st->base + reg)
> > +#define at91_adc_writel(st, reg, val) writel_relaxed(val, st->base + reg)
> > +
> > +struct at91_adc_soc_info {
> > + unsigned startup_time;
> > + unsigned min_f_adc;
> > + unsigned max_f_adc;
> > + const struct iio_chan_spec *channels;
> > + int num_channels;
> > +};
> > +
> > +struct at91_adc_state {
> > + void __iomem *base;
> > + struct clk *per_clk;
> > + struct clk *adc_clk;
> > + int irq;
> > + struct regulator *reg;
> > + struct regulator *vref;
> > + u32 vref_uv;
> > + struct at91_adc_soc_info *soc_info;
> > + wait_queue_head_t wq_data_available;
> > + bool done;
> > + const struct iio_chan_spec *chan;
> > + u32 last_value;
> > + struct mutex lock;
> > +};
> > +
> > +static struct at91_adc_soc_info at91_adc_sama5d2_soc_info = {
> > + .startup_time = 4,
> > + .min_f_adc = 200000,
> > + .max_f_adc = 20000000,
> These rather look like things that should be in the device tree if they
> are going to change between SoC varients.
I have no strong position about it, I use to put parameters relative
to a SoC family into the driver. They will change only for next SoC
family not varients. When moving to a new family, the adc device could
be a new one.
My only concern is to not put as many parameters in the device tree as
at91_adc does.
> > +};
> > +
> > +static const struct iio_chan_spec at91_adc_channels[] = {
> > + AT91_ADC_CHAN(0, 0x50),
> > + AT91_ADC_CHAN(1, 0x54),
> > + AT91_ADC_CHAN(2, 0x58),
> > + AT91_ADC_CHAN(3, 0x5c),
> > + AT91_ADC_CHAN(4, 0x60),
> > + AT91_ADC_CHAN(5, 0x64),
> > + AT91_ADC_CHAN(6, 0x68),
> > + AT91_ADC_CHAN(7, 0x6c),
> > + AT91_ADC_CHAN(8, 0x70),
> > + AT91_ADC_CHAN(9, 0x74),
> > + AT91_ADC_CHAN(10, 0x78),
> > + AT91_ADC_CHAN(11, 0x7c),
> > +};
> > +
> > +static irqreturn_t at91_adc_interrupt(int irq, void *private)
> > +{
> > + struct iio_dev *indio = private;
> > + struct at91_adc_state *st = iio_priv(indio);
> > + u32 status = at91_adc_readl(st, ADC_ISR);
> > +
> > + status &= at91_adc_readl(st, ADC_IMR);
> > + if (status & 0xFFF) {
> > + st->last_value = at91_adc_readl(st, st->chan->address);
> If this is a polled read - is there any reason to read this value here
> rather than outside the interrupt?
No it can be done outside the interrupt. I have taken some parts from the
previous driver but it was reading a register used by all the channels
when it has been designed. So yes there is probably no more reason to
read it into the interrupt.
> > + st->done = true;
> > + wake_up_interruptible(&st->wq_data_available);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static bool at91_adc_freq_supported(struct at91_adc_state *st, unsigned freq)
> > +{
> > + return ((freq >= st->soc_info->min_f_adc)
> > + && (freq <= st->soc_info->max_f_adc));
> > +}
> > +
> > +static unsigned at91_adc_startup_time(unsigned startup_time_min,
> > + unsigned adc_clk_khz)
> > +{
> > + const unsigned startup_lookup[] = {
> > + 0, 8, 16, 24,
> > + 64, 80, 96, 112,
> > + 512, 576, 640, 704,
> > + 768, 832, 896, 960
> > + };
> > + unsigned ticks_min, i;
> > +
> > + /*
> > + * Since the adc frequency is checked before, there is no reason
> > + * to not meet the startup time constraint.
> > + */
> > +
> > + ticks_min = startup_time_min * adc_clk_khz / 1000;
> > + for (i = 0; i < ARRAY_SIZE(startup_lookup); i++)
> > + if (startup_lookup[i] > ticks_min)
> > + break;
> > +
> > + return i;
> > +}
> > +
> > +static int at91_adc_init(struct at91_adc_state *st)
> > +{
> > + struct iio_dev *indio_dev = iio_priv_to_dev(st);
> > + unsigned f_adc, f_per, prescal, startup;
> > +
> > + at91_adc_writel(st, ADC_CR, ADC_CR_SWRST);
> > + at91_adc_writel(st, ADC_IDR, 0xffffffff);
> > +
> > + f_adc = clk_get_rate(st->adc_clk);
> > + if (!at91_adc_freq_supported(st, f_adc)) {
> > + dev_err(&indio_dev->dev, "unsupported adc clock frequency\n");
> > + return -EINVAL;
> > + }
> > +
> > + f_per = clk_get_rate(st->per_clk);
> > + prescal = (f_per / (2 * f_adc)) - 1;
> > +
> > + startup = at91_adc_startup_time(st->soc_info->startup_time,
> > + f_adc / 1000);
> > +
> > + at91_adc_writel(st, ADC_MR,
> > + ADC_MR_TRANSFER(2)
> > + | ADC_MR_STARTUP(startup)
> > + | ADC_MR_PRESCAL(prescal));
> > +
> > + dev_dbg(&indio_dev->dev, "startup: %u, prescal: %u\n",
> > + startup, prescal);
> > +
> > + return 0;
> > +}
> > +
> > +static int at91_adc_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct at91_adc_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + st->chan = chan;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + mutex_lock(&st->lock);
> > +
> > + at91_adc_writel(st, ADC_CHER, BIT(chan->channel));
> > + at91_adc_writel(st, ADC_IER, BIT(chan->channel));
> > + at91_adc_writel(st, ADC_CR, ADC_CR_START);
> > +
> > + ret = wait_event_interruptible_timeout(st->wq_data_available,
> > + st->done,
> > + msecs_to_jiffies(1000));
> > + if (ret == 0)
> > + ret = -ETIMEDOUT;
> > +
> > + if (ret <= 0) {
> > + at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> > + at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> > + mutex_unlock(&st->lock);
> > + return ret;
> > + }
> > +
> > + if (chan->scan_type.sign == 's')
> > + *val = sign_extend32(st->last_value,
> > + chan->scan_type.realbits - 1);
> > + else
> > + *val = st->last_value;
> > + at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> > + at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> > + st->done = false;
> > +
> > + mutex_unlock(&st->lock);
> > + return IIO_VAL_INT;
> > +
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = st->vref_uv / 1000;
> > + *val2 = chan->scan_type.realbits;
> > + return IIO_VAL_FRACTIONAL_LOG2;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct iio_info at91_adc_info = {
> > + .read_raw = &at91_adc_read_raw,
> > + .driver_module = THIS_MODULE,
> > +};
> > +
> > +static int at91_adc_probe(struct platform_device *pdev)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct at91_adc_state *st;
> > + struct resource *res;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&pdev->dev,
> > + sizeof(struct at91_adc_state));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + indio_dev->dev.parent = &pdev->dev;
> > + indio_dev->name = dev_name(&pdev->dev);
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &at91_adc_info;
> > + indio_dev->channels = at91_adc_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
> > +
> > + st = iio_priv(indio_dev);
> > + st->soc_info = (struct at91_adc_soc_info *)
> > + of_device_get_match_data(&pdev->dev);
> > + init_waitqueue_head(&st->wq_data_available);
> > + mutex_init(&st->lock);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -EINVAL;
> > +
> > + st->base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(st->base))
> > + return PTR_ERR(st->base);
> > +
> > + st->irq = platform_get_irq(pdev, 0);
> > + if (st->irq < 0)
> could be be 0 (effectively marked as not present) - probably want to check
> for that and fault out if so.
ok.
> > + return st->irq;
> > +
> > + st->per_clk = devm_clk_get(&pdev->dev, "adc_clk");
> > + if (IS_ERR(st->per_clk))
> > + return PTR_ERR(st->per_clk);
> > +
> > + st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
> > + if (IS_ERR(st->adc_clk))
> > + return PTR_ERR(st->adc_clk);
> > +
> > + st->reg = devm_regulator_get(&pdev->dev, "vddana");
> > + if (IS_ERR(st->reg))
> > + return PTR_ERR(st->reg);
> You get this regulator but never explicitly request that it is on...
> I can understand that for a reference like below (though probably want
> to turn that on as well really).
Yes, my regulators are always enabled, that's why I didn't think about
turning them on.
> > +
> > + st->vref = devm_regulator_get(&pdev->dev, "vref");
> > + if (IS_ERR(st->vref))
> > + return PTR_ERR(st->vref);
> > +
> > + ret = devm_request_irq(&pdev->dev, st->irq, at91_adc_interrupt, 0,
> > + pdev->dev.driver->name, indio_dev);
> > + if (ret)
> > + return ret;
> > +
> > + st->vref_uv = regulator_get_voltage(st->vref);
> > + if (st->vref_uv <= 0) {
> > + ret = -EINVAL;
> > + goto error;
> > + }
> > +
> > + ret = at91_adc_init(st);
> > + if (ret)
> > + goto error;
> > +
> > + ret = clk_prepare_enable(st->adc_clk);
> Handle these possible errors.
> > + ret = clk_prepare_enable(st->per_clk);
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + dev_info(&pdev->dev, "version: %x\n",
> > + readl_relaxed(st->base + ADC_VERSION));
> > +
> > +error:
> > + return ret;
> > +}
> > +
> > +static int at91_adc_remove(struct platform_device *pdev)
> > +{
> > + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > + struct at91_adc_state *st = iio_priv(indio_dev);
> > +
> > + iio_device_unregister(indio_dev);
> > +
> > + clk_disable_unprepare(st->per_clk);
> > + clk_disable_unprepare(st->adc_clk);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id at91_adc_dt_match[] = {
> > + {
> > + .compatible = "atmel,sama5d2-adc",
> > + .data = &at91_adc_sama5d2_soc_info
> > + }, {
> > + /* sentinel */
> > + }
> > +};
> > +MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
> > +
> > +static struct platform_driver at91_adc_driver = {
> > + .probe = at91_adc_probe,
> > + .remove = at91_adc_remove,
> > + .driver = {
> > + .name = "at91_adc8xx",
> > + .of_match_table = at91_adc_dt_match,
> > + },
> > +};
> > +module_platform_driver(at91_adc_driver)
> > +
> > +MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches@atmel.com>");
> > +MODULE_DESCRIPTION("Atmel AT91 ADC 8xx");
> > +MODULE_LICENSE("GPL v2");
> >
>
Thanks.
Ludovic
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-23 10:27 ` Ludovic Desroches
@ 2015-12-23 10:48 ` Ludovic Desroches
2015-12-23 17:21 ` Jonathan Cameron
2015-12-31 19:23 ` Jonathan Cameron
1 sibling, 1 reply; 18+ messages in thread
From: Ludovic Desroches @ 2015-12-23 10:48 UTC (permalink / raw)
To: Jonathan Cameron, nicolas.ferre, alexandre.belloni, devicetree,
linux-kernel, linux-iio, plagnioj, linux-arm-kernel
On Wed, Dec 23, 2015 at 11:27:00AM +0100, Ludovic Desroches wrote:
> On Tue, Dec 22, 2015 at 06:34:00PM +0000, Jonathan Cameron wrote:
> > On 21/12/15 09:24, Ludovic Desroches wrote:
> > > This driver supports the new version of the Atmel ADC device introduced
> > > with the SAMA5D2 SoC family.
> > >
[...]
> > > +static irqreturn_t at91_adc_interrupt(int irq, void *private)
> > > +{
> > > + struct iio_dev *indio = private;
> > > + struct at91_adc_state *st = iio_priv(indio);
> > > + u32 status = at91_adc_readl(st, ADC_ISR);
> > > +
> > > + status &= at91_adc_readl(st, ADC_IMR);
> > > + if (status & 0xFFF) {
> > > + st->last_value = at91_adc_readl(st, st->chan->address);
> > If this is a polled read - is there any reason to read this value here
> > rather than outside the interrupt?
>
> No it can be done outside the interrupt. I have taken some parts from the
> previous driver but it was reading a register used by all the channels
> when it has been designed. So yes there is probably no more reason to
> read it into the interrupt.
>
Thinking about it. Is it really useful to move reading outside the
interrupt?
By the way this is not a polled read.
> > > + st->done = true;
> > > + wake_up_interruptible(&st->wq_data_available);
> > > + }
> > > +
> > > + return IRQ_HANDLED;
> > > +}
Ludovic
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-23 10:48 ` Ludovic Desroches
@ 2015-12-23 17:21 ` Jonathan Cameron
2016-01-05 13:18 ` Ludovic Desroches
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2015-12-23 17:21 UTC (permalink / raw)
To: Ludovic Desroches, Jonathan Cameron, nicolas.ferre,
alexandre.belloni, devicetree, linux-kernel, linux-iio, plagnioj,
linux-arm-kernel
On 23 December 2015 10:48:33 GMT+00:00, Ludovic Desroches <ludovic.desroches@atmel.com> wrote:
>On Wed, Dec 23, 2015 at 11:27:00AM +0100, Ludovic Desroches wrote:
>> On Tue, Dec 22, 2015 at 06:34:00PM +0000, Jonathan Cameron wrote:
>> > On 21/12/15 09:24, Ludovic Desroches wrote:
>> > > This driver supports the new version of the Atmel ADC device
>introduced
>> > > with the SAMA5D2 SoC family.
>> > >
>
>[...]
>
>> > > +static irqreturn_t at91_adc_interrupt(int irq, void *private)
>> > > +{
>> > > + struct iio_dev *indio = private;
>> > > + struct at91_adc_state *st = iio_priv(indio);
>> > > + u32 status = at91_adc_readl(st, ADC_ISR);
>> > > +
>> > > + status &= at91_adc_readl(st, ADC_IMR);
>> > > + if (status & 0xFFF) {
>> > > + st->last_value = at91_adc_readl(st, st->chan->address);
>> > If this is a polled read - is there any reason to read this value
>here
>> > rather than outside the interrupt?
>>
>> No it can be done outside the interrupt. I have taken some parts from
>the
>> previous driver but it was reading a register used by all the
>channels
>> when it has been designed. So yes there is probably no more reason to
>> read it into the interrupt.
>>
>
>Thinking about it. Is it really useful to move reading outside the
>interrupt?
It avoids needing to know the channel in here, hence simplifying the code a bit.
>
>By the way this is not a polled read.
I meant that it is an individually requested conversion rather than a free running
sampling system where the value read might change before it is read out.
>
>> > > + st->done = true;
>> > > + wake_up_interruptible(&st->wq_data_available);
>> > > + }
>> > > +
>> > > + return IRQ_HANDLED;
>> > > +}
>
>Ludovic
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-23 17:21 ` Jonathan Cameron
@ 2016-01-05 13:18 ` Ludovic Desroches
0 siblings, 0 replies; 18+ messages in thread
From: Ludovic Desroches @ 2016-01-05 13:18 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Ludovic Desroches, Jonathan Cameron, nicolas.ferre,
alexandre.belloni, devicetree, linux-kernel, linux-iio, plagnioj,
linux-arm-kernel
On Wed, Dec 23, 2015 at 05:21:25PM +0000, Jonathan Cameron wrote:
>
>
> On 23 December 2015 10:48:33 GMT+00:00, Ludovic Desroches <ludovic.desroches@atmel.com> wrote:
> >On Wed, Dec 23, 2015 at 11:27:00AM +0100, Ludovic Desroches wrote:
> >> On Tue, Dec 22, 2015 at 06:34:00PM +0000, Jonathan Cameron wrote:
> >> > On 21/12/15 09:24, Ludovic Desroches wrote:
> >> > > This driver supports the new version of the Atmel ADC device
> >introduced
> >> > > with the SAMA5D2 SoC family.
> >> > >
> >
> >[...]
> >
> >> > > +static irqreturn_t at91_adc_interrupt(int irq, void *private)
> >> > > +{
> >> > > + struct iio_dev *indio = private;
> >> > > + struct at91_adc_state *st = iio_priv(indio);
> >> > > + u32 status = at91_adc_readl(st, ADC_ISR);
> >> > > +
> >> > > + status &= at91_adc_readl(st, ADC_IMR);
> >> > > + if (status & 0xFFF) {
> >> > > + st->last_value = at91_adc_readl(st, st->chan->address);
> >> > If this is a polled read - is there any reason to read this value
> >here
> >> > rather than outside the interrupt?
> >>
> >> No it can be done outside the interrupt. I have taken some parts from
> >the
> >> previous driver but it was reading a register used by all the
> >channels
> >> when it has been designed. So yes there is probably no more reason to
> >> read it into the interrupt.
> >>
> >
> >Thinking about it. Is it really useful to move reading outside the
> >interrupt?
> It avoids needing to know the channel in here, hence simplifying the code a bit.
I am not sure it will simplify the code since the interrupt is cleared
when reading the conversion not the interrupt status register.
> >
> >By the way this is not a polled read.
> I meant that it is an individually requested conversion rather than a free running
> sampling system where the value read might change before it is read out.
> >
> >> > > + st->done = true;
> >> > > + wake_up_interruptible(&st->wq_data_available);
> >> > > + }
> >> > > +
> >> > > + return IRQ_HANDLED;
> >> > > +}
> >
> >Ludovic
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-23 10:27 ` Ludovic Desroches
2015-12-23 10:48 ` Ludovic Desroches
@ 2015-12-31 19:23 ` Jonathan Cameron
1 sibling, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2015-12-31 19:23 UTC (permalink / raw)
To: nicolas.ferre, alexandre.belloni, devicetree, linux-kernel,
linux-iio, plagnioj, linux-arm-kernel
On 23/12/15 10:27, Ludovic Desroches wrote:
> On Tue, Dec 22, 2015 at 06:34:00PM +0000, Jonathan Cameron wrote:
>> On 21/12/15 09:24, Ludovic Desroches wrote:
>>> This driver supports the new version of the Atmel ADC device introduced
>>> with the SAMA5D2 SoC family.
>>>
>>> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
>> A few more bits and bobs from me. Mostly looking good.
>>
>> Jonathan
>>> ---
>>> .../devicetree/bindings/iio/adc/at91_adc8xx.txt | 27 ++
>>> drivers/iio/adc/Kconfig | 11 +
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/at91_adc8xx.c | 417 +++++++++++++++++++++
>>> 4 files changed, 456 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
>>> create mode 100644 drivers/iio/adc/at91_adc8xx.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
>>> new file mode 100644
>>> index 0000000..64ad6a5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
>>> @@ -0,0 +1,27 @@
>>> +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
>>> +
>>> +Required properties:
>>> + - compatible: Should be "atmel,sama5d2-adc".
>>> + - reg: Should contain ADC registers location and length.
>>> + - interrupts: Should contain the IRQ line for the ADC.
>>> + - clocks: phandles to clocks.
>>> + - clock-names: tuple listing clock names.
>>> + Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
>>> + clock, "adc_clk" is the sampling clock.
>>> + - vref-supply: Supply used as reference for conversions.
>>> +
>>> +Optional properties:
>>> + - vddana-supply: Supply for the adc device.
>>> +
>>> +
>>> +Example:
>>> +
>>> +adc: adc@fc030000 {
>>> + compatible = "atmel,sama5d2-adc";
>>> + reg = <0xfc030000 0x100>;
>>> + interrupts = <40 IRQ_TYPE_LEVEL_HIGH 7>;
>>> + clocks = <&adc_clk>, <&adc_op_clk>;
>>> + clock-names = "adc_clk", "adc_op_clk";
>>> + vddana-supply = <&vdd_3v3_lp_reg>;
>>> + vref-supply = <&vdd_3v3_lp_reg>;
>>> +}
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 9162dfe..5819e41 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -131,6 +131,17 @@ config AT91_ADC
>>> To compile this driver as a module, choose M here: the module will be
>>> called at91_adc.
>>>
>>> +config AT91_ADC8xx
>>> + tristate "Atmel AT91 ADC 8xx"
>>> + depends on ARCH_AT91
>>> + depends on INPUT
>>> + help
>>> + Say yes here to build support for Atmel ADC 8xx which is available
>>> + from SAMA5D2 SoC family.
>>> +
>>> + To compile this driver as a module, choose M here: the module will be
>>> + called at91_adc8xx.
>>> +
>>> config AXP288_ADC
>>> tristate "X-Powers AXP288 ADC driver"
>>> depends on MFD_AXP20X
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index 91a65bf..d684a52 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_AT91_ADC8xx) += at91_adc8xx.o
>>> obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
>>> obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>>> obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>>> diff --git a/drivers/iio/adc/at91_adc8xx.c b/drivers/iio/adc/at91_adc8xx.c
>>> new file mode 100644
>>> index 0000000..8b4a6e7
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/at91_adc8xx.c
>>> @@ -0,0 +1,417 @@
>>> +/*
>>> + * Atmel ADC driver for SAMA5D2 devices and later.
>>> + *
>>> + * Copyright (C) 2015 Atmel,
>>> + * 2015 Ludovic Desroches <ludovic.desroches@atmel.com>
>>> + *
>>> + * This software is licensed under the terms of the GNU General Public
>>> + * License version 2, as published by the Free Software Foundation, and
>>> + * may be copied, distributed, and modified under those terms.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/sched.h>
>>> +#include <linux/wait.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/regulator/consumer.h>
>>> +
>>> +#define ADC_CR 0x00 /* Control Register */
>>> +#define ADC_CR_SWRST BIT(0) /* Software Reset */
>>> +#define ADC_CR_START BIT(1) /* Start Conversion */
>>> +#define ADC_CR_TSCALIB BIT(2) /* Touchscreen Calibration */
>>> +#define ADC_CR_CMPRST BIT(4) /* Comparison Restart */
>>> +#define ADC_MR 0x04 /* Mode Register */
>>> +#define ADC_MR_TRGSEL(v) (v << 1) /* Trigger Selection */
>>> +#define ADC_MR_TRGSEL_TRIG0 0 /* ADTRG */
>>> +#define ADC_MR_TRGSEL_TRIG1 1 /* TIOA0 */
>>> +#define ADC_MR_TRGSEL_TRIG2 2 /* TIOA1 */
>>> +#define ADC_MR_TRGSEL_TRIG3 3 /* TIOA2 */
>>> +#define ADC_MR_TRGSEL_TRIG4 4 /* PWM event line 0 */
>>> +#define ADC_MR_TRGSEL_TRIG5 5 /* PWM event line 1 */
>>> +#define ADC_MR_TRGSEL_TRIG6 6 /* TIOA3 */
>>> +#define ADC_MR_TRGSEL_TRIG7 7 /* RTCOUT0 */
>>> +#define ADC_MR_SLEEP BIT(5) /* Sleep Mode */
>>> +#define ADC_MR_FWUP BIT(6) /* Fast Wake Up */
>>> +#define ADC_MR_PRESCAL(v) (v << 8) /* Prescaler Rate Selection */
>>> +#define ADC_MR_PRESCAL_MAX 0xffff
>>> +#define ADC_MR_STARTUP(v) (v << 16) /* Startup Time */
>>> +#define ADC_MR_STARTUP_SUT0 0 /* 0 period of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT8 1 /* 8 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT16 2 /* 16 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT24 3 /* 24 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT64 4 /* 64 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT80 5 /* 80 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT96 6 /* 96 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT112 7 /* 112 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT512 8 /* 512 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT576 9 /* 576 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT640 10 /* 640 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT704 11 /* 704 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT768 12 /* 768 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT832 13 /* 832 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT896 14 /* 896 periods of ADCCLK */
>>> +#define ADC_MR_STARTUP_SUT960 15 /* 960 periods of ADCCLK */
>>> +#define ADC_MR_ANACH BIT(23) /* Analog Change */
>>> +#define ADC_MR_TRACKTIM(v) (v << 24) /* Tracking Time */
>>> +#define ADC_MR_TRACKTIM_MAX 0xff
>>> +#define ADC_MR_TRANSFER(v) (v << 28) /* Transfer Time */
>>> +#define ADC_MR_TRANSFER_MAX 0x3
>>> +#define ADC_MR_USEQ BIT(31) /* Use Sequence Enable */
>>> +#define ADC_SEQR1 0x08 /* Channel Sequence Register 1 */
>>> +#define ADC_SEQR2 0x0c /* Channel Sequence Register 2 */
>>> +#define ADC_CHER 0x10 /* Channel Enable Register */
>>> +#define ADC_CHDR 0x14 /* Channel Disable Register */
>>> +#define ADC_CHSR 0x18 /* Channel Status Register */
>>> +#define ADC_LCDR 0x20 /* Last Converted Data Register */
>>> +#define ADC_IER 0x24 /* Interrupt Enable Register */
>>> +#define ADC_IDR 0x28 /* Interrupt Disable Register */
>>> +#define ADC_IMR 0x2c /* Interrupt Mask Register */
>>> +#define ADC_ISR 0x30 /* Interrupt Status Register */
>>> +#define ADC_LCTMR 0x34 /* Last Channel Trigger Mode Register */
>>> +#define ADC_LCCWR 0x38 /* Last Channel Compare Window Register */
>>> +#define ADC_OVER 0x3c /* Overrun Status Register */
>>> +#define ADC_EMR 0x40 /* Extended Mode Register */
>>> +#define ADC_CWR 0x44 /* Compare Window Register */
>>> +#define ADC_CGR 0x48 /* Channel Gain Register */
>>> +#define ADC_COR 0x4c /* Channel Offset Register */
>>> +#define ADC_CDR0 0x50 /* Channel Data Register 0 */
>>> +#define ADC_ACR 0x94 /* Analog Control Register */
>>> +#define ADC_TSMR 0xb0 /* Touchscreen Mode Register */
>>> +#define ADC_XPOSR 0xb4 /* Touchscreen X Position Register */
>>> +#define ADC_YPOSR 0xb8 /* Touchscreen Y Position Register */
>>> +#define ADC_PRESSR 0xbc /* Touchscreen Pressure Register */
>>> +#define ADC_TRGR 0xc0 /* Trigger Register */
>>> +#define ADC_COSR 0xd0 /* Correction Select Register */
>>> +#define ADC_CVR 0xd4 /* Correction Value Register */
>>> +#define ADC_CECR 0xd8 /* Channel Error Correction Register */
>>> +#define ADC_WPMR 0xe4 /* Write Protection Mode Register */
>>> +#define ADC_WPSR 0xe8 /* Write Protection Status Register */
>>> +#define ADC_VERSION 0xfc /* Version Register */
>>> +
>>> +#define AT91_ADC_CHAN(num, addr) \
>>> + { \
>>> + .type = IIO_VOLTAGE, \
>>> + .channel = num, \
>>> + .address = addr, \
>>> + .scan_type = { \
>>> + .sign = 'u', \
>>> + .realbits = 12, \
>>> + .storagebits = 14, \
>>> + }, \
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
>>> + .datasheet_name = "CH"#num, \
>>> + .indexed = 1, \
>>> + }
>>> +
>>> +#define at91_adc_readl(st, reg) readl_relaxed(st->base + reg)
>>> +#define at91_adc_writel(st, reg, val) writel_relaxed(val, st->base + reg)
>>> +
>>> +struct at91_adc_soc_info {
>>> + unsigned startup_time;
>>> + unsigned min_f_adc;
>>> + unsigned max_f_adc;
>>> + const struct iio_chan_spec *channels;
>>> + int num_channels;
>>> +};
>>> +
>>> +struct at91_adc_state {
>>> + void __iomem *base;
>>> + struct clk *per_clk;
>>> + struct clk *adc_clk;
>>> + int irq;
>>> + struct regulator *reg;
>>> + struct regulator *vref;
>>> + u32 vref_uv;
>>> + struct at91_adc_soc_info *soc_info;
>>> + wait_queue_head_t wq_data_available;
>>> + bool done;
>>> + const struct iio_chan_spec *chan;
>>> + u32 last_value;
>>> + struct mutex lock;
>>> +};
>>> +
>>> +static struct at91_adc_soc_info at91_adc_sama5d2_soc_info = {
>>> + .startup_time = 4,
>>> + .min_f_adc = 200000,
>>> + .max_f_adc = 20000000,
>> These rather look like things that should be in the device tree if they
>> are going to change between SoC varients.
>
> I have no strong position about it, I use to put parameters relative
> to a SoC family into the driver. They will change only for next SoC
> family not varients. When moving to a new family, the adc device could
> be a new one.
If we have to add these parameters to the devicetree later (to support a new
soc) that will be fine as long as the defaults are these (so it will work
with the older part).
>
> My only concern is to not put as many parameters in the device tree as
> at91_adc does.
A reasonable aim!
>
>>> +};
>>> +
>>> +static const struct iio_chan_spec at91_adc_channels[] = {
>>> + AT91_ADC_CHAN(0, 0x50),
>>> + AT91_ADC_CHAN(1, 0x54),
>>> + AT91_ADC_CHAN(2, 0x58),
>>> + AT91_ADC_CHAN(3, 0x5c),
>>> + AT91_ADC_CHAN(4, 0x60),
>>> + AT91_ADC_CHAN(5, 0x64),
>>> + AT91_ADC_CHAN(6, 0x68),
>>> + AT91_ADC_CHAN(7, 0x6c),
>>> + AT91_ADC_CHAN(8, 0x70),
>>> + AT91_ADC_CHAN(9, 0x74),
>>> + AT91_ADC_CHAN(10, 0x78),
>>> + AT91_ADC_CHAN(11, 0x7c),
>>> +};
>>> +
>>> +static irqreturn_t at91_adc_interrupt(int irq, void *private)
>>> +{
>>> + struct iio_dev *indio = private;
>>> + struct at91_adc_state *st = iio_priv(indio);
>>> + u32 status = at91_adc_readl(st, ADC_ISR);
>>> +
>>> + status &= at91_adc_readl(st, ADC_IMR);
>>> + if (status & 0xFFF) {
>>> + st->last_value = at91_adc_readl(st, st->chan->address);
>> If this is a polled read - is there any reason to read this value here
>> rather than outside the interrupt?
>
> No it can be done outside the interrupt. I have taken some parts from the
> previous driver but it was reading a register used by all the channels
> when it has been designed. So yes there is probably no more reason to
> read it into the interrupt.
>
>>> + st->done = true;
>>> + wake_up_interruptible(&st->wq_data_available);
>>> + }
>>> +
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static bool at91_adc_freq_supported(struct at91_adc_state *st, unsigned freq)
>>> +{
>>> + return ((freq >= st->soc_info->min_f_adc)
>>> + && (freq <= st->soc_info->max_f_adc));
>>> +}
>>> +
>>> +static unsigned at91_adc_startup_time(unsigned startup_time_min,
>>> + unsigned adc_clk_khz)
>>> +{
>>> + const unsigned startup_lookup[] = {
>>> + 0, 8, 16, 24,
>>> + 64, 80, 96, 112,
>>> + 512, 576, 640, 704,
>>> + 768, 832, 896, 960
>>> + };
>>> + unsigned ticks_min, i;
>>> +
>>> + /*
>>> + * Since the adc frequency is checked before, there is no reason
>>> + * to not meet the startup time constraint.
>>> + */
>>> +
>>> + ticks_min = startup_time_min * adc_clk_khz / 1000;
>>> + for (i = 0; i < ARRAY_SIZE(startup_lookup); i++)
>>> + if (startup_lookup[i] > ticks_min)
>>> + break;
>>> +
>>> + return i;
>>> +}
>>> +
>>> +static int at91_adc_init(struct at91_adc_state *st)
>>> +{
>>> + struct iio_dev *indio_dev = iio_priv_to_dev(st);
>>> + unsigned f_adc, f_per, prescal, startup;
>>> +
>>> + at91_adc_writel(st, ADC_CR, ADC_CR_SWRST);
>>> + at91_adc_writel(st, ADC_IDR, 0xffffffff);
>>> +
>>> + f_adc = clk_get_rate(st->adc_clk);
>>> + if (!at91_adc_freq_supported(st, f_adc)) {
>>> + dev_err(&indio_dev->dev, "unsupported adc clock frequency\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + f_per = clk_get_rate(st->per_clk);
>>> + prescal = (f_per / (2 * f_adc)) - 1;
>>> +
>>> + startup = at91_adc_startup_time(st->soc_info->startup_time,
>>> + f_adc / 1000);
>>> +
>>> + at91_adc_writel(st, ADC_MR,
>>> + ADC_MR_TRANSFER(2)
>>> + | ADC_MR_STARTUP(startup)
>>> + | ADC_MR_PRESCAL(prescal));
>>> +
>>> + dev_dbg(&indio_dev->dev, "startup: %u, prescal: %u\n",
>>> + startup, prescal);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int at91_adc_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val, int *val2, long mask)
>>> +{
>>> + struct at91_adc_state *st = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + st->chan = chan;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + mutex_lock(&st->lock);
>>> +
>>> + at91_adc_writel(st, ADC_CHER, BIT(chan->channel));
>>> + at91_adc_writel(st, ADC_IER, BIT(chan->channel));
>>> + at91_adc_writel(st, ADC_CR, ADC_CR_START);
>>> +
>>> + ret = wait_event_interruptible_timeout(st->wq_data_available,
>>> + st->done,
>>> + msecs_to_jiffies(1000));
>>> + if (ret == 0)
>>> + ret = -ETIMEDOUT;
>>> +
>>> + if (ret <= 0) {
>>> + at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
>>> + at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
>>> + mutex_unlock(&st->lock);
>>> + return ret;
>>> + }
>>> +
>>> + if (chan->scan_type.sign == 's')
>>> + *val = sign_extend32(st->last_value,
>>> + chan->scan_type.realbits - 1);
>>> + else
>>> + *val = st->last_value;
>>> + at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
>>> + at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
>>> + st->done = false;
>>> +
>>> + mutex_unlock(&st->lock);
>>> + return IIO_VAL_INT;
>>> +
>>> + case IIO_CHAN_INFO_SCALE:
>>> + *val = st->vref_uv / 1000;
>>> + *val2 = chan->scan_type.realbits;
>>> + return IIO_VAL_FRACTIONAL_LOG2;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +static const struct iio_info at91_adc_info = {
>>> + .read_raw = &at91_adc_read_raw,
>>> + .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +static int at91_adc_probe(struct platform_device *pdev)
>>> +{
>>> + struct iio_dev *indio_dev;
>>> + struct at91_adc_state *st;
>>> + struct resource *res;
>>> + int ret;
>>> +
>>> + indio_dev = devm_iio_device_alloc(&pdev->dev,
>>> + sizeof(struct at91_adc_state));
>>> + if (!indio_dev)
>>> + return -ENOMEM;
>>> +
>>> + indio_dev->dev.parent = &pdev->dev;
>>> + indio_dev->name = dev_name(&pdev->dev);
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> + indio_dev->info = &at91_adc_info;
>>> + indio_dev->channels = at91_adc_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
>>> +
>>> + st = iio_priv(indio_dev);
>>> + st->soc_info = (struct at91_adc_soc_info *)
>>> + of_device_get_match_data(&pdev->dev);
>>> + init_waitqueue_head(&st->wq_data_available);
>>> + mutex_init(&st->lock);
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + if (!res)
>>> + return -EINVAL;
>>> +
>>> + st->base = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(st->base))
>>> + return PTR_ERR(st->base);
>>> +
>>> + st->irq = platform_get_irq(pdev, 0);
>>> + if (st->irq < 0)
>> could be be 0 (effectively marked as not present) - probably want to check
>> for that and fault out if so.
>
> ok.
>
>>> + return st->irq;
>>> +
>>> + st->per_clk = devm_clk_get(&pdev->dev, "adc_clk");
>>> + if (IS_ERR(st->per_clk))
>>> + return PTR_ERR(st->per_clk);
>>> +
>>> + st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
>>> + if (IS_ERR(st->adc_clk))
>>> + return PTR_ERR(st->adc_clk);
>>> +
>>> + st->reg = devm_regulator_get(&pdev->dev, "vddana");
>>> + if (IS_ERR(st->reg))
>>> + return PTR_ERR(st->reg);
>> You get this regulator but never explicitly request that it is on...
>> I can understand that for a reference like below (though probably want
>> to turn that on as well really).
>
> Yes, my regulators are always enabled, that's why I didn't think about
> turning them on.
>
>>> +
>>> + st->vref = devm_regulator_get(&pdev->dev, "vref");
>>> + if (IS_ERR(st->vref))
>>> + return PTR_ERR(st->vref);
>>> +
>>> + ret = devm_request_irq(&pdev->dev, st->irq, at91_adc_interrupt, 0,
>>> + pdev->dev.driver->name, indio_dev);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + st->vref_uv = regulator_get_voltage(st->vref);
>>> + if (st->vref_uv <= 0) {
>>> + ret = -EINVAL;
>>> + goto error;
>>> + }
>>> +
>>> + ret = at91_adc_init(st);
>>> + if (ret)
>>> + goto error;
>>> +
>>> + ret = clk_prepare_enable(st->adc_clk);
>> Handle these possible errors.
>>> + ret = clk_prepare_enable(st->per_clk);
>>> +
>>> + ret = iio_device_register(indio_dev);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + dev_info(&pdev->dev, "version: %x\n",
>>> + readl_relaxed(st->base + ADC_VERSION));
>>> +
>>> +error:
>>> + return ret;
>>> +}
>>> +
>>> +static int at91_adc_remove(struct platform_device *pdev)
>>> +{
>>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> + struct at91_adc_state *st = iio_priv(indio_dev);
>>> +
>>> + iio_device_unregister(indio_dev);
>>> +
>>> + clk_disable_unprepare(st->per_clk);
>>> + clk_disable_unprepare(st->adc_clk);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct of_device_id at91_adc_dt_match[] = {
>>> + {
>>> + .compatible = "atmel,sama5d2-adc",
>>> + .data = &at91_adc_sama5d2_soc_info
>>> + }, {
>>> + /* sentinel */
>>> + }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
>>> +
>>> +static struct platform_driver at91_adc_driver = {
>>> + .probe = at91_adc_probe,
>>> + .remove = at91_adc_remove,
>>> + .driver = {
>>> + .name = "at91_adc8xx",
>>> + .of_match_table = at91_adc_dt_match,
>>> + },
>>> +};
>>> +module_platform_driver(at91_adc_driver)
>>> +
>>> +MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches@atmel.com>");
>>> +MODULE_DESCRIPTION("Atmel AT91 ADC 8xx");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>
> Thanks.
>
> Ludovic
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-21 9:24 ` [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver Ludovic Desroches
2015-12-21 9:38 ` Peter Meerwald-Stadler
2015-12-22 18:34 ` Jonathan Cameron
@ 2015-12-23 0:51 ` Rob Herring
2015-12-23 10:29 ` Ludovic Desroches
2015-12-23 10:27 ` Alexandre Belloni
3 siblings, 1 reply; 18+ messages in thread
From: Rob Herring @ 2015-12-23 0:51 UTC (permalink / raw)
To: Ludovic Desroches
Cc: jic23, nicolas.ferre, alexandre.belloni, devicetree, linux-kernel,
linux-iio, plagnioj, linux-arm-kernel
On Mon, Dec 21, 2015 at 10:24:08AM +0100, Ludovic Desroches wrote:
> This driver supports the new version of the Atmel ADC device introduced
> with the SAMA5D2 SoC family.
>
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
> .../devicetree/bindings/iio/adc/at91_adc8xx.txt | 27 ++
> drivers/iio/adc/Kconfig | 11 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/at91_adc8xx.c | 417 +++++++++++++++++++++
> 4 files changed, 456 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> create mode 100644 drivers/iio/adc/at91_adc8xx.c
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> new file mode 100644
> index 0000000..64ad6a5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> @@ -0,0 +1,27 @@
> +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> +
> +Required properties:
> + - compatible: Should be "atmel,sama5d2-adc".
> + - reg: Should contain ADC registers location and length.
> + - interrupts: Should contain the IRQ line for the ADC.
> + - clocks: phandles to clocks.
> + - clock-names: tuple listing clock names.
> + Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> + clock, "adc_clk" is the sampling clock.
> + - vref-supply: Supply used as reference for conversions.
> +
> +Optional properties:
> + - vddana-supply: Supply for the adc device.
What makes a supply optional? If chip dependent, then then you need a
more specific compatible string.
Rob
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-23 0:51 ` Rob Herring
@ 2015-12-23 10:29 ` Ludovic Desroches
0 siblings, 0 replies; 18+ messages in thread
From: Ludovic Desroches @ 2015-12-23 10:29 UTC (permalink / raw)
To: Rob Herring
Cc: Ludovic Desroches, jic23, nicolas.ferre, alexandre.belloni,
devicetree, linux-kernel, linux-iio, plagnioj, linux-arm-kernel
On Tue, Dec 22, 2015 at 06:51:18PM -0600, Rob Herring wrote:
> On Mon, Dec 21, 2015 at 10:24:08AM +0100, Ludovic Desroches wrote:
> > This driver supports the new version of the Atmel ADC device introduced
> > with the SAMA5D2 SoC family.
> >
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > ---
> > .../devicetree/bindings/iio/adc/at91_adc8xx.txt | 27 ++
> > drivers/iio/adc/Kconfig | 11 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/at91_adc8xx.c | 417 +++++++++++++++++++++
> > 4 files changed, 456 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > create mode 100644 drivers/iio/adc/at91_adc8xx.c
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > new file mode 100644
> > index 0000000..64ad6a5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > @@ -0,0 +1,27 @@
> > +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> > +
> > +Required properties:
> > + - compatible: Should be "atmel,sama5d2-adc".
> > + - reg: Should contain ADC registers location and length.
> > + - interrupts: Should contain the IRQ line for the ADC.
> > + - clocks: phandles to clocks.
> > + - clock-names: tuple listing clock names.
> > + Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> > + clock, "adc_clk" is the sampling clock.
> > + - vref-supply: Supply used as reference for conversions.
> > +
> > +Optional properties:
> > + - vddana-supply: Supply for the adc device.
>
> What makes a supply optional? If chip dependent, then then you need a
> more specific compatible string.
>
I thought I had read that we can supply an external vddana or having a
default one. Double checking the datasheet. I can't find it, I probably
read it on a draft version. Will move the vddana supply to required
properties.
Thanks
Ludovic
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver
2015-12-21 9:24 ` [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver Ludovic Desroches
` (2 preceding siblings ...)
2015-12-23 0:51 ` Rob Herring
@ 2015-12-23 10:27 ` Alexandre Belloni
3 siblings, 0 replies; 18+ messages in thread
From: Alexandre Belloni @ 2015-12-23 10:27 UTC (permalink / raw)
To: Ludovic Desroches
Cc: jic23, nicolas.ferre, devicetree, linux-kernel, linux-iio,
plagnioj, linux-arm-kernel
On 21/12/2015 at 10:24:08 +0100, Ludovic Desroches wrote :
> +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> @@ -0,0 +1,27 @@
> +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> +
> +Required properties:
> + - compatible: Should be "atmel,sama5d2-adc".
> + - reg: Should contain ADC registers location and length.
> + - interrupts: Should contain the IRQ line for the ADC.
> + - clocks: phandles to clocks.
> + - clock-names: tuple listing clock names.
> + Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> + clock, "adc_clk" is the sampling clock.
I think we should not have adc_op_clk but rather have a property like
vf610 has fsl,adck-max-frequency.
Even better, would be the ability to change it with
/sys/bus/iio/devices/iio:deviceX/sampling_frequency
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 18+ messages in thread