Skip to contentSkip to conversation

iio: Minor cleanups to spear ADC driver followed by staging graduation.

PATCH Superseded Jonathan Cameron 3 participants 16 messages Mar 15, 2014 – Feb 5, 2017

Subsystems (3)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jonathan Cameron

author maintainer
#0 lore raw

v2 cover iio: Minor cleanups to spear ADC driver followed by staging graduation.

Changes since V1. Fixed the issues Harmuk Knaack pointed out in patch 4 by dropping the unused variables and ensure mask was as it should be.

Previous description:
This driver has been reasonably clean since it was first merged within the staging tree. If it had been submitted after the IIO core was outside of staging we might have taken it as is. I did a final review and the first 4 patches of this series rework various minor aspects of the driver.

Whilst it is a slow driver, the device tree matches at the lowest level seemed a little unwise so the largest rework is concerned with centralising that logic using a classic ops structure of function pointers. The other large change was to move to using the info_mask_shared_by_all approach to handle sampling frequency. This is something I am slowly propogating across the whole tree, but might as well be done whilst we are looking at this driver.

I don't have hardware and whilst I think these are all changes that are possible to easily verify by eye, a tested by would be most welcome!

Note this is the first driver I'm suggesting moving out of staging following Greg's entirely reasonable question of 'What's wrong with the stuff that is left?' Any help with this sort of final cleanup and move would be most welcome.

Jonathan

Jonathan Cameron (4):
staging:iio:adc:spear adc - prefix defines to avoid namespace clashes. staging:iio:adc:spear_adc drop initialization of unused scan_type staging:iio:adc:spear_adc use info_mask_shared_by_all for samp freq iio:adc:spear_adc move out of staging

 drivers/iio/adc/Kconfig             |   8 +
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/spear_adc.c         | 405 +++++++++++++++++++++++++++++++++
 drivers/staging/iio/adc/Kconfig     |   8 -
 drivers/staging/iio/adc/Makefile    |   1 -
 drivers/staging/iio/adc/spear_adc.c | 432 ------------------------------------
 6 files changed, 414 insertions(+), 441 deletions(-)
 create mode 100644 drivers/iio/adc/spear_adc.c
 delete mode 100644 drivers/staging/iio/adc/spear_adc.c
Signature · 1 line
1.9.0
author maintainer
#1 lore raw

PATCH v2 1/4 staging:iio:adc:spear adc - prefix defines to avoid namespace clashes.

---
 drivers/staging/iio/adc/spear_adc.c | 73 ++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 37 deletions(-)
drivers/staging/iio/adc/spear_adc.c +36 -37
index 970d9ed..9234e05 100644
@@ -22,39 +22,36 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
-/*
- * SPEAR registers definitions
- */
-
-#define SCAN_RATE_LO(x)		((x) & 0xFFFF)
-#define SCAN_RATE_HI(x)		(((x) >> 0x10) & 0xFFFF)
-#define CLK_LOW(x)		(((x) & 0xf) << 0)
-#define CLK_HIGH(x)		(((x) & 0xf) << 4)
+/* SPEAR registers definitions */
+#define SPEAR600_ADC_SCAN_RATE_LO(x)	((x) & 0xFFFF)
+#define SPEAR600_ADC_SCAN_RATE_HI(x)	(((x) >> 0x10) & 0xFFFF)
+#define SPEAR_ADC_CLK_LOW(x)		(((x) & 0xf) << 0)
+#define SPEAR_ADC_CLK_HIGH(x)		(((x) & 0xf) << 4)
 
 /* Bit definitions for SPEAR_ADC_STATUS */
-#define START_CONVERSION	(1 << 0)
-#define CHANNEL_NUM(x)		((x) << 1)
-#define ADC_ENABLE		(1 << 4)
-#define AVG_SAMPLE(x)		((x) << 5)
-#define VREF_INTERNAL		(1 << 9)
+#define SPEAR_ADC_STATUS_START_CONVERSION	(1 << 0)
+#define SPEAR_ADC_STATUS_CHANNEL_NUM(x)		((x) << 1)
+#define SPEAR_ADC_STATUS_ADC_ENABLE		(1 << 4)
+#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)		((x) << 5)
+#define SPEAR_ADC_STATUS_VREF_INTERNAL		(1 << 9)
 
-#define DATA_MASK		0x03ff
-#define DATA_BITS		10
+#define SPEAR_ADC_DATA_MASK		0x03ff
+#define SPEAR_ADC_DATA_BITS		10
 
-#define MOD_NAME "spear-adc"
+#define SPEAR_ADC_MOD_NAME "spear-adc"
 
-#define ADC_CHANNEL_NUM		8
+#define SPEAR_ADC_CHANNEL_NUM		8
 
