[RFC resolvers] How to handle fault conditions..

[RFC resolvers] How to handle fault conditions..

[Jonathan Cameron]

Hi all,

The attached patch is a trivial bit of code cleanup to give us a clean
starting point for a discussion of suitable interface for resolvers.
(I was having a dull afternoon yesterday so thought I'd take a look
at a corner of IIO I didn't really know anything about ;)

Anyhow, we have some 'unusual' interrupt sources on these and
without knowing their use cases I'm not sure how we want to handle them.

They are:

1) LOS Loss of signal
 * A low voltage threshold applying to either quadrature signal.
 * Disconnected pin
 * Clipping is occuring.
 * config parity error occured.
2) DOS Degradation of signal
 * High voltage theshold on either signal is exceeded,
 * Missmatch between the quadrature signals by more than some threshold.
3) LOT Loss of tracking.
 * Internal error signal on the tracker is greater than some threshold.
 * Input exeeds max tracking rate.

So we kind of have:

in0_or_in1_thresh_rising, in0_or_in1_thresh_falling 

in1-in0_mag_rising

+ that loss of tracking one which is going to need a new type..
+ clipping of signal and those disconnected errors.
For this last one I wonder if we want it to go through the
event system at all, or whether we should have some out of
band sysfs attributes like the fault ones in hwmon?

Whave do people think?  No rush on this obviously, just thought
I'd get the ball rolling!

onathan Cameron (1):
  staging:iio:resolver:ad2s1210 general driver cleanup.

 drivers/staging/iio/resolver/ad2s1210.c |  856 ++++++++++++++-----------------
 1 files changed, 399 insertions(+), 457 deletions(-)

-- 
1.7.3.4

[PATCH] staging:iio:resolver:ad2s1210 general driver cleanup.

[Jonathan Cameron]

Note I haven't made any changes to the userspace interface as yet.
This is all about cleaning up what was actually there (handling
all errors etc).

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/resolver/ad2s1210.c |  856 ++++++++++++++-----------------
 1 files changed, 399 insertions(+), 457 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 09f4fcf..43521ce 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -19,44 +19,41 @@
 
 #include "../iio.h"
 #include "../sysfs.h"
+#include "ad2s1210.h"
 
 #define DRV_NAME "ad2s1210"
 
-#define DEF_CONTROL		0x7E
-
-#define MSB_IS_HIGH		0x80
-#define MSB_IS_LOW		0x7F
-#define PHASE_LOCK_RANGE_44	0x20
-#define ENABLE_HYSTERESIS	0x10
-#define SET_ENRES1		0x08
-#define SET_ENRES0		0x04
-#define SET_RES1		0x02
-#define SET_RES0		0x01
-
-#define SET_ENRESOLUTION	(SET_ENRES1 | SET_ENRES0)
-#define SET_RESOLUTION		(SET_RES1 | SET_RES0)
-
-#define REG_POSITION		0x80
-#define REG_VELOCITY		0x82
-#define REG_LOS_THRD		0x88
-#define REG_DOS_OVR_THRD	0x89
-#define REG_DOS_MIS_THRD	0x8A
-#define REG_DOS_RST_MAX_THRD	0x8B
-#define REG_DOS_RST_MIN_THRD	0x8C
-#define REG_LOT_HIGH_THRD	0x8D
-#define REG_LOT_LOW_THRD	0x8E
-#define REG_EXCIT_FREQ		0x91
-#define REG_CONTROL		0x92
-#define REG_SOFT_RESET		0xF0
-#define REG_FAULT		0xFF
+#define AD2S1210_DEF_CONTROL		0x7E
+
+#define AD2S1210_MSB_IS_HIGH		0x80
+#define AD2S1210_MSB_IS_LOW		0x7F
+#define AD2S1210_PHASE_LOCK_RANGE_44	0x20
+#define AD2S1210_ENABLE_HYSTERESIS	0x10
+#define AD2S1210_SET_ENRES1		0x08
+#define AD2S1210_SET_ENRES0		0x04
+#define AD2S1210_SET_RES1		0x02
+#define AD2S1210_SET_RES0		0x01
+
+#define AD2S1210_SET_ENRESOLUTION	(AD2S1210_SET_ENRES1 |	\
+					 AD2S1210_SET_ENRES0)
+#define AD2S1210_SET_RESOLUTION		(AD2S1210_SET_RES1 | AD2S1210_SET_RES0)
+
+#define AD2S1210_REG_POSITION		0x80
+#define AD2S1210_REG_VELOCITY		0x82
+#define AD2S1210_REG_LOS_THRD		0x88
+#define AD2S1210_REG_DOS_OVR_THRD	0x89
+#define AD2S1210_REG_DOS_MIS_THRD	0x8A
+#define AD2S1210_REG_DOS_RST_MAX_THRD	0x8B
+#define AD2S1210_REG_DOS_RST_MIN_THRD	0x8C
+#define AD2S1210_REG_LOT_HIGH_THRD	0x8D
+#define AD2S1210_REG_LOT_LOW_THRD	0x8E
+#define AD2S1210_REG_EXCIT_FREQ		0x91
+#define AD2S1210_REG_CONTROL		0x92
+#define AD2S1210_REG_SOFT_RESET		0xF0
+#define AD2S1210_REG_FAULT		0xFF
 
 /* pin SAMPLE, A0, A1, RES0, RES1, is controlled by driver */
 #define AD2S1210_SAA		3
-#if defined(CONFIG_AD2S1210_GPIO_INPUT) || defined(CONFIG_AD2S1210_GPIO_OUTPUT)
-# define AD2S1210_RES		2
-#else
-# define AD2S1210_RES		0
-#endif
 #define AD2S1210_PN		(AD2S1210_SAA + AD2S1210_RES)
 
 #define AD2S1210_MIN_CLKIN	6144000
@@ -75,190 +72,153 @@
 enum ad2s1210_mode {
 	MOD_POS = 0,
 	MOD_VEL,
-	MOD_RESERVED,
 	MOD_CONFIG,
+	MOD_RESERVED,
 };
 
-enum ad2s1210_res {
-	RES_10 = 10,
-	RES_12 = 12,
-	RES_14 = 14,
-	RES_16 = 16,
-};
-
-static unsigned int resolution_value[] = {
-		RES_10, RES_12, RES_14, RES_16};
+static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
 
 struct ad2s1210_state {
+	const struct ad2s1210_platform_data *pdata;
 	struct mutex lock;
-	struct iio_dev *idev;
 	struct spi_device *sdev;
-	struct spi_transfer xfer;
-	unsigned int hysteresis;
-	unsigned int old_data;
-	enum ad2s1210_mode mode;
-	enum ad2s1210_res resolution;
 	unsigned int fclkin;
 	unsigned int fexcit;
-	unsigned short sample;
-	unsigned short a0;
-	unsigned short a1;
-	unsigned short res0;
-	unsigned short res1;
-	u8 rx[3];
-	u8 tx[3];
+	bool hysteresis;
+	bool old_data;
+	u8 resolution;
+	enum ad2s1210_mode mode;
+	u8 rx[2] ____cacheline_aligned;
+	u8 tx[2] ____cacheline_aligned;
 };
 
