Skip to contentSkip to conversation

staging:iio:ad2s1210: Rework read/write operation for fclkin and fexin

PATCH In review Rodrigo Siqueira 2 participants 6 messages Mar 13, 2018 – Mar 20, 2018

Subsystems (4)
Staging IIO Analog Devices IIO drivers Staging - industrial io
lore
Share & tools
lore

Rodrigo Siqueira

author
#0 lore raw

v2 cover staging:iio:ad2s1210: Rework read/write operation for fclkin and fexin

This patchset changes the way that frequency of clock input (fclkin) and the excitation frequency (fexcit) are exposed to the userspace. The original code uses the IIO_DEVICE_ATTR to export the configuration of fclkin and fexcit to the userspace which is not in agreement with the current ABI. This patchset adds one channel per clock configuration, updating the read_raw function by adding a handler for the new channels, and introduces the write_raw function. Furthermore, this patchset removes the legacy code responsible for the read and write operation related to fclkin and fexcit. Finally, this patchset uses parts of the original code for handling frequency configuration as an attempt to avoid problems.

Changes in v2:
- Removes unnecessary switch case in the write_raw function.
- Replaces the use of goto for direct return.
- Removes definition with string.
- Removes unecessary label.

Rodrigo Siqueira (3):
staging:iio:ad2s1210: Add channel for fclkin and fexcit staging:iio:ad2s1210: Add frequency handler in read_raw staging:iio:ad2s1210: Add write_raw to handle frequency

 drivers/staging/iio/resolver/ad2s1210.c | 191 +++++++++++++++-----------------
 1 file changed, 92 insertions(+), 99 deletions(-)
Signature · 1 line
2.16.2
author
#1 lore raw

PATCH v2 1/3 staging:iio:ad2s1210: Add channel for fclkin and fexcit

The ad2s1210 does not contain any channel for the fclkin and fexcit. As a result, it uses IIO_DEVICE_ATTR to expose this information. This patch adds one channel for fclkin and another for fexcit. It also adds an enum to easily address the correct channel.

Signed-off-by Rodrigo Siqueira <[email protected]>
---
 drivers/staging/iio/resolver/ad2s1210.c | 43 ++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 14 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +29 -14
index ac13b99bd9cb..28c3fd439663 100644
@@ -67,6 +67,11 @@ enum ad2s1210_mode {
 	MOD_RESERVED,
 };
 
+enum ad2s1210_frequency_channel {
+	FCLKIN = 0,
+	FEXCIT,
+};
+
 static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
 
 struct ad2s1210_state {

@@ -88,6 +93,30 @@ static const int ad2s1210_mode_vals[4][2] = {
 	[MOD_CONFIG] = { 1, 0 },
 };
 
+static const struct iio_chan_spec ad2s1210_channels[] = {
+	{
+		.type = IIO_ANGL,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_ANGL_VEL,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_CHAN_INFO_FREQUENCY,
+		.indexed = 1,
+		.channel = FCLKIN,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_CHAN_INFO_FREQUENCY,
+		.indexed = 1,
+		.channel = FEXCIT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	},
+};
+
 static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
 				     struct ad2s1210_state *st)
 {

@@ -552,20 +581,6 @@ static IIO_DEVICE_ATTR(lot_low_thrd, 0644,
 		       ad2s1210_show_reg, ad2s1210_store_reg,
 		       AD2S1210_REG_LOT_LOW_THRD);
 
-static const struct iio_chan_spec ad2s1210_channels[] = {
-	{
-		.type = IIO_ANGL,
-		.indexed = 1,
-		.channel = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-	}, {
-		.type = IIO_ANGL_VEL,
-		.indexed = 1,
-		.channel = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-	}
-};
-
 static struct attribute *ad2s1210_attributes[] = {
 	&iio_dev_attr_fclkin.dev_attr.attr,
 	&iio_dev_attr_fexcit.dev_attr.attr,
Signature · 1 line
2.16.2
author
#4 lore raw

PATCH v2 2/3 staging:iio:ad2s1210: Add frequency handler in read_raw

In reply to Rodrigo Siqueira

Read data from fclkin and fexcit does not utilize the ad2s1210_read_raw function. This patch, append the required handler in the ad2s1210_read_raw to return the correct value. Also, this patch removes the legacy code related to the read function

Signed-off-by Rodrigo Siqueira <[email protected]>
---
 drivers/staging/iio/resolver/ad2s1210.c | 45 +++++++++++++++------------------
 1 file changed, 20 insertions(+), 25 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +20 -25
index 28c3fd439663..27a42ed10fcd 100644
@@ -210,15 +210,6 @@ static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
 	return ad2s1210_config_write(st, 0x0);
 }
 
-static ssize_t ad2s1210_show_fclkin(struct device *dev,
-				    struct device_attribute *attr,
-				    char *buf)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-
-	return sprintf(buf, "%u\n", st->fclkin);
-}
-
 static ssize_t ad2s1210_store_fclkin(struct device *dev,
 				     struct device_attribute *attr,
 				     const char *buf,

@@ -249,15 +240,6 @@ static ssize_t ad2s1210_store_fclkin(struct device *dev,
 	return ret < 0 ? ret : len;
 }
 
-static ssize_t ad2s1210_show_fexcit(struct device *dev,
-				    struct device_attribute *attr,
-				    char *buf)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-
-	return sprintf(buf, "%u\n", st->fexcit);
-}
-
 static ssize_t ad2s1210_store_fexcit(struct device *dev,
 				     struct device_attribute *attr,
 				     const char *buf, size_t len)

