Re: [PATCH v2 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver

[Ludovic Desroches]

Hi Jonathan,

On Sun, Jan 10, 2016 at 11:28:00AM +0000, Jonathan Cameron wrote:
> On 06/01/16 16:12, 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@xxxxxxxxx>
> Hi Ludovic,
> 
> A couple of bits and bobs inline + I'd ideally like a device tree ack
> (or as conventions says I'll have to wait a while before ignoring them ;)
> 
> The new sampling_frequency attr should be done through info_mask_shared_by_all.
> 
> Also, you are effectively claiming to have handled a set of interrupts that
> you weren't expecting.  Convention would be to say 'not me!' if that happens
> rather than IRQ handled.  Obviously they won't currently happen unless
> something odd is going on, but best to get it 'obviously' right!
> 

You are right, I'll fix that.

> On the whether to read in or our of the interrupt handler, I'm happy with it
> being where it is for the reasons you stated in the previous thread.
> I'd gotten myself confused on this so lets pretend I never mentioned it.
> Just possible we'll later want to move to a threaded interrupt as you add
> more functionality to the handler (thresholds etc) but that can be revisited
> when it matters.
> 
> Looking good in general though.  There are some features in this part
> that I hope you get time to look at adding in future. I'm just
> noting them down here for my own records as I've been reading the datasheet.
> 
> Easy stuff:
> * differential inputs.
> * gain and offset corrections
> * sequencer (leading to wanting to use the buffered IIO side of things)
>   The interaction of available channel count vs sequence length is unusual
>   but won't effect standard sample everything once usage..
> 
> More 'interesting'
> * in and out of window events.
> * lots of triggers including different triggering for the last channel
>  (interesting)
> * DMA. Looks like a sensible setup so will be interesting to see how
>   this fits in with the new DMA buffer support in IIO (at first glance
>   I think it will drop straight in but I could be wrong).
> 
> Stuff I don't care about ;) (never have screens on the boards I use)
> * touchscreen - one day we'll figure out if the way these are done
> are generic enough to share infrastructure across different parts...
> 
> Very sensible to do a basic driver first though and build from there.
> 

For sure, it is probably more work but it is better for our customer to
have somethingthan nothing. Next step will be be signed and differential
channels. 

> Thanks,
> 
> Jonathan
> 
> > ---
> >  .../devicetree/bindings/iio/adc/at91_adc8xx.txt    |  28 ++
> >  drivers/iio/adc/Kconfig                            |  11 +
> >  drivers/iio/adc/Makefile                           |   1 +
> >  drivers/iio/adc/at91_adc8xx.c                      | 461 +++++++++++++++++++++
> >  4 files changed, 501 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..14fe441
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > @@ -0,0 +1,28 @@
> > +* 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: phandle to device clock.
> > +  - clock-names: Must be "adc_clk".
> > +  - vref-supply: Supply used as reference for conversions.
> > +  - vddana-supply: Supply for the adc device.
> > +  - atmel,min-sample-rate: Minimum sampling rate, it depends on SoC.
> > +  - atmel,max-sample-rate: Maximum sampling rate, it depends on SoC.
> Hmm. Just a thought - should we have units for the sample-rate ones as
> well?  It's kind of obvious they are in Hz but maybe we want it anyway
> for consistency...
> 

I was also wondering if I have to add the unit. As you said, I'll add it
for consistency.

> Otherwise the bindings look fine to me.  I always appreciate
> a quick look from a device tree maintainer though as I've been
> wrong many times before!
> 
> > +  - atmel,startup-time-ms: Startup time expressed in ms, it depends on SoC.
> > +
> > +Example:
> > +
> > +adc: adc@fc030000 {
> > +	compatible = "atmel,sama5d2-adc";
> > +	reg = <0xfc030000 0x100>;
> > +	interrupts = <40 IRQ_TYPE_LEVEL_HIGH 7>;
> > +	clocks = <&adc_clk>;
> > +	clock-names = "adc_clk";
> > +	atmel,min-sample-rate = <200000>;
> > +	atmel,max-sample-rate = <20000000>;
> > +	atmel,startup-time-ms = <4>;
> > +	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 605ff42..ee45468 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 6435780..c0d5d02 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
> Hohum.  There's a fairly strong hint here that we might be using names
> that are too generic for atmel parts ;)
> 
> Strange question, where does the 8xx naming come from?
> I wonder if we are better off taking the convention we tend
> to use for discrete parts and naming it after the first one
> supported.  So this would be
> at91_sama5d2_adc then listing parts we know are supported in the
> Kconfig help.
> 

I would say we have big naming issues! For sure the name of several
driver is too generic... 8xx is the version of the ADC.

I am aware of this convention and use it for the compatible string. I
didn't use it for the driver name because sama5d2 comes after sama5d3 or
sama5d4 so there is no 'chronological' logic with the naming of our
SoCs. For that reason, I thought using the version of the device should
be better.

> >  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..bed9574
> > --- /dev/null
> > +++ b/drivers/iio/adc/at91_adc8xx.c
> > @@ -0,0 +1,461 @@
> > +/*
> > + * Atmel ADC driver for SAMA5D2 devices and later.
> You are an optimist if you think it won't change in the future.
> Lets be cynical and say 'and compatible'. 
> > + *
> > + * Copyright (C) 2015 Atmel,
> > + *               2015 Ludovic Desroches <ludovic.desroches@xxxxxxxxx>
> > + *
> > + * 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/iio/sysfs.h>
> > +#include <linux/regulator/consumer.h>
> > +
> Rather than being tight on space for the coments, my personal preference
> is
> /* comment */
> #define FOO
> 
> That way there is lots of room to add as much documentation as makes
> sense rather than thinking - wow this line is too long, better cut some
> of it out!
> 

Okay, I'll change it.

> > +#define AT91_ADC8XX_CR		0x00			/* Control Register */
> > +#define		AT91_ADC8XX_CR_SWRST		BIT(0)		/* Software Reset */
> > +#define		AT91_ADC8XX_CR_START		BIT(1)		/* Start Conversion */
> > +#define		AT91_ADC8XX_CR_TSCALIB		BIT(2)		/* Touchscreen Calibration */
> > +#define		AT91_ADC8XX_CR_CMPRST		BIT(4)		/* Comparison Restart */
> > +#define AT91_ADC8XX_MR		0x04			/* Mode Register */
> > +#define		AT91_ADC8XX_MR_TRGSEL(v)	((v) << 1)	/* Trigger Selection */
> > +#define			AT91_ADC8XX_MR_TRGSEL_TRIG0	0		/* ADTRG */
> > +#define			AT91_ADC8XX_MR_TRGSEL_TRIG1	1		/* TIOA0 */
> > +#define			AT91_ADC8XX_MR_TRGSEL_TRIG2	2		/* TIOA1 */
> > +#define			AT91_ADC8XX_MR_TRGSEL_TRIG3	3		/* TIOA2 */
> > +#define			AT91_ADC8XX_MR_TRGSEL_TRIG4	4		/* PWM event line 0 */
> > +#define			AT91_ADC8XX_MR_TRGSEL_TRIG5	5		/* PWM event line 1 */
> > +#define			AT91_ADC8XX_MR_TRGSEL_TRIG6	6		/* TIOA3 */
> > +#define			AT91_ADC8XX_MR_TRGSEL_TRIG7	7		/* RTCOUT0 */
> > +#define		AT91_ADC8XX_MR_SLEEP		BIT(5)		/* Sleep Mode */
> > +#define		AT91_ADC8XX_MR_FWUP		BIT(6)		/* Fast Wake Up */
> > +#define		AT91_ADC8XX_MR_PRESCAL(v)	((v) << AT91_ADC8XX_MR_PRESCAL_OFFSET)	/* Prescaler Rate Selection */
> > +#define			AT91_ADC8XX_MR_PRESCAL_OFFSET	8
> > +#define			AT91_ADC8XX_MR_PRESCAL_MAX	0xff
> > +#define		AT91_ADC8XX_MR_STARTUP(v)	((v) << 16)	/* Startup Time */
> > +#define		AT91_ADC8XX_MR_ANACH		BIT(23)		/* Analog Change */
> > +#define		AT91_ADC8XX_MR_TRACKTIM(v)	((v) << 24)	/* Tracking Time */
> > +#define			AT91_ADC8XX_MR_TRACKTIM_MAX	0xff
> > +#define		AT91_ADC8XX_MR_TRANSFER(v)	((v) << 28)	/* Transfer Time */
> > +#define			AT91_ADC8XX_MR_TRANSFER_MAX	0x3
> > +#define		AT91_ADC8XX_MR_USEQ		BIT(31)		/* Use Sequence Enable */
> > +#define AT91_ADC8XX_SEQR1	0x08			/* Channel Sequence Register 1 */
> > +#define AT91_ADC8XX_SEQR2	0x0c			/* Channel Sequence Register 2 */
> > +#define AT91_ADC8XX_CHER	0x10			/* Channel Enable Register */
> > +#define AT91_ADC8XX_CHDR	0x14			/* Channel Disable Register */
> > +#define AT91_ADC8XX_CHSR	0x18			/* Channel Status Register */
> > +#define AT91_ADC8XX_LCDR	0x20			/* Last Converted Data Register */
> > +#define AT91_ADC8XX_IER		0x24			/* Interrupt Enable Register */
> > +#define AT91_ADC8XX_IDR		0x28			/* Interrupt Disable Register */
> > +#define AT91_ADC8XX_IMR		0x2c			/* Interrupt Mask Register */
> > +#define AT91_ADC8XX_ISR		0x30			/* Interrupt Status Register */
> > +#define AT91_ADC8XX_LCTMR	0x34			/* Last Channel Trigger Mode Register */
> > +#define AT91_ADC8XX_LCCWR	0x38			/* Last Channel Compare Window Register */
> > +#define AT91_ADC8XX_OVER	0x3c			/* Overrun Status Register */
> > +#define AT91_ADC8XX_EMR		0x40			/* Extended Mode Register */
> > +#define AT91_ADC8XX_CWR		0x44			/* Compare Window Register */
> > +#define AT91_ADC8XX_CGR		0x48			/* Channel Gain Register */
> > +#define AT91_ADC8XX_COR		0x4c			/* Channel Offset Register */
> > +#define AT91_ADC8XX_CDR0	0x50			/* Channel Data Register 0 */
> > +#define AT91_ADC8XX_ACR		0x94			/* Analog Control Register */
> > +#define AT91_ADC8XX_TSMR	0xb0			/* Touchscreen Mode Register */
> > +#define AT91_ADC8XX_XPOSR	0xb4			/* Touchscreen X Position Register */
> > +#define AT91_ADC8XX_YPOSR	0xb8			/* Touchscreen Y Position Register */
> > +#define AT91_ADC8XX_PRESSR	0xbc			/* Touchscreen Pressure Register */
> > +#define AT91_ADC8XX_TRGR	0xc0			/* Trigger Register */
> > +#define AT91_ADC8XX_COSR	0xd0			/* Correction Select Register */
> > +#define AT91_ADC8XX_CVR		0xd4			/* Correction Value Register */
> > +#define AT91_ADC8XX_CECR	0xd8			/* Channel Error Correction Register */
> > +#define AT91_ADC8XX_WPMR	0xe4			/* Write Protection Mode Register */
> > +#define AT91_ADC8XX_WPSR	0xe8			/* Write Protection Status Register */
> > +#define AT91_ADC8XX_VERSION	0xfc			/* Version Register */
> > +
> > +#define AT91_AT91_ADC8XX_CHAN(num, addr)				\
> > +	{								\
> > +		.type = IIO_VOLTAGE,					\
> > +		.channel = num,						\
> > +		.address = addr,					\
> > +		.scan_type = {						\
> > +			.sign = 'u',					\
> > +			.realbits = 12,					\
> > +		},							\
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> as noted below you also want
> .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> though under the ABI that could also be shared by type if you prefer.
> 

ok.

> > +		.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_sample_rate;
> > +	unsigned			max_sample_rate;
> > +};
> > +
> > +struct at91_adc_state {
> > +	void __iomem			*base;
> > +	int				irq;
> > +	struct clk			*per_clk;
> > +	struct regulator		*reg;
> > +	struct regulator		*vref;
> > +	u32				vref_uv;
> > +	const struct iio_chan_spec	*chan;
> > +	bool				conversion_done;
> > +	u32				conversion_value;
> > +	struct at91_adc_soc_info	soc_info;
> > +	wait_queue_head_t		wq_data_available;
> > +	struct mutex			lock;
> > +};
> > +
> > +static const struct iio_chan_spec at91_adc_channels[] = {
> > +	AT91_AT91_ADC8XX_CHAN(0, 0x50),
> > +	AT91_AT91_ADC8XX_CHAN(1, 0x54),
> > +	AT91_AT91_ADC8XX_CHAN(2, 0x58),
> > +	AT91_AT91_ADC8XX_CHAN(3, 0x5c),
> > +	AT91_AT91_ADC8XX_CHAN(4, 0x60),
> > +	AT91_AT91_ADC8XX_CHAN(5, 0x64),
> > +	AT91_AT91_ADC8XX_CHAN(6, 0x68),
> > +	AT91_AT91_ADC8XX_CHAN(7, 0x6c),
> > +	AT91_AT91_ADC8XX_CHAN(8, 0x70),
> > +	AT91_AT91_ADC8XX_CHAN(9, 0x74),
> > +	AT91_AT91_ADC8XX_CHAN(10, 0x78),
> > +	AT91_AT91_ADC8XX_CHAN(11, 0x7c),
> > +};
> > +
> > +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 void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
> > +{
> > +	struct iio_dev *indio_dev = iio_priv_to_dev(st);
> > +	unsigned f_per, prescal, startup;
> > +
> > +	f_per = clk_get_rate(st->per_clk);
> > +	prescal = (f_per / (2 * freq)) - 1;
> > +
> > +	startup = at91_adc_startup_time(st->soc_info.startup_time,
> > +					freq / 1000);
> > +
> > +	at91_adc_writel(st, AT91_ADC8XX_MR,
> > +			AT91_ADC8XX_MR_TRANSFER(2)
> > +			| AT91_ADC8XX_MR_STARTUP(startup)
> > +			| AT91_ADC8XX_MR_PRESCAL(prescal));
> > +
> > +	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
> > +		freq, startup, prescal);
> > +}
> > +
> > +static ssize_t at91_adc_show_samp_freq(struct device *dev,
> > +				       struct device_attribute *attr,
> > +				       char *buf)
> > +{
> > +	struct at91_adc_state *st = iio_priv(dev_to_iio_dev(dev));
> > +	unsigned f_adc, f_per = clk_get_rate(st->per_clk);
> > +	unsigned mr, prescal;
> > +
> > +	mr = at91_adc_readl(st, AT91_ADC8XX_MR);
> > +	prescal = (mr >> AT91_ADC8XX_MR_PRESCAL_OFFSET)
> > +		  & AT91_ADC8XX_MR_PRESCAL_MAX;
> > +	f_adc = f_per / (2 * (prescal + 1));
> > +
> > +	return sprintf(buf, "%d\n", f_adc);
> > +}
> > +
> > +static ssize_t at91_adc_store_samp_freq(struct device *dev,
> > +					struct device_attribute *attr,
> > +					const char *buf, size_t len)
> > +{
> > +	struct at91_adc_state *st = iio_priv(dev_to_iio_dev(dev));
> > +	unsigned val;
> > +	int ret;
> > +
> > +	ret = kstrtouint(buf, 10, &val);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	if (val < st->soc_info.min_sample_rate ||
> > +	    val > st->soc_info.max_sample_rate)
> > +		return -EINVAL;
> > +
> > +	at91_adc_setup_samp_freq(st, val);
> > +
> > +	return len;
> > +}
> > +
> > +static IIO_DEV_ATTR_SAMP_FREQ(S_IRUGO | S_IWUSR,
> > +			      at91_adc_show_samp_freq,
> > +			      at91_adc_store_samp_freq);
> > +
> > +static struct attribute *at91_adc_event_attributes[] = {
> > +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> Use the info_mask_shared_by_all bitmap to specify this and
> read it through read_raw.  That makes the funcationality available
> to in kernel client drivers as well as userspace.
> 

ok

> > +	NULL,
> > +};
> > +
> > +static struct attribute_group at91_adc_event_attribute_group = {
> > +	.attrs = at91_adc_event_attributes,
> > +};
> > +
> > +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, AT91_ADC8XX_ISR);
> > +
> > +	status &= at91_adc_readl(st, AT91_ADC8XX_IMR);
> 
> This is somewhat of an oddity.  If the interrupt is not enabled and we
> get it then we have a nasty problem and so shouldn't claim to have
> handled the interrupt (let the interrupt storm prevention stuff kill the
> interrupt off this happens).
> 
> You should only be checking for those interrupts that this driver might
> have caused - handling those and returning IRQ_NONE if you haven't caused
> them to occur.
> 
> In this case you are enabling one of bits 0-11 that I can see so should
> only be handling those.
> 