-static inline void start_sample(struct ad2s1210_state *st)
-{
-	gpio_set_value(st->sample, 0);
-}
-
-static inline void stop_sample(struct ad2s1210_state *st)
-{
-	gpio_set_value(st->sample, 1);
-}
-
-static inline void set_mode(enum ad2s1210_mode mode, struct ad2s1210_state *st)
+static const int ad2s1210_mode_vals[4][2] = {
+	[MOD_POS] = { 0, 0 },
+	[MOD_VEL] = { 0, 1 },
+	[MOD_CONFIG] = { 1, 0 },
+};
+static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
+				     struct ad2s1210_state *st)
 {
-	switch (mode) {
-	case MOD_POS:
-		gpio_set_value(st->a0, 0);
-		gpio_set_value(st->a1, 0);
-		break;
-	case MOD_VEL:
-		gpio_set_value(st->a0, 0);
-		gpio_set_value(st->a1, 1);
-		break;
-	case MOD_CONFIG:
-		gpio_set_value(st->a0, 1);
-		gpio_set_value(st->a1, 1);
-		break;
-	default:
-		/* set to reserved mode */
-		gpio_set_value(st->a0, 1);
-		gpio_set_value(st->a1, 0);
-	}
+	gpio_set_value(st->pdata->a[0], ad2s1210_mode_vals[mode][0]);
+	gpio_set_value(st->pdata->a[1], ad2s1210_mode_vals[mode][1]);
 	st->mode = mode;
 }
 
 /* write 1 bytes (address or data) to the chip */
-static int config_write(struct ad2s1210_state *st,
-					unsigned char data)
+static int ad2s1210_config_write(struct ad2s1210_state *st, u8 data)
 {
-	struct spi_message msg;
-	int ret = 0;
-
-	st->xfer.len = 1;
-	set_mode(MOD_CONFIG, st);
+	int ret;
 
-	spi_message_init(&msg);
-	spi_message_add_tail(&st->xfer, &msg);
+	ad2s1210_set_mode(MOD_CONFIG, st);
 	st->tx[0] = data;
-	ret = spi_sync(st->sdev, &msg);
-	if (ret)
+	ret = spi_write(st->sdev, st->tx, 1);
+	if (ret < 0)
 		return ret;
-	st->old_data = 1;
-	return ret;
+	st->old_data = true;
+
+	return 0;
 }
 
 /* read value from one of the registers */
-static int config_read(struct ad2s1210_state *st,
-				unsigned char address,
-					unsigned char *data)
-{
+static int ad2s1210_config_read(struct ad2s1210_state *st,
+		       unsigned char address)
+{
+	struct spi_transfer xfer = {
+		.len = 2,
+		.rx_buf = st->rx,
+		.tx_buf = st->tx,
+	};
 	struct spi_message msg;
 	int ret = 0;
 
-	st->xfer.len = 2;
-	set_mode(MOD_CONFIG, st);
-
+	ad2s1210_set_mode(MOD_CONFIG, st);
 	spi_message_init(&msg);
-	spi_message_add_tail(&st->xfer, &msg);
-	st->tx[0] = address | MSB_IS_HIGH;
-	st->tx[1] = REG_FAULT;
+	spi_message_add_tail(&xfer, &msg);
+	st->tx[0] = address | AD2S1210_MSB_IS_HIGH;
+	st->tx[1] = AD2S1210_REG_FAULT;
 	ret = spi_sync(st->sdev, &msg);
-	if (ret)
+	if (ret < 0)
 		return ret;
-	*data = st->rx[1];
-	st->old_data = 1;
-	return ret;
+	st->old_data = true;
+
+	return st->rx[1];
 }
 
-static inline void update_frequency_control_word(struct ad2s1210_state *st)
+static inline
+int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)
 {
+	int ret;
 	unsigned char fcw;
+
 	fcw = (unsigned char)(st->fexcit * (1 << 15) / st->fclkin);
-	if (fcw >= AD2S1210_MIN_FCW && fcw <= AD2S1210_MAX_FCW) {
-		config_write(st, REG_EXCIT_FREQ);
-		config_write(st, fcw);
-	} else
+	if (fcw < AD2S1210_MIN_FCW || fcw > AD2S1210_MAX_FCW) {
 		pr_err("ad2s1210: FCW out of range\n");
+		return -ERANGE;
+	}
+	
+	ret = ad2s1210_config_write(st, AD2S1210_REG_EXCIT_FREQ);
+	if (ret < 0)
+		return ret;
+
+	return ad2s1210_config_write(st, fcw);
 }
 
-#if defined(CONFIG_AD2S1210_GPIO_INPUT)
-static inline unsigned char read_resolution_pin(struct ad2s1210_state *st)
+static inline unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)
 {
-	unsigned int data;
-	data = (gpio_get_value(st->res0) << 1)  |
-			gpio_get_value(st->res1);
-	return resolution_value[data];
+	return ad2s1210_resolution_value[
+		(gpio_get_value(st->pdata->res[0]) << 1) |
+		gpio_get_value(st->pdata->res[1])];
 }
-#elif defined(CONFIG_AD2S1210_GPIO_OUTPUT)
-static inline void set_resolution_pin(struct ad2s1210_state *st)
+
+static const int ad2s1210_res_pins[4][2] = {
+	{ 0, 0 }, {0, 1}, {1, 0}, {1, 1}
+};
+
+static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
 {
-	switch (st->resolution) {
-	case RES_10:
-		gpio_set_value(st->res0, 0);
-		gpio_set_value(st->res1, 0);
-		break;
-	case RES_12:
-		gpio_set_value(st->res0, 0);
-		gpio_set_value(st->res1, 1);
-		break;
-	case RES_14:
-		gpio_set_value(st->res0, 1);
-		gpio_set_value(st->res1, 0);
-		break;
-	case RES_16:
-		gpio_set_value(st->res0, 1);
-		gpio_set_value(st->res1, 1);
-		break;
-	}
+	gpio_set_value(st->pdata->res[0],
+		       ad2s1210_res_pins[(st->resolution - 10)/2][0]);
+	gpio_set_value(st->pdata->res[1],
+		       ad2s1210_res_pins[(st->resolution - 10)/2][1]);
 }
-#endif
 
-static inline void soft_reset(struct ad2s1210_state *st)
+static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
 {
-	config_write(st, REG_SOFT_RESET);
-	config_write(st, 0x0);
+	int ret;
+
+	ret = ad2s1210_config_write(st, AD2S1210_REG_SOFT_RESET);
+	if (ret < 0)
+		return ret;
+
+	return ad2s1210_config_write(st, 0x0);
 }
 
 
 /* return the OLD DATA since last spi bus write */
 static ssize_t ad2s1210_show_raw(struct device *dev,
-			struct device_attribute *attr, char *buf)
+				 struct device_attribute *attr,
+				 char *buf)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
-	int ret;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
+	int ret = 0;
 
 	mutex_lock(&st->lock);
 	if (st->old_data) {
 		ret = sprintf(buf, "0x%x\n", st->rx[0]);
-		st->old_data = 0;
-	} else
-		ret = 0;
+		st->old_data = false;
+	}
 	mutex_unlock(&st->lock);
+
 	return ret;
 }
 
 static ssize_t ad2s1210_store_raw(struct device *dev,
-		struct device_attribute *attr,
-		const char *buf, size_t len)
+				  struct device_attribute *attr,
+				  const char *buf,
+				  size_t len)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 	unsigned long udata;
 	unsigned char data;
 	int ret;
@@ -266,139 +226,157 @@ static ssize_t ad2s1210_store_raw(struct device *dev,
 	ret = strict_strtoul(buf, 16, &udata);
 	if (ret)
 		return -EINVAL;
+
 	data = udata & 0xff;
 	mutex_lock(&st->lock);
-	config_write(st, data);
+	ret = ad2s1210_config_write(st, data);
 	mutex_unlock(&st->lock);
-	return 1;
+
+	return ret < 0 ? ret : len;
 }
 
 static ssize_t ad2s1210_store_softreset(struct device *dev,
-			struct device_attribute *attr,
-			const char *buf, size_t len)
+					struct device_attribute *attr,
+					const char *buf,
+					size_t len)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
+	int ret;
+
 	mutex_lock(&st->lock);