-#define CLK_MIN			2500000
-#define CLK_MAX			20000000
+#define SPEAR_ADC_CLK_MIN			2500000
+#define SPEAR_ADC_CLK_MAX			20000000
 
 struct adc_regs_spear3xx {
 	u32 status;
 	u32 average;
 	u32 scan_rate;
 	u32 clk;	/* Not avail for 1340 & 1310 */
-	u32 ch_ctrl[ADC_CHANNEL_NUM];
-	u32 ch_data[ADC_CHANNEL_NUM];
+	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+	u32 ch_data[SPEAR_ADC_CHANNEL_NUM];
 };
 
 struct chan_data {
@@ -66,8 +63,8 @@ struct adc_regs_spear6xx {
 	u32 status;
 	u32 pad[2];
 	u32 clk;
-	u32 ch_ctrl[ADC_CHANNEL_NUM];
-	struct chan_data ch_data[ADC_CHANNEL_NUM];
+	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+	struct chan_data ch_data[SPEAR_ADC_CHANNEL_NUM];
 	u32 scan_rate_lo;
 	u32 scan_rate_hi;
 	struct chan_data average;
@@ -106,7 +103,7 @@ static void spear_adc_set_clk(struct spear_adc_info *info, u32 val)
 	clk_high = count - clk_low;
 	info->current_clk = apb_clk / count;
 
-	__raw_writel(CLK_LOW(clk_low) | CLK_HIGH(clk_high),
+	__raw_writel(SPEAR_ADC_CLK_LOW(clk_low) | SPEAR_ADC_CLK_HIGH(clk_high),
 		     &info->adc_base_spear6xx->clk);
 }
 
@@ -120,19 +117,19 @@ static u32 spear_adc_get_average(struct spear_adc_info *info)
 {
 	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
 		return __raw_readl(&info->adc_base_spear6xx->average.msb) &
-			DATA_MASK;
+			SPEAR_ADC_DATA_MASK;
 	} else {
 		return __raw_readl(&info->adc_base_spear3xx->average) &
-			DATA_MASK;
+			SPEAR_ADC_DATA_MASK;
 	}
 }
 
 static void spear_adc_set_scanrate(struct spear_adc_info *info, u32 rate)
 {
 	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
-		__raw_writel(SCAN_RATE_LO(rate),
+		__raw_writel(SPEAR600_ADC_SCAN_RATE_LO(rate),
 			     &info->adc_base_spear6xx->scan_rate_lo);
-		__raw_writel(SCAN_RATE_HI(rate),
+		__raw_writel(SPEAR600_ADC_SCAN_RATE_HI(rate),
 			     &info->adc_base_spear6xx->scan_rate_hi);
 	} else {
 		__raw_writel(rate, &info->adc_base_spear3xx->scan_rate);
@@ -152,11 +149,12 @@ static int spear_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_RAW:
 		mutex_lock(&indio_dev->mlock);
 
-		status = CHANNEL_NUM(chan->channel) |
-			AVG_SAMPLE(info->avg_samples) |
-			START_CONVERSION | ADC_ENABLE;
+		status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
+			SPEAR_ADC_STATUS_AVG_SAMPLE(info->avg_samples) |
+			SPEAR_ADC_STATUS_START_CONVERSION |
+			SPEAR_ADC_STATUS_ADC_ENABLE;
 		if (info->vref_external == 0)
-			status |= VREF_INTERNAL;
+			status |= SPEAR_ADC_STATUS_VREF_INTERNAL;
 
 		spear_adc_set_status(info, status);
 		wait_for_completion(&info->completion); /* set by ISR */
@@ -168,7 +166,7 @@ static int spear_read_raw(struct iio_dev *indio_dev,
 
 	case IIO_CHAN_INFO_SCALE:
 		*val = info->vref_external;
-		*val2 = DATA_BITS;
+		*val2 = SPEAR_ADC_DATA_BITS;
 		return IIO_VAL_FRACTIONAL_LOG2;
 	}
 
@@ -253,7 +251,7 @@ static ssize_t spear_adc_write_frequency(struct device *dev,
 
 	mutex_lock(&indio_dev->mlock);
 
-	if ((lval < CLK_MIN) || (lval > CLK_MAX)) {
+	if ((lval < SPEAR_ADC_CLK_MIN) || (lval > SPEAR_ADC_CLK_MAX)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -339,7 +337,8 @@ static int spear_adc_probe(struct platform_device *pdev)
 		goto errout3;
 	}
 
-	ret = devm_request_irq(dev, irq, spear_adc_isr, 0, MOD_NAME, info);
+	ret = devm_request_irq(dev, irq, spear_adc_isr, 0, SPEAR_ADC_MOD_NAME,
+			info);
 	if (ret < 0) {
 		dev_err(dev, "failed requesting interrupt\n");
 		goto errout3;
@@ -370,7 +369,7 @@ static int spear_adc_probe(struct platform_device *pdev)
 
 	init_completion(&info->completion);
 
-	iodev->name = MOD_NAME;
+	iodev->name = SPEAR_ADC_MOD_NAME;
 	iodev->dev.parent = dev;
 	iodev->info = &spear_adc_iio_info;
 	iodev->modes = INDIO_DIRECT_MODE;
@@ -419,7 +418,7 @@ static struct platform_driver spear_adc_driver = {
 	.probe		= spear_adc_probe,
 	.remove		= spear_adc_remove,
 	.driver		= {
-		.name	= MOD_NAME,
+		.name	= SPEAR_ADC_MOD_NAME,
 		.owner	= THIS_MODULE,
 		.of_match_table = of_match_ptr(spear_adc_dt_ids),
 	},
Signature · 1 line
1.9.0
#2 lore raw

Re: [PATCH 1/4] staging:iio:adc:spear adc - prefix defines to avoid namespace clashes.

Quoted 148 lines · Jonathan Cameron
---
 drivers/staging/iio/adc/spear_adc.c | 73 ++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 37 deletions(-)
drivers/staging/iio/adc/spear_adc.c +32 -34
index 970d9ed..9234e05 100644
@@ -22,39 +22,36 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
-/*
- * SPEAR registers definitions
- */
-
-#define SCAN_RATE_LO(x)		((x) & 0xFFFF)
-#define SCAN_RATE_HI(x)		(((x) >> 0x10) & 0xFFFF)
-#define CLK_LOW(x)		(((x) & 0xf) << 0)
-#define CLK_HIGH(x)		(((x) & 0xf) << 4)
+/* SPEAR registers definitions */
+#define SPEAR600_ADC_SCAN_RATE_LO(x)	((x) & 0xFFFF)
+#define SPEAR600_ADC_SCAN_RATE_HI(x)	(((x) >> 0x10) & 0xFFFF)
+#define SPEAR_ADC_CLK_LOW(x)		(((x) & 0xf) << 0)
+#define SPEAR_ADC_CLK_HIGH(x)		(((x) & 0xf) << 4)
 
 /* Bit definitions for SPEAR_ADC_STATUS */
-#define START_CONVERSION	(1 << 0)
-#define CHANNEL_NUM(x)		((x) << 1)
-#define ADC_ENABLE		(1 << 4)
-#define AVG_SAMPLE(x)		((x) << 5)
-#define VREF_INTERNAL		(1 << 9)
+#define SPEAR_ADC_STATUS_START_CONVERSION	(1 << 0)
+#define SPEAR_ADC_STATUS_CHANNEL_NUM(x)		((x) << 1)
+#define SPEAR_ADC_STATUS_ADC_ENABLE		(1 << 4)
+#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)		((x) << 5)
+#define SPEAR_ADC_STATUS_VREF_INTERNAL		(1 << 9)
 
-#define DATA_MASK		0x03ff
-#define DATA_BITS		10
+#define SPEAR_ADC_DATA_MASK		0x03ff
+#define SPEAR_ADC_DATA_BITS		10
 
-#define MOD_NAME "spear-adc"
+#define SPEAR_ADC_MOD_NAME "spear-adc"
 
-#define ADC_CHANNEL_NUM		8
+#define SPEAR_ADC_CHANNEL_NUM		8
 
-#define CLK_MIN			2500000
-#define CLK_MAX			20000000
+#define SPEAR_ADC_CLK_MIN			2500000
+#define SPEAR_ADC_CLK_MAX			20000000
 
 struct adc_regs_spear3xx {
 	u32 status;
 	u32 average;
 	u32 scan_rate;
 	u32 clk;	/* Not avail for 1340 & 1310 */
-	u32 ch_ctrl[ADC_CHANNEL_NUM];
-	u32 ch_data[ADC_CHANNEL_NUM];
+	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+	u32 ch_data[SPEAR_ADC_CHANNEL_NUM];
 };
 
 struct chan_data {
@@ -66,8 +63,8 @@ struct adc_regs_spear6xx {
 	u32 status;
 	u32 pad[2];
 	u32 clk;
-	u32 ch_ctrl[ADC_CHANNEL_NUM];
-	struct chan_data ch_data[ADC_CHANNEL_NUM];
+	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+	struct chan_data ch_data[SPEAR_ADC_CHANNEL_NUM];
 	u32 scan_rate_lo;
 	u32 scan_rate_hi;
 	struct chan_data average;
@@ -106,7 +103,7 @@ static void spear_adc_set_clk(struct spear_adc_info *info, u32 val)
 	clk_high = count - clk_low;
 	info->current_clk = apb_clk / count;
 
-	__raw_writel(CLK_LOW(clk_low) | CLK_HIGH(clk_high),
+	__raw_writel(SPEAR_ADC_CLK_LOW(clk_low) | SPEAR_ADC_CLK_HIGH(clk_high),
 		     &info->adc_base_spear6xx->clk);
 }
 
@@ -120,19 +117,19 @@ static u32 spear_adc_get_average(struct spear_adc_info *info)
 {
 	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
 		return __raw_readl(&info->adc_base_spear6xx->average.msb) &
-			DATA_MASK;
+			SPEAR_ADC_DATA_MASK;
 	} else {
 		return __raw_readl(&info->adc_base_spear3xx->average) &
-			DATA_MASK;
+			SPEAR_ADC_DATA_MASK;
 	}
 }
 
 static void spear_adc_set_scanrate(struct spear_adc_info *info, u32 rate)
 {
 	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
-		__raw_writel(SCAN_RATE_LO(rate),
+		__raw_writel(SPEAR600_ADC_SCAN_RATE_LO(rate),
 			     &info->adc_base_spear6xx->scan_rate_lo);
-		__raw_writel(SCAN_RATE_HI(rate),
+		__raw_writel(SPEAR600_ADC_SCAN_RATE_HI(rate),
 			     &info->adc_base_spear6xx->scan_rate_hi);
 	} else {
 		__raw_writel(rate, &info->adc_base_spear3xx->scan_rate);
@@ -152,11 +149,12 @@ static int spear_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_RAW:
 		mutex_lock(&indio_dev->mlock);
 
-		status = CHANNEL_NUM(chan->channel) |
-			AVG_SAMPLE(info->avg_samples) |
-			START_CONVERSION | ADC_ENABLE;
+		status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
+			SPEAR_ADC_STATUS_AVG_SAMPLE(info->avg_samples) |
+			SPEAR_ADC_STATUS_START_CONVERSION |
+			SPEAR_ADC_STATUS_ADC_ENABLE;
 		if (info->vref_external == 0)
-			status |= VREF_INTERNAL;
+			status |= SPEAR_ADC_STATUS_VREF_INTERNAL;
 
 		spear_adc_set_status(info, status);
 		wait_for_completion(&info->completion); /* set by ISR */
@@ -168,7 +166,7 @@ static int spear_read_raw(struct iio_dev *indio_dev,
 
 	case IIO_CHAN_INFO_SCALE:
 		*val = info->vref_external;
-		*val2 = DATA_BITS;
+		*val2 = SPEAR_ADC_DATA_BITS;
 		return IIO_VAL_FRACTIONAL_LOG2;
 	}
 
@@ -253,7 +251,7 @@ static ssize_t spear_adc_write_frequency(struct device *dev,
 
 	mutex_lock(&indio_dev->mlock);
 
-	if ((lval < CLK_MIN) || (lval > CLK_MAX)) {
+	if ((lval < SPEAR_ADC_CLK_MIN) || (lval > SPEAR_ADC_CLK_MAX)) {
 		ret = -EINVAL;
 		goto out;
 	}

drivers/staging/iio/adc/spear_adc.c static int spear_adc_probe(struct platform_device *pdev) · line 341

Quoted 6 lines
drivers/staging/iio/adc/spear_adc.c +2 -1
@@ -339,7 +337,8 @@ static int spear_adc_probe(struct platform_device *pdev)
 		goto errout3;
 	}
 
-	ret = devm_request_irq(dev, irq, spear_adc_isr, 0, MOD_NAME, info);
+	ret = devm_request_irq(dev, irq, spear_adc_isr, 0, SPEAR_ADC_MOD_NAME,
+			info);

Better indent with opening parenthesis.

Quoted 21 lines
drivers/staging/iio/adc/spear_adc.c +2 -2
 	if (ret < 0) {
 		dev_err(dev, "failed requesting interrupt\n");
 		goto errout3;
@@ -370,7 +369,7 @@ static int spear_adc_probe(struct platform_device *pdev)
 
 	init_completion(&info->completion);
 
-	iodev->name = MOD_NAME;
+	iodev->name = SPEAR_ADC_MOD_NAME;
 	iodev->dev.parent = dev;
 	iodev->info = &spear_adc_iio_info;
 	iodev->modes = INDIO_DIRECT_MODE;
@@ -419,7 +418,7 @@ static struct platform_driver spear_adc_driver = {
 	.probe		= spear_adc_probe,
 	.remove		= spear_adc_remove,
 	.driver		= {
-		.name	= MOD_NAME,
+		.name	= SPEAR_ADC_MOD_NAME,
 		.owner	= THIS_MODULE,
 		.of_match_table = of_match_ptr(spear_adc_dt_ids),
 	},
author maintainer
#3 lore raw

Re: [PATCH 1/4] staging:iio:adc:spear adc - prefix defines to avoid namespace clashes.

Quoted 152 lines · Hartmut Knaack
Quoted 151 lines · Jonathan Cameron
---
  drivers/staging/iio/adc/spear_adc.c | 73 ++++++++++++++++++-------------------
  1 file changed, 36 insertions(+), 37 deletions(-)
drivers/staging/iio/adc/spear_adc.c +32 -34
index 970d9ed..9234e05 100644
@@ -22,39 +22,36 @@
  #include <linux/iio/iio.h>
  #include <linux/iio/sysfs.h>
 
-/*
- * SPEAR registers definitions
- */
-
-#define SCAN_RATE_LO(x)		((x) & 0xFFFF)
-#define SCAN_RATE_HI(x)		(((x) >> 0x10) & 0xFFFF)
-#define CLK_LOW(x)		(((x) & 0xf) << 0)
-#define CLK_HIGH(x)		(((x) & 0xf) << 4)
+/* SPEAR registers definitions */
+#define SPEAR600_ADC_SCAN_RATE_LO(x)	((x) & 0xFFFF)
+#define SPEAR600_ADC_SCAN_RATE_HI(x)	(((x) >> 0x10) & 0xFFFF)
+#define SPEAR_ADC_CLK_LOW(x)		(((x) & 0xf) << 0)
+#define SPEAR_ADC_CLK_HIGH(x)		(((x) & 0xf) << 4)
 
  /* Bit definitions for SPEAR_ADC_STATUS */
-#define START_CONVERSION	(1 << 0)
-#define CHANNEL_NUM(x)		((x) << 1)
-#define ADC_ENABLE		(1 << 4)
-#define AVG_SAMPLE(x)		((x) << 5)
-#define VREF_INTERNAL		(1 << 9)
+#define SPEAR_ADC_STATUS_START_CONVERSION	(1 << 0)
+#define SPEAR_ADC_STATUS_CHANNEL_NUM(x)		((x) << 1)
+#define SPEAR_ADC_STATUS_ADC_ENABLE		(1 << 4)
+#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)		((x) << 5)
+#define SPEAR_ADC_STATUS_VREF_INTERNAL		(1 << 9)
 
-#define DATA_MASK		0x03ff
-#define DATA_BITS		10
+#define SPEAR_ADC_DATA_MASK		0x03ff
+#define SPEAR_ADC_DATA_BITS		10
 
-#define MOD_NAME "spear-adc"
+#define SPEAR_ADC_MOD_NAME "spear-adc"
 
-#define ADC_CHANNEL_NUM		8
+#define SPEAR_ADC_CHANNEL_NUM		8
 
-#define CLK_MIN			2500000
-#define CLK_MAX			20000000
+#define SPEAR_ADC_CLK_MIN			2500000
+#define SPEAR_ADC_CLK_MAX			20000000
 
  struct adc_regs_spear3xx {
  	u32 status;
  	u32 average;
  	u32 scan_rate;
  	u32 clk;	/* Not avail for 1340 & 1310 */
-	u32 ch_ctrl[ADC_CHANNEL_NUM];
-	u32 ch_data[ADC_CHANNEL_NUM];
+	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+	u32 ch_data[SPEAR_ADC_CHANNEL_NUM];
  };
 
  struct chan_data {
@@ -66,8 +63,8 @@ struct adc_regs_spear6xx {
  	u32 status;
  	u32 pad[2];
  	u32 clk;
-	u32 ch_ctrl[ADC_CHANNEL_NUM];
-	struct chan_data ch_data[ADC_CHANNEL_NUM];
+	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+	struct chan_data ch_data[SPEAR_ADC_CHANNEL_NUM];
  	u32 scan_rate_lo;
  	u32 scan_rate_hi;
  	struct chan_data average;
@@ -106,7 +103,7 @@ static void spear_adc_set_clk(struct spear_adc_info *info, u32 val)
  	clk_high = count - clk_low;
  	info->current_clk = apb_clk / count;
 
-	__raw_writel(CLK_LOW(clk_low) | CLK_HIGH(clk_high),
+	__raw_writel(SPEAR_ADC_CLK_LOW(clk_low) | SPEAR_ADC_CLK_HIGH(clk_high),
  		     &info->adc_base_spear6xx->clk);
  }
 
@@ -120,19 +117,19 @@ static u32 spear_adc_get_average(struct spear_adc_info *info)
  {
  	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
  		return __raw_readl(&info->adc_base_spear6xx->average.msb) &
-			DATA_MASK;
+			SPEAR_ADC_DATA_MASK;
  	} else {
  		return __raw_readl(&info->adc_base_spear3xx->average) &
-			DATA_MASK;
+			SPEAR_ADC_DATA_MASK;
  	}
  }
 
  static void spear_adc_set_scanrate(struct spear_adc_info *info, u32 rate)
  {
  	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
-		__raw_writel(SCAN_RATE_LO(rate),
+		__raw_writel(SPEAR600_ADC_SCAN_RATE_LO(rate),
  			     &info->adc_base_spear6xx->scan_rate_lo);
-		__raw_writel(SCAN_RATE_HI(rate),
+		__raw_writel(SPEAR600_ADC_SCAN_RATE_HI(rate),
  			     &info->adc_base_spear6xx->scan_rate_hi);
  	} else {
  		__raw_writel(rate, &info->adc_base_spear3xx->scan_rate);
@@ -152,11 +149,12 @@ static int spear_read_raw(struct iio_dev *indio_dev,
  	case IIO_CHAN_INFO_RAW:
  		mutex_lock(&indio_dev->mlock);
 
-		status = CHANNEL_NUM(chan->channel) |
-			AVG_SAMPLE(info->avg_samples) |
-			START_CONVERSION | ADC_ENABLE;
+		status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
+			SPEAR_ADC_STATUS_AVG_SAMPLE(info->avg_samples) |
+			SPEAR_ADC_STATUS_START_CONVERSION |
+			SPEAR_ADC_STATUS_ADC_ENABLE;
  		if (info->vref_external == 0)
-			status |= VREF_INTERNAL;
+			status |= SPEAR_ADC_STATUS_VREF_INTERNAL;
 
  		spear_adc_set_status(info, status);
  		wait_for_completion(&info->completion); /* set by ISR */
@@ -168,7 +166,7 @@ static int spear_read_raw(struct iio_dev *indio_dev,
 
  	case IIO_CHAN_INFO_SCALE:
  		*val = info->vref_external;
-		*val2 = DATA_BITS;
+		*val2 = SPEAR_ADC_DATA_BITS;
  		return IIO_VAL_FRACTIONAL_LOG2;
  	}
 
@@ -253,7 +251,7 @@ static ssize_t spear_adc_write_frequency(struct device *dev,
 
  	mutex_lock(&indio_dev->mlock);
 
-	if ((lval < CLK_MIN) || (lval > CLK_MAX)) {
+	if ((lval < SPEAR_ADC_CLK_MIN) || (lval > SPEAR_ADC_CLK_MAX)) {
  		ret = -EINVAL;
  		goto out;
  	}
@@ -339,7 +337,8 @@ static int spear_adc_probe(struct platform_device *pdev)
  		goto errout3;
  	}
 
Quoted 4 lines

drivers/staging/iio/adc/spear_adc.c static int spear_adc_probe(struct platform_device *pdev) · line 341

Quoted 3 lines
drivers/staging/iio/adc/spear_adc.c +2 -1
-	ret = devm_request_irq(dev, irq, spear_adc_isr, 0, MOD_NAME, info);
+	ret = devm_request_irq(dev, irq, spear_adc_isr, 0, SPEAR_ADC_MOD_NAME,
+			info);

Better indent with opening parenthesis.

Honestly this is in the category of really doesn't matter, but what the heck, I'm redoing the series anyway so might as well fix this as well ;)

Quoted 22 lines
Quoted 21 lines
drivers/staging/iio/adc/spear_adc.c +2 -2
  	if (ret < 0) {
  		dev_err(dev, "failed requesting interrupt\n");
  		goto errout3;
@@ -370,7 +369,7 @@ static int spear_adc_probe(struct platform_device *pdev)
 
  	init_completion(&info->completion);
 
-	iodev->name = MOD_NAME;
+	iodev->name = SPEAR_ADC_MOD_NAME;
  	iodev->dev.parent = dev;
  	iodev->info = &spear_adc_iio_info;
  	iodev->modes = INDIO_DIRECT_MODE;
@@ -419,7 +418,7 @@ static struct platform_driver spear_adc_driver = {
  	.probe		= spear_adc_probe,
  	.remove		= spear_adc_remove,
  	.driver		= {
-		.name	= MOD_NAME,
+		.name	= SPEAR_ADC_MOD_NAME,
  		.owner	= THIS_MODULE,
  		.of_match_table = of_match_ptr(spear_adc_dt_ids),
  	},
author maintainer
#4 lore raw

PATCH v2 2/4 staging:iio:adc:spear_adc drop initialization of unused scan_type

In reply to Jonathan Cameron

As the driver does not support the buffered interfaces of IIO this is not used.

---
 drivers/staging/iio/adc/spear_adc.c | 4 ----
 1 file changed, 4 deletions(-)
drivers/staging/iio/adc/spear_adc.c +0 -4
index 9234e05..fa1e2b2 100644
@@ -179,10 +179,6 @@ static int spear_read_raw(struct iio_dev *indio_dev,
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
 	.channel = idx,					\
-	.scan_type = {					\
-		.sign = 'u',				\
-		.storagebits = 16,			\
-	},						\
 }
 
 static const struct iio_chan_spec spear_adc_iio_channels[] = {
Signature · 1 line
1.9.0
author maintainer
#5 lore raw

PATCH v2 3/4 staging:iio:adc:spear_adc use info_mask_shared_by_all for samp freq

In reply to Jonathan Cameron

Using the core support makes this element available to in kernel users as well as to userspace under exactly the same interface as before. The intent is to move all sampling frequency control to this approach throughout IIO.

Drop unused clk_high and clk_low whilst we are at it.

---
 drivers/staging/iio/adc/spear_adc.c | 96 ++++++++++++++-----------------------
 1 file changed, 37 insertions(+), 59 deletions(-)
drivers/staging/iio/adc/spear_adc.c +37 -59
index fa1e2b2..18a0a40 100644
@@ -168,16 +168,52 @@ static int spear_read_raw(struct iio_dev *indio_dev,
 		*val = info->vref_external;
 		*val2 = SPEAR_ADC_DATA_BITS;
 		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = info->current_clk;
+		return IIO_VAL_INT;
 	}
 
 	return -EINVAL;
 }
 
+static int spear_adc_write_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int val,
+			       int val2,
+			       long mask)
+{
+	struct spear_adc_info *info = iio_priv(indio_dev);
+	u32 count;
+	u32 apb_clk = clk_get_rate(info->clk);
+	int ret = 0;
+
+	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
+		return -EINVAL;
+
+	mutex_lock(&indio_dev->mlock);
+
+	if ((val < SPEAR_ADC_CLK_MIN) ||
+		(val > SPEAR_ADC_CLK_MAX) ||
+		(val2 != 0)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	count = (apb_clk + val - 1) / val;
+	info->current_clk = apb_clk / count;
+	spear_adc_set_clk(info, val);
+
+out:
+	mutex_unlock(&indio_dev->mlock);
+	return ret;
+}
+
 #define SPEAR_ADC_CHAN(idx) {				\
 	.type = IIO_VOLTAGE,				\
 	.indexed = 1,					\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
 	.channel = idx,					\
 }
 
@@ -219,67 +255,9 @@ static int spear_adc_configure(struct spear_adc_info *info)
 	return 0;
 }
 
-static ssize_t spear_adc_read_frequency(struct device *dev,
-					struct device_attribute *attr,
-					char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct spear_adc_info *info = iio_priv(indio_dev);
-
-	return sprintf(buf, "%d\n", info->current_clk);
-}
-
-static ssize_t spear_adc_write_frequency(struct device *dev,
-					 struct device_attribute *attr,
-					 const char *buf,
-					 size_t len)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct spear_adc_info *info = iio_priv(indio_dev);
-	u32 clk_high, clk_low, count;
-	u32 apb_clk = clk_get_rate(info->clk);
-	unsigned long lval;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &lval);
-	if (ret)
-		return ret;
-
-	mutex_lock(&indio_dev->mlock);
-
-	if ((lval < SPEAR_ADC_CLK_MIN) || (lval > SPEAR_ADC_CLK_MAX)) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	count = (apb_clk + lval - 1) / lval;
-	clk_low = count / 2;
-	clk_high = count - clk_low;
-	info->current_clk = apb_clk / count;
-	spear_adc_set_clk(info, lval);
-
-out:
-	mutex_unlock(&indio_dev->mlock);
-
-	return ret ? ret : len;
-}
-
-static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
-			      spear_adc_read_frequency,
-			      spear_adc_write_frequency);
-
-static struct attribute *spear_attributes[] = {
-	&iio_dev_attr_sampling_frequency.dev_attr.attr,
-	NULL
-};
-
-static const struct attribute_group spear_attribute_group = {
-	.attrs = spear_attributes,
-};
-
 static const struct iio_info spear_adc_iio_info = {
 	.read_raw = &spear_read_raw,
-	.attrs = &spear_attribute_group,
+	.write_raw = &spear_adc_write_raw,
 	.driver_module = THIS_MODULE,
 };
 
Signature · 1 line
1.9.0
#6 lore raw

Re: [PATCH 3/4] staging:iio:adc:spear_adc use info_mask_shared_by_all for samp freq

Quoted 45 lines · Jonathan Cameron

Using the core support makes this element available to in kernel users as well as to userspace under exactly the same interface as before. The intent is to move all sampling frequency control to this approach throughout IIO.

Drop unused clk_high and clk_low whilst we are at it.

---
 drivers/staging/iio/adc/spear_adc.c | 96 ++++++++++++++-----------------------
 1 file changed, 37 insertions(+), 59 deletions(-)
drivers/staging/iio/adc/spear_adc.c +19 -0
index fa1e2b2..18a0a40 100644
@@ -168,16 +168,52 @@ static int spear_read_raw(struct iio_dev *indio_dev,
 		*val = info->vref_external;
 		*val2 = SPEAR_ADC_DATA_BITS;
 		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = info->current_clk;
+		return IIO_VAL_INT;
 	}
 
 	return -EINVAL;
 }
 
+static int spear_adc_write_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int val,
+			       int val2,
+			       long mask)
+{
+	struct spear_adc_info *info = iio_priv(indio_dev);
+	u32 count;
+	u32 apb_clk = clk_get_rate(info->clk);
+	int ret = 0;
+
+	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
+		return -EINVAL;
+
+	mutex_lock(&indio_dev->mlock);
+

drivers/staging/iio/adc/spear_adc.c static int spear_read_raw(struct iio_dev *indio_dev, · line 204

Quoted 10 lines
drivers/staging/iio/adc/spear_adc.c +10 -0
+	if ((val < SPEAR_ADC_CLK_MIN) ||
+		(val > SPEAR_ADC_CLK_MAX) ||
+		(val2 != 0)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	count = (apb_clk + val - 1) / val;
+	info->current_clk = apb_clk / count;
+	spear_adc_set_clk(info, val);

It's me again, same spot, but this time on caffeine and sugar ;-) This time I got curious about what spear_adc_set_clk actually does. And guess what (make sure to be seated): it does the same calculation of count and info->current_clk. So these two lines can leave, as well. My thoughts about that function will follow in patch 4.

Quoted 84 lines
drivers/staging/iio/adc/spear_adc.c +8 -59
+
+out:
+	mutex_unlock(&indio_dev->mlock);
+	return ret;
+}
+
 #define SPEAR_ADC_CHAN(idx) {				\
 	.type = IIO_VOLTAGE,				\
 	.indexed = 1,					\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
 	.channel = idx,					\
 }
 
@@ -219,67 +255,9 @@ static int spear_adc_configure(struct spear_adc_info *info)
 	return 0;
 }
 
-static ssize_t spear_adc_read_frequency(struct device *dev,
-					struct device_attribute *attr,
-					char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct spear_adc_info *info = iio_priv(indio_dev);
-
-	return sprintf(buf, "%d\n", info->current_clk);
-}
-
-static ssize_t spear_adc_write_frequency(struct device *dev,
-					 struct device_attribute *attr,
-					 const char *buf,
-					 size_t len)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct spear_adc_info *info = iio_priv(indio_dev);
-	u32 clk_high, clk_low, count;
-	u32 apb_clk = clk_get_rate(info->clk);
-	unsigned long lval;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &lval);
-	if (ret)
-		return ret;
-
-	mutex_lock(&indio_dev->mlock);
-
-	if ((lval < SPEAR_ADC_CLK_MIN) || (lval > SPEAR_ADC_CLK_MAX)) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	count = (apb_clk + lval - 1) / lval;
-	clk_low = count / 2;
-	clk_high = count - clk_low;
-	info->current_clk = apb_clk / count;
-	spear_adc_set_clk(info, lval);
-
-out:
-	mutex_unlock(&indio_dev->mlock);
-
-	return ret ? ret : len;
-}
-
-static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
-			      spear_adc_read_frequency,
-			      spear_adc_write_frequency);
-
-static struct attribute *spear_attributes[] = {
-	&iio_dev_attr_sampling_frequency.dev_attr.attr,
-	NULL
-};
-
-static const struct attribute_group spear_attribute_group = {
-	.attrs = spear_attributes,
-};
-
 static const struct iio_info spear_adc_iio_info = {
 	.read_raw = &spear_read_raw,
-	.attrs = &spear_attribute_group,
+	.write_raw = &spear_adc_write_raw,
 	.driver_module = THIS_MODULE,
 };
 
author maintainer
#7 lore raw

Re: [PATCH 3/4] staging:iio:adc:spear_adc use info_mask_shared_by_all for samp freq

Quoted 53 lines · Hartmut Knaack
Quoted 52 lines · Jonathan Cameron

Using the core support makes this element available to in kernel users as well as to userspace under exactly the same interface as before. The intent is to move all sampling frequency control to this approach throughout IIO.

Drop unused clk_high and clk_low whilst we are at it.

---
  drivers/staging/iio/adc/spear_adc.c | 96 ++++++++++++++-----------------------
  1 file changed, 37 insertions(+), 59 deletions(-)
drivers/staging/iio/adc/spear_adc.c +26 -0
index fa1e2b2..18a0a40 100644
@@ -168,16 +168,52 @@ static int spear_read_raw(struct iio_dev *indio_dev,
  		*val = info->vref_external;
  		*val2 = SPEAR_ADC_DATA_BITS;
  		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = info->current_clk;
+		return IIO_VAL_INT;
  	}
 
  	return -EINVAL;
  }
 
+static int spear_adc_write_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int val,
+			       int val2,
+			       long mask)
+{
+	struct spear_adc_info *info = iio_priv(indio_dev);
+	u32 count;
+	u32 apb_clk = clk_get_rate(info->clk);
+	int ret = 0;
+
+	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
+		return -EINVAL;
+
+	mutex_lock(&indio_dev->mlock);
+
+	if ((val < SPEAR_ADC_CLK_MIN) ||
+		(val > SPEAR_ADC_CLK_MAX) ||
+		(val2 != 0)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
Quoted 8 lines

drivers/staging/iio/adc/spear_adc.c static int spear_read_raw(struct iio_dev *indio_dev, · line 204

Quoted 3 lines
drivers/staging/iio/adc/spear_adc.c +3 -0
+	count = (apb_clk + val - 1) / val;
+	info->current_clk = apb_clk / count;
+	spear_adc_set_clk(info, val);

It's me again, same spot, but this time on caffeine and sugar ;-) This time I got curious about what spear_adc_set_clk actually does. And guess what (make sure to be seated): it does the same calculation of count and info->current_clk. So these two lines can leave, as well. My thoughts about that function will follow in patch 4.

Gah! I should have been less lazy and actually taken a look at what was going on myself.

Anyhow, will fix this up for the next version.

Quoted 90 lines
Quoted 74 lines
drivers/staging/iio/adc/spear_adc.c +7 -55
+
+out:
+	mutex_unlock(&indio_dev->mlock);
+	return ret;
+}
+
  #define SPEAR_ADC_CHAN(idx) {				\
  	.type = IIO_VOLTAGE,				\
  	.indexed = 1,					\
  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
  	.channel = idx,					\
  }
 
@@ -219,67 +255,9 @@ static int spear_adc_configure(struct spear_adc_info *info)
  	return 0;
  }
 
-static ssize_t spear_adc_read_frequency(struct device *dev,
-					struct device_attribute *attr,
-					char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct spear_adc_info *info = iio_priv(indio_dev);
-
-	return sprintf(buf, "%d\n", info->current_clk);
-}
-
-static ssize_t spear_adc_write_frequency(struct device *dev,
-					 struct device_attribute *attr,
-					 const char *buf,
-					 size_t len)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct spear_adc_info *info = iio_priv(indio_dev);
-	u32 clk_high, clk_low, count;
-	u32 apb_clk = clk_get_rate(info->clk);
-	unsigned long lval;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &lval);
-	if (ret)
-		return ret;
-
-	mutex_lock(&indio_dev->mlock);
-
-	if ((lval < SPEAR_ADC_CLK_MIN) || (lval > SPEAR_ADC_CLK_MAX)) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	count = (apb_clk + lval - 1) / lval;
-	clk_low = count / 2;
-	clk_high = count - clk_low;
-	info->current_clk = apb_clk / count;
-	spear_adc_set_clk(info, lval);
-
-out:
-	mutex_unlock(&indio_dev->mlock);
-
-	return ret ? ret : len;
-}
-
-static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
-			      spear_adc_read_frequency,
-			      spear_adc_write_frequency);
-
-static struct attribute *spear_attributes[] = {
-	&iio_dev_attr_sampling_frequency.dev_attr.attr,
-	NULL
-};
-
-static const struct attribute_group spear_attribute_group = {

drivers/staging/iio/adc/spear_adc.c static int spear_adc_configure(struct spear_adc_info *info) · line 263

Quoted 10 lines
drivers/staging/iio/adc/spear_adc.c +1 -4
-	.attrs = spear_attributes,
-};
-
  static const struct iio_info spear_adc_iio_info = {
  	.read_raw = &spear_read_raw,
-	.attrs = &spear_attribute_group,
+	.write_raw = &spear_adc_write_raw,
  	.driver_module = THIS_MODULE,
  };
 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author maintainer
#8 lore raw

PATCH v2 4/4 iio:adc:spear_adc move out of staging

In reply to Jonathan Cameron

This simple driver is ready to move out of staging.

---
 drivers/iio/adc/Kconfig             |   8 +
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/spear_adc.c         | 405 ++++++++++++++++++++++++++++++++++++
 drivers/staging/iio/adc/Kconfig     |   8 -
 drivers/staging/iio/adc/Makefile    |   1 -
 drivers/staging/iio/adc/spear_adc.c | 405 ------------------------------------
 6 files changed, 414 insertions(+), 414 deletions(-)
drivers/iio/adc/Kconfig +8 -0
index 5553206..2e3e1b0 100644
@@ -164,6 +164,14 @@ config NAU7802
 	  To compile this driver as a module, choose M here: the
 	  module will be called nau7802.
 
+config SPEAR_ADC
+	tristate "ST SPEAr ADC"
+	depends on PLAT_SPEAR || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Say yes here to build support for the integrated ADC inside the
+	  ST SPEAr SoC. Provides direct access via sysfs.
+
 config TI_ADC081C
 	tristate "Texas Instruments ADC081C021/027"
 	depends on I2C
drivers/iio/adc/Makefile +1 -0
index 89f1216..8378fb2 100644
@@ -18,6 +18,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
 obj-$(CONFIG_NAU7802) += nau7802.o
+obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
 obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
drivers/iio/adc/spear_adc.c new file +405 -0

405 lines not on this page · raw message

drivers/staging/iio/adc/Kconfig +0 -8
index 3633298..a37d001 100644
@@ -126,12 +126,4 @@ config MXS_LRADC
 	  To compile this driver as a module, choose M here: the
 	  module will be called mxs-lradc.
 
-config SPEAR_ADC
-	tristate "ST SPEAr ADC"
-	depends on PLAT_SPEAR || COMPILE_TEST
-	depends on HAS_IOMEM
-	help
-	  Say yes here to build support for the integrated ADC inside the
-	  ST SPEAr SoC. Provides direct access via sysfs.
-
 endmenu
drivers/staging/iio/adc/Makefile +0 -1
index 3e9fb14..b5915c1 100644
@@ -19,4 +19,3 @@ obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_AD7280) += ad7280a.o
 obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
 obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
-obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
drivers/staging/iio/adc/spear_adc.c deleted +0 -405

405 lines not on this page · raw message

Signature · 1 line
1.9.0
#9 lore raw

Re: [PATCH 4/4] iio:adc:spear_adc move out of staging

Quoted 50 lines · Jonathan Cameron

This simple driver is ready to move out of staging.

---
 drivers/iio/adc/Kconfig             |   8 +
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/spear_adc.c         | 405 ++++++++++++++++++++++++++++++++++++
 drivers/staging/iio/adc/Kconfig     |   8 -
 drivers/staging/iio/adc/Makefile    |   1 -
 drivers/staging/iio/adc/spear_adc.c | 405 ------------------------------------
 6 files changed, 414 insertions(+), 414 deletions(-)
drivers/iio/adc/Kconfig +8 -0
index 5553206..2e3e1b0 100644
@@ -164,6 +164,14 @@ config NAU7802
 	  To compile this driver as a module, choose M here: the
 	  module will be called nau7802.
 
+config SPEAR_ADC
+	tristate "ST SPEAr ADC"
+	depends on PLAT_SPEAR || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Say yes here to build support for the integrated ADC inside the
+	  ST SPEAr SoC. Provides direct access via sysfs.
+
 config TI_ADC081C
 	tristate "Texas Instruments ADC081C021/027"
 	depends on I2C
drivers/iio/adc/Makefile +1 -0
index 89f1216..8378fb2 100644
@@ -18,6 +18,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
 obj-$(CONFIG_NAU7802) += nau7802.o
+obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
 obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
drivers/iio/adc/spear_adc.c new file +0 -0
new file mode 100644
index 0000000..18a0a40

drivers/iio/adc/spear_adc.c line 4

Quoted 4 lines
drivers/iio/adc/spear_adc.c new file +4 -0
@@ -0,0 +1,405 @@
+/*
+ * ST SPEAr ADC driver
+ *
+ * Copyright 2012 Stefan Roese <[email protected]>

I found a datasheet at http://www.st.com/st-web-ui/static/active/en/resource/technical/document/reference_manual/DM00034813.pdf
ADC is covered on pages 594ff.

Quoted 32 lines
drivers/iio/adc/spear_adc.c new file +32 -0
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/completion.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+/* SPEAR registers definitions */
+#define SPEAR600_ADC_SCAN_RATE_LO(x)	((x) & 0xFFFF)
+#define SPEAR600_ADC_SCAN_RATE_HI(x)	(((x) >> 0x10) & 0xFFFF)
+#define SPEAR_ADC_CLK_LOW(x)		(((x) & 0xf) << 0)
+#define SPEAR_ADC_CLK_HIGH(x)		(((x) & 0xf) << 4)
+
+/* Bit definitions for SPEAR_ADC_STATUS */
+#define SPEAR_ADC_STATUS_START_CONVERSION	(1 << 0)
+#define SPEAR_ADC_STATUS_CHANNEL_NUM(x)		((x) << 1)
+#define SPEAR_ADC_STATUS_ADC_ENABLE		(1 << 4)
+#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)		((x) << 5)
+#define SPEAR_ADC_STATUS_VREF_INTERNAL		(1 << 9)

drivers/iio/adc/spear_adc.c line 46

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +10 -0
+
+#define SPEAR_ADC_DATA_MASK		0x03ff
+#define SPEAR_ADC_DATA_BITS		10
+
+#define SPEAR_ADC_MOD_NAME "spear-adc"
+
+#define SPEAR_ADC_CHANNEL_NUM		8
+
+#define SPEAR_ADC_CLK_MIN			2500000
+#define SPEAR_ADC_CLK_MAX			20000000

Datasheet page 599: "The max frequency of CLK_ADC is 14 MHz as the minimum is 3 MHz"

Quoted 29 lines
drivers/iio/adc/spear_adc.c new file +29 -0
+
+struct adc_regs_spear3xx {
+	u32 status;
+	u32 average;
+	u32 scan_rate;
+	u32 clk;	/* Not avail for 1340 & 1310 */
+	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+	u32 ch_data[SPEAR_ADC_CHANNEL_NUM];
+};
+
+struct chan_data {
+	u32 lsb;
+	u32 msb;
+};
+
+struct adc_regs_spear6xx {
+	u32 status;
+	u32 pad[2];
+	u32 clk;
+	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+	struct chan_data ch_data[SPEAR_ADC_CHANNEL_NUM];
+	u32 scan_rate_lo;
+	u32 scan_rate_hi;
+	struct chan_data average;
+};
+
+struct spear_adc_info {
+	struct device_node *np;
+	struct adc_regs_spear3xx __iomem *adc_base_spear3xx;

drivers/iio/adc/spear_adc.c line 85

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +10 -0
+	struct adc_regs_spear6xx __iomem *adc_base_spear6xx;
+	struct clk *clk;
+	struct completion completion;
+	u32 current_clk;
+	u32 sampling_freq;
+	u32 avg_samples;
+	u32 vref_external;
+	u32 value;
+};
+

I would prefer to see some comments like in other drivers, to get an idea what units are assumed (especially for vref_external). Also, I would rename it to the scheme, that the majority of the drivers seem to use: struct spear_adc_state (and the instances *st instead of *info).

drivers/iio/adc/spear_adc.c line 94

Quoted 9 lines
drivers/iio/adc/spear_adc.c new file +9 -0
+/*
+ * Functions to access some SPEAr ADC register. Abstracted into
+ * static inline functions, because of different register offsets
+ * on different SoC variants (SPEAr300 vs SPEAr600 etc).
+ */
+static void spear_adc_set_status(struct spear_adc_info *info, u32 val)
+{
+	__raw_writel(val, &info->adc_base_spear6xx->status);
+}

Datasheet page 597 says: "It can be written only if both values ADC_STATUS_REG [8] and ADC_STATUS_REG [0] are ‘0’." This can be a problem, since bit 0 gets set by spear_read_raw().

drivers/iio/adc/spear_adc.c line 105

Quoted 11 lines
drivers/iio/adc/spear_adc.c new file +11 -0
+
+static void spear_adc_set_clk(struct spear_adc_info *info, u32 val)
+{
+	u32 clk_high, clk_low, count;
+	u32 apb_clk = clk_get_rate(info->clk);
+
+	count = (apb_clk + val - 1) / val;
+	clk_low = count / 2;
+	clk_high = count - clk_low;
+	info->current_clk = apb_clk / count;
+

I've got some doubts about this calculation. The datasheet say on page 599: "...the [adc-]frequency is the APB clock frequency divided by the sum of these values[clk_low and clk_high].". So I would represent it as following:

val = apb_clk / (clk_low + clk_high) = apb_clk / count

So, first thing we want is count, so multiply by count and divide by val:

count = apb_clk / val

The rest seems fine, but it should be kept in mind, that clk_low and clk_high are just 4 bit size, each. So, using u8 would be sufficient (also for count), and clk_low and clk_high should be checked to not exceed a value of 0xF (or count for 0x1F). Thus, changing the function type to int would also help to return an error code.

drivers/iio/adc/spear_adc.c line 115

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +10 -0
+	__raw_writel(SPEAR_ADC_CLK_LOW(clk_low) | SPEAR_ADC_CLK_HIGH(clk_high),
+		     &info->adc_base_spear6xx->clk);
+}
+
+static void spear_adc_set_ctrl(struct spear_adc_info *info, int n,
+			       u32 val)
+{
+	__raw_writel(val, &info->adc_base_spear6xx->ch_ctrl[n]);
+}
+

Datasheet page 599 says: "It is possible to write these registers only if ENABLE is ‘0’."
Although spear_adc_configure() takes care of that under the current circumstances, it might cause some troubles in case someone makes use of this function any time later. So either check for it here, or leave a comment.

drivers/iio/adc/spear_adc.c line 120

Quoted 5 lines
drivers/iio/adc/spear_adc.c new file +5 -0
+static u32 spear_adc_get_average(struct spear_adc_info *info)
+{
+	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
+		return __raw_readl(&info->adc_base_spear6xx->average.msb) &
+			SPEAR_ADC_DATA_MASK;

Datasheet page 601 says: "The AVERAGE_REG_MSB register has latched the word of the conversion ant it can be read only if both values CONVERSION READY and ENABLE are ‘1’." Page 595 says: "At the end of conversion, the CONVERSION READY bit in ADC_STATUS_REG is set and an interrupt signal is generated."
So, since this function is only called during interrupt-handling, it is fine. But maybe also add a comment.

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+	} else {
+		return __raw_readl(&info->adc_base_spear3xx->average) &
+			SPEAR_ADC_DATA_MASK;
+	}
+}
+
+static void spear_adc_set_scanrate(struct spear_adc_info *info, u32 rate)

drivers/iio/adc/spear_adc.c line 137

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +10 -0
+{
+	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
+		__raw_writel(SPEAR600_ADC_SCAN_RATE_LO(rate),
+			     &info->adc_base_spear6xx->scan_rate_lo);
+		__raw_writel(SPEAR600_ADC_SCAN_RATE_HI(rate),
+			     &info->adc_base_spear6xx->scan_rate_hi);
+	} else {
+		__raw_writel(rate, &info->adc_base_spear3xx->scan_rate);
+	}
+}

Datasheet page 600/601: "This register can be written only if both values ADC_STATUS_REG [8] and ADC_STATUS_REG [0] are ‘0’." Same as above, a comment would be good.

Quoted 21 lines
drivers/iio/adc/spear_adc.c new file +21 -0
+
+static int spear_read_raw(struct iio_dev *indio_dev,
+			  struct iio_chan_spec const *chan,
+			  int *val,
+			  int *val2,
+			  long mask)
+{
+	struct spear_adc_info *info = iio_priv(indio_dev);
+	u32 status;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&indio_dev->mlock);
+
+		status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
+			SPEAR_ADC_STATUS_AVG_SAMPLE(info->avg_samples) |
+			SPEAR_ADC_STATUS_START_CONVERSION |
+			SPEAR_ADC_STATUS_ADC_ENABLE;
+		if (info->vref_external == 0)
+			status |= SPEAR_ADC_STATUS_VREF_INTERNAL;
+

drivers/iio/adc/spear_adc.c line 168

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +10 -0
+		spear_adc_set_status(info, status);
+		wait_for_completion(&info->completion); /* set by ISR */
+		*val = info->value;
+
+		mutex_unlock(&indio_dev->mlock);
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = info->vref_external;

What if internal reference is selected? In my opinion, this driver should also be converted to use regulator framework (the boards will certainly contain some regulator, anyway).

drivers/iio/adc/spear_adc.c line 176

Quoted 8 lines
drivers/iio/adc/spear_adc.c new file +8 -0
+		*val2 = SPEAR_ADC_DATA_BITS;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = info->current_clk;
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;

Catch invalid masks with default: section?

Quoted 6 lines
drivers/iio/adc/spear_adc.c new file +6 -0
+}
+
+static int spear_adc_write_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int val,
+			       int val2,

drivers/iio/adc/spear_adc.c line 192

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +10 -0
+			       long mask)
+{
+	struct spear_adc_info *info = iio_priv(indio_dev);
+	u32 count;
+	u32 apb_clk = clk_get_rate(info->clk);
+	int ret = 0;
+
+	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
+		return -EINVAL;
+

IMHO using switch(mask) like in the read_raw functions would be more beautiful (though at the cost of a deeper indent level), yet this also does the job.

drivers/iio/adc/spear_adc.c line 196

Quoted 4 lines
drivers/iio/adc/spear_adc.c new file +4 -0
+	mutex_lock(&indio_dev->mlock);
+
+	if ((val < SPEAR_ADC_CLK_MIN) ||
+		(val > SPEAR_ADC_CLK_MAX) ||

As mentioned above, I don't know if these values are accurate, or if it would even make more sense to just check that clk_low and clk_high fit into 4 bits, each.

drivers/iio/adc/spear_adc.c line 204

Quoted 8 lines
drivers/iio/adc/spear_adc.c new file +8 -0
+		(val2 != 0)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	count = (apb_clk + val - 1) / val;
+	info->current_clk = apb_clk / count;
+	spear_adc_set_clk(info, val);

As mentioned in the previous patch, get rid of count and apb_clk here.

Quoted 30 lines
drivers/iio/adc/spear_adc.c new file +30 -0
+
+out:
+	mutex_unlock(&indio_dev->mlock);
+	return ret;
+}
+
+#define SPEAR_ADC_CHAN(idx) {				\
+	.type = IIO_VOLTAGE,				\
+	.indexed = 1,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
+	.channel = idx,					\
+}
+
+static const struct iio_chan_spec spear_adc_iio_channels[] = {
+	SPEAR_ADC_CHAN(0),
+	SPEAR_ADC_CHAN(1),
+	SPEAR_ADC_CHAN(2),
+	SPEAR_ADC_CHAN(3),
+	SPEAR_ADC_CHAN(4),
+	SPEAR_ADC_CHAN(5),
+	SPEAR_ADC_CHAN(6),
+	SPEAR_ADC_CHAN(7),
+};
+
+static irqreturn_t spear_adc_isr(int irq, void *dev_id)
+{
+	struct spear_adc_info *info = (struct spear_adc_info *)dev_id;
+

drivers/iio/adc/spear_adc.c line 244

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +10 -0
+	/* Read value to clear IRQ */
+	info->value = spear_adc_get_average(info);
+	complete(&info->completion);
+
+	return IRQ_HANDLED;
+}
+
+static int spear_adc_configure(struct spear_adc_info *info)
+{
+	int i;

u8 i; would also do the job.

drivers/iio/adc/spear_adc.c line 249

Quoted 5 lines
drivers/iio/adc/spear_adc.c new file +5 -0
+
+	/* Reset ADC core */
+	spear_adc_set_status(info, 0);
+	__raw_writel(0, &info->adc_base_spear6xx->clk);
+	for (i = 0; i < 8; i++)

i < ARRAY_SIZE or i < ADC_CHANNEL_NUM to prevent buffer overflows.

drivers/iio/adc/spear_adc.c line 260

Quoted 11 lines
drivers/iio/adc/spear_adc.c new file +11 -0
+		spear_adc_set_ctrl(info, i, 0);
+	spear_adc_set_scanrate(info, 0);
+
+	spear_adc_set_clk(info, info->sampling_freq);
+
+	return 0;
+}
+
+static const struct iio_info spear_adc_iio_info = {
+	.read_raw = &spear_read_raw,
+	.write_raw = &spear_adc_write_raw,

Better use the same prefix for those functions (also rename one of them, above).

drivers/iio/adc/spear_adc.c line 269

Quoted 9 lines
drivers/iio/adc/spear_adc.c new file +9 -0
+	.driver_module = THIS_MODULE,
+};
+
+static int spear_adc_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct spear_adc_info *info;
+	struct iio_dev *iodev = NULL;

Not really wrong, but nobody does that these days anymore. And we normally call it indio_dev

Quoted 88 lines
drivers/iio/adc/spear_adc.c new file +88 -0
+	int ret = -ENODEV;
+	int irq;
+
+	iodev = devm_iio_device_alloc(dev, sizeof(struct spear_adc_info));
+	if (!iodev) {
+		dev_err(dev, "failed allocating iio device\n");
+		return -ENOMEM;
+	}
+
+	info = iio_priv(iodev);
+	info->np = np;
+
+	/*
+	 * SPEAr600 has a different register layout than other SPEAr SoC's
+	 * (e.g. SPEAr3xx). Let's provide two register base addresses
+	 * to support multi-arch kernels.
+	 */
+	info->adc_base_spear6xx = of_iomap(np, 0);
+	if (!info->adc_base_spear6xx) {
+		dev_err(dev, "failed mapping memory\n");
+		return -ENOMEM;
+	}
+	info->adc_base_spear3xx =
+		(struct adc_regs_spear3xx __iomem *)info->adc_base_spear6xx;
+
+	info->clk = clk_get(dev, NULL);
+	if (IS_ERR(info->clk)) {
+		dev_err(dev, "failed getting clock\n");
+		goto errout1;
+	}
+
+	ret = clk_prepare_enable(info->clk);
+	if (ret) {
+		dev_err(dev, "failed enabling clock\n");
+		goto errout2;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0) {
+		dev_err(dev, "failed getting interrupt resource\n");
+		ret = -EINVAL;
+		goto errout3;
+	}
+
+	ret = devm_request_irq(dev, irq, spear_adc_isr, 0, SPEAR_ADC_MOD_NAME,
+			info);
+	if (ret < 0) {
+		dev_err(dev, "failed requesting interrupt\n");
+		goto errout3;
+	}
+
+	if (of_property_read_u32(np, "sampling-frequency",
+				 &info->sampling_freq)) {
+		dev_err(dev, "sampling-frequency missing in DT\n");
+		ret = -EINVAL;
+		goto errout3;
+	}
+
+	/*
+	 * Optional avg_samples defaults to 0, resulting in single data
+	 * conversion
+	 */
+	of_property_read_u32(np, "average-samples", &info->avg_samples);
+
+	/*
+	 * Optional vref_external defaults to 0, resulting in internal vref
+	 * selection
+	 */
+	of_property_read_u32(np, "vref-external", &info->vref_external);
+
+	spear_adc_configure(info);
+
+	platform_set_drvdata(pdev, iodev);
+
+	init_completion(&info->completion);
+
+	iodev->name = SPEAR_ADC_MOD_NAME;
+	iodev->dev.parent = dev;
+	iodev->info = &spear_adc_iio_info;
+	iodev->modes = INDIO_DIRECT_MODE;
+	iodev->channels = spear_adc_iio_channels;
+	iodev->num_channels = ARRAY_SIZE(spear_adc_iio_channels);
+
+	ret = iio_device_register(iodev);
+	if (ret)
+		goto errout3;
+
+	dev_info(dev, "SPEAR ADC driver loaded, IRQ %d\n", irq);

drivers/iio/adc/spear_adc.c line 367

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +10 -0
+
+	return 0;
+
+errout3:
+	clk_disable_unprepare(info->clk);
+errout2:
+	clk_put(info->clk);
+errout1:
+	iounmap(info->adc_base_spear6xx);
+	return ret;

These labels could have more appropriate names.

Quoted 20 lines
drivers/iio/adc/spear_adc.c new file +20 -0
+}
+
+static int spear_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *iodev = platform_get_drvdata(pdev);
+	struct spear_adc_info *info = iio_priv(iodev);
+
+	iio_device_unregister(iodev);
+	clk_disable_unprepare(info->clk);
+	clk_put(info->clk);
+	iounmap(info->adc_base_spear6xx);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id spear_adc_dt_ids[] = {
+	{ .compatible = "st,spear600-adc", },
+	{ /* sentinel */ }
+};

drivers/iio/adc/spear_adc.c line 397

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +10 -0
+MODULE_DEVICE_TABLE(of, spear_adc_dt_ids);
+#endif
+
+static struct platform_driver spear_adc_driver = {
+	.probe		= spear_adc_probe,
+	.remove		= spear_adc_remove,
+	.driver		= {
+		.name	= SPEAR_ADC_MOD_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(spear_adc_dt_ids),

What if CONFIG_OF is disabled?

Quoted 445 lines
drivers/iio/adc/spear_adc.c new file +8 -0
+	},
+};
+
+module_platform_driver(spear_adc_driver);
+
+MODULE_AUTHOR("Stefan Roese <[email protected]>");
+MODULE_DESCRIPTION("SPEAr ADC driver");
+MODULE_LICENSE("GPL");
drivers/staging/iio/adc/Kconfig +0 -8
index 3633298..a37d001 100644
@@ -126,12 +126,4 @@ config MXS_LRADC
 	  To compile this driver as a module, choose M here: the
 	  module will be called mxs-lradc.
 
-config SPEAR_ADC
-	tristate "ST SPEAr ADC"
-	depends on PLAT_SPEAR || COMPILE_TEST
-	depends on HAS_IOMEM
-	help
-	  Say yes here to build support for the integrated ADC inside the
-	  ST SPEAr SoC. Provides direct access via sysfs.
-
 endmenu
drivers/staging/iio/adc/Makefile +0 -1
index 3e9fb14..b5915c1 100644
@@ -19,4 +19,3 @@ obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_AD7280) += ad7280a.o
 obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
 obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
-obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
drivers/staging/iio/adc/spear_adc.c deleted +0 -405
deleted file mode 100644
index 18a0a40..0000000
@@ -1,405 +0,0 @@
-/*
- * ST SPEAr ADC driver
- *
- * Copyright 2012 Stefan Roese <[email protected]>
- *
- * Licensed under the GPL-2.
- */
-
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/interrupt.h>
-#include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/io.h>
-#include <linux/clk.h>
-#include <linux/err.h>
-#include <linux/completion.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-
-/* SPEAR registers definitions */
-#define SPEAR600_ADC_SCAN_RATE_LO(x)	((x) & 0xFFFF)
-#define SPEAR600_ADC_SCAN_RATE_HI(x)	(((x) >> 0x10) & 0xFFFF)
-#define SPEAR_ADC_CLK_LOW(x)		(((x) & 0xf) << 0)
-#define SPEAR_ADC_CLK_HIGH(x)		(((x) & 0xf) << 4)
-
-/* Bit definitions for SPEAR_ADC_STATUS */
-#define SPEAR_ADC_STATUS_START_CONVERSION	(1 << 0)
-#define SPEAR_ADC_STATUS_CHANNEL_NUM(x)		((x) << 1)
-#define SPEAR_ADC_STATUS_ADC_ENABLE		(1 << 4)
-#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)		((x) << 5)
-#define SPEAR_ADC_STATUS_VREF_INTERNAL		(1 << 9)
-
-#define SPEAR_ADC_DATA_MASK		0x03ff
-#define SPEAR_ADC_DATA_BITS		10
-
-#define SPEAR_ADC_MOD_NAME "spear-adc"
-
-#define SPEAR_ADC_CHANNEL_NUM		8
-
-#define SPEAR_ADC_CLK_MIN			2500000
-#define SPEAR_ADC_CLK_MAX			20000000
-
-struct adc_regs_spear3xx {
-	u32 status;
-	u32 average;
-	u32 scan_rate;
-	u32 clk;	/* Not avail for 1340 & 1310 */
-	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
-	u32 ch_data[SPEAR_ADC_CHANNEL_NUM];
-};
-
-struct chan_data {
-	u32 lsb;
-	u32 msb;
-};
-
-struct adc_regs_spear6xx {
-	u32 status;
-	u32 pad[2];
-	u32 clk;
-	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
-	struct chan_data ch_data[SPEAR_ADC_CHANNEL_NUM];
-	u32 scan_rate_lo;
-	u32 scan_rate_hi;
-	struct chan_data average;
-};
-
-struct spear_adc_info {
-	struct device_node *np;
-	struct adc_regs_spear3xx __iomem *adc_base_spear3xx;
-	struct adc_regs_spear6xx __iomem *adc_base_spear6xx;
-	struct clk *clk;
-	struct completion completion;
-	u32 current_clk;
-	u32 sampling_freq;
-	u32 avg_samples;
-	u32 vref_external;
-	u32 value;
-};
-
-/*
- * Functions to access some SPEAr ADC register. Abstracted into
- * static inline functions, because of different register offsets
- * on different SoC variants (SPEAr300 vs SPEAr600 etc).
- */
-static void spear_adc_set_status(struct spear_adc_info *info, u32 val)
-{
-	__raw_writel(val, &info->adc_base_spear6xx->status);
-}
-
-static void spear_adc_set_clk(struct spear_adc_info *info, u32 val)
-{
-	u32 clk_high, clk_low, count;
-	u32 apb_clk = clk_get_rate(info->clk);
-
-	count = (apb_clk + val - 1) / val;
-	clk_low = count / 2;
-	clk_high = count - clk_low;
-	info->current_clk = apb_clk / count;
-
-	__raw_writel(SPEAR_ADC_CLK_LOW(clk_low) | SPEAR_ADC_CLK_HIGH(clk_high),
-		     &info->adc_base_spear6xx->clk);
-}
-
-static void spear_adc_set_ctrl(struct spear_adc_info *info, int n,
-			       u32 val)
-{
-	__raw_writel(val, &info->adc_base_spear6xx->ch_ctrl[n]);
-}
-
-static u32 spear_adc_get_average(struct spear_adc_info *info)
-{
-	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
-		return __raw_readl(&info->adc_base_spear6xx->average.msb) &
-			SPEAR_ADC_DATA_MASK;
-	} else {
-		return __raw_readl(&info->adc_base_spear3xx->average) &
-			SPEAR_ADC_DATA_MASK;
-	}
-}
-
-static void spear_adc_set_scanrate(struct spear_adc_info *info, u32 rate)
-{
-	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
-		__raw_writel(SPEAR600_ADC_SCAN_RATE_LO(rate),
-			     &info->adc_base_spear6xx->scan_rate_lo);
-		__raw_writel(SPEAR600_ADC_SCAN_RATE_HI(rate),
-			     &info->adc_base_spear6xx->scan_rate_hi);
-	} else {
-		__raw_writel(rate, &info->adc_base_spear3xx->scan_rate);
-	}
-}
-
-static int spear_read_raw(struct iio_dev *indio_dev,
-			  struct iio_chan_spec const *chan,
-			  int *val,
-			  int *val2,
-			  long mask)
-{
-	struct spear_adc_info *info = iio_priv(indio_dev);
-	u32 status;
-
-	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
-
-		status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
-			SPEAR_ADC_STATUS_AVG_SAMPLE(info->avg_samples) |
-			SPEAR_ADC_STATUS_START_CONVERSION |
-			SPEAR_ADC_STATUS_ADC_ENABLE;
-		if (info->vref_external == 0)
-			status |= SPEAR_ADC_STATUS_VREF_INTERNAL;
-
-		spear_adc_set_status(info, status);
-		wait_for_completion(&info->completion); /* set by ISR */
-		*val = info->value;
-
-		mutex_unlock(&indio_dev->mlock);
-
-		return IIO_VAL_INT;
-
-	case IIO_CHAN_INFO_SCALE:
-		*val = info->vref_external;
-		*val2 = SPEAR_ADC_DATA_BITS;
-		return IIO_VAL_FRACTIONAL_LOG2;
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		*val = info->current_clk;
-		return IIO_VAL_INT;
-	}
-
-	return -EINVAL;
-}
-
-static int spear_adc_write_raw(struct iio_dev *indio_dev,
-			       struct iio_chan_spec const *chan,
-			       int val,
-			       int val2,
-			       long mask)
-{
-	struct spear_adc_info *info = iio_priv(indio_dev);
-	u32 count;
-	u32 apb_clk = clk_get_rate(info->clk);
-	int ret = 0;
-
-	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
-		return -EINVAL;
-
-	mutex_lock(&indio_dev->mlock);
-
-	if ((val < SPEAR_ADC_CLK_MIN) ||
-		(val > SPEAR_ADC_CLK_MAX) ||
-		(val2 != 0)) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	count = (apb_clk + val - 1) / val;
-	info->current_clk = apb_clk / count;
-	spear_adc_set_clk(info, val);
-
-out:
-	mutex_unlock(&indio_dev->mlock);
-	return ret;
-}
-
-#define SPEAR_ADC_CHAN(idx) {				\
-	.type = IIO_VOLTAGE,				\
-	.indexed = 1,					\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
-	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
-	.channel = idx,					\
-}
-
-static const struct iio_chan_spec spear_adc_iio_channels[] = {
-	SPEAR_ADC_CHAN(0),
-	SPEAR_ADC_CHAN(1),
-	SPEAR_ADC_CHAN(2),
-	SPEAR_ADC_CHAN(3),
-	SPEAR_ADC_CHAN(4),
-	SPEAR_ADC_CHAN(5),
-	SPEAR_ADC_CHAN(6),
-	SPEAR_ADC_CHAN(7),
-};
-
-static irqreturn_t spear_adc_isr(int irq, void *dev_id)
-{
-	struct spear_adc_info *info = (struct spear_adc_info *)dev_id;
-
-	/* Read value to clear IRQ */
-	info->value = spear_adc_get_average(info);
-	complete(&info->completion);
-
-	return IRQ_HANDLED;
-}
-
-static int spear_adc_configure(struct spear_adc_info *info)
-{
-	int i;
-
-	/* Reset ADC core */
-	spear_adc_set_status(info, 0);
-	__raw_writel(0, &info->adc_base_spear6xx->clk);
-	for (i = 0; i < 8; i++)
-		spear_adc_set_ctrl(info, i, 0);
-	spear_adc_set_scanrate(info, 0);
-
-	spear_adc_set_clk(info, info->sampling_freq);
-
-	return 0;
-}
-
-static const struct iio_info spear_adc_iio_info = {
-	.read_raw = &spear_read_raw,
-	.write_raw = &spear_adc_write_raw,
-	.driver_module = THIS_MODULE,
-};
-
-static int spear_adc_probe(struct platform_device *pdev)
-{
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
-	struct spear_adc_info *info;
-	struct iio_dev *iodev = NULL;
-	int ret = -ENODEV;
-	int irq;
-
-	iodev = devm_iio_device_alloc(dev, sizeof(struct spear_adc_info));
-	if (!iodev) {
-		dev_err(dev, "failed allocating iio device\n");
-		return -ENOMEM;
-	}
-
-	info = iio_priv(iodev);
-	info->np = np;
-
-	/*
-	 * SPEAr600 has a different register layout than other SPEAr SoC's
-	 * (e.g. SPEAr3xx). Let's provide two register base addresses
-	 * to support multi-arch kernels.
-	 */
-	info->adc_base_spear6xx = of_iomap(np, 0);
-	if (!info->adc_base_spear6xx) {
-		dev_err(dev, "failed mapping memory\n");
-		return -ENOMEM;
-	}
-	info->adc_base_spear3xx =
-		(struct adc_regs_spear3xx __iomem *)info->adc_base_spear6xx;
-
-	info->clk = clk_get(dev, NULL);
-	if (IS_ERR(info->clk)) {
-		dev_err(dev, "failed getting clock\n");
-		goto errout1;
-	}
-
-	ret = clk_prepare_enable(info->clk);
-	if (ret) {
-		dev_err(dev, "failed enabling clock\n");
-		goto errout2;
-	}
-
-	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
-		dev_err(dev, "failed getting interrupt resource\n");
-		ret = -EINVAL;
-		goto errout3;
-	}
-
-	ret = devm_request_irq(dev, irq, spear_adc_isr, 0, SPEAR_ADC_MOD_NAME,
-			info);
-	if (ret < 0) {
-		dev_err(dev, "failed requesting interrupt\n");
-		goto errout3;
-	}
-
-	if (of_property_read_u32(np, "sampling-frequency",
-				 &info->sampling_freq)) {
-		dev_err(dev, "sampling-frequency missing in DT\n");
-		ret = -EINVAL;
-		goto errout3;
-	}
-
-	/*
-	 * Optional avg_samples defaults to 0, resulting in single data
-	 * conversion
-	 */
-	of_property_read_u32(np, "average-samples", &info->avg_samples);
-
-	/*
-	 * Optional vref_external defaults to 0, resulting in internal vref
-	 * selection
-	 */
-	of_property_read_u32(np, "vref-external", &info->vref_external);
-
-	spear_adc_configure(info);
-
-	platform_set_drvdata(pdev, iodev);
-
-	init_completion(&info->completion);
-
-	iodev->name = SPEAR_ADC_MOD_NAME;
-	iodev->dev.parent = dev;
-	iodev->info = &spear_adc_iio_info;
-	iodev->modes = INDIO_DIRECT_MODE;
-	iodev->channels = spear_adc_iio_channels;
-	iodev->num_channels = ARRAY_SIZE(spear_adc_iio_channels);
-
-	ret = iio_device_register(iodev);
-	if (ret)
-		goto errout3;
-
-	dev_info(dev, "SPEAR ADC driver loaded, IRQ %d\n", irq);
-
-	return 0;
-
-errout3:
-	clk_disable_unprepare(info->clk);
-errout2:
-	clk_put(info->clk);
-errout1:
-	iounmap(info->adc_base_spear6xx);
-	return ret;
-}
-
-static int spear_adc_remove(struct platform_device *pdev)
-{
-	struct iio_dev *iodev = platform_get_drvdata(pdev);
-	struct spear_adc_info *info = iio_priv(iodev);
-
-	iio_device_unregister(iodev);
-	clk_disable_unprepare(info->clk);
-	clk_put(info->clk);
-	iounmap(info->adc_base_spear6xx);
-
-	return 0;
-}
-
-#ifdef CONFIG_OF
-static const struct of_device_id spear_adc_dt_ids[] = {
-	{ .compatible = "st,spear600-adc", },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, spear_adc_dt_ids);
-#endif
-
-static struct platform_driver spear_adc_driver = {
-	.probe		= spear_adc_probe,
-	.remove		= spear_adc_remove,
-	.driver		= {
-		.name	= SPEAR_ADC_MOD_NAME,
-		.owner	= THIS_MODULE,
-		.of_match_table = of_match_ptr(spear_adc_dt_ids),
-	},
-};
-
-module_platform_driver(spear_adc_driver);
-
-MODULE_AUTHOR("Stefan Roese <[email protected]>");
-MODULE_DESCRIPTION("SPEAr ADC driver");
-MODULE_LICENSE("GPL");
author maintainer
#10 lore raw