@@ -486,7 +468,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int *val,
 			     int *val2,
-			     long m)
+			     long mask)
 {
 	struct ad2s1210_state *st = iio_priv(indio_dev);
 	u16 negative;

@@ -535,23 +517,36 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 		*val = vel;
 		ret = IIO_VAL_INT;
 		break;
+	case IIO_CHAN_INFO_FREQUENCY:
+		ret = IIO_VAL_INT;
+		switch (chan->channel) {
+		case FCLKIN:
+			*val = st->fclkin;
+			goto unlock_mutex;
+		case FEXCIT:
+			*val = st->fexcit;
+			goto unlock_mutex;
+		default:
+			ret = -EINVAL;
+			goto unlock_mutex;
+		}
+		break;
 	default:
-		mutex_unlock(&st->lock);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto unlock_mutex;
 	}
 
 error_ret:
 	gpio_set_value(st->pdata->sample, 1);
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
+unlock_mutex:
 	mutex_unlock(&st->lock);
 	return ret;
 }
 
-static IIO_DEVICE_ATTR(fclkin, 0644,
-		       ad2s1210_show_fclkin, ad2s1210_store_fclkin, 0);
-static IIO_DEVICE_ATTR(fexcit, 0644,
-		       ad2s1210_show_fexcit,	ad2s1210_store_fexcit, 0);
+static IIO_DEVICE_ATTR(fclkin, 0644, NULL, ad2s1210_store_fclkin, 0);
+static IIO_DEVICE_ATTR(fexcit, 0644, NULL, ad2s1210_store_fexcit, 0);
 static IIO_DEVICE_ATTR(control, 0644,
 		       ad2s1210_show_control, ad2s1210_store_control, 0);
 static IIO_DEVICE_ATTR(bits, 0644,
Signature · 1 line
2.16.2
author
#5 lore raw

PATCH v2 3/3 staging:iio:ad2s1210: Add write_raw to handle frequency

In reply to Rodrigo Siqueira

The write interface of AD2S1210 utilizes IIO_DEVICE_ATTR, which violate the official IIO ABI. This patch, add the write_raw function responsible for handling the fclkin and fexcit channel; also it removes the use of IIO_DEVICE_ATTR for fclkin and fexcit.

Signed-off-by Rodrigo Siqueira <[email protected]>

---
Changes in v2:
- Removes unnecessary switch case in the write_raw function.
- Replaces the use of goto for direct return.
- Removes definition with string.
- Removes unecessary label.

 drivers/staging/iio/resolver/ad2s1210.c | 107 ++++++++++++++------------------
 1 file changed, 45 insertions(+), 62 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +45 -62
index 27a42ed10fcd..001ddca92e8f 100644
@@ -210,64 +210,6 @@ static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
 	return ad2s1210_config_write(st, 0x0);
 }
 