-	soft_reset(st);
+	ret = ad2s1210_soft_reset(st);
 	mutex_unlock(&st->lock);
-	return len;
+
+	return ret < 0 ? ret : len;
 }
 
 static ssize_t ad2s1210_show_fclkin(struct device *dev,
-			struct device_attribute *attr, char *buf)
+				    struct device_attribute *attr,
+				    char *buf)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 	return sprintf(buf, "%d\n", st->fclkin);
 }
 
 static ssize_t ad2s1210_store_fclkin(struct device *dev,
-			struct device_attribute *attr,
-			const char *buf, size_t len)
+				     struct device_attribute *attr,
+				     const char *buf,
+				     size_t len)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 	unsigned long fclkin;
 	int ret;
 
 	ret = strict_strtoul(buf, 10, &fclkin);
-	if (!ret && fclkin >= AD2S1210_MIN_CLKIN &&
-				fclkin <= AD2S1210_MAX_CLKIN) {
-		mutex_lock(&st->lock);
-		st->fclkin = fclkin;
-	} else {
+	if (ret)
+		return ret;
+	if (fclkin < AD2S1210_MIN_CLKIN || fclkin > AD2S1210_MAX_CLKIN) {
 		pr_err("ad2s1210: fclkin out of range\n");
 		return -EINVAL;
 	}
-	update_frequency_control_word(st);
-	soft_reset(st);
+
+	mutex_lock(&st->lock);
+	st->fclkin = fclkin;
+
+	ret = ad2s1210_update_frequency_control_word(st);
+	if (ret < 0)
+		goto error_ret;
+	ret = ad2s1210_soft_reset(st);
+error_ret:
 	mutex_unlock(&st->lock);
-	return len;
+
+	return ret < 0 ? ret : len;
 }
 
 static ssize_t ad2s1210_show_fexcit(struct device *dev,
-			struct device_attribute *attr, char *buf)
+				    struct device_attribute *attr,
+				    char *buf)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 	return sprintf(buf, "%d\n", st->fexcit);
 }
 
 static ssize_t ad2s1210_store_fexcit(struct device *dev,
-			struct device_attribute *attr,
-			const char *buf, size_t len)
+				     struct device_attribute *attr,
+				     const char *buf, size_t len)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 	unsigned long fexcit;
 	int ret;
 
 	ret = strict_strtoul(buf, 10, &fexcit);
-	if (!ret && fexcit >= AD2S1210_MIN_EXCIT &&
-				fexcit <= AD2S1210_MAX_EXCIT) {
-		mutex_lock(&st->lock);
-		st->fexcit = fexcit;
-	} else {
+	if (ret < 0)
+		return ret;
+	if (fexcit < AD2S1210_MIN_EXCIT || fexcit > AD2S1210_MAX_EXCIT) {
 		pr_err("ad2s1210: excitation frequency out of range\n");
 		return -EINVAL;
 	}
-	update_frequency_control_word(st);
-	soft_reset(st);
+	mutex_lock(&st->lock);
+	st->fexcit = fexcit;
+	ret = ad2s1210_update_frequency_control_word(st);
+	if (ret < 0)
+		goto error_ret;
+	ret = ad2s1210_soft_reset(st);
+error_ret:
 	mutex_unlock(&st->lock);
-	return len;
+
+	return ret < 0 ? ret : len;
 }
 
 static ssize_t ad2s1210_show_control(struct device *dev,
-			struct device_attribute *attr, char *buf)
+				     struct device_attribute *attr,
+				     char *buf)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
-	unsigned char data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
+	int ret;
 	mutex_lock(&st->lock);
-	config_read(st, REG_CONTROL, &data);
+	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
 	mutex_unlock(&st->lock);
-	return sprintf(buf, "0x%x\n", data);
+	return ret < 0 ? ret : sprintf(buf, "0x%x\n", ret);
 }
 
 static ssize_t ad2s1210_store_control(struct device *dev,
 			struct device_attribute *attr,
 			const char *buf, size_t len)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 	unsigned long udata;
 	unsigned char data;
 	int ret;
 
 	ret = strict_strtoul(buf, 16, &udata);
-	if (ret) {
-		ret = -EINVAL;
-		goto error_ret;
-	}
+	if (ret)
+		return -EINVAL;
+
 	mutex_lock(&st->lock);
-	config_write(st, REG_CONTROL);
-	data = udata & MSB_IS_LOW;
-	config_write(st, data);
-	config_read(st, REG_CONTROL, &data);
-	if (data & MSB_IS_HIGH) {
+	ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
+	if (ret < 0)
+		goto error_ret;
+	data = udata & AD2S1210_MSB_IS_LOW;
+	ret = ad2s1210_config_write(st, data);
+	if (ret < 0)
+		goto error_ret;
+
+	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
+	if (ret < 0)
+		goto error_ret;
+	if (ret & AD2S1210_MSB_IS_HIGH) {
 		ret = -EIO;
 		pr_err("ad2s1210: write control register fail\n");
 		goto error_ret;
 	}
-	st->resolution = resolution_value[data & SET_RESOLUTION];
-#if defined(CONFIG_AD2S1210_GPIO_INPUT)
-	data = read_resolution_pin(st);
-	if (data != st->resolution)
-		pr_warning("ad2s1210: resolution settings not match\n");
-#elif defined(CONFIG_AD2S1210_GPIO_OUTPUT)
-	set_resolution_pin(st);
-#endif
+	st->resolution
+		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
+	if (st->pdata->gpioin) {
+		data = ad2s1210_read_resolution_pin(st);
+		if (data != st->resolution)
+			pr_warning("ad2s1210: resolution settings not match\n");
+	} else
+		ad2s1210_set_resolution_pin(st);
+
 	ret = len;
-	if (data & ENABLE_HYSTERESIS)
-		st->hysteresis = 1;
-	else
-		st->hysteresis = 0;
+	st->hysteresis = !!(data & AD2S1210_ENABLE_HYSTERESIS);
+
 error_ret:
 	mutex_unlock(&st->lock);
 	return ret;
@@ -407,8 +385,7 @@ error_ret:
 static ssize_t ad2s1210_show_resolution(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 	return sprintf(buf, "%d\n", st->resolution);
 }
 
@@ -416,103 +393,109 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
 			struct device_attribute *attr,
 			const char *buf, size_t len)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 	unsigned char data;
 	unsigned long udata;
 	int ret;
 
 	ret = strict_strtoul(buf, 10, &udata);