Re: [PATCH 4/4] iio:adc:spear_adc move out of staging

Quoted 5 lines · Hartmut Knaack
Quoted 4 lines · Jonathan Cameron

This simple driver is ready to move out of staging.

Stefan, there are some 'what is going on?' questions in here you might want to answer!

Quoted 9 lines
Quoted 9 lines
---
  drivers/iio/adc/Kconfig             |   8 +
  drivers/iio/adc/Makefile            |   1 +
  drivers/iio/adc/spear_adc.c         | 405 ++++++++++++++++++++++++++++++++++++
  drivers/staging/iio/adc/Kconfig     |   8 -
  drivers/staging/iio/adc/Makefile    |   1 -
  drivers/staging/iio/adc/spear_adc.c | 405 ------------------------------------
  6 files changed, 414 insertions(+), 414 deletions(-)
Quoted 6 lines

drivers/iio/adc/Kconfig config NAU7802 · line 164

Quoted 6 lines
drivers/iio/adc/Kconfig +0 -0
index 5553206..2e3e1b0 100644
@@ -164,6 +164,14 @@ config NAU7802
  	  To compile this driver as a module, choose M here: the

s>> module will be called nau7802.

Quoted 27 lines
Quoted 27 lines
drivers/iio/adc/Kconfig +8 -0
+config SPEAR_ADC
+	tristate "ST SPEAr ADC"
+	depends on PLAT_SPEAR || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Say yes here to build support for the integrated ADC inside the
+	  ST SPEAr SoC. Provides direct access via sysfs.
+
  config TI_ADC081C
  	tristate "Texas Instruments ADC081C021/027"
  	depends on I2C
drivers/iio/adc/Makefile +1 -0
index 89f1216..8378fb2 100644
@@ -18,6 +18,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
  obj-$(CONFIG_MCP320X) += mcp320x.o
  obj-$(CONFIG_MCP3422) += mcp3422.o
  obj-$(CONFIG_NAU7802) += nau7802.o
+obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
  obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
drivers/iio/adc/spear_adc.c new file +0 -0
new file mode 100644
index 0000000..18a0a40
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 4

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +4 -0
@@ -0,0 +1,405 @@
+/*
+ * ST SPEAr ADC driver
+ *
+ * Copyright 2012 Stefan Roese <[email protected]>

I found a datasheet at http://www.st.com/st-web-ui/static/active/en/resource/technical/document/reference_manual/DM00034813.pdf

THis is one of a number I think.. Not sure which parts will work out of the box with this driver though... Stefan? What was it written against?

Quoted 36 lines

ADC is covered on pages 594ff.

Quoted 35 lines
drivers/iio/adc/spear_adc.c new file +35 -0
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/completion.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+/* SPEAR registers definitions */
+#define SPEAR600_ADC_SCAN_RATE_LO(x)	((x) & 0xFFFF)
+#define SPEAR600_ADC_SCAN_RATE_HI(x)	(((x) >> 0x10) & 0xFFFF)
+#define SPEAR_ADC_CLK_LOW(x)		(((x) & 0xf) << 0)
+#define SPEAR_ADC_CLK_HIGH(x)		(((x) & 0xf) << 4)
+
+/* Bit definitions for SPEAR_ADC_STATUS */
+#define SPEAR_ADC_STATUS_START_CONVERSION	(1 << 0)
+#define SPEAR_ADC_STATUS_CHANNEL_NUM(x)		((x) << 1)
+#define SPEAR_ADC_STATUS_ADC_ENABLE		(1 << 4)
+#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)		((x) << 5)
+#define SPEAR_ADC_STATUS_VREF_INTERNAL		(1 << 9)
+
+#define SPEAR_ADC_DATA_MASK		0x03ff
+#define SPEAR_ADC_DATA_BITS		10
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 46

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+
+#define SPEAR_ADC_MOD_NAME "spear-adc"
+
+#define SPEAR_ADC_CHANNEL_NUM		8
+
+#define SPEAR_ADC_CLK_MIN			2500000
+#define SPEAR_ADC_CLK_MAX			20000000

Datasheet page 599: "The max frequency of CLK_ADC is 14 MHz as the minimum is 3 MHz"

Err... Stefan?
We might be looking at a part that comes with a range of options...

Also, we have it current written as a sampling frequency attribute. Having dug a little at this I suspect it isn't, but really is refering to the raw clk (and multiple clocks are clearly required per sample)

Quoted 32 lines
Quoted 32 lines
drivers/iio/adc/spear_adc.c new file +32 -0
+
+struct adc_regs_spear3xx {
+	u32 status;
+	u32 average;
+	u32 scan_rate;
+	u32 clk;	/* Not avail for 1340 & 1310 */
+	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+	u32 ch_data[SPEAR_ADC_CHANNEL_NUM];
+};
+
+struct chan_data {
+	u32 lsb;
+	u32 msb;
+};
+
+struct adc_regs_spear6xx {
+	u32 status;
+	u32 pad[2];
+	u32 clk;
+	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+	struct chan_data ch_data[SPEAR_ADC_CHANNEL_NUM];
+	u32 scan_rate_lo;
+	u32 scan_rate_hi;
+	struct chan_data average;
+};
+
+struct spear_adc_info {
+	struct device_node *np;
+	struct adc_regs_spear3xx __iomem *adc_base_spear3xx;
+	struct adc_regs_spear6xx __iomem *adc_base_spear6xx;
+	struct clk *clk;
+	struct completion completion;
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 85

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+	u32 current_clk;
+	u32 sampling_freq;
+	u32 avg_samples;
+	u32 vref_external;
+	u32 value;
+};
+

I would prefer to see some comments like in other drivers, to get an idea what units are assumed (especially for vref_external).

Hmm. Snag here is I have no idea what the units are :)
I'd be tempted to leave this as Stefan, if you have time could you add some docs for this? Whilst nice and something we might ask for in a new driver, I don't think it is a reason to hold up a move from staging.

Quoted 1 line

Also, I would rename it to the scheme, that the majority of the drivers seem to use: struct spear_adc_state (and the instances *st instead of *info).

Hmm. Normally I don't mind naming changes from the conventions that have come about, as long as they aren't confusing... However you make a good point in this particular case as the name chosen clashes with the uses of _info within IIO. Hence next series will include a renaming patch.

Quoted 10 lines

drivers/iio/adc/spear_adc.c line 94

Quoted 9 lines
drivers/iio/adc/spear_adc.c new file +9 -0
+/*
+ * Functions to access some SPEAr ADC register. Abstracted into
+ * static inline functions, because of different register offsets
+ * on different SoC variants (SPEAr300 vs SPEAr600 etc).
+ */
+static void spear_adc_set_status(struct spear_adc_info *info, u32 val)
+{
+	__raw_writel(val, &info->adc_base_spear6xx->status);
+}

Datasheet page 597 says: "It can be written only if both values ADC_STATUS_REG [8] and ADC_STATUS_REG [0] are ‘0’."

So we need conversion_ready and enable to be 0. hmm.

Quoted 1 line

This can be a problem, since bit 0 gets set by spear_read_raw().

I have to say that I find the datasheet for the spear 300 extremely confusing on this point.

We have stated:

The ADC_STATUS_REG is a RW register reporting the ADC status. This register can be written to only if both bit[8], CONVERSION READY, and bit[0], ENABLE, of the same register are set to ‘b0.

Which is fine. However conversion ready once set is not documented as clearing unless a new conversion is started (that is the enable bit is set). Hence once you've have started one reading you can not necessarily ever write to this register. Hence something is clearly 'interesting' in the datasheet.

Stefan, when can you actually write these?

My assumption is that you can do it whenever a conversion is not in flight. As far as I can tell that is fine everywhere here. In the read_raw the bit is set, but then the conversion ready indicates that it is done. I'm really not clear what resets the conversion enable. There is a reference that states that if the power down bit is set then the enable is kept high after a read is finished. So at a quick read, that seems to imply you can't ever write to the register which is nuts...

Quoted 20 lines

drivers/iio/adc/spear_adc.c line 105