Thinking about next features of this driver, I have extra or inacurrate stuff.
I'll clean this part.

> > +	if (status & 0xffff) {
> > +		st->conversion_value = at91_adc_readl(st, st->chan->address);
> I'm happy enough with your logic here on reading this in the interrupt routine.
> > +		st->conversion_done = true;
> > +		wake_up_interruptible(&st->wq_data_available);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +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;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		mutex_lock(&st->lock);
> > +
> > +		st->chan = chan;
> > +
> > +		at91_adc_writel(st, AT91_ADC8XX_CHER, BIT(chan->channel));
> > +		at91_adc_writel(st, AT91_ADC8XX_IER, BIT(chan->channel));
> > +		at91_adc_writel(st, AT91_ADC8XX_CR, AT91_ADC8XX_CR_START);
> > +
> > +		ret = wait_event_interruptible_timeout(st->wq_data_available,
> > +						       st->conversion_done,
> > +						       msecs_to_jiffies(1000));
> > +		if (ret == 0)
> > +			ret = -ETIMEDOUT;
> > +
> > +		if (ret > 0) {
> > +			*val = st->conversion_value;
> > +			ret = IIO_VAL_INT;
> > +			st->conversion_done = false;
> > +		}
> > +
> > +		at91_adc_writel(st, AT91_ADC8XX_IDR, BIT(chan->channel));
> > +		at91_adc_writel(st, AT91_ADC8XX_CHDR, BIT(chan->channel));
> > +
> > +		mutex_unlock(&st->lock);
> > +		return ret;
> > +
> > +	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,
> > +	.event_attrs = &at91_adc_event_attribute_group,
> > +};
> > +
> > +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);
> > +
> > +	ret = of_property_read_u32(pdev->dev.of_node, "atmel,min-sample-rate",
> > +				   &st->soc_info.min_sample_rate);
> > +	if (ret) {
> > +		dev_err(&pdev->dev,
> > +			"invalid or missing value for atmel,min-sample-rate\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = of_property_read_u32(pdev->dev.of_node, "atmel,max-sample-rate",
> > +				   &st->soc_info.max_sample_rate);
> > +	if (ret) {
> > +		dev_err(&pdev->dev,
> > +			"invalid or missing value for atmel,max-sample-rate\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = of_property_read_u32(pdev->dev.of_node, "atmel,startup-time-ms",
> > +				   &st->soc_info.startup_time);
> > +	if (ret) {
> > +		dev_err(&pdev->dev,
> > +			"invalid or missing value for atmel,startup-time-ms\n");
> > +		return ret;
> > +	}
> > +
> > +	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) {
> > +		if (!st->irq)
> > +			st->irq = -ENXIO;
> > +
> > +		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->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;
> > +
> > +	ret = regulator_enable(st->reg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regulator_enable(st->vref);
> > +	if (ret)
> > +		goto reg_disable;
> > +
> > +	st->vref_uv = regulator_get_voltage(st->vref);
> > +	if (st->vref_uv <= 0) {
> > +		ret = -EINVAL;
> > +		goto vref_disable;
> > +	}
> > +
> > +	at91_adc_writel(st, AT91_ADC8XX_CR, AT91_ADC8XX_CR_SWRST);
> > +	at91_adc_writel(st, AT91_ADC8XX_IDR, 0xffffffff);
> > +
> > +	at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
> > +
> > +	ret = clk_prepare_enable(st->per_clk);
> > +	if (ret)
> > +		goto vref_disable;
> > +
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret < 0)
> > +		goto per_clk_disable_unprepare;
> > +
> > +	dev_info(&pdev->dev, "version: %x\n",
> > +		 readl_relaxed(st->base + AT91_ADC8XX_VERSION));
> > +
> > +	return 0;
> > +
> > +per_clk_disable_unprepare:
> > +	clk_disable_unprepare(st->per_clk);
> > +vref_disable:
> > +	regulator_disable(st->vref);
> > +reg_disable:
> > +	regulator_disable(st->reg);
> > +	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);
> > +
> > +	regulator_disable(st->vref);
> > +	regulator_disable(st->reg);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id at91_adc_dt_match[] = {
> > +	{
> > +		.compatible = "atmel,sama5d2-adc",
> > +	}, {
> > +		/* 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@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("Atmel AT91 ADC 8xx");
> > +MODULE_LICENSE("GPL v2");
> > 

Regards

Ludovic