-	if (ret || udata < RES_10 || udata > RES_16) {
+	if (ret || udata < 10 || udata > 16) {
 		pr_err("ad2s1210: resolution out of range\n");
 		return -EINVAL;
 	}
 	mutex_lock(&st->lock);
-	config_read(st, REG_CONTROL, &data);
-	data &= ~SET_RESOLUTION;
-	data |= (udata - RES_10) >> 1;
-	config_write(st, REG_CONTROL);
-	config_write(st, data & MSB_IS_LOW);
-	config_read(st, REG_CONTROL, &data);
-	if (data & MSB_IS_HIGH) {
+	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
+	if (ret < 0)
+		goto error_ret;
+	data = ret;
+	data &= ~AD2S1210_SET_RESOLUTION;
+	data |= (udata - 10) >> 1;
+	ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
+	if (ret < 0)
+		goto error_ret;
+	ret = ad2s1210_config_write(st, data & AD2S1210_MSB_IS_LOW);
+	if (ret < 0)
+		goto error_ret;
+	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
+	if (ret < 0)
+		goto error_ret;
+	data = ret;
+	if (data & AD2S1210_MSB_IS_HIGH) {
 		ret = -EIO;
 		pr_err("ad2s1210: setting resolution fail\n");
 		goto error_ret;
 	}
-	st->resolution = resolution_value[data & SET_RESOLUTION];
-#if defined(CONFIG_AD2S1210_GPIO_INPUT)
-	data = read_resolution_pin(st);
-	if (data != st->resolution)
-		pr_warning("ad2s1210: resolution settings not match\n");
-#elif defined(CONFIG_AD2S1210_GPIO_OUTPUT)
-	set_resolution_pin(st);
-#endif
+	st->resolution
+		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
+	if (st->pdata->gpioin) {
+		data = ad2s1210_read_resolution_pin(st);
+		if (data != st->resolution)
+			pr_warning("ad2s1210: resolution settings not match\n");
+	} else
+		ad2s1210_set_resolution_pin(st);
 	ret = len;
 error_ret:
 	mutex_unlock(&st->lock);
 	return ret;
 }
+
 /* read the fault register since last sample */
 static ssize_t ad2s1210_show_fault(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
-	int ret = 0;
-	ssize_t len = 0;
-	unsigned char data;
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
+	int ret;
 
 	mutex_lock(&st->lock);
-	ret = config_read(st, REG_FAULT, &data);
-
-	if (ret)
-		goto error_ret;
-	len = sprintf(buf, "0x%x\n", data);
-error_ret:
+	ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
 	mutex_unlock(&st->lock);
-	return ret ? ret : len;
+
+	return ret ? ret : sprintf(buf, "0x%x\n", ret);
 }
 
 static ssize_t ad2s1210_clear_fault(struct device *dev,
-			struct device_attribute *attr,
-			const char *buf, size_t len)
+				    struct device_attribute *attr,
+				    const char *buf,
+				    size_t len)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
-	unsigned char data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
+	int ret;
 
 	mutex_lock(&st->lock);
-	start_sample(st);
+	gpio_set_value(st->pdata->sample, 0);
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
-	stop_sample(st);
-	config_read(st, REG_FAULT, &data);
-	start_sample(st);
-	stop_sample(st);
+	gpio_set_value(st->pdata->sample, 1);
+	ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
+	if (ret < 0)
+		goto error_ret;
+	gpio_set_value(st->pdata->sample, 0);
+	gpio_set_value(st->pdata->sample, 1);
+error_ret:
 	mutex_unlock(&st->lock);
 
-	return 0;
+	return ret < 0 ? ret : len;
 }
 
 static ssize_t ad2s1210_show_reg(struct device *dev,
-			struct device_attribute *attr, char *buf)
+				 struct device_attribute *attr,
+				 char *buf)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
-	unsigned char data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 	struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
+	int ret;
 
 	mutex_lock(&st->lock);
-	config_read(st, iattr->address, &data);
+	ret = ad2s1210_config_read(st, iattr->address);
 	mutex_unlock(&st->lock);
-	return sprintf(buf, "%d\n", data);
+
+	return ret < 0 ? ret : sprintf(buf, "%d\n", ret);
 }
 
 static ssize_t ad2s1210_store_reg(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t len)
 {
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 	unsigned long data;
 	int ret;
 	struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
@@ -521,183 +504,121 @@ static ssize_t ad2s1210_store_reg(struct device *dev,
 	if (ret)
 		return -EINVAL;
 	mutex_lock(&st->lock);
-	config_write(st, iattr->address);
-	config_write(st, data & MSB_IS_LOW);
+	ret = ad2s1210_config_write(st, iattr->address);
+	if (ret < 0)
+		goto error_ret;
+	ret = ad2s1210_config_write(st, data & AD2S1210_MSB_IS_LOW);
+error_ret:
 	mutex_unlock(&st->lock);
-	return len;
+	return ret < 0 ? ret : len;
 }
 
 static ssize_t ad2s1210_show_pos(struct device *dev,
-			struct device_attribute *attr, char *buf)
+				 struct device_attribute *attr,
+				 char *buf)
 {
-	struct spi_message msg;
 	int ret = 0;
 	ssize_t len = 0;
 	u16 pos;
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 
-	st->xfer.len = 2;
 	mutex_lock(&st->lock);
-	start_sample(st);
+	gpio_set_value(st->pdata->sample, 0);
 	/* delay (6 * tck + 20) nano seconds */
 	udelay(1);
 
-	set_mode(MOD_POS, st);
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&st->xfer, &msg);
-	ret = spi_sync(st->sdev, &msg);
+	ad2s1210_set_mode(MOD_POS, st);
+	ret = spi_read(st->sdev, st->rx, 2);
 	if (ret)
 		goto error_ret;
-	pos = ((((u16)(st->rx[0])) << 8) | (st->rx[1]));
+	pos = be16_to_cpup((u16 *)st->rx);
 	if (st->hysteresis)
 		pos >>= 16 - st->resolution;
 	len = sprintf(buf, "%d\n", pos);
 error_ret:
-	stop_sample(st);
+	gpio_set_value(st->pdata->sample, 1);
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
 	mutex_unlock(&st->lock);
 
-	return ret ? ret : len;
+	return ret < 0 ? ret : len;
 }
 
 static ssize_t ad2s1210_show_vel(struct device *dev,
-			struct device_attribute *attr, char *buf)
+				 struct device_attribute *attr,
+				 char *buf)
 {
-	struct spi_message msg;
 	unsigned short negative;
 	int ret = 0;
 	ssize_t len = 0;
 	s16 vel;
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
+	struct ad2s1210_state *st = iio_priv(dev_get_drvdata(dev));
 
-	st->xfer.len = 2;
 	mutex_lock(&st->lock);
-	start_sample(st);
+	gpio_set_value(st->pdata->sample, 0);
 	/* delay (6 * tck + 20) nano seconds */
 	udelay(1);
 
-	set_mode(MOD_VEL, st);
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&st->xfer, &msg);
-	ret = spi_sync(st->sdev, &msg);
+	ad2s1210_set_mode(MOD_VEL, st);
+	ret = spi_read(st->sdev, st->rx, 2);
 	if (ret)
 		goto error_ret;
 	negative = st->rx[0] & 0x80;
-	vel = ((((s16)(st->rx[0])) << 8) | (st->rx[1]));
+	vel = be16_to_cpup((s16 *)st->rx);
 	vel >>= 16 - st->resolution;
-	if (negative) {
+	if (vel & 0x8000) {
 		negative = (0xffff >> st->resolution) << st->resolution;
 		vel |= negative;
 	}
 	len = sprintf(buf, "%d\n", vel);
 error_ret:
-	stop_sample(st);
-	/* delay (2 * tck + 20) nano seconds */
-	udelay(1);
-	mutex_unlock(&st->lock);
-
-	return ret ? ret : len;
-}
-
-static ssize_t ad2s1210_show_pos_vel(struct device *dev,
-			struct device_attribute *attr, char *buf)
-{
-	struct spi_message msg;
-	unsigned short negative;
-	int ret = 0;
-	ssize_t len = 0;
-	u16 pos;
-	s16 vel;
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s1210_state *st = idev->dev_data;
-
-	st->xfer.len = 2;
-	mutex_lock(&st->lock);
-	start_sample(st);
-	/* delay (6 * tck + 20) nano seconds */
-	udelay(1);
-
-	set_mode(MOD_POS, st);
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&st->xfer, &msg);
-	ret = spi_sync(st->sdev, &msg);
-	if (ret)
-		goto error_ret;
-	pos = ((((u16)(st->rx[0])) << 8) | (st->rx[1]));
-	if (st->hysteresis)
-		pos >>= 16 - st->resolution;
-	len = sprintf(buf, "%d ", pos);
-
-	st->xfer.len = 2;
-	set_mode(MOD_VEL, st);
-	spi_message_init(&msg);
-	spi_message_add_tail(&st->xfer, &msg);
-	ret = spi_sync(st->sdev, &msg);
-	if (ret)
-		goto error_ret;
-	negative = st->rx[0] & 0x80;
-	vel = ((((s16)(st->rx[0])) << 8) | (st->rx[1]));
-	vel >>= 16 - st->resolution;
-	if (negative) {
-		negative = (0xffff >> st->resolution) << st->resolution;
-		vel |= negative;
-	}
-	len += sprintf(buf + len, "%d\n", vel);
-error_ret:
-	stop_sample(st);
+	gpio_set_value(st->pdata->sample, 1);
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
 	mutex_unlock(&st->lock);
 