Quoted 11 lines
drivers/iio/adc/spear_adc.c new file +11 -0
+
+static void spear_adc_set_clk(struct spear_adc_info *info, u32 val)
+{
+	u32 clk_high, clk_low, count;
+	u32 apb_clk = clk_get_rate(info->clk);
+
+	count = (apb_clk + val - 1) / val;
+	clk_low = count / 2;
+	clk_high = count - clk_low;
+	info->current_clk = apb_clk / count;
+

I've got some doubts about this calculation. The datasheet say on page 599: "...the [adc-]frequency is the APB clock frequency divided by the sum of these values[clk_low and clk_high].". So I would represent it as following:

val = apb_clk / (clk_low + clk_high) = apb_clk / count

So, first thing we want is count, so multiply by count and divide by val:

count = apb_clk / val

The rest seems fine, but it should be kept in mind, that clk_low and clk_high are just 4 bit size, each. So, using u8 would be sufficient (also for count), and clk_low and clk_high should be checked to not exceed a value of 0xF (or count for 0x1F). Thus, changing the function type to int would also help to return an error code.

Quoted 12 lines

drivers/iio/adc/spear_adc.c line 115

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +10 -0
+	__raw_writel(SPEAR_ADC_CLK_LOW(clk_low) | SPEAR_ADC_CLK_HIGH(clk_high),
+		     &info->adc_base_spear6xx->clk);
+}
+
+static void spear_adc_set_ctrl(struct spear_adc_info *info, int n,
+			       u32 val)
+{
+	__raw_writel(val, &info->adc_base_spear6xx->ch_ctrl[n]);
+}
+

Datasheet page 599 says: "It is possible to write these registers only if ENABLE is ‘0’."
Although spear_adc_configure() takes care of that under the current circumstances, it might cause some troubles in case someone makes use of this function any time later. So either check for it here, or leave a comment.

Perhaps a comment makes sense. Lets wait and see if we can work out what is actually going on with this register given the unclear docs.

Quoted 18 lines

drivers/iio/adc/spear_adc.c line 120

Quoted 5 lines
drivers/iio/adc/spear_adc.c new file +5 -0
+static u32 spear_adc_get_average(struct spear_adc_info *info)
+{
+	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
+		return __raw_readl(&info->adc_base_spear6xx->average.msb) &
+			SPEAR_ADC_DATA_MASK;

Datasheet page 601 says: "The AVERAGE_REG_MSB register has latched the word of the conversion ant it can be read only if both values CONVERSION READY and ENABLE are ‘1’." Page 595 says: "At the end of conversion, the CONVERSION READY bit in ADC_STATUS_REG is set and an interrupt signal is generated."
So, since this function is only called during interrupt-handling, it is fine. But maybe also add a comment.

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +10 -0
+	} else {
+		return __raw_readl(&info->adc_base_spear3xx->average) &
+			SPEAR_ADC_DATA_MASK;
+	}
+}
+
+static void spear_adc_set_scanrate(struct spear_adc_info *info, u32 rate)
+{
+	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
+		__raw_writel(SPEAR600_ADC_SCAN_RATE_LO(rate),
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 137

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+			     &info->adc_base_spear6xx->scan_rate_lo);
+		__raw_writel(SPEAR600_ADC_SCAN_RATE_HI(rate),
+			     &info->adc_base_spear6xx->scan_rate_hi);
+	} else {
+		__raw_writel(rate, &info->adc_base_spear3xx->scan_rate);
+	}
+}

Datasheet page 600/601: "This register can be written only if both values ADC_STATUS_REG [8] and ADC_STATUS_REG [0] are ‘0’." Same as above, a comment would be good.

Again, I'd assume you can't write this whilst a conversion is in flight rhater than as is implied by the above statement where I think it is saying you can't do it if a conversion has started or one has finished (thus effectively never).

Quoted 24 lines
Quoted 24 lines
drivers/iio/adc/spear_adc.c new file +24 -0
+
+static int spear_read_raw(struct iio_dev *indio_dev,
+			  struct iio_chan_spec const *chan,
+			  int *val,
+			  int *val2,
+			  long mask)
+{
+	struct spear_adc_info *info = iio_priv(indio_dev);
+	u32 status;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&indio_dev->mlock);
+
+		status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
+			SPEAR_ADC_STATUS_AVG_SAMPLE(info->avg_samples) |
+			SPEAR_ADC_STATUS_START_CONVERSION |
+			SPEAR_ADC_STATUS_ADC_ENABLE;
+		if (info->vref_external == 0)
+			status |= SPEAR_ADC_STATUS_VREF_INTERNAL;
+
+		spear_adc_set_status(info, status);
+		wait_for_completion(&info->completion); /* set by ISR */
+		*val = info->value;
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 168

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+
+		mutex_unlock(&indio_dev->mlock);
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = info->vref_external;

What if internal reference is selected?

Interesting question.... Stefan?

Quoted 1 line

In my opinion, this driver should also be converted to use regulator framework (the boards will certainly contain some regulator, anyway).

The conversion to the regulator framework would require all existing boards to be converted. Whilst nice, it's probably a job for another day and shouldn't be a requirement for this to move out of staging.

Quoted 9 lines

drivers/iio/adc/spear_adc.c line 176

Quoted 8 lines
drivers/iio/adc/spear_adc.c new file +8 -0
+		*val2 = SPEAR_ADC_DATA_BITS;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = info->current_clk;
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;

Catch invalid masks with default: section?

Could do, but really doesn't matter...

Quoted 9 lines
Quoted 9 lines
drivers/iio/adc/spear_adc.c new file +9 -0
+}
+
+static int spear_adc_write_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int val,
+			       int val2,
+			       long mask)
+{
+	struct spear_adc_info *info = iio_priv(indio_dev);
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 192

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+	u32 count;
+	u32 apb_clk = clk_get_rate(info->clk);
+	int ret = 0;
+
+	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
+		return -EINVAL;
+

IMHO using switch(mask) like in the read_raw functions would be more beautiful (though at the cost of a deeper indent level), yet this also does the job.

This actually makes things messier in this case because of the locking. It's much cleaner like this...

Quoted 5 lines

drivers/iio/adc/spear_adc.c line 196

Quoted 4 lines
drivers/iio/adc/spear_adc.c new file +4 -0
+	mutex_lock(&indio_dev->mlock);
+
+	if ((val < SPEAR_ADC_CLK_MIN) ||
+		(val > SPEAR_ADC_CLK_MAX) ||

As mentioned above, I don't know if these values are accurate, or if it would even make more sense to just check that clk_low and clk_high fit into 4 bits, each.

Not a clue. Stefan?

Quoted 9 lines

drivers/iio/adc/spear_adc.c line 204

Quoted 8 lines
drivers/iio/adc/spear_adc.c new file +8 -0
+		(val2 != 0)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	count = (apb_clk + val - 1) / val;
+	info->current_clk = apb_clk / count;
+	spear_adc_set_clk(info, val);

As mentioned in the previous patch, get rid of count and apb_clk here.

Done

Quoted 33 lines
Quoted 33 lines
drivers/iio/adc/spear_adc.c new file +33 -0
+
+out:
+	mutex_unlock(&indio_dev->mlock);
+	return ret;
+}
+
+#define SPEAR_ADC_CHAN(idx) {				\
+	.type = IIO_VOLTAGE,				\
+	.indexed = 1,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
+	.channel = idx,					\
+}
+
+static const struct iio_chan_spec spear_adc_iio_channels[] = {
+	SPEAR_ADC_CHAN(0),
+	SPEAR_ADC_CHAN(1),
+	SPEAR_ADC_CHAN(2),
+	SPEAR_ADC_CHAN(3),
+	SPEAR_ADC_CHAN(4),
+	SPEAR_ADC_CHAN(5),
+	SPEAR_ADC_CHAN(6),
+	SPEAR_ADC_CHAN(7),
+};
+
+static irqreturn_t spear_adc_isr(int irq, void *dev_id)
+{
+	struct spear_adc_info *info = (struct spear_adc_info *)dev_id;
+
+	/* Read value to clear IRQ */
+	info->value = spear_adc_get_average(info);
+	complete(&info->completion);
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 244

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+
+	return IRQ_HANDLED;
+}
+
+static int spear_adc_configure(struct spear_adc_info *info)
+{
+	int i;

u8 i; would also do the job.

General practice is not to do this for iteration variables as the compiler will resize them as appropriate anyway so there is no need to care ;)

Quoted 6 lines

drivers/iio/adc/spear_adc.c line 249

Quoted 5 lines
drivers/iio/adc/spear_adc.c new file +5 -0
+
+	/* Reset ADC core */
+	spear_adc_set_status(info, 0);
+	__raw_writel(0, &info->adc_base_spear6xx->clk);
+	for (i = 0; i < 8; i++)

i < ARRAY_SIZE or i < ADC_CHANNEL_NUM to prevent buffer overflows.

Quoted 12 lines

drivers/iio/adc/spear_adc.c line 260

Quoted 11 lines
drivers/iio/adc/spear_adc.c new file +11 -0
+		spear_adc_set_ctrl(info, i, 0);
+	spear_adc_set_scanrate(info, 0);
+
+	spear_adc_set_clk(info, info->sampling_freq);
+
+	return 0;
+}
+
+static const struct iio_info spear_adc_iio_info = {
+	.read_raw = &spear_read_raw,
+	.write_raw = &spear_adc_write_raw,

Better use the same prefix for those functions (also rename one of them, above).

Good point. Bit odd otherwise. Will add a patch for this as the spear_adc prefix is the dominant one in this driver.

Quoted 10 lines

drivers/iio/adc/spear_adc.c line 269

Quoted 9 lines
drivers/iio/adc/spear_adc.c new file +9 -0
+	.driver_module = THIS_MODULE,
+};
+
+static int spear_adc_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct spear_adc_info *info;
+	struct iio_dev *iodev = NULL;

Not really wrong, but nobody does that these days anymore. And we normally call it indio_dev

Hmm. That did sort of end up as the convention over time. I'll add a patch for this.

Quoted 91 lines
Quoted 91 lines
drivers/iio/adc/spear_adc.c new file +91 -0
+	int ret = -ENODEV;
+	int irq;
+
+	iodev = devm_iio_device_alloc(dev, sizeof(struct spear_adc_info));
+	if (!iodev) {
+		dev_err(dev, "failed allocating iio device\n");
+		return -ENOMEM;
+	}
+
+	info = iio_priv(iodev);
+	info->np = np;
+
+	/*
+	 * SPEAr600 has a different register layout than other SPEAr SoC's
+	 * (e.g. SPEAr3xx). Let's provide two register base addresses
+	 * to support multi-arch kernels.
+	 */
+	info->adc_base_spear6xx = of_iomap(np, 0);
+	if (!info->adc_base_spear6xx) {
+		dev_err(dev, "failed mapping memory\n");
+		return -ENOMEM;
+	}
+	info->adc_base_spear3xx =
+		(struct adc_regs_spear3xx __iomem *)info->adc_base_spear6xx;
+
+	info->clk = clk_get(dev, NULL);
+	if (IS_ERR(info->clk)) {
+		dev_err(dev, "failed getting clock\n");
+		goto errout1;
+	}
+
+	ret = clk_prepare_enable(info->clk);
+	if (ret) {
+		dev_err(dev, "failed enabling clock\n");
+		goto errout2;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0) {
+		dev_err(dev, "failed getting interrupt resource\n");
+		ret = -EINVAL;
+		goto errout3;
+	}
+
+	ret = devm_request_irq(dev, irq, spear_adc_isr, 0, SPEAR_ADC_MOD_NAME,
+			info);
+	if (ret < 0) {
+		dev_err(dev, "failed requesting interrupt\n");
+		goto errout3;
+	}
+
+	if (of_property_read_u32(np, "sampling-frequency",
+				 &info->sampling_freq)) {
+		dev_err(dev, "sampling-frequency missing in DT\n");
+		ret = -EINVAL;
+		goto errout3;
+	}
+
+	/*
+	 * Optional avg_samples defaults to 0, resulting in single data
+	 * conversion
+	 */
+	of_property_read_u32(np, "average-samples", &info->avg_samples);
+
+	/*
+	 * Optional vref_external defaults to 0, resulting in internal vref
+	 * selection
+	 */
+	of_property_read_u32(np, "vref-external", &info->vref_external);
+
+	spear_adc_configure(info);
+
+	platform_set_drvdata(pdev, iodev);
+
+	init_completion(&info->completion);
+
+	iodev->name = SPEAR_ADC_MOD_NAME;
+	iodev->dev.parent = dev;
+	iodev->info = &spear_adc_iio_info;
+	iodev->modes = INDIO_DIRECT_MODE;
+	iodev->channels = spear_adc_iio_channels;
+	iodev->num_channels = ARRAY_SIZE(spear_adc_iio_channels);
+
+	ret = iio_device_register(iodev);
+	if (ret)
+		goto errout3;
+
+	dev_info(dev, "SPEAR ADC driver loaded, IRQ %d\n", irq);
+
+	return 0;
+
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 367

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+errout3:
+	clk_disable_unprepare(info->clk);
+errout2:
+	clk_put(info->clk);
+errout1:
+	iounmap(info->adc_base_spear6xx);
+	return ret;

These labels could have more appropriate names.

This is one of several common conventions. Could be better named, but not worth the patch to do it in my view.

Quoted 23 lines
Quoted 23 lines
drivers/iio/adc/spear_adc.c new file +23 -0
+}
+
+static int spear_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *iodev = platform_get_drvdata(pdev);
+	struct spear_adc_info *info = iio_priv(iodev);
+
+	iio_device_unregister(iodev);
+	clk_disable_unprepare(info->clk);
+	clk_put(info->clk);
+	iounmap(info->adc_base_spear6xx);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id spear_adc_dt_ids[] = {
+	{ .compatible = "st,spear600-adc", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, spear_adc_dt_ids);
+#endif
+
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 397

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+static struct platform_driver spear_adc_driver = {
+	.probe		= spear_adc_probe,
+	.remove		= spear_adc_remove,
+	.driver		= {
+		.name	= SPEAR_ADC_MOD_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(spear_adc_dt_ids),

What if CONFIG_OF is disabled?

That's what of_match_ptr is taking care of.
#define of_match_ptr(_ptr) NULL
People presumably got bored of surrounding this single line case with ifdefs ;)

Quoted 451 lines
Quoted 435 lines
drivers/iio/adc/spear_adc.c new file +8 -0
+	},
+};
+
+module_platform_driver(spear_adc_driver);
+
+MODULE_AUTHOR("Stefan Roese <[email protected]>");
+MODULE_DESCRIPTION("SPEAr ADC driver");
+MODULE_LICENSE("GPL");
drivers/staging/iio/adc/Kconfig +0 -8
index 3633298..a37d001 100644
@@ -126,12 +126,4 @@ config MXS_LRADC
  	  To compile this driver as a module, choose M here: the
  	  module will be called mxs-lradc.
 
-config SPEAR_ADC
-	tristate "ST SPEAr ADC"
-	depends on PLAT_SPEAR || COMPILE_TEST
-	depends on HAS_IOMEM
-	help
-	  Say yes here to build support for the integrated ADC inside the
-	  ST SPEAr SoC. Provides direct access via sysfs.
-
  endmenu
drivers/staging/iio/adc/Makefile +0 -1
index 3e9fb14..b5915c1 100644
@@ -19,4 +19,3 @@ obj-$(CONFIG_AD7192) += ad7192.o
  obj-$(CONFIG_AD7280) += ad7280a.o
  obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
  obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
-obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
drivers/staging/iio/adc/spear_adc.c deleted +0 -395
deleted file mode 100644
index 18a0a40..0000000
@@ -1,405 +0,0 @@
-/*
- * ST SPEAr ADC driver
- *
- * Copyright 2012 Stefan Roese <[email protected]>
- *
- * Licensed under the GPL-2.
- */
-
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/interrupt.h>
-#include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/io.h>
-#include <linux/clk.h>
-#include <linux/err.h>
-#include <linux/completion.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-
-/* SPEAR registers definitions */
-#define SPEAR600_ADC_SCAN_RATE_LO(x)	((x) & 0xFFFF)
-#define SPEAR600_ADC_SCAN_RATE_HI(x)	(((x) >> 0x10) & 0xFFFF)
-#define SPEAR_ADC_CLK_LOW(x)		(((x) & 0xf) << 0)
-#define SPEAR_ADC_CLK_HIGH(x)		(((x) & 0xf) << 4)
-
-/* Bit definitions for SPEAR_ADC_STATUS */
-#define SPEAR_ADC_STATUS_START_CONVERSION	(1 << 0)
-#define SPEAR_ADC_STATUS_CHANNEL_NUM(x)		((x) << 1)
-#define SPEAR_ADC_STATUS_ADC_ENABLE		(1 << 4)
-#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)		((x) << 5)
-#define SPEAR_ADC_STATUS_VREF_INTERNAL		(1 << 9)
-
-#define SPEAR_ADC_DATA_MASK		0x03ff
-#define SPEAR_ADC_DATA_BITS		10
-
-#define SPEAR_ADC_MOD_NAME "spear-adc"
-
-#define SPEAR_ADC_CHANNEL_NUM		8
-
-#define SPEAR_ADC_CLK_MIN			2500000
-#define SPEAR_ADC_CLK_MAX			20000000
-
-struct adc_regs_spear3xx {
-	u32 status;
-	u32 average;
-	u32 scan_rate;
-	u32 clk;	/* Not avail for 1340 & 1310 */
-	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
-	u32 ch_data[SPEAR_ADC_CHANNEL_NUM];
-};
-
-struct chan_data {
-	u32 lsb;
-	u32 msb;
-};
-
-struct adc_regs_spear6xx {
-	u32 status;
-	u32 pad[2];
-	u32 clk;
-	u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
-	struct chan_data ch_data[SPEAR_ADC_CHANNEL_NUM];
-	u32 scan_rate_lo;
-	u32 scan_rate_hi;
-	struct chan_data average;
-};
-
-struct spear_adc_info {
-	struct device_node *np;
-	struct adc_regs_spear3xx __iomem *adc_base_spear3xx;
-	struct adc_regs_spear6xx __iomem *adc_base_spear6xx;
-	struct clk *clk;
-	struct completion completion;
-	u32 current_clk;
-	u32 sampling_freq;
-	u32 avg_samples;
-	u32 vref_external;
-	u32 value;
-};
-
-/*
- * Functions to access some SPEAr ADC register. Abstracted into
- * static inline functions, because of different register offsets
- * on different SoC variants (SPEAr300 vs SPEAr600 etc).
- */
-static void spear_adc_set_status(struct spear_adc_info *info, u32 val)
-{
-	__raw_writel(val, &info->adc_base_spear6xx->status);
-}
-
-static void spear_adc_set_clk(struct spear_adc_info *info, u32 val)
-{
-	u32 clk_high, clk_low, count;
-	u32 apb_clk = clk_get_rate(info->clk);
-
-	count = (apb_clk + val - 1) / val;
-	clk_low = count / 2;
-	clk_high = count - clk_low;
-	info->current_clk = apb_clk / count;
-
-	__raw_writel(SPEAR_ADC_CLK_LOW(clk_low) | SPEAR_ADC_CLK_HIGH(clk_high),
-		     &info->adc_base_spear6xx->clk);
-}
-
-static void spear_adc_set_ctrl(struct spear_adc_info *info, int n,
-			       u32 val)
-{
-	__raw_writel(val, &info->adc_base_spear6xx->ch_ctrl[n]);
-}
-
-static u32 spear_adc_get_average(struct spear_adc_info *info)
-{
-	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
-		return __raw_readl(&info->adc_base_spear6xx->average.msb) &
-			SPEAR_ADC_DATA_MASK;
-	} else {
-		return __raw_readl(&info->adc_base_spear3xx->average) &
-			SPEAR_ADC_DATA_MASK;
-	}
-}
-
-static void spear_adc_set_scanrate(struct spear_adc_info *info, u32 rate)
-{
-	if (of_device_is_compatible(info->np, "st,spear600-adc")) {
-		__raw_writel(SPEAR600_ADC_SCAN_RATE_LO(rate),
-			     &info->adc_base_spear6xx->scan_rate_lo);
-		__raw_writel(SPEAR600_ADC_SCAN_RATE_HI(rate),
-			     &info->adc_base_spear6xx->scan_rate_hi);
-	} else {
-		__raw_writel(rate, &info->adc_base_spear3xx->scan_rate);
-	}
-}
-
-static int spear_read_raw(struct iio_dev *indio_dev,
-			  struct iio_chan_spec const *chan,
-			  int *val,
-			  int *val2,
-			  long mask)
-{
-	struct spear_adc_info *info = iio_priv(indio_dev);
-	u32 status;
-
-	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
-
-		status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
-			SPEAR_ADC_STATUS_AVG_SAMPLE(info->avg_samples) |
-			SPEAR_ADC_STATUS_START_CONVERSION |
-			SPEAR_ADC_STATUS_ADC_ENABLE;
-		if (info->vref_external == 0)
-			status |= SPEAR_ADC_STATUS_VREF_INTERNAL;
-
-		spear_adc_set_status(info, status);
-		wait_for_completion(&info->completion); /* set by ISR */
-		*val = info->value;
-
-		mutex_unlock(&indio_dev->mlock);
-
-		return IIO_VAL_INT;
-
-	case IIO_CHAN_INFO_SCALE:
-		*val = info->vref_external;
-		*val2 = SPEAR_ADC_DATA_BITS;
-		return IIO_VAL_FRACTIONAL_LOG2;
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		*val = info->current_clk;
-		return IIO_VAL_INT;
-	}
-
-	return -EINVAL;
-}
-
-static int spear_adc_write_raw(struct iio_dev *indio_dev,
-			       struct iio_chan_spec const *chan,
-			       int val,
-			       int val2,
-			       long mask)
-{
-	struct spear_adc_info *info = iio_priv(indio_dev);
-	u32 count;
-	u32 apb_clk = clk_get_rate(info->clk);
-	int ret = 0;
-
-	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
-		return -EINVAL;
-
-	mutex_lock(&indio_dev->mlock);
-
-	if ((val < SPEAR_ADC_CLK_MIN) ||
-		(val > SPEAR_ADC_CLK_MAX) ||
-		(val2 != 0)) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	count = (apb_clk + val - 1) / val;
-	info->current_clk = apb_clk / count;
-	spear_adc_set_clk(info, val);
-
-out:
-	mutex_unlock(&indio_dev->mlock);
-	return ret;
-}
-
-#define SPEAR_ADC_CHAN(idx) {				\
-	.type = IIO_VOLTAGE,				\
-	.indexed = 1,					\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
-	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
-	.channel = idx,					\
-}
-
-static const struct iio_chan_spec spear_adc_iio_channels[] = {
-	SPEAR_ADC_CHAN(0),
-	SPEAR_ADC_CHAN(1),
-	SPEAR_ADC_CHAN(2),
-	SPEAR_ADC_CHAN(3),
-	SPEAR_ADC_CHAN(4),
-	SPEAR_ADC_CHAN(5),
-	SPEAR_ADC_CHAN(6),
-	SPEAR_ADC_CHAN(7),
-};
-
-static irqreturn_t spear_adc_isr(int irq, void *dev_id)
-{
-	struct spear_adc_info *info = (struct spear_adc_info *)dev_id;
-
-	/* Read value to clear IRQ */
-	info->value = spear_adc_get_average(info);
-	complete(&info->completion);
-
-	return IRQ_HANDLED;
-}
-
-static int spear_adc_configure(struct spear_adc_info *info)
-{
-	int i;
-
-	/* Reset ADC core */
-	spear_adc_set_status(info, 0);
-	__raw_writel(0, &info->adc_base_spear6xx->clk);
-	for (i = 0; i < 8; i++)
-		spear_adc_set_ctrl(info, i, 0);
-	spear_adc_set_scanrate(info, 0);
-
-	spear_adc_set_clk(info, info->sampling_freq);
-
-	return 0;
-}
-
-static const struct iio_info spear_adc_iio_info = {
-	.read_raw = &spear_read_raw,
-	.write_raw = &spear_adc_write_raw,
-	.driver_module = THIS_MODULE,
-};
-
-static int spear_adc_probe(struct platform_device *pdev)
-{
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
-	struct spear_adc_info *info;
-	struct iio_dev *iodev = NULL;
-	int ret = -ENODEV;
-	int irq;
-
-	iodev = devm_iio_device_alloc(dev, sizeof(struct spear_adc_info));
-	if (!iodev) {
-		dev_err(dev, "failed allocating iio device\n");
-		return -ENOMEM;
-	}
-
-	info = iio_priv(iodev);
-	info->np = np;
-
-	/*
-	 * SPEAr600 has a different register layout than other SPEAr SoC's
-	 * (e.g. SPEAr3xx). Let's provide two register base addresses
-	 * to support multi-arch kernels.
-	 */
-	info->adc_base_spear6xx = of_iomap(np, 0);
-	if (!info->adc_base_spear6xx) {
-		dev_err(dev, "failed mapping memory\n");
-		return -ENOMEM;
-	}
-	info->adc_base_spear3xx =
-		(struct adc_regs_spear3xx __iomem *)info->adc_base_spear6xx;
-
-	info->clk = clk_get(dev, NULL);
-	if (IS_ERR(info->clk)) {
-		dev_err(dev, "failed getting clock\n");
-		goto errout1;
-	}
-
-	ret = clk_prepare_enable(info->clk);
-	if (ret) {
-		dev_err(dev, "failed enabling clock\n");
-		goto errout2;
-	}
-
-	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
-		dev_err(dev, "failed getting interrupt resource\n");
-		ret = -EINVAL;
-		goto errout3;
-	}
-
-	ret = devm_request_irq(dev, irq, spear_adc_isr, 0, SPEAR_ADC_MOD_NAME,
-			info);
-	if (ret < 0) {
-		dev_err(dev, "failed requesting interrupt\n");
-		goto errout3;
-	}
-
-	if (of_property_read_u32(np, "sampling-frequency",
-				 &info->sampling_freq)) {
-		dev_err(dev, "sampling-frequency missing in DT\n");
-		ret = -EINVAL;
-		goto errout3;
-	}
-
-	/*
-	 * Optional avg_samples defaults to 0, resulting in single data
-	 * conversion
-	 */
-	of_property_read_u32(np, "average-samples", &info->avg_samples);
-
-	/*
-	 * Optional vref_external defaults to 0, resulting in internal vref
-	 * selection
-	 */
-	of_property_read_u32(np, "vref-external", &info->vref_external);
-
-	spear_adc_configure(info);
-
-	platform_set_drvdata(pdev, iodev);
-
-	init_completion(&info->completion);
-
-	iodev->name = SPEAR_ADC_MOD_NAME;
-	iodev->dev.parent = dev;
-	iodev->info = &spear_adc_iio_info;
-	iodev->modes = INDIO_DIRECT_MODE;
-	iodev->channels = spear_adc_iio_channels;
-	iodev->num_channels = ARRAY_SIZE(spear_adc_iio_channels);
-
-	ret = iio_device_register(iodev);
-	if (ret)
-		goto errout3;
-
-	dev_info(dev, "SPEAR ADC driver loaded, IRQ %d\n", irq);
-
-	return 0;
-
-errout3:
-	clk_disable_unprepare(info->clk);
-errout2:
-	clk_put(info->clk);
-errout1:
-	iounmap(info->adc_base_spear6xx);
-	return ret;
-}
-
-static int spear_adc_remove(struct platform_device *pdev)
-{
-	struct iio_dev *iodev = platform_get_drvdata(pdev);
-	struct spear_adc_info *info = iio_priv(iodev);
-
-	iio_device_unregister(iodev);
-	clk_disable_unprepare(info->clk);
-	clk_put(info->clk);
-	iounmap(info->adc_base_spear6xx);
-
-	return 0;
-}
-
-#ifdef CONFIG_OF
-static const struct of_device_id spear_adc_dt_ids[] = {
-	{ .compatible = "st,spear600-adc", },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, spear_adc_dt_ids);
-#endif
-
-static struct platform_driver spear_adc_driver = {
-	.probe		= spear_adc_probe,
-	.remove		= spear_adc_remove,
-	.driver		= {
-		.name	= SPEAR_ADC_MOD_NAME,

drivers/staging/iio/adc/spear_adc.c line 405

Quoted 10 lines
drivers/staging/iio/adc/spear_adc.c deleted +0 -10
-		.owner	= THIS_MODULE,
-		.of_match_table = of_match_ptr(spear_adc_dt_ids),
-	},
-};
-
-module_platform_driver(spear_adc_driver);
-
-MODULE_AUTHOR("Stefan Roese <[email protected]>");
-MODULE_DESCRIPTION("SPEAr ADC driver");
-MODULE_LICENSE("GPL");

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#11 lore raw

Re: [PATCH 4/4] iio:adc:spear_adc move out of staging

Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · Hartmut Knaack
Quoted 4 lines · Jonathan Cameron

This simple driver is ready to move out of staging.

Stefan, there are some 'what is going on?' questions in here you might want to answer!

I'll give it a try.

Quoted 56 lines
Quoted 11 lines
Quoted 11 lines
---
  drivers/iio/adc/Kconfig             |   8 +
  drivers/iio/adc/Makefile            |   1 +
  drivers/iio/adc/spear_adc.c         | 405
++++++++++++++++++++++++++++++++++++
  drivers/staging/iio/adc/Kconfig     |   8 -
  drivers/staging/iio/adc/Makefile    |   1 -
  drivers/staging/iio/adc/spear_adc.c | 405
------------------------------------
  6 files changed, 414 insertions(+), 414 deletions(-)
Quoted 6 lines

drivers/iio/adc/Kconfig config NAU7802 · line 164

Quoted 6 lines
drivers/iio/adc/Kconfig +0 -0
index 5553206..2e3e1b0 100644
@@ -164,6 +164,14 @@ config NAU7802
        To compile this driver as a module, choose M here: the

s>> module will be called nau7802.

Quoted 28 lines
Quoted 28 lines
drivers/iio/adc/Kconfig +8 -0
+config SPEAR_ADC
+    tristate "ST SPEAr ADC"
+    depends on PLAT_SPEAR || COMPILE_TEST
+    depends on HAS_IOMEM
+    help
+      Say yes here to build support for the integrated ADC inside the
+      ST SPEAr SoC. Provides direct access via sysfs.
+
  config TI_ADC081C
      tristate "Texas Instruments ADC081C021/027"
      depends on I2C
drivers/iio/adc/Makefile +1 -0
index 89f1216..8378fb2 100644
@@ -18,6 +18,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
  obj-$(CONFIG_MCP320X) += mcp320x.o
  obj-$(CONFIG_MCP3422) += mcp3422.o
  obj-$(CONFIG_NAU7802) += nau7802.o
+obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
  obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
drivers/iio/adc/spear_adc.c new file +0 -1
new file mode 100644
index 0000000..18a0a40
--- /dev/null
Quoted 9 lines

drivers/iio/adc/spear_adc.c line 4

Quoted 6 lines

+++ b/drivers/iio/adc/spear_adc.c

drivers/iio/adc/spear_adc.c new file +4 -0
@@ -0,0 +1,405 @@
+/*
+ * ST SPEAr ADC driver
+ *
+ * Copyright 2012 Stefan Roese <[email protected]>

I found a datasheet at
http://www.st.com/st-web-ui/static/active/en/resource/technical/document/reference_manual/DM00034813.pdf

Quoted 3 lines

THis is one of a number I think.. Not sure which parts will work out of the box
with this driver though... Stefan? What was it written against?

I wrote and tested this driver for the SPEAr600 in 2011/2012. The manual mentioned above didn't exist at that time. I used one called "UM510_rev3.0.pdf" from Feb 2011. If interested I can send you the manual I used at that time.

I don't have access to the SPEAr600 hardware anymore. I'm afraid but I can't be of much help here.

Thanks,
Stefan

author maintainer
#12 lore raw

Re: [PATCH 4/4] iio:adc:spear_adc move out of staging

Quoted 68 lines · Stefan Roese
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · Hartmut Knaack
Quoted 4 lines · Jonathan Cameron

This simple driver is ready to move out of staging.

Stefan, there are some 'what is going on?' questions in here you might want to answer!

I'll give it a try.

Quoted 56 lines
Quoted 11 lines
Quoted 11 lines
---
  drivers/iio/adc/Kconfig             |   8 +
  drivers/iio/adc/Makefile            |   1 +
  drivers/iio/adc/spear_adc.c         | 405
++++++++++++++++++++++++++++++++++++
  drivers/staging/iio/adc/Kconfig     |   8 -
  drivers/staging/iio/adc/Makefile    |   1 -
  drivers/staging/iio/adc/spear_adc.c | 405
------------------------------------
  6 files changed, 414 insertions(+), 414 deletions(-)
Quoted 6 lines

drivers/iio/adc/Kconfig config NAU7802 · line 164

Quoted 6 lines
drivers/iio/adc/Kconfig +0 -0
index 5553206..2e3e1b0 100644
@@ -164,6 +164,14 @@ config NAU7802
        To compile this driver as a module, choose M here: the

s>> module will be called nau7802.

Quoted 37 lines
Quoted 30 lines
drivers/iio/adc/Kconfig +8 -0
+config SPEAR_ADC
+    tristate "ST SPEAr ADC"
+    depends on PLAT_SPEAR || COMPILE_TEST
+    depends on HAS_IOMEM
+    help
+      Say yes here to build support for the integrated ADC inside the
+      ST SPEAr SoC. Provides direct access via sysfs.
+
  config TI_ADC081C
      tristate "Texas Instruments ADC081C021/027"
      depends on I2C
drivers/iio/adc/Makefile +1 -0
index 89f1216..8378fb2 100644
@@ -18,6 +18,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
  obj-$(CONFIG_MCP320X) += mcp320x.o
  obj-$(CONFIG_MCP3422) += mcp3422.o
  obj-$(CONFIG_NAU7802) += nau7802.o
+obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
  obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
drivers/iio/adc/spear_adc.c new file +0 -0
new file mode 100644
index 0000000..18a0a40

drivers/iio/adc/spear_adc.c line 4

Quoted 4 lines
drivers/iio/adc/spear_adc.c new file +4 -0
@@ -0,0 +1,405 @@
+/*
+ * ST SPEAr ADC driver
+ *
+ * Copyright 2012 Stefan Roese <[email protected]>

I found a datasheet at
http://www.st.com/st-web-ui/static/active/en/resource/technical/document/reference_manual/DM00034813.pdf

Quoted 8 lines
Quoted 3 lines

THis is one of a number I think.. Not sure which parts will work out of the box
with this driver though... Stefan? What was it written against?

I wrote and tested this driver for the SPEAr600 in 2011/2012. The manual mentioned above didn't exist at that time. I used one called "UM510_rev3.0.pdf" from Feb 2011. If interested I can send you the manual I used at that time.

I don't have access to the SPEAr600 hardware anymore. I'm afraid but I can't be of much help here.

I'm tempted to say that, given it worked for stefan back then, we do what non functional clean ups make sense and push the driver out of staging whilst avoiding the issues highlighted by the 'interesting' datasheets.

Either that, or if someone else wants to take on getting clarifications from ST on how it actually works, that is fine by me.

J

Quoted 2 lines

Thanks,
Stefan

#13 lore raw

Re: [PATCH 4/4] iio:adc:spear_adc move out of staging

Quoted 73 lines · Jonathan Cameron
Quoted 65 lines · Stefan Roese
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · Hartmut Knaack
Quoted 4 lines · Jonathan Cameron

This simple driver is ready to move out of staging.

Stefan, there are some 'what is going on?' questions in here you might want to answer!

I'll give it a try.

Quoted 54 lines
Quoted 11 lines
Quoted 11 lines
---
  drivers/iio/adc/Kconfig             |   8 +
  drivers/iio/adc/Makefile            |   1 +
  drivers/iio/adc/spear_adc.c         | 405
++++++++++++++++++++++++++++++++++++
  drivers/staging/iio/adc/Kconfig     |   8 -
  drivers/staging/iio/adc/Makefile    |   1 -
  drivers/staging/iio/adc/spear_adc.c | 405
------------------------------------
  6 files changed, 414 insertions(+), 414 deletions(-)
Quoted 6 lines

drivers/iio/adc/Kconfig config NAU7802 · line 164

Quoted 6 lines
drivers/iio/adc/Kconfig +0 -0
index 5553206..2e3e1b0 100644
@@ -164,6 +164,14 @@ config NAU7802
        To compile this driver as a module, choose M here: the

s>> module will be called nau7802.

Quoted 36 lines
Quoted 29 lines
drivers/iio/adc/Kconfig +8 -0
+config SPEAR_ADC
+    tristate "ST SPEAr ADC"
+    depends on PLAT_SPEAR || COMPILE_TEST
+    depends on HAS_IOMEM
+    help
+      Say yes here to build support for the integrated ADC inside the
+      ST SPEAr SoC. Provides direct access via sysfs.
+
  config TI_ADC081C
      tristate "Texas Instruments ADC081C021/027"
      depends on I2C
drivers/iio/adc/Makefile +1 -0
index 89f1216..8378fb2 100644
@@ -18,6 +18,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
  obj-$(CONFIG_MCP320X) += mcp320x.o
  obj-$(CONFIG_MCP3422) += mcp3422.o
  obj-$(CONFIG_NAU7802) += nau7802.o
+obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
  obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
drivers/iio/adc/spear_adc.c new file +0 -0
new file mode 100644
index 0000000..18a0a40

drivers/iio/adc/spear_adc.c line 4

Quoted 4 lines
drivers/iio/adc/spear_adc.c new file +4 -0
@@ -0,0 +1,405 @@
+/*
+ * ST SPEAr ADC driver
+ *
+ * Copyright 2012 Stefan Roese <[email protected]>

I found a datasheet at
http://www.st.com/st-web-ui/static/active/en/resource/technical/document/reference_manual/DM00034813.pdf

Quoted 7 lines
Quoted 3 lines

THis is one of a number I think.. Not sure which parts will work out of the box
with this driver though... Stefan? What was it written against?

I wrote and tested this driver for the SPEAr600 in 2011/2012. The manual mentioned above didn't exist at that time. I used one called "UM510_rev3.0.pdf" from Feb 2011. If interested I can send you the manual I used at that time.

I don't have access to the SPEAr600 hardware anymore. I'm afraid but I can't be of much help here.

Quoted 8 lines

I'm tempted to say that, given it worked for stefan back then, we do what non functional clean ups make sense and push the driver out of staging whilst avoiding the issues highlighted by the 'interesting' datasheets.

Either that, or if someone else wants to take on getting clarifications from ST on how it actually works, that is fine by me.

J

I would agree to fix the issues which obviously violate the datasheet (get rid of SPEAR_ADC_CLK_MIN/MAX, rework calculation of count in spear_adc_set_clk and check boundaries of clk_low and clk_high, make spear_read_raw aware of the currently selected reference voltage), and just place comments about required set/cleared bits on top of those functions, where the datasheet is confusing (spear_adc_set_status, spear_adc_set_ctrl, spear_adc_get_average, spear_adc_set_scanrate). Now, that I looked over it again, I would also propose to move the content of spear_adc_set_clk back into spear_write_raw, where it seems to have been located originally, and get rid of it afterwards (it doesn't do anything fancy, anyway). Sorry for bringing that up so late.
Btw: during the last weeks, I did some review of about 75% of the stable adc drivers and piled up about 50 cleanup-patches so far, that I would send out soon. I also plan to do the same on the staging adc drivers afterwards. So, if you can be patient, I might do already a lot of cleanup, that you don't like to waste your time on ;-)
Thanks,

Hartmut

Quoted 7 lines
Quoted 2 lines

Thanks,
Stefan

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author maintainer
#14 lore raw

Re: [PATCH 4/4] iio:adc:spear_adc move out of staging

Quoted 84 lines · Hartmut Knaack
Quoted 78 lines · Jonathan Cameron
Quoted 65 lines · Stefan Roese
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · Hartmut Knaack
Quoted 4 lines · Jonathan Cameron

This simple driver is ready to move out of staging.

Stefan, there are some 'what is going on?' questions in here you might want to answer!

I'll give it a try.

Quoted 54 lines
Quoted 11 lines
Quoted 11 lines
---
   drivers/iio/adc/Kconfig             |   8 +
   drivers/iio/adc/Makefile            |   1 +
   drivers/iio/adc/spear_adc.c         | 405
++++++++++++++++++++++++++++++++++++
   drivers/staging/iio/adc/Kconfig     |   8 -
   drivers/staging/iio/adc/Makefile    |   1 -
   drivers/staging/iio/adc/spear_adc.c | 405
------------------------------------
   6 files changed, 414 insertions(+), 414 deletions(-)
Quoted 6 lines

drivers/iio/adc/Kconfig config NAU7802 · line 164

Quoted 6 lines
drivers/iio/adc/Kconfig +0 -0
index 5553206..2e3e1b0 100644
@@ -164,6 +164,14 @@ config NAU7802
         To compile this driver as a module, choose M here: the

s>> module will be called nau7802.

Quoted 36 lines
Quoted 29 lines
drivers/iio/adc/Kconfig +8 -0
+config SPEAR_ADC
+    tristate "ST SPEAr ADC"
+    depends on PLAT_SPEAR || COMPILE_TEST
+    depends on HAS_IOMEM
+    help
+      Say yes here to build support for the integrated ADC inside the
+      ST SPEAr SoC. Provides direct access via sysfs.
+
   config TI_ADC081C
       tristate "Texas Instruments ADC081C021/027"
       depends on I2C
drivers/iio/adc/Makefile +1 -0
index 89f1216..8378fb2 100644
@@ -18,6 +18,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
   obj-$(CONFIG_MCP320X) += mcp320x.o
   obj-$(CONFIG_MCP3422) += mcp3422.o
   obj-$(CONFIG_NAU7802) += nau7802.o
+obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
   obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
   obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
   obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
drivers/iio/adc/spear_adc.c new file +0 -0
new file mode 100644
index 0000000..18a0a40

drivers/iio/adc/spear_adc.c line 4

Quoted 4 lines
drivers/iio/adc/spear_adc.c new file +4 -0
@@ -0,0 +1,405 @@
+/*
+ * ST SPEAr ADC driver
+ *
+ * Copyright 2012 Stefan Roese <[email protected]>

I found a datasheet at
http://www.st.com/st-web-ui/static/active/en/resource/technical/document/reference_manual/DM00034813.pdf

Quoted 7 lines
Quoted 3 lines

THis is one of a number I think.. Not sure which parts will work out of the box
with this driver though... Stefan? What was it written against?

I wrote and tested this driver for the SPEAr600 in 2011/2012. The manual mentioned above didn't exist at that time. I used one called "UM510_rev3.0.pdf" from Feb 2011. If interested I can send you the manual I used at that time.

I don't have access to the SPEAr600 hardware anymore. I'm afraid but I can't be of much help here.

I'm tempted to say that, given it worked for stefan back then, we do what non functional clean ups make sense and push the driver out of staging whilst avoiding the issues highlighted by the 'interesting' datasheets.

Quoted 5 lines

Either that, or if someone else wants to take on getting clarifications from ST on how it actually works, that is fine by me.

J

Quoted 12 lines

I would agree to fix the issues which obviously violate the datasheet (get rid of SPEAR_ADC_CLK_MIN/MAX, rework calculation of count in spear_adc_set_clk and check boundaries of clk_low and clk_high, make spear_read_raw aware of the currently selected reference voltage), and just place comments about required set/cleared bits on top of those functions, where the datasheet is confusing (spear_adc_set_status, spear_adc_set_ctrl, spear_adc_get_average, spear_adc_set_scanrate). Now, that I looked over it again, I would also propose to move the content of spear_adc_set_clk back into spear_write_raw, where it seems to have been located originally, and get rid of it afterwards (it doesn't do anything fancy, anyway). Sorry for bringing that up so late.

That's fine. What I'll probably do is to post the existing patches without the one moving the driver out of staging.
Lets get those in then deal with these remaining bits and bobs in a new series.

Quoted 6 lines

Btw: during the last weeks, I did
some review of about 75% of the stable adc drivers and piled up about 50 cleanup-patches so far, that I would send out soon. I also plan to do the same on the staging adc drivers afterwards. So, if you can be patient, I might do already a lot of cleanup, that you don't like to waste your time on ;-)

Cool. If you want to send patches out in batches, it does make things a little more manageable! Doesn't matter if not, I'll just review and apply them in batches ;)

J

Quoted 15 lines

Thanks,

Hartmut

Quoted 6 lines
Quoted 1 line

Thanks, Stefan

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author maintainer
#15 lore raw

Re: [PATCH 4/4] iio:adc:spear_adc move out of staging

In reply to Jonathan Cameron
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · Hartmut Knaack
Quoted 4 lines · Jonathan Cameron

This simple driver is ready to move out of staging.

Stefan, there are some 'what is going on?' questions in here you might want to answer!

It's been quite a while, but I'd like to make progress on getting this one out of staging.

I'm not necessarily convinced we have to get 'everything' right before we do. The alternative is to decide to drop the driver. I don't want to continue limping on with it it in staging for ever!

Anyhow, this email had the review Harmut did in it so I'll run through this step by step.

There were a few more emails in the thread, but they cut chunks of the code out.

I think having read this I'm just going to propose leaving it as is and moving out of staging.

If Harmut or anyone else wants to take another look that would of course be great!

Jonathan

Quoted 52 lines
Quoted 9 lines
Quoted 9 lines
---
  drivers/iio/adc/Kconfig             |   8 +
  drivers/iio/adc/Makefile            |   1 +
  drivers/iio/adc/spear_adc.c         | 405 ++++++++++++++++++++++++++++++++++++
  drivers/staging/iio/adc/Kconfig     |   8 -
  drivers/staging/iio/adc/Makefile    |   1 -
  drivers/staging/iio/adc/spear_adc.c | 405 ------------------------------------
  6 files changed, 414 insertions(+), 414 deletions(-)
Quoted 6 lines

drivers/iio/adc/Kconfig config NAU7802 · line 164

Quoted 6 lines
drivers/iio/adc/Kconfig +0 -0
index 5553206..2e3e1b0 100644
@@ -164,6 +164,14 @@ config NAU7802
        To compile this driver as a module, choose M here: the

s>> module will be called nau7802.

Quoted 27 lines
Quoted 27 lines
drivers/iio/adc/Kconfig +8 -0
+config SPEAR_ADC
+    tristate "ST SPEAr ADC"
+    depends on PLAT_SPEAR || COMPILE_TEST
+    depends on HAS_IOMEM
+    help
+      Say yes here to build support for the integrated ADC inside the
+      ST SPEAr SoC. Provides direct access via sysfs.
+
  config TI_ADC081C
      tristate "Texas Instruments ADC081C021/027"
      depends on I2C
drivers/iio/adc/Makefile +1 -0
index 89f1216..8378fb2 100644
@@ -18,6 +18,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
  obj-$(CONFIG_MCP320X) += mcp320x.o
  obj-$(CONFIG_MCP3422) += mcp3422.o
  obj-$(CONFIG_NAU7802) += nau7802.o
+obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
  obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
drivers/iio/adc/spear_adc.c new file +0 -0
new file mode 100644
index 0000000..18a0a40
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 4

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +4 -0
@@ -0,0 +1,405 @@
+/*
+ * ST SPEAr ADC driver
+ *
+ * Copyright 2012 Stefan Roese <[email protected]>

I found a datasheet at http://www.st.com/st-web-ui/static/active/en/resource/technical/document/reference_manual/DM00034813.pdf

Quoted 2 lines

THis is one of a number I think.. Not sure which parts will work out of the box with this driver though... Stefan? What was it written against?

Stefan replied
I wrote and tested this driver for the SPEAr600 in 2011/2012. The manual mentioned above didn't exist at that time. I used one called "UM510_rev3.0.pdf" from Feb 2011. If interested I can send you the manual I used at that time.

I don't have access to the SPEAr600 hardware anymore. I'm afraid but I can't be of much help here.

So I'm inclined to think we should stick with known working values unless someone comes out of the woodwork to say they have one.

Quoted 44 lines
Quoted 36 lines

ADC is covered on pages 594ff.

Quoted 35 lines
drivers/iio/adc/spear_adc.c new file +35 -0
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/completion.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+/* SPEAR registers definitions */
+#define SPEAR600_ADC_SCAN_RATE_LO(x)    ((x) & 0xFFFF)
+#define SPEAR600_ADC_SCAN_RATE_HI(x)    (((x) >> 0x10) & 0xFFFF)
+#define SPEAR_ADC_CLK_LOW(x)        (((x) & 0xf) << 0)
+#define SPEAR_ADC_CLK_HIGH(x)        (((x) & 0xf) << 4)
+
+/* Bit definitions for SPEAR_ADC_STATUS */
+#define SPEAR_ADC_STATUS_START_CONVERSION    (1 << 0)
+#define SPEAR_ADC_STATUS_CHANNEL_NUM(x)        ((x) << 1)
+#define SPEAR_ADC_STATUS_ADC_ENABLE        (1 << 4)
+#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)        ((x) << 5)
+#define SPEAR_ADC_STATUS_VREF_INTERNAL        (1 << 9)
+
+#define SPEAR_ADC_DATA_MASK        0x03ff
+#define SPEAR_ADC_DATA_BITS        10
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 46

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+
+#define SPEAR_ADC_MOD_NAME "spear-adc"
+
+#define SPEAR_ADC_CHANNEL_NUM        8
+
+#define SPEAR_ADC_CLK_MIN            2500000
+#define SPEAR_ADC_CLK_MAX            20000000