-static ssize_t ad2s1210_store_fclkin(struct device *dev,
-				     struct device_attribute *attr,
-				     const char *buf,
-				     size_t len)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-	unsigned int fclkin;
-	int ret;
-
-	ret = kstrtouint(buf, 10, &fclkin);
-	if (ret)
-		return ret;
-	if (fclkin < AD2S1210_MIN_CLKIN || fclkin > AD2S1210_MAX_CLKIN) {
-		dev_err(dev, "ad2s1210: fclkin out of range\n");
-		return -EINVAL;
-	}
-
-	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 ret < 0 ? ret : len;
-}
-
-static ssize_t ad2s1210_store_fexcit(struct device *dev,
-				     struct device_attribute *attr,
-				     const char *buf, size_t len)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-	unsigned int fexcit;
-	int ret;
-
-	ret = kstrtouint(buf, 10, &fexcit);
-	if (ret < 0)
-		return ret;
-	if (fexcit < AD2S1210_MIN_EXCIT || fexcit > AD2S1210_MAX_EXCIT) {
-		dev_err(dev,
-			"ad2s1210: excitation frequency out of range\n");
-		return -EINVAL;
-	}
-	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 ret < 0 ? ret : len;
-}
-
 static ssize_t ad2s1210_show_control(struct device *dev,
 				     struct device_attribute *attr,
 				     char *buf)