-	return ret ? ret : len;
+	return ret < 0 ? ret : len;
 }
 
-static IIO_CONST_ATTR(description,
-	"Variable Resolution, 10-Bit to 16Bit R/D\n\
-Converter with Reference Oscillator");
 static IIO_DEVICE_ATTR(raw_io, S_IRUGO | S_IWUSR,
-		ad2s1210_show_raw, ad2s1210_store_raw, 0);
+		       ad2s1210_show_raw, ad2s1210_store_raw, 0);
 static IIO_DEVICE_ATTR(reset, S_IWUSR,
-		NULL, ad2s1210_store_softreset, 0);
+		       NULL, ad2s1210_store_softreset, 0);
 static IIO_DEVICE_ATTR(fclkin, S_IRUGO | S_IWUSR,
-		ad2s1210_show_fclkin, ad2s1210_store_fclkin, 0);
+		       ad2s1210_show_fclkin, ad2s1210_store_fclkin, 0);
 static IIO_DEVICE_ATTR(fexcit, S_IRUGO | S_IWUSR,
-		ad2s1210_show_fexcit,	ad2s1210_store_fexcit, 0);
+		       ad2s1210_show_fexcit,	ad2s1210_store_fexcit, 0);
 static IIO_DEVICE_ATTR(control, S_IRUGO | S_IWUSR,
-		ad2s1210_show_control, ad2s1210_store_control, 0);
+		       ad2s1210_show_control, ad2s1210_store_control, 0);
 static IIO_DEVICE_ATTR(bits, S_IRUGO | S_IWUSR,
-		ad2s1210_show_resolution, ad2s1210_store_resolution, 0);
+		       ad2s1210_show_resolution, ad2s1210_store_resolution, 0);
 static IIO_DEVICE_ATTR(fault, S_IRUGO | S_IWUSR,
-		ad2s1210_show_fault, ad2s1210_clear_fault, 0);
-static IIO_DEVICE_ATTR(pos, S_IRUGO,
-		ad2s1210_show_pos, NULL, 0);
-static IIO_DEVICE_ATTR(vel, S_IRUGO,
-		ad2s1210_show_vel, NULL, 0);
-static IIO_DEVICE_ATTR(pos_vel, S_IRUGO,
-		ad2s1210_show_pos_vel, NULL, 0);
+		       ad2s1210_show_fault, ad2s1210_clear_fault, 0);
+static IIO_DEVICE_ATTR(pos, S_IRUGO, ad2s1210_show_pos, NULL, 0);
+static IIO_DEVICE_ATTR(vel, S_IRUGO,  ad2s1210_show_vel, NULL, 0);
 static IIO_DEVICE_ATTR(los_thrd, S_IRUGO | S_IWUSR,
-		ad2s1210_show_reg, ad2s1210_store_reg, REG_LOS_THRD);
+		       ad2s1210_show_reg, ad2s1210_store_reg,
+		       AD2S1210_REG_LOS_THRD);
 static IIO_DEVICE_ATTR(dos_ovr_thrd, S_IRUGO | S_IWUSR,
-		ad2s1210_show_reg, ad2s1210_store_reg, REG_DOS_OVR_THRD);
+		       ad2s1210_show_reg, ad2s1210_store_reg,
+		       AD2S1210_REG_DOS_OVR_THRD);
 static IIO_DEVICE_ATTR(dos_mis_thrd, S_IRUGO | S_IWUSR,
-		ad2s1210_show_reg, ad2s1210_store_reg, REG_DOS_MIS_THRD);
+		       ad2s1210_show_reg, ad2s1210_store_reg,
+		       AD2S1210_REG_DOS_MIS_THRD);
 static IIO_DEVICE_ATTR(dos_rst_max_thrd, S_IRUGO | S_IWUSR,
-		ad2s1210_show_reg, ad2s1210_store_reg, REG_DOS_RST_MAX_THRD);
+		       ad2s1210_show_reg, ad2s1210_store_reg,
+		       AD2S1210_REG_DOS_RST_MAX_THRD);
 static IIO_DEVICE_ATTR(dos_rst_min_thrd, S_IRUGO | S_IWUSR,
-		ad2s1210_show_reg, ad2s1210_store_reg, REG_DOS_RST_MIN_THRD);
+		       ad2s1210_show_reg, ad2s1210_store_reg,
+		       AD2S1210_REG_DOS_RST_MIN_THRD);
 static IIO_DEVICE_ATTR(lot_high_thrd, S_IRUGO | S_IWUSR,
-		ad2s1210_show_reg, ad2s1210_store_reg, REG_LOT_HIGH_THRD);
+		       ad2s1210_show_reg, ad2s1210_store_reg,
+		       AD2S1210_REG_LOT_HIGH_THRD);
 static IIO_DEVICE_ATTR(lot_low_thrd, S_IRUGO | S_IWUSR,
-		ad2s1210_show_reg, ad2s1210_store_reg, REG_LOT_LOW_THRD);
+		       ad2s1210_show_reg, ad2s1210_store_reg,
+		       AD2S1210_REG_LOT_LOW_THRD);
 
 static struct attribute *ad2s1210_attributes[] = {
-	&iio_const_attr_description.dev_attr.attr,
 	&iio_dev_attr_raw_io.dev_attr.attr,
 	&iio_dev_attr_reset.dev_attr.attr,
 	&iio_dev_attr_fclkin.dev_attr.attr,
@@ -707,7 +628,6 @@ static struct attribute *ad2s1210_attributes[] = {
 	&iio_dev_attr_fault.dev_attr.attr,
 	&iio_dev_attr_pos.dev_attr.attr,
 	&iio_dev_attr_vel.dev_attr.attr,
-	&iio_dev_attr_pos_vel.dev_attr.attr,
 	&iio_dev_attr_los_thrd.dev_attr.attr,
 	&iio_dev_attr_dos_ovr_thrd.dev_attr.attr,
 	&iio_dev_attr_dos_mis_thrd.dev_attr.attr,
@@ -729,27 +649,32 @@ static int __devinit ad2s1210_initial(struct ad2s1210_state *st)
 	int ret;
 
 	mutex_lock(&st->lock);
-#if defined(CONFIG_AD2S1210_GPIO_INPUT)
-	st->resolution = read_resolution_pin(st);
-#elif defined(CONFIG_AD2S1210_GPIO_OUTPUT)
-	set_resolution_pin(st);
-#endif
-
-	config_write(st, REG_CONTROL);
-	data = DEF_CONTROL & ~(SET_RESOLUTION);
-	data |= (st->resolution - RES_10) >> 1;
-	config_write(st, data);
-	ret = config_read(st, REG_CONTROL, &data);
-	if (ret)
+	if (st->pdata->gpioin)
+		st->resolution = ad2s1210_read_resolution_pin(st);
+	else
+		ad2s1210_set_resolution_pin(st);
+
+	ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
+	if (ret < 0)
+		goto error_ret;
+	data = AD2S1210_DEF_CONTROL & ~(AD2S1210_SET_RESOLUTION);
+	data |= (st->resolution - 10) >> 1;
+	ret = ad2s1210_config_write(st, data);
+	if (ret < 0)
+		goto error_ret;
+	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
+	if (ret < 0)
 		goto error_ret;
 
-	if (data & MSB_IS_HIGH) {
+	if (ret & AD2S1210_MSB_IS_HIGH) {
 		ret = -EIO;
 		goto error_ret;
 	}
 
-	update_frequency_control_word(st);
-	soft_reset(st);
+	ret = ad2s1210_update_frequency_control_word(st);
+	if (ret < 0)
+		goto error_ret;
+	ret = ad2s1210_soft_reset(st);
 error_ret:
 	mutex_unlock(&st->lock);
 	return ret;
@@ -760,90 +685,107 @@ static const struct iio_info ad2s1210_info = {
 	.driver_module = THIS_MODULE,
 };
 
+static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
+{
+	int ret;
+	unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
+
+	ret = gpio_request_one(st->pdata->sample, GPIOF_DIR_IN, "sample");
+	if (ret < 0)
+		goto error_ret;
+	ret = gpio_request_one(st->pdata->a[0], flags, "a0");
+	if (ret < 0)
+		goto error_free_sample;
+	ret = gpio_request_one(st->pdata->a[1], flags, "a1");
+	if (ret < 0)
+		goto error_free_a0;
+	ret = gpio_request_one(st->pdata->res[1], flags, "res0");
+	if (ret < 0)
+		goto error_free_a1;
+	ret = gpio_request_one(st->pdata->res[1], flags, "res1");
+	if (ret < 0)
+		goto error_free_res0;
+
+	return 0;
+error_free_res0:
+	gpio_free(st->pdata->res[0]);
+error_free_a1:
+	gpio_free(st->pdata->a[1]);
+error_free_a0:
+	gpio_free(st->pdata->a[0]);
+error_free_sample:
+	gpio_free(st->pdata->sample);
+error_ret:
+	return ret;
+}
+
+static void ad2s1210_free_gpios(struct ad2s1210_state *st)
+{
+	gpio_free(st->pdata->res[1]);
+	gpio_free(st->pdata->res[0]);
+	gpio_free(st->pdata->a[1]);
+	gpio_free(st->pdata->a[0]);
+	gpio_free(st->pdata->sample);
+}
+
 static int __devinit ad2s1210_probe(struct spi_device *spi)
 {
+	struct iio_dev *indio_dev;
 	struct ad2s1210_state *st;
-	int pn, ret = 0;
-	unsigned short *pins = spi->dev.platform_data;
-
-	for (pn = 0; pn < AD2S1210_PN; pn++) {
-		if (gpio_request(pins[pn], DRV_NAME)) {
-			pr_err("%s: request gpio pin %d failed\n",
-						DRV_NAME, pins[pn]);
-			goto error_ret;
-		}
-		if (pn < AD2S1210_SAA)
-			gpio_direction_output(pins[pn], 1);
-		else {
-#if defined(CONFIG_AD2S1210_GPIO_INPUT)
-			gpio_direction_input(pins[pn]);
-#elif defined(CONFIG_AD2S1210_GPIO_OUTPUT)
-			gpio_direction_output(pins[pn], 1);
-#endif
-		}
-	}
+	int ret;
+
+	if (spi->dev.platform_data == NULL)
+		return -EINVAL;
 
-	st = kzalloc(sizeof(*st), GFP_KERNEL);
-	if (st == NULL) {
+	indio_dev = iio_allocate_device(sizeof(*st));
+	if (indio_dev == NULL) {
 		ret = -ENOMEM;
 		goto error_ret;
 	}
-	spi_set_drvdata(spi, st);
+	st = iio_priv(indio_dev);
+	st->pdata = spi->dev.platform_data;
+	ret = ad2s1210_setup_gpios(st);
+	if (ret < 0)
+		goto error_free_dev;
+
+	spi_set_drvdata(spi, indio_dev);
 
 	mutex_init(&st->lock);
 	st->sdev = spi;
-	st->xfer.tx_buf = st->tx;
-	st->xfer.rx_buf = st->rx;
-	st->hysteresis = 1;
+	st->hysteresis = true;
 	st->mode = MOD_CONFIG;
-	st->resolution = RES_12;
-	st->fclkin = AD2S1210_DEF_CLKIN;
+	st->resolution = 12;
 	st->fexcit = AD2S1210_DEF_EXCIT;
-	st->sample = pins[0];
-	st->a0 = pins[1];
-	st->a1 = pins[2];
-	st->res0 = pins[3];
-	st->res1 = pins[4];
-
-	st->idev = iio_allocate_device(0);
-	if (st->idev == NULL) {
-		ret = -ENOMEM;
-		goto error_free_st;
-	}
-	st->idev->dev.parent = &spi->dev;
 
-	st->idev->info = &ad2s1210_info;
-	st->idev->dev_data = (void *)(st);
-	st->idev->modes = INDIO_DIRECT_MODE;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &ad2s1210_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	ret = iio_device_register(st->idev);
+	ret = iio_device_register(indio_dev);
 	if (ret)
-		goto error_free_dev;
+		goto error_free_gpios;
 
-	if (spi->max_speed_hz != AD2S1210_DEF_CLKIN)
-		st->fclkin = spi->max_speed_hz;
+	st->fclkin = spi->max_speed_hz;
 	spi->mode = SPI_MODE_3;
 	spi_setup(spi);
-
 	ad2s1210_initial(st);
+
 	return 0;
 
+error_free_gpios:
+	ad2s1210_free_gpios(st);
 error_free_dev:
-	iio_free_device(st->idev);
-error_free_st:
-	kfree(st);
+	iio_free_device(indio_dev);
 error_ret:
-	for (--pn; pn >= 0; pn--)
-		gpio_free(pins[pn]);
 	return ret;
 }
 
 static int __devexit ad2s1210_remove(struct spi_device *spi)
 {
-	struct ad2s1210_state *st = spi_get_drvdata(spi);
-
-	iio_device_unregister(st->idev);
-	kfree(st);
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct ad2s1210_state *st = iio_priv(indio_dev);
+	iio_device_unregister(indio_dev);
+	ad2s1210_free_gpios(st);
 
 	return 0;
 }
-- 
1.7.3.4

[PATCH 0/2] IIO more resolver cleanups.

[Jonathan Cameron]

What can I say, this morning has turned out to be rather dull as well
and these other two drivers are small so here is a cleanup series for them.

Note in the ad2s1210 driver I forgot to clear the IN / OUT of resolution bit
out of the Kconfig file. I'm also not entirely sure I understand how that
is supposed to work, so may well have it wrong.

Thanks,


Jonathan Cameron (2):
  staging:iio:resolver:ad2s120x cleanup.
  staging:iio:resolver:ad2s90 general cleanup

 drivers/staging/iio/resolver/ad2s120x.c |  196 +++++--------------------------
 drivers/staging/iio/resolver/ad2s90.c   |   50 ++------
 2 files changed, 44 insertions(+), 202 deletions(-)

-- 
1.7.3.4

[PATCH 1/2] staging:iio:resolver:ad2s120x cleanup.

[Jonathan Cameron]

I've currently squashed the vel + pos combined attribute.  If people need them
precisely paired I doubt they will get them from the sysfs interface anyway.
If that is a requirement it should come via a buffer implementation.

Note this patch leaves the completely non standard interface alone.
That will get fixed later.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/resolver/ad2s120x.c |  196 +++++--------------------------
 1 files changed, 31 insertions(+), 165 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s120x.c b/drivers/staging/iio/resolver/ad2s120x.c
index f83e142..bed4c72 100644
--- a/drivers/staging/iio/resolver/ad2s120x.c
+++ b/drivers/staging/iio/resolver/ad2s120x.c
@@ -32,161 +32,46 @@
 
 struct ad2s120x_state {
 	struct mutex lock;
-	struct iio_dev *idev;
 	struct spi_device *sdev;
-	unsigned short sample;
-	unsigned short rdvel;
-	u8 rx[2];
-	u8 tx[2];
+	int sample;
+	int rdvel;
+	u8 rx[2] ____cacheline_aligned;
 };
 
-static ssize_t ad2s120x_show_pos_vel(struct device *dev,
+static ssize_t ad2s120x_show_val(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
-	struct spi_message msg;
-	struct spi_transfer xfer;
 	int ret = 0;
 	ssize_t len = 0;
 	u16 pos;
 	s16 vel;
 	u8 status;
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s120x_state *st = idev->dev_data;
+	struct ad2s120x_state *st = iio_priv(dev_get_drvdata(dev));
+	struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
 
-	xfer.len = 1;
-	xfer.tx_buf = st->tx;
-	xfer.rx_buf = st->rx;
 	mutex_lock(&st->lock);
 
 	gpio_set_value(st->sample, 0);
 	/* delay (6 * AD2S120X_TSCLK + 20) nano seconds */
 	udelay(1);
 	gpio_set_value(st->sample, 1);
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&xfer, &msg);
-	ret = spi_sync(st->sdev, &msg);
-	if (ret)
-		goto error_ret;
-	status = st->rx[1];
-	pos = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
-	len = sprintf(buf, "%d %c%c%c%c ", pos,
-				(status & 0x8) ? 'P' : 'V',
-				(status & 0x4) ? 'd' : '_',
-				(status & 0x2) ? 'l' : '_',
-				(status & 0x1) ? '1' : '0');
-
-	/* delay 18 ns */
-	/* ndelay(18); */
-
-	gpio_set_value(st->rdvel, 0);
-	/* ndelay(5);*/
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&xfer, &msg);
-	ret = spi_sync(st->sdev, &msg);
-	if (ret)
+	gpio_set_value(st->rdvel, iattr->address);
+	ret = spi_read(st->sdev, st->rx, 2);
+	if (ret < 0)
 		goto error_ret;
 	status = st->rx[1];
-	vel = (st->rx[0] & 0x80) ? 0xf000 : 0;
-	vel |= (((s16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
-	len += sprintf(buf + len, "%d %c%c%c%c\n", vel,
-				(status & 0x8) ? 'P' : 'V',
-				(status & 0x4) ? 'd' : '_',
-				(status & 0x2) ? 'l' : '_',
-				(status & 0x1) ? '1' : '0');
-error_ret:
-	gpio_set_value(st->rdvel, 1);
-	/* delay (2 * AD2S120X_TSCLK + 20) ns for sample pulse */
-	udelay(1);
-	mutex_unlock(&st->lock);
-
-	return ret ? ret : len;
-}
-
-static ssize_t ad2s120x_show_pos(struct device *dev,
-			struct device_attribute *attr, char *buf)
-{
-	struct spi_message msg;
-	struct spi_transfer xfer;
-	int ret = 0;
-	ssize_t len = 0;
-	u16 pos;
-	u8 status;
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s120x_state *st = idev->dev_data;
-
-	xfer.len = 1;
-	xfer.tx_buf = st->tx;
-	xfer.rx_buf = st->rx;
-	mutex_lock(&st->lock);
-
-	gpio_set_value(st->sample, 0);
-	/* delay (6 * AD2S120X_TSCLK + 20) nano seconds */
-	udelay(1);
-	gpio_set_value(st->sample, 1);
-	gpio_set_value(st->rdvel, 1);
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&xfer, &msg);
-	ret = spi_sync(st->sdev, &msg);
-	if (ret)
-		goto error_ret;
-	status = st->rx[1];
-	pos = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
-	len = sprintf(buf, "%d %c%c%c%c ", pos,
-				(status & 0x8) ? 'P' : 'V',
-				(status & 0x4) ? 'd' : '_',
-				(status & 0x2) ? 'l' : '_',
-				(status & 0x1) ? '1' : '0');
-error_ret:
-	/* delay (2 * AD2S120X_TSCLK + 20) ns for sample pulse */
-	udelay(1);
-	mutex_unlock(&st->lock);
-
-	return ret ? ret : len;
-}
-
-static ssize_t ad2s120x_show_vel(struct device *dev,
-			struct device_attribute *attr, char *buf)
-{
-	struct spi_message msg;
-	struct spi_transfer xfer;
-	int ret = 0;
-	ssize_t len = 0;
-	s16 vel;
-	u8 status;
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s120x_state *st = idev->dev_data;
-
-	xfer.len = 1;
-	xfer.tx_buf = st->tx;
-	xfer.rx_buf = st->rx;
-	mutex_lock(&st->lock);
-
-	gpio_set_value(st->sample, 0);
-	/* delay (6 * AD2S120X_TSCLK + 20) nano seconds */
-	udelay(1);
-	gpio_set_value(st->sample, 1);
-
-	gpio_set_value(st->rdvel, 0);
-	/* ndelay(5);*/
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&xfer, &msg);
-	ret = spi_sync(st->sdev, &msg);
-	if (ret)
-		goto error_ret;
-	status = st->rx[1];
-	vel = (st->rx[0] & 0x80) ? 0xf000 : 0;
-	vel |= (((s16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
-	len += sprintf(buf + len, "%d %c%c%c%c\n", vel,
+	if (iattr->address)
+		pos = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
+	else {
+		vel = (st->rx[0] & 0x80) ? 0xf000 : 0;
+		vel |= (((s16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
+	}
+	len = sprintf(buf, "%d %c%c%c%c ", iattr->address ? pos : vel,
 				(status & 0x8) ? 'P' : 'V',
 				(status & 0x4) ? 'd' : '_',
 				(status & 0x2) ? 'l' : '_',
 				(status & 0x1) ? '1' : '0');
 error_ret:
-	gpio_set_value(st->rdvel, 1);
 	/* delay (2 * AD2S120X_TSCLK + 20) ns for sample pulse */
 	udelay(1);
 	mutex_unlock(&st->lock);
@@ -194,15 +79,10 @@ error_ret:
 	return ret ? ret : len;
 }
 
-static IIO_CONST_ATTR(description,
-	"12-Bit R/D Converter with Reference Oscillator");
-static IIO_DEVICE_ATTR(pos_vel, S_IRUGO, ad2s120x_show_pos_vel, NULL, 0);
-static IIO_DEVICE_ATTR(pos, S_IRUGO, ad2s120x_show_pos, NULL, 0);
-static IIO_DEVICE_ATTR(vel, S_IRUGO, ad2s120x_show_vel, NULL, 0);
+static IIO_DEVICE_ATTR(pos, S_IRUGO, ad2s120x_show_val, NULL, 1);
+static IIO_DEVICE_ATTR(vel, S_IRUGO, ad2s120x_show_val, NULL, 0);
 
 static struct attribute *ad2s120x_attributes[] = {
-	&iio_const_attr_description.dev_attr.attr,
-	&iio_dev_attr_pos_vel.dev_attr.attr,
 	&iio_dev_attr_pos.dev_attr.attr,
 	&iio_dev_attr_vel.dev_attr.attr,
 	NULL,
@@ -220,42 +100,33 @@ static const struct iio_info ad2s120x_info = {
 static int __devinit ad2s120x_probe(struct spi_device *spi)
 {
 	struct ad2s120x_state *st;
+	struct iio_dev *indio_dev;
 	int pn, ret = 0;
 	unsigned short *pins = spi->dev.platform_data;
 
-	for (pn = 0; pn < AD2S120X_PN; pn++) {
-		if (gpio_request(pins[pn], DRV_NAME)) {
+	for (pn = 0; pn < AD2S120X_PN; pn++)
+		if (gpio_request_one(pins[pn], GPIOF_DIR_OUT, DRV_NAME)) {
 			pr_err("%s: request gpio pin %d failed\n",
 						DRV_NAME, pins[pn]);
 			goto error_ret;
 		}
-		gpio_direction_output(pins[pn], 1);
-	}
-
-	st = kzalloc(sizeof(*st), GFP_KERNEL);
-	if (st == NULL) {
+	indio_dev = iio_allocate_device(sizeof(*st));
+	if (indio_dev == NULL) {
 		ret = -ENOMEM;
 		goto error_ret;
 	}
-	spi_set_drvdata(spi, st);
-
+	spi_set_drvdata(spi, indio_dev);
+	st = iio_priv(indio_dev);
 	mutex_init(&st->lock);
 	st->sdev = spi;
 	st->sample = pins[0];
 	st->rdvel = pins[1];
 
-	st->idev = iio_allocate_device(0);
-	if (st->idev == NULL) {
-		ret = -ENOMEM;
-		goto error_free_st;
-	}
-	st->idev->dev.parent = &spi->dev;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &ad2s120x_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	st->idev->info = &ad2s120x_info;
-	st->idev->dev_data = (void *)(st);
-	st->idev->modes = INDIO_DIRECT_MODE;
-
-	ret = iio_device_register(st->idev);
+	ret = iio_device_register(indio_dev);
 	if (ret)
 		goto error_free_dev;
 
@@ -266,9 +137,7 @@ static int __devinit ad2s120x_probe(struct spi_device *spi)
 	return 0;
 
 error_free_dev:
-	iio_free_device(st->idev);
-error_free_st:
-	kfree(st);
+	iio_free_device(indio_dev);
 error_ret:
 	for (--pn; pn >= 0; pn--)
 		gpio_free(pins[pn]);
@@ -277,10 +146,7 @@ error_ret:
 
 static int __devexit ad2s120x_remove(struct spi_device *spi)
 {
-	struct ad2s120x_state *st = spi_get_drvdata(spi);
-
-	iio_device_unregister(st->idev);
-	kfree(st);
+	iio_device_unregister(spi_get_drvdata(spi));
 
 	return 0;
 }
-- 
1.7.3.4

[PATCH 2/2] staging:iio:resolver:ad2s90 general cleanup

[Jonathan Cameron]

Very simple driver, so not much to do.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/resolver/ad2s90.c |   50 ++++++++------------------------
 1 files changed, 13 insertions(+), 37 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index 9b72a95..29d0fbe 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -24,48 +24,35 @@ struct ad2s90_state {
 	struct mutex lock;
 	struct iio_dev *idev;
 	struct spi_device *sdev;
-	u8 rx[2];
-	u8 tx[2];
+	u8 rx[2] ____cacheline_aligned;
 };
 
 static ssize_t ad2s90_show_angular(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
-	struct spi_message msg;
-	struct spi_transfer xfer;
 	int ret;
 	ssize_t len = 0;
 	u16 val;
-	struct iio_dev *idev = dev_get_drvdata(dev);
-	struct ad2s90_state *st = idev->dev_data;
+	struct ad2s90_state *st = iio_priv(dev_get_drvdata(dev));
 
-	xfer.len = 1;
-	xfer.tx_buf = st->tx;
-	xfer.rx_buf = st->rx;
 	mutex_lock(&st->lock);
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&xfer, &msg);
-	ret = spi_sync(st->sdev, &msg);
+	ret = spi_read(st->sdev, st->rx, 2);
 	if (ret)
 		goto error_ret;
 	val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
 	len = sprintf(buf, "%d\n", val);
 error_ret:
 	mutex_unlock(&st->lock);
-
+	
 	return ret ? ret : len;
 }
 
 #define IIO_DEV_ATTR_SIMPLE_RESOLVER(_show) \
 	IIO_DEVICE_ATTR(angular, S_IRUGO, _show, NULL, 0)
 
-static IIO_CONST_ATTR(description,
-	"Low Cost, Complete 12-Bit Resolver-to-Digital Converter");
 static IIO_DEV_ATTR_SIMPLE_RESOLVER(ad2s90_show_angular);
 
 static struct attribute *ad2s90_attributes[] = {
-	&iio_const_attr_description.dev_attr.attr,
 	&iio_dev_attr_angular.dev_attr.attr,
 	NULL,
 };
@@ -82,29 +69,23 @@ static const struct iio_info ad2s90_info = {
 
 static int __devinit ad2s90_probe(struct spi_device *spi)
 {
+	struct iio_dev *indio_dev;
 	struct ad2s90_state *st;
 	int ret = 0;
 
-	st = kzalloc(sizeof(*st), GFP_KERNEL);
-	if (st == NULL) {
+	indio_dev = iio_allocate_device(sizeof(*st));
+	if (indio_dev == NULL) {
 		ret = -ENOMEM;
 		goto error_ret;
 	}
-	spi_set_drvdata(spi, st);
+	st = iio_priv(indio_dev);
+	spi_set_drvdata(spi, indio_dev);
 
 	mutex_init(&st->lock);
 	st->sdev = spi;
-
-	st->idev = iio_allocate_device(0);
-	if (st->idev == NULL) {
-		ret = -ENOMEM;
-		goto error_free_st;
-	}
-	st->idev->dev.parent = &spi->dev;
-
-	st->idev->info = &ad2s90_info;
-	st->idev->dev_data = (void *)(st);
-	st->idev->modes = INDIO_DIRECT_MODE;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &ad2s90_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = iio_device_register(st->idev);
 	if (ret)
@@ -119,18 +100,13 @@ static int __devinit ad2s90_probe(struct spi_device *spi)
 
 error_free_dev:
 	iio_free_device(st->idev);
-error_free_st:
-	kfree(st);
 error_ret:
 	return ret;
 }
 
 static int __devexit ad2s90_remove(struct spi_device *spi)
 {
-	struct ad2s90_state *st = spi_get_drvdata(spi);
-
-	iio_device_unregister(st->idev);
-	kfree(st);
+	iio_device_unregister(spi_get_drvdata(spi));
 
 	return 0;
 }
-- 
1.7.3.4

RE: [PATCH 0/2] IIO more resolver cleanups.

[Hennerich, Michael]

Jonathan Cameron wrote on 2011-05-20:
> What can I say, this morning has turned out to be rather dull as well
> and these other two drivers are small so here is a cleanup series for
> them.
>
> Note in the ad2s1210 driver I forgot to clear the IN / OUT of
> resolution bit out of the Kconfig file. I'm also not entirely sure I
> understand how that is supposed to work, so may well have it wrong.
>
> Thanks,

Hi Jonathan,

Thanks for having dull times.
Really nice cleanup!

Give me some time to review these - unfortunately I don't have hardware to
test at the moment.

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Gesch=
aeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret=
 Seif

Re: [PATCH 0/2] IIO more resolver cleanups.

[Jonathan Cameron]

On 05/23/11 15:52, Hennerich, Michael wrote:
> Jonathan Cameron wrote on 2011-05-20:
>> What can I say, this morning has turned out to be rather dull as well
>> and these other two drivers are small so here is a cleanup series for
>> them.
>>
>> Note in the ad2s1210 driver I forgot to clear the IN / OUT of
>> resolution bit out of the Kconfig file. I'm also not entirely sure I
>> understand how that is supposed to work, so may well have it wrong.
>>
>> Thanks,
> 
> Hi Jonathan,
> 
> Thanks for having dull times.
> Really nice cleanup!
> 
> Give me some time to review these - unfortunately I don't have hardware to
> test at the moment.
> 
Sure.  This is in the next big round anyway as I don't really want to push
anything else in this merge window.  Feel I've been mean enough to Greg
already!

Jonathan