Datasheet page 599: "The max frequency of CLK_ADC is 14 MHz as the minimum is 3 MHz"

Quoted 6 lines

Err... Stefan?
We might be looking at a part that comes with a range of options...

Also, we have it current written as a sampling frequency attribute. Having dug a little at this I suspect it isn't, but really is refering to the raw clk (and multiple clocks are clearly required per sample)

Not sure what we do about this one.

Can attempt to work it out from the datasheets I guess.

Quoted 42 lines
Quoted 34 lines
Quoted 34 lines
drivers/iio/adc/spear_adc.c new file +32 -0
+
+struct adc_regs_spear3xx {
+    u32 status;
+    u32 average;
+    u32 scan_rate;
+    u32 clk;    /* Not avail for 1340 & 1310 */
+    u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+    u32 ch_data[SPEAR_ADC_CHANNEL_NUM];
+};
+
+struct chan_data {
+    u32 lsb;
+    u32 msb;
+};
+
+struct adc_regs_spear6xx {
+    u32 status;
+    u32 pad[2];
+    u32 clk;
+    u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
+    struct chan_data ch_data[SPEAR_ADC_CHANNEL_NUM];
+    u32 scan_rate_lo;
+    u32 scan_rate_hi;
+    struct chan_data average;
+};
+
+struct spear_adc_info {
+    struct device_node *np;
+    struct adc_regs_spear3xx __iomem *adc_base_spear3xx;
+    struct adc_regs_spear6xx __iomem *adc_base_spear6xx;
+    struct clk *clk;
+    struct completion completion;
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 85

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+    u32 current_clk;
+    u32 sampling_freq;
+    u32 avg_samples;
+    u32 vref_external;
+    u32 value;
+};
+

I would prefer to see some comments like in other drivers, to get an idea what units are assumed (especially for vref_external).

Quoted 4 lines

Hmm. Snag here is I have no idea what the units are :)
I'd be tempted to leave this as Stefan, if you have time could you add some docs for this? Whilst nice and something we might ask for in a new driver, I don't think it is a reason to hold up a move from staging.

I'm inclined to leave this one as is. Not ideal, but kind of where we end up with old drivers.

Quoted 7 lines
Quoted 2 lines

Also, I would rename it to the scheme, that the majority of the drivers seem to use: struct spear_adc_state (and the instances *st instead of *info).

Hmm. Normally I don't mind naming changes from the conventions that have come about, as long as they aren't confusing... However you make a good point in this particular case as the name chosen clashes with the uses of _info within IIO. Hence next series will include a renaming patch.

This has been dealt with.

Quoted 31 lines
Quoted 11 lines

drivers/iio/adc/spear_adc.c line 94

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +9 -0
+/*
+ * Functions to access some SPEAr ADC register. Abstracted into
+ * static inline functions, because of different register offsets
+ * on different SoC variants (SPEAr300 vs SPEAr600 etc).
+ */
+static void spear_adc_set_status(struct spear_adc_info *info, u32 val)
+{
+    __raw_writel(val, &info->adc_base_spear6xx->status);
+}

Datasheet page 597 says: "It can be written only if both values ADC_STATUS_REG [8] and ADC_STATUS_REG [0] are ‘0’."

So we need conversion_ready and enable to be 0. hmm.

Quoted 1 line

This can be a problem, since bit 0 gets set by spear_read_raw().

I have to say that I find the datasheet for the spear 300 extremely confusing on this point.

We have stated:

The ADC_STATUS_REG is a RW register reporting the ADC status. This register can be written to only if both bit[8], CONVERSION READY, and bit[0], ENABLE, of the same register are set to ‘b0.

Which is fine. However conversion ready once set is not documented as clearing unless a new conversion is started (that is the enable bit is set). Hence once you've have started one reading you can not necessarily ever write to this register. Hence something is clearly 'interesting' in the datasheet.

Stefan, when can you actually write these?

Quoted 7 lines

My assumption is that you can do it whenever a conversion is not in flight. As far as I can tell that is fine everywhere here. In the read_raw the bit is set, but then the conversion ready indicates that it is done. I'm really not clear what resets the conversion enable. There is a reference that states that if the power down bit is set then the enable is kept high after a read is finished. So at a quick read, that seems to imply you can't ever write to the register which is nuts...

Shall we conclude datasheet wrong / confusing and leave this stuff as is?

Quoted 17 lines
Quoted 17 lines

drivers/iio/adc/spear_adc.c line 105

Quoted 12 lines
drivers/iio/adc/spear_adc.c new file +11 -0
+
+static void spear_adc_set_clk(struct spear_adc_info *info, u32 val)
+{
+    u32 clk_high, clk_low, count;
+    u32 apb_clk = clk_get_rate(info->clk);
+
+    count = (apb_clk + val - 1) / val;
+    clk_low = count / 2;
+    clk_high = count - clk_low;
+    info->current_clk = apb_clk / count;
+

I've got some doubts about this calculation. The datasheet say on page 599: "...the [adc-]frequency is the APB clock frequency divided by the sum of these values[clk_low and clk_high].". So I would represent it as following:

Quoted 6 lines
Quoted 6 lines

val = apb_clk / (clk_low + clk_high) = apb_clk / count

So, first thing we want is count, so multiply by count and divide by val:

count = apb_clk / val

I'm lost. Hartmut, I know this was a while ago, but I don't suppose you can remember / figure out what you meant and propose a patch? Actually I think this one was done as the code is different.

Quoted 20 lines
Quoted 8 lines

The rest seems fine, but it should be kept in mind, that clk_low and clk_high are just 4 bit size, each. So, using u8 would be sufficient (also for count), and clk_low and clk_high should be checked to not exceed a value of 0xF (or count for 0x1F). Thus, changing the function type to int would also help to return an error code.

+ __raw_writel(SPEAR_ADC_CLK_LOW(clk_low) | SPEAR_ADC_CLK_HIGH(clk_high),

Quoted 11 lines

drivers/iio/adc/spear_adc.c line 115

Quoted 9 lines
drivers/iio/adc/spear_adc.c new file +9 -0
+             &info->adc_base_spear6xx->clk);
+}
+
+static void spear_adc_set_ctrl(struct spear_adc_info *info, int n,
+                   u32 val)
+{
+    __raw_writel(val, &info->adc_base_spear6xx->ch_ctrl[n]);
+}
+

Datasheet page 599 says: "It is possible to write these registers only if ENABLE is ‘0’."
Although spear_adc_configure() takes care of that under the current circumstances, it might cause some troubles in case someone makes use of this function any time later. So either check for it here, or leave a comment.

Quoted 2 lines

Perhaps a comment makes sense. Lets wait and see if we can work out what is actually going on with this register given the unclear docs.

I'm inclined to ignore this one as datasheet garbage.

Quoted 73 lines
Quoted 26 lines

drivers/iio/adc/spear_adc.c line 120

Quoted 6 lines
drivers/iio/adc/spear_adc.c new file +5 -0
+static u32 spear_adc_get_average(struct spear_adc_info *info)
+{
+    if (of_device_is_compatible(info->np, "st,spear600-adc")) {
+        return __raw_readl(&info->adc_base_spear6xx->average.msb) &
+            SPEAR_ADC_DATA_MASK;

Datasheet page 601 says: "The AVERAGE_REG_MSB register has latched the word of the conversion ant it can be read only if both values CONVERSION READY and ENABLE are ‘1’." Page 595 says: "At the end of conversion, the CONVERSION READY bit in ADC_STATUS_REG is set and an interrupt signal is generated." So, since this function is only called during interrupt-handling, it is fine. But maybe also add a comment.

Quoted 12 lines
drivers/iio/adc/spear_adc.c new file +12 -0
+    } else {
+        return __raw_readl(&info->adc_base_spear3xx->average) &
+            SPEAR_ADC_DATA_MASK;
+    }
+}
+
+static void spear_adc_set_scanrate(struct spear_adc_info *info, u32 rate)
+{
+    if (of_device_is_compatible(info->np, "st,spear600-adc")) {
+        __raw_writel(SPEAR600_ADC_SCAN_RATE_LO(rate),
+                 &info->adc_base_spear6xx->scan_rate_lo);
+        __raw_writel(SPEAR600_ADC_SCAN_RATE_HI(rate),
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 137

Quoted 5 lines
drivers/iio/adc/spear_adc.c new file +5 -0
+                 &info->adc_base_spear6xx->scan_rate_hi);
+    } else {
+        __raw_writel(rate, &info->adc_base_spear3xx->scan_rate);
+    }
+}

Datasheet page 600/601: "This register can be written only if both values ADC_STATUS_REG [8] and ADC_STATUS_REG [0] are ‘0’." Same as above, a comment would be good.

Again, I'd assume you can't write this whilst a conversion is in flight rhater than as is implied by the above statement where I think it is saying you can't do it if a conversion has started or one has finished (thus effectively never).

Quoted 24 lines
Quoted 24 lines
drivers/iio/adc/spear_adc.c new file +24 -0
+
+static int spear_read_raw(struct iio_dev *indio_dev,
+              struct iio_chan_spec const *chan,
+              int *val,
+              int *val2,
+              long mask)
+{
+    struct spear_adc_info *info = iio_priv(indio_dev);
+    u32 status;
+
+    switch (mask) {
+    case IIO_CHAN_INFO_RAW:
+        mutex_lock(&indio_dev->mlock);
+
+        status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
+            SPEAR_ADC_STATUS_AVG_SAMPLE(info->avg_samples) |
+            SPEAR_ADC_STATUS_START_CONVERSION |
+            SPEAR_ADC_STATUS_ADC_ENABLE;
+        if (info->vref_external == 0)
+            status |= SPEAR_ADC_STATUS_VREF_INTERNAL;
+
+        spear_adc_set_status(info, status);
+        wait_for_completion(&info->completion); /* set by ISR */
+        *val = info->value;
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 168

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+
+        mutex_unlock(&indio_dev->mlock);
+
+        return IIO_VAL_INT;
+
+    case IIO_CHAN_INFO_SCALE:
+        *val = info->vref_external;

What if internal reference is selected?

Quoted 5 lines

Interesting question.... Stefan?

Quoted 1 line

In my opinion, this driver should also be converted to use regulator framework (the boards will certainly contain some regulator, anyway).

The conversion to the regulator framework would require all existing boards to be converted. Whilst nice, it's probably a job for another day and shouldn't be a requirement for this to move out of staging.

I'll stick to that view. Nice to have - not necessary.

Quoted 32 lines
Quoted 11 lines

drivers/iio/adc/spear_adc.c line 176

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +8 -0
+        *val2 = SPEAR_ADC_DATA_BITS;
+        return IIO_VAL_FRACTIONAL_LOG2;
+    case IIO_CHAN_INFO_SAMP_FREQ:
+        *val = info->current_clk;
+        return IIO_VAL_INT;
+    }
+
+    return -EINVAL;

Catch invalid masks with default: section?

Could do, but really doesn't matter...

Quoted 11 lines
Quoted 11 lines
drivers/iio/adc/spear_adc.c new file +11 -0
+}
+
+static int spear_adc_write_raw(struct iio_dev *indio_dev,
+                   struct iio_chan_spec const *chan,
+                   int val,
+                   int val2,
+                   long mask)
+{
+    struct spear_adc_info *info = iio_priv(indio_dev);
+    u32 count;
+    u32 apb_clk = clk_get_rate(info->clk);
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 192

Quoted 5 lines
drivers/iio/adc/spear_adc.c new file +5 -0
+    int ret = 0;
+
+    if (mask != IIO_CHAN_INFO_SAMP_FREQ)
+        return -EINVAL;
+

IMHO using switch(mask) like in the read_raw functions would be more beautiful (though at the cost of a deeper indent level), yet this also does the job.

Quoted 8 lines

This actually makes things messier in this case because of the locking. It's much cleaner like this...

Quoted 5 lines

drivers/iio/adc/spear_adc.c line 196

Quoted 4 lines
drivers/iio/adc/spear_adc.c new file +4 -0
+    mutex_lock(&indio_dev->mlock);
+
+    if ((val < SPEAR_ADC_CLK_MIN) ||
+        (val > SPEAR_ADC_CLK_MAX) ||

As mentioned above, I don't know if these values are accurate, or if it would even make more sense to just check that clk_low and clk_high fit into 4 bits, each.

Not a clue. Stefan?

Given it worked for Stefan, failing detailed info or boards to test on perhaps leave it be?

Quoted 73 lines
Quoted 9 lines

drivers/iio/adc/spear_adc.c line 204

Quoted 8 lines
drivers/iio/adc/spear_adc.c new file +8 -0
+        (val2 != 0)) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    count = (apb_clk + val - 1) / val;
+    info->current_clk = apb_clk / count;
+    spear_adc_set_clk(info, val);

As mentioned in the previous patch, get rid of count and apb_clk here.

Done

Quoted 33 lines
Quoted 33 lines
drivers/iio/adc/spear_adc.c new file +33 -0
+
+out:
+    mutex_unlock(&indio_dev->mlock);
+    return ret;
+}
+
+#define SPEAR_ADC_CHAN(idx) {                \
+    .type = IIO_VOLTAGE,                \
+    .indexed = 1,                    \
+    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),    \
+    .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),    \
+    .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
+    .channel = idx,                    \
+}
+
+static const struct iio_chan_spec spear_adc_iio_channels[] = {
+    SPEAR_ADC_CHAN(0),
+    SPEAR_ADC_CHAN(1),
+    SPEAR_ADC_CHAN(2),
+    SPEAR_ADC_CHAN(3),
+    SPEAR_ADC_CHAN(4),
+    SPEAR_ADC_CHAN(5),
+    SPEAR_ADC_CHAN(6),
+    SPEAR_ADC_CHAN(7),
+};
+
+static irqreturn_t spear_adc_isr(int irq, void *dev_id)
+{
+    struct spear_adc_info *info = (struct spear_adc_info *)dev_id;
+
+    /* Read value to clear IRQ */
+    info->value = spear_adc_get_average(info);
+    complete(&info->completion);
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 244

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+
+    return IRQ_HANDLED;
+}
+
+static int spear_adc_configure(struct spear_adc_info *info)
+{
+    int i;

u8 i; would also do the job.

General practice is not to do this for iteration variables as the compiler will resize them as appropriate anyway so there is no need to care ;)

Quoted 6 lines

drivers/iio/adc/spear_adc.c line 249

Quoted 5 lines
drivers/iio/adc/spear_adc.c new file +5 -0
+
+    /* Reset ADC core */
+    spear_adc_set_status(info, 0);
+    __raw_writel(0, &info->adc_base_spear6xx->clk);
+    for (i = 0; i < 8; i++)