@@ -545,8 +487,50 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static IIO_DEVICE_ATTR(fclkin, 0644, NULL, ad2s1210_store_fclkin, 0);
-static IIO_DEVICE_ATTR(fexcit, 0644, NULL, ad2s1210_store_fexcit, 0);
+static int ad2s1210_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, int val,
+			      int val2, long mask)
+{
+	struct ad2s1210_state *st = iio_priv(indio_dev);
+	unsigned int clk = val;
+	int ret;
+
+	if (mask != IIO_CHAN_INFO_FREQUENCY)
+		return -EINVAL;
+
+	switch (chan->channel) {
+	case FCLKIN:
+		if (clk < AD2S1210_MIN_CLKIN ||
+		    clk > AD2S1210_MAX_CLKIN) {
+			dev_err(&indio_dev->dev,
+				"ad2s1210: fclkin out of range\n");
+			return -EINVAL;
+		}
+		mutex_lock(&st->lock);
+		st->fclkin = clk;
+		break;
+	case FEXCIT:
+		if (clk < AD2S1210_MIN_EXCIT ||
+		    clk > AD2S1210_MAX_EXCIT) {
+			dev_err(&indio_dev->dev,
+				"ad2s1210: excitation frequency out of range\n");
+			return -EINVAL;
+		}
+		mutex_lock(&st->lock);
+		st->fexcit = clk;
+		break;
+	default:
+		return -EINVAL;
+	}
+	ret = ad2s1210_update_frequency_control_word(st);
+	if (ret < 0)
+		goto error_unlock_mutex;
+	ret = ad2s1210_soft_reset(st);
+error_unlock_mutex:
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
 static IIO_DEVICE_ATTR(control, 0644,
 		       ad2s1210_show_control, ad2s1210_store_control, 0);
 static IIO_DEVICE_ATTR(bits, 0644,

@@ -577,8 +561,6 @@ static IIO_DEVICE_ATTR(lot_low_thrd, 0644,
 		       AD2S1210_REG_LOT_LOW_THRD);
 
 static struct attribute *ad2s1210_attributes[] = {
-	&iio_dev_attr_fclkin.dev_attr.attr,
-	&iio_dev_attr_fexcit.dev_attr.attr,
 	&iio_dev_attr_control.dev_attr.attr,
 	&iio_dev_attr_bits.dev_attr.attr,
 	&iio_dev_attr_fault.dev_attr.attr,

@@ -635,6 +617,7 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
 
 static const struct iio_info ad2s1210_info = {
 	.read_raw = ad2s1210_read_raw,
+	.write_raw = ad2s1210_write_raw,
 	.attrs = &ad2s1210_attribute_group,
 };
Signature · 1 line
2.16.2
maintainer
#2 lore raw

Re: [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and fexcit

In reply to Rodrigo Siqueira
Quoted 6 lines · On Tue, 13 Mar 2018 13:05:28 -0300 Rodrigo Siqueira…

The ad2s1210 does not contain any channel for the fclkin and fexcit. As a result, it uses IIO_DEVICE_ATTR to expose this information. This patch adds one channel for fclkin and another for fexcit. It also adds an enum to easily address the correct channel.

Signed-off-by Rodrigo Siqueira <[email protected]>

Take a step back. What are these new channels actually for? We aren't looking at a general purpose frequency input and output. (mind you they are both currently inputs which makes even less sense!)

These are controls for the excitation frequency for a resolver. What is userspace going to do with them? Nothing of any use certainly.

So what do they actually change that matters to an application?
1) The speed at which we can detect a loss of signal condition.
2) The achievable resolution of the sensor.

So how would userspace know how to configure it? I'm not sure it would, but it is these things that should be driving the choice not the actual value of the frequency (which is really just a weird internal value in the way the resolver system works).

There is a pretty strong argument that we should leave the excitation frequency alone at 10kHz unless the platform designer knows better, in which case they should supply it via devicetree rather than from userspace.

Anyhow, it needs a rethink - exposing these as channels is not the way to go!

Jonathan

Quoted 38 lines
---
 drivers/staging/iio/resolver/ad2s1210.c | 43 ++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 14 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +19 -0
index ac13b99bd9cb..28c3fd439663 100644
@@ -67,6 +67,11 @@ enum ad2s1210_mode {
 	MOD_RESERVED,
 };
 
+enum ad2s1210_frequency_channel {
+	FCLKIN = 0,
+	FEXCIT,
+};
+
 static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
 
 struct ad2s1210_state {

@@ -88,6 +93,30 @@ static const int ad2s1210_mode_vals[4][2] = {
 	[MOD_CONFIG] = { 1, 0 },
 };
 
+static const struct iio_chan_spec ad2s1210_channels[] = {
+	{
+		.type = IIO_ANGL,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_ANGL_VEL,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_CHAN_INFO_FREQUENCY,
+		.indexed = 1,

drivers/staging/iio/resolver/ad2s1210.c static const int ad2s1210_mode_vals[4][2] = { · line 119

Quoted 10 lines
drivers/staging/iio/resolver/ad2s1210.c +10 -0
+		.channel = FCLKIN,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_CHAN_INFO_FREQUENCY,
+		.indexed = 1,
+		.channel = FEXCIT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	},
+};
+

This seems broken, you can't just add a channel and not support it until the following patches.

Quoted 14 lines
drivers/staging/iio/resolver/ad2s1210.c +0 -7
 static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
 				     struct ad2s1210_state *st)
 {

@@ -552,20 +581,6 @@ static IIO_DEVICE_ATTR(lot_low_thrd, 0644,
 		       ad2s1210_show_reg, ad2s1210_store_reg,
 		       AD2S1210_REG_LOT_LOW_THRD);
 
-static const struct iio_chan_spec ad2s1210_channels[] = {
-	{
-		.type = IIO_ANGL,
-		.indexed = 1,
-		.channel = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-	}, {

drivers/staging/iio/resolver/ad2s1210.c static IIO_DEVICE_ATTR(lot_low_thrd, 0644, · line 586

Quoted 10 lines
drivers/staging/iio/resolver/ad2s1210.c +0 -7
-		.type = IIO_ANGL_VEL,
-		.indexed = 1,
-		.channel = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-	}
-};
-
 static struct attribute *ad2s1210_attributes[] = {
 	&iio_dev_attr_fclkin.dev_attr.attr,
 	&iio_dev_attr_fexcit.dev_attr.attr,

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

author
#3 lore raw

Re: [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and fexcit

Hi,

Quoted 30 lines · Jonathan Cameron
Quoted 6 lines · On Tue, 13 Mar 2018 13:05:28 -0300 Rodrigo Siqueira…

The ad2s1210 does not contain any channel for the fclkin and fexcit. As a result, it uses IIO_DEVICE_ATTR to expose this information. This patch adds one channel for fclkin and another for fexcit. It also adds an enum to easily address the correct channel.

Signed-off-by Rodrigo Siqueira <[email protected]>

Take a step back. What are these new channels actually for? We aren't looking at a general purpose frequency input and output. (mind you they are both currently inputs which makes even less sense!)

These are controls for the excitation frequency for a resolver. What is userspace going to do with them? Nothing of any use certainly.

So what do they actually change that matters to an application?
1) The speed at which we can detect a loss of signal condition.
2) The achievable resolution of the sensor.

So how would userspace know how to configure it? I'm not sure it would, but it is these things that should be driving the choice not the actual value of the frequency (which is really just a weird internal value in the way the resolver system works).

There is a pretty strong argument that we should leave the excitation frequency alone at 10kHz unless the platform designer knows better, in which case they should supply it via devicetree rather than from userspace.

Quoted 4 lines

Anyhow, it needs a rethink - exposing these as channels is not the way to go!

Jonathan

Thanks for the feedback.
Now I am thinking about this module (and IIO subsystem) from another perspective :)

I will rethink and came with another approach.

Quoted 77 lines
Quoted 39 lines
---
 drivers/staging/iio/resolver/ad2s1210.c | 43 ++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 14 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +19 -0
index ac13b99bd9cb..28c3fd439663 100644
@@ -67,6 +67,11 @@ enum ad2s1210_mode {
 	MOD_RESERVED,
 };
 
+enum ad2s1210_frequency_channel {
+	FCLKIN = 0,
+	FEXCIT,
+};
+
 static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
 
 struct ad2s1210_state {

@@ -88,6 +93,30 @@ static const int ad2s1210_mode_vals[4][2] = {
 	[MOD_CONFIG] = { 1, 0 },
 };
 
+static const struct iio_chan_spec ad2s1210_channels[] = {
+	{
+		.type = IIO_ANGL,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_ANGL_VEL,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_CHAN_INFO_FREQUENCY,
+		.indexed = 1,

drivers/staging/iio/resolver/ad2s1210.c static const int ad2s1210_mode_vals[4][2] = { · line 119

Quoted 10 lines
drivers/staging/iio/resolver/ad2s1210.c +10 -0
+		.channel = FCLKIN,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_CHAN_INFO_FREQUENCY,
+		.indexed = 1,
+		.channel = FEXCIT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	},
+};
+

This seems broken, you can't just add a channel and not support it until the following patches.

Quoted 24 lines
drivers/staging/iio/resolver/ad2s1210.c +0 -14
 static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
 				     struct ad2s1210_state *st)
 {

@@ -552,20 +581,6 @@ static IIO_DEVICE_ATTR(lot_low_thrd, 0644,
 		       ad2s1210_show_reg, ad2s1210_store_reg,
 		       AD2S1210_REG_LOT_LOW_THRD);
 
-static const struct iio_chan_spec ad2s1210_channels[] = {
-	{
-		.type = IIO_ANGL,
-		.indexed = 1,
-		.channel = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-	}, {
-		.type = IIO_ANGL_VEL,
-		.indexed = 1,
-		.channel = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-	}
-};
-
 static struct attribute *ad2s1210_attributes[] = {
 	&iio_dev_attr_fclkin.dev_attr.attr,
 	&iio_dev_attr_fexcit.dev_attr.attr,