i < ARRAY_SIZE or i < ADC_CHANNEL_NUM to prevent buffer overflows.

Quoted 12 lines

drivers/iio/adc/spear_adc.c line 260

Quoted 11 lines
drivers/iio/adc/spear_adc.c new file +11 -0
+        spear_adc_set_ctrl(info, i, 0);
+    spear_adc_set_scanrate(info, 0);
+
+    spear_adc_set_clk(info, info->sampling_freq);
+
+    return 0;
+}
+
+static const struct iio_info spear_adc_iio_info = {
+    .read_raw = &spear_read_raw,
+    .write_raw = &spear_adc_write_raw,

Better use the same prefix for those functions (also rename one of them, above).

Quoted 2 lines

Good point. Bit odd otherwise. Will add a patch for this as the spear_adc prefix is the dominant one in this driver.

Done

Quoted 605 lines
Quoted 11 lines

drivers/iio/adc/spear_adc.c line 269

Quoted 10 lines
drivers/iio/adc/spear_adc.c new file +9 -0
+    .driver_module = THIS_MODULE,
+};
+
+static int spear_adc_probe(struct platform_device *pdev)
+{
+    struct device_node *np = pdev->dev.of_node;
+    struct device *dev = &pdev->dev;
+    struct spear_adc_info *info;
+    struct iio_dev *iodev = NULL;

Not really wrong, but nobody does that these days anymore. And we normally call it indio_dev

Hmm. That did sort of end up as the convention over time. I'll add a patch for this.

Quoted 91 lines
Quoted 91 lines
drivers/iio/adc/spear_adc.c new file +91 -0
+    int ret = -ENODEV;
+    int irq;
+
+    iodev = devm_iio_device_alloc(dev, sizeof(struct spear_adc_info));
+    if (!iodev) {
+        dev_err(dev, "failed allocating iio device\n");
+        return -ENOMEM;
+    }
+
+    info = iio_priv(iodev);
+    info->np = np;
+
+    /*
+     * SPEAr600 has a different register layout than other SPEAr SoC's
+     * (e.g. SPEAr3xx). Let's provide two register base addresses
+     * to support multi-arch kernels.
+     */
+    info->adc_base_spear6xx = of_iomap(np, 0);
+    if (!info->adc_base_spear6xx) {
+        dev_err(dev, "failed mapping memory\n");
+        return -ENOMEM;
+    }
+    info->adc_base_spear3xx =
+        (struct adc_regs_spear3xx __iomem *)info->adc_base_spear6xx;
+
+    info->clk = clk_get(dev, NULL);
+    if (IS_ERR(info->clk)) {
+        dev_err(dev, "failed getting clock\n");
+        goto errout1;
+    }
+
+    ret = clk_prepare_enable(info->clk);
+    if (ret) {
+        dev_err(dev, "failed enabling clock\n");
+        goto errout2;
+    }
+
+    irq = platform_get_irq(pdev, 0);
+    if (irq <= 0) {
+        dev_err(dev, "failed getting interrupt resource\n");
+        ret = -EINVAL;
+        goto errout3;
+    }
+
+    ret = devm_request_irq(dev, irq, spear_adc_isr, 0, SPEAR_ADC_MOD_NAME,
+            info);
+    if (ret < 0) {
+        dev_err(dev, "failed requesting interrupt\n");
+        goto errout3;
+    }
+
+    if (of_property_read_u32(np, "sampling-frequency",
+                 &info->sampling_freq)) {
+        dev_err(dev, "sampling-frequency missing in DT\n");
+        ret = -EINVAL;
+        goto errout3;
+    }
+
+    /*
+     * Optional avg_samples defaults to 0, resulting in single data
+     * conversion
+     */
+    of_property_read_u32(np, "average-samples", &info->avg_samples);
+
+    /*
+     * Optional vref_external defaults to 0, resulting in internal vref
+     * selection
+     */
+    of_property_read_u32(np, "vref-external", &info->vref_external);
+
+    spear_adc_configure(info);
+
+    platform_set_drvdata(pdev, iodev);
+
+    init_completion(&info->completion);
+
+    iodev->name = SPEAR_ADC_MOD_NAME;
+    iodev->dev.parent = dev;
+    iodev->info = &spear_adc_iio_info;
+    iodev->modes = INDIO_DIRECT_MODE;
+    iodev->channels = spear_adc_iio_channels;
+    iodev->num_channels = ARRAY_SIZE(spear_adc_iio_channels);
+
+    ret = iio_device_register(iodev);
+    if (ret)
+        goto errout3;
+
+    dev_info(dev, "SPEAR ADC driver loaded, IRQ %d\n", irq);
+
+    return 0;
+
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 367

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+errout3:
+    clk_disable_unprepare(info->clk);
+errout2:
+    clk_put(info->clk);
+errout1:
+    iounmap(info->adc_base_spear6xx);
+    return ret;

These labels could have more appropriate names.

This is one of several common conventions. Could be better named, but not worth the patch to do it in my view.

Quoted 23 lines
Quoted 23 lines
drivers/iio/adc/spear_adc.c new file +23 -0
+}
+
+static int spear_adc_remove(struct platform_device *pdev)
+{
+    struct iio_dev *iodev = platform_get_drvdata(pdev);
+    struct spear_adc_info *info = iio_priv(iodev);
+
+    iio_device_unregister(iodev);
+    clk_disable_unprepare(info->clk);
+    clk_put(info->clk);
+    iounmap(info->adc_base_spear6xx);
+
+    return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id spear_adc_dt_ids[] = {
+    { .compatible = "st,spear600-adc", },
+    { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, spear_adc_dt_ids);
+#endif
+
Quoted 8 lines

drivers/iio/adc/spear_adc.c line 397

Quoted 7 lines
drivers/iio/adc/spear_adc.c new file +7 -0
+static struct platform_driver spear_adc_driver = {
+    .probe        = spear_adc_probe,
+    .remove        = spear_adc_remove,
+    .driver        = {
+        .name    = SPEAR_ADC_MOD_NAME,
+        .owner    = THIS_MODULE,
+        .of_match_table = of_match_ptr(spear_adc_dt_ids),

What if CONFIG_OF is disabled?

That's what of_match_ptr is taking care of.
#define of_match_ptr(_ptr) NULL
People presumably got bored of surrounding this single line case with ifdefs ;)

Quoted 451 lines
Quoted 445 lines
drivers/iio/adc/spear_adc.c new file +8 -0
+    },
+};
+
+module_platform_driver(spear_adc_driver);
+
+MODULE_AUTHOR("Stefan Roese <[email protected]>");
+MODULE_DESCRIPTION("SPEAr ADC driver");
+MODULE_LICENSE("GPL");
drivers/staging/iio/adc/Kconfig +0 -8
index 3633298..a37d001 100644
@@ -126,12 +126,4 @@ config MXS_LRADC
        To compile this driver as a module, choose M here: the
        module will be called mxs-lradc.
 
-config SPEAR_ADC
-    tristate "ST SPEAr ADC"
-    depends on PLAT_SPEAR || COMPILE_TEST
-    depends on HAS_IOMEM
-    help
-      Say yes here to build support for the integrated ADC inside the
-      ST SPEAr SoC. Provides direct access via sysfs.
-
  endmenu
drivers/staging/iio/adc/Makefile +0 -1
index 3e9fb14..b5915c1 100644
@@ -19,4 +19,3 @@ obj-$(CONFIG_AD7192) += ad7192.o
  obj-$(CONFIG_AD7280) += ad7280a.o
  obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
  obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
-obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
drivers/staging/iio/adc/spear_adc.c deleted +0 -405
deleted file mode 100644
index 18a0a40..0000000
@@ -1,405 +0,0 @@
-/*
- * ST SPEAr ADC driver
- *
- * Copyright 2012 Stefan Roese <[email protected]>
- *
- * Licensed under the GPL-2.
- */
-
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/interrupt.h>
-#include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/io.h>
-#include <linux/clk.h>
-#include <linux/err.h>
-#include <linux/completion.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-
-/* SPEAR registers definitions */
-#define SPEAR600_ADC_SCAN_RATE_LO(x)    ((x) & 0xFFFF)
-#define SPEAR600_ADC_SCAN_RATE_HI(x)    (((x) >> 0x10) & 0xFFFF)
-#define SPEAR_ADC_CLK_LOW(x)        (((x) & 0xf) << 0)
-#define SPEAR_ADC_CLK_HIGH(x)        (((x) & 0xf) << 4)
-
-/* Bit definitions for SPEAR_ADC_STATUS */
-#define SPEAR_ADC_STATUS_START_CONVERSION    (1 << 0)
-#define SPEAR_ADC_STATUS_CHANNEL_NUM(x)        ((x) << 1)
-#define SPEAR_ADC_STATUS_ADC_ENABLE        (1 << 4)
-#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)        ((x) << 5)
-#define SPEAR_ADC_STATUS_VREF_INTERNAL        (1 << 9)
-
-#define SPEAR_ADC_DATA_MASK        0x03ff
-#define SPEAR_ADC_DATA_BITS        10
-
-#define SPEAR_ADC_MOD_NAME "spear-adc"
-
-#define SPEAR_ADC_CHANNEL_NUM        8
-
-#define SPEAR_ADC_CLK_MIN            2500000
-#define SPEAR_ADC_CLK_MAX            20000000
-
-struct adc_regs_spear3xx {
-    u32 status;
-    u32 average;
-    u32 scan_rate;
-    u32 clk;    /* Not avail for 1340 & 1310 */
-    u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
-    u32 ch_data[SPEAR_ADC_CHANNEL_NUM];
-};
-
-struct chan_data {
-    u32 lsb;
-    u32 msb;
-};
-
-struct adc_regs_spear6xx {
-    u32 status;
-    u32 pad[2];
-    u32 clk;
-    u32 ch_ctrl[SPEAR_ADC_CHANNEL_NUM];
-    struct chan_data ch_data[SPEAR_ADC_CHANNEL_NUM];
-    u32 scan_rate_lo;
-    u32 scan_rate_hi;
-    struct chan_data average;
-};
-
-struct spear_adc_info {
-    struct device_node *np;
-    struct adc_regs_spear3xx __iomem *adc_base_spear3xx;
-    struct adc_regs_spear6xx __iomem *adc_base_spear6xx;
-    struct clk *clk;
-    struct completion completion;
-    u32 current_clk;
-    u32 sampling_freq;
-    u32 avg_samples;
-    u32 vref_external;
-    u32 value;
-};
-
-/*
- * Functions to access some SPEAr ADC register. Abstracted into
- * static inline functions, because of different register offsets
- * on different SoC variants (SPEAr300 vs SPEAr600 etc).
- */
-static void spear_adc_set_status(struct spear_adc_info *info, u32 val)
-{
-    __raw_writel(val, &info->adc_base_spear6xx->status);
-}
-
-static void spear_adc_set_clk(struct spear_adc_info *info, u32 val)
-{
-    u32 clk_high, clk_low, count;
-    u32 apb_clk = clk_get_rate(info->clk);
-
-    count = (apb_clk + val - 1) / val;
-    clk_low = count / 2;
-    clk_high = count - clk_low;
-    info->current_clk = apb_clk / count;
-
-    __raw_writel(SPEAR_ADC_CLK_LOW(clk_low) | SPEAR_ADC_CLK_HIGH(clk_high),
-             &info->adc_base_spear6xx->clk);
-}
-
-static void spear_adc_set_ctrl(struct spear_adc_info *info, int n,
-                   u32 val)
-{
-    __raw_writel(val, &info->adc_base_spear6xx->ch_ctrl[n]);
-}
-
-static u32 spear_adc_get_average(struct spear_adc_info *info)
-{
-    if (of_device_is_compatible(info->np, "st,spear600-adc")) {
-        return __raw_readl(&info->adc_base_spear6xx->average.msb) &
-            SPEAR_ADC_DATA_MASK;
-    } else {
-        return __raw_readl(&info->adc_base_spear3xx->average) &
-            SPEAR_ADC_DATA_MASK;
-    }
-}
-
-static void spear_adc_set_scanrate(struct spear_adc_info *info, u32 rate)
-{
-    if (of_device_is_compatible(info->np, "st,spear600-adc")) {
-        __raw_writel(SPEAR600_ADC_SCAN_RATE_LO(rate),
-                 &info->adc_base_spear6xx->scan_rate_lo);
-        __raw_writel(SPEAR600_ADC_SCAN_RATE_HI(rate),
-                 &info->adc_base_spear6xx->scan_rate_hi);
-    } else {
-        __raw_writel(rate, &info->adc_base_spear3xx->scan_rate);
-    }
-}
-
-static int spear_read_raw(struct iio_dev *indio_dev,
-              struct iio_chan_spec const *chan,
-              int *val,
-              int *val2,
-              long mask)
-{
-    struct spear_adc_info *info = iio_priv(indio_dev);
-    u32 status;
-
-    switch (mask) {
-    case IIO_CHAN_INFO_RAW:
-        mutex_lock(&indio_dev->mlock);
-
-        status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
-            SPEAR_ADC_STATUS_AVG_SAMPLE(info->avg_samples) |
-            SPEAR_ADC_STATUS_START_CONVERSION |
-            SPEAR_ADC_STATUS_ADC_ENABLE;
-        if (info->vref_external == 0)
-            status |= SPEAR_ADC_STATUS_VREF_INTERNAL;
-
-        spear_adc_set_status(info, status);
-        wait_for_completion(&info->completion); /* set by ISR */
-        *val = info->value;
-
-        mutex_unlock(&indio_dev->mlock);
-
-        return IIO_VAL_INT;
-
-    case IIO_CHAN_INFO_SCALE:
-        *val = info->vref_external;
-        *val2 = SPEAR_ADC_DATA_BITS;
-        return IIO_VAL_FRACTIONAL_LOG2;
-    case IIO_CHAN_INFO_SAMP_FREQ:
-        *val = info->current_clk;
-        return IIO_VAL_INT;
-    }
-
-    return -EINVAL;
-}
-
-static int spear_adc_write_raw(struct iio_dev *indio_dev,
-                   struct iio_chan_spec const *chan,
-                   int val,
-                   int val2,
-                   long mask)
-{
-    struct spear_adc_info *info = iio_priv(indio_dev);
-    u32 count;
-    u32 apb_clk = clk_get_rate(info->clk);
-    int ret = 0;
-
-    if (mask != IIO_CHAN_INFO_SAMP_FREQ)
-        return -EINVAL;
-
-    mutex_lock(&indio_dev->mlock);
-
-    if ((val < SPEAR_ADC_CLK_MIN) ||
-        (val > SPEAR_ADC_CLK_MAX) ||
-        (val2 != 0)) {
-        ret = -EINVAL;
-        goto out;
-    }
-
-    count = (apb_clk + val - 1) / val;
-    info->current_clk = apb_clk / count;
-    spear_adc_set_clk(info, val);
-
-out:
-    mutex_unlock(&indio_dev->mlock);
-    return ret;
-}
-
-#define SPEAR_ADC_CHAN(idx) {                \
-    .type = IIO_VOLTAGE,                \
-    .indexed = 1,                    \
-    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),    \
-    .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),    \
-    .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
-    .channel = idx,                    \
-}
-
-static const struct iio_chan_spec spear_adc_iio_channels[] = {
-    SPEAR_ADC_CHAN(0),
-    SPEAR_ADC_CHAN(1),
-    SPEAR_ADC_CHAN(2),
-    SPEAR_ADC_CHAN(3),
-    SPEAR_ADC_CHAN(4),
-    SPEAR_ADC_CHAN(5),
-    SPEAR_ADC_CHAN(6),
-    SPEAR_ADC_CHAN(7),
-};
-
-static irqreturn_t spear_adc_isr(int irq, void *dev_id)
-{
-    struct spear_adc_info *info = (struct spear_adc_info *)dev_id;
-
-    /* Read value to clear IRQ */
-    info->value = spear_adc_get_average(info);
-    complete(&info->completion);
-
-    return IRQ_HANDLED;
-}
-
-static int spear_adc_configure(struct spear_adc_info *info)
-{
-    int i;
-
-    /* Reset ADC core */
-    spear_adc_set_status(info, 0);
-    __raw_writel(0, &info->adc_base_spear6xx->clk);
-    for (i = 0; i < 8; i++)
-        spear_adc_set_ctrl(info, i, 0);
-    spear_adc_set_scanrate(info, 0);
-
-    spear_adc_set_clk(info, info->sampling_freq);
-
-    return 0;
-}
-
-static const struct iio_info spear_adc_iio_info = {
-    .read_raw = &spear_read_raw,
-    .write_raw = &spear_adc_write_raw,
-    .driver_module = THIS_MODULE,
-};
-
-static int spear_adc_probe(struct platform_device *pdev)
-{
-    struct device_node *np = pdev->dev.of_node;
-    struct device *dev = &pdev->dev;
-    struct spear_adc_info *info;
-    struct iio_dev *iodev = NULL;
-    int ret = -ENODEV;
-    int irq;
-
-    iodev = devm_iio_device_alloc(dev, sizeof(struct spear_adc_info));
-    if (!iodev) {
-        dev_err(dev, "failed allocating iio device\n");
-        return -ENOMEM;
-    }
-
-    info = iio_priv(iodev);
-    info->np = np;
-
-    /*
-     * SPEAr600 has a different register layout than other SPEAr SoC's
-     * (e.g. SPEAr3xx). Let's provide two register base addresses
-     * to support multi-arch kernels.
-     */
-    info->adc_base_spear6xx = of_iomap(np, 0);
-    if (!info->adc_base_spear6xx) {
-        dev_err(dev, "failed mapping memory\n");
-        return -ENOMEM;
-    }
-    info->adc_base_spear3xx =
-        (struct adc_regs_spear3xx __iomem *)info->adc_base_spear6xx;
-
-    info->clk = clk_get(dev, NULL);
-    if (IS_ERR(info->clk)) {
-        dev_err(dev, "failed getting clock\n");
-        goto errout1;
-    }
-
-    ret = clk_prepare_enable(info->clk);
-    if (ret) {
-        dev_err(dev, "failed enabling clock\n");
-        goto errout2;
-    }
-
-    irq = platform_get_irq(pdev, 0);
-    if (irq <= 0) {
-        dev_err(dev, "failed getting interrupt resource\n");
-        ret = -EINVAL;
-        goto errout3;
-    }
-
-    ret = devm_request_irq(dev, irq, spear_adc_isr, 0, SPEAR_ADC_MOD_NAME,
-            info);
-    if (ret < 0) {
-        dev_err(dev, "failed requesting interrupt\n");
-        goto errout3;
-    }
-
-    if (of_property_read_u32(np, "sampling-frequency",
-                 &info->sampling_freq)) {
-        dev_err(dev, "sampling-frequency missing in DT\n");
-        ret = -EINVAL;
-        goto errout3;
-    }
-
-    /*
-     * Optional avg_samples defaults to 0, resulting in single data
-     * conversion
-     */
-    of_property_read_u32(np, "average-samples", &info->avg_samples);
-
-    /*
-     * Optional vref_external defaults to 0, resulting in internal vref
-     * selection
-     */
-    of_property_read_u32(np, "vref-external", &info->vref_external);
-
-    spear_adc_configure(info);
-
-    platform_set_drvdata(pdev, iodev);
-
-    init_completion(&info->completion);
-
-    iodev->name = SPEAR_ADC_MOD_NAME;
-    iodev->dev.parent = dev;
-    iodev->info = &spear_adc_iio_info;
-    iodev->modes = INDIO_DIRECT_MODE;
-    iodev->channels = spear_adc_iio_channels;
-    iodev->num_channels = ARRAY_SIZE(spear_adc_iio_channels);
-
-    ret = iio_device_register(iodev);
-    if (ret)
-        goto errout3;
-
-    dev_info(dev, "SPEAR ADC driver loaded, IRQ %d\n", irq);
-
-    return 0;
-
-errout3:
-    clk_disable_unprepare(info->clk);
-errout2:
-    clk_put(info->clk);
-errout1:
-    iounmap(info->adc_base_spear6xx);
-    return ret;
-}
-
-static int spear_adc_remove(struct platform_device *pdev)
-{
-    struct iio_dev *iodev = platform_get_drvdata(pdev);
-    struct spear_adc_info *info = iio_priv(iodev);
-
-    iio_device_unregister(iodev);
-    clk_disable_unprepare(info->clk);
-    clk_put(info->clk);
-    iounmap(info->adc_base_spear6xx);
-
-    return 0;
-}
-
-#ifdef CONFIG_OF
-static const struct of_device_id spear_adc_dt_ids[] = {
-    { .compatible = "st,spear600-adc", },
-    { /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, spear_adc_dt_ids);
-#endif
-
-static struct platform_driver spear_adc_driver = {
-    .probe        = spear_adc_probe,
-    .remove        = spear_adc_remove,
-    .driver        = {
-        .name    = SPEAR_ADC_MOD_NAME,
-        .owner    = THIS_MODULE,
-        .of_match_table = of_match_ptr(spear_adc_dt_ids),
-    },
-};
-
-module_platform_driver(spear_adc_driver);
-
-MODULE_AUTHOR("Stefan Roese <[email protected]>");
-MODULE_DESCRIPTION("SPEAr ADC driver");
-MODULE_LICENSE("GPL");
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html