linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] staging:iio:ad2s1210: Rework read/write operation for fclkin and fexin
@ 2018-03-13 16:05 Rodrigo Siqueira
  2018-03-13 16:05 ` [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and fexcit Rodrigo Siqueira
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Rodrigo Siqueira @ 2018-03-13 16:05 UTC (permalink / raw)
  To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
	Graff Yang
  Cc: linux-iio, devel, daniel.baluta, linux-kernel

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(-)

-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and fexcit
  2018-03-13 16:05 [PATCH v2 0/3] staging:iio:ad2s1210: Rework read/write operation for fclkin and fexin Rodrigo Siqueira
@ 2018-03-13 16:05 ` Rodrigo Siqueira
  2018-03-17 21:12   ` Jonathan Cameron
  2018-03-13 16:05 ` [PATCH v2 2/3] staging:iio:ad2s1210: Add frequency handler in read_raw Rodrigo Siqueira
  2018-03-13 16:06 ` [PATCH v2 3/3] staging:iio:ad2s1210: Add write_raw to handle frequency Rodrigo Siqueira
  2 siblings, 1 reply; 6+ messages in thread
From: Rodrigo Siqueira @ 2018-03-13 16:05 UTC (permalink / raw)
  To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
	Graff Yang
  Cc: linux-iio, devel, daniel.baluta, linux-kernel

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 <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1210.c | 43 ++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index ac13b99bd9cb..28c3fd439663 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -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,
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/3] staging:iio:ad2s1210: Add frequency handler in read_raw
  2018-03-13 16:05 [PATCH v2 0/3] staging:iio:ad2s1210: Rework read/write operation for fclkin and fexin Rodrigo Siqueira
  2018-03-13 16:05 ` [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and fexcit Rodrigo Siqueira
@ 2018-03-13 16:05 ` Rodrigo Siqueira
  2018-03-13 16:06 ` [PATCH v2 3/3] staging:iio:ad2s1210: Add write_raw to handle frequency Rodrigo Siqueira
  2 siblings, 0 replies; 6+ messages in thread
From: Rodrigo Siqueira @ 2018-03-13 16:05 UTC (permalink / raw)
  To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
	Graff Yang
  Cc: linux-iio, devel, daniel.baluta, linux-kernel

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 <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1210.c | 45 +++++++++++++++------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 28c3fd439663..27a42ed10fcd 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -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,
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 3/3] staging:iio:ad2s1210: Add write_raw to handle frequency
  2018-03-13 16:05 [PATCH v2 0/3] staging:iio:ad2s1210: Rework read/write operation for fclkin and fexin Rodrigo Siqueira
  2018-03-13 16:05 ` [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and fexcit Rodrigo Siqueira
  2018-03-13 16:05 ` [PATCH v2 2/3] staging:iio:ad2s1210: Add frequency handler in read_raw Rodrigo Siqueira
@ 2018-03-13 16:06 ` Rodrigo Siqueira
  2 siblings, 0 replies; 6+ messages in thread
From: Rodrigo Siqueira @ 2018-03-13 16:06 UTC (permalink / raw)
  To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
	Graff Yang
  Cc: linux-iio, devel, daniel.baluta, linux-kernel

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 <rodrigosiqueiramelo@gmail.com>
---
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(-)

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 27a42ed10fcd..001ddca92e8f 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -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,
 };
 
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and fexcit
  2018-03-13 16:05 ` [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and fexcit Rodrigo Siqueira
@ 2018-03-17 21:12   ` Jonathan Cameron
  2018-03-20  1:33     ` Rodrigo Siqueira
  0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2018-03-17 21:12 UTC (permalink / raw)
  To: Rodrigo Siqueira
  Cc: devel, Graff Yang, Lars-Peter Clausen, Michael Hennerich,
	linux-iio, Greg Kroah-Hartman, linux-kernel,
	Peter Meerwald-Stadler, Hartmut Knaack, daniel.baluta

On Tue, 13 Mar 2018 13:05:28 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> 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 <rodrigosiqueiramelo@gmail.com>
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


> ---
>  drivers/staging/iio/resolver/ad2s1210.c | 43 ++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index ac13b99bd9cb..28c3fd439663 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -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),
> +	},
> +};
> +
This seems broken, you can't just add a channel and not support
it until the following patches.

>  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,

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and fexcit
  2018-03-17 21:12   ` Jonathan Cameron
@ 2018-03-20  1:33     ` Rodrigo Siqueira
  0 siblings, 0 replies; 6+ messages in thread
From: Rodrigo Siqueira @ 2018-03-20  1:33 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: devel, Graff Yang, Lars-Peter Clausen, Michael Hennerich,
	linux-iio, Greg Kroah-Hartman, linux-kernel,
	Peter Meerwald-Stadler, Hartmut Knaack, daniel.baluta

Hi,

On 03/17, Jonathan Cameron wrote:
> On Tue, 13 Mar 2018 13:05:28 -0300
> Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:
> 
> > 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 <rodrigosiqueiramelo@gmail.com>
> 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

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. 
 
> 
> > ---
> >  drivers/staging/iio/resolver/ad2s1210.c | 43 ++++++++++++++++++++++-----------
> >  1 file changed, 29 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> > index ac13b99bd9cb..28c3fd439663 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -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),
> > +	},
> > +};
> > +
> This seems broken, you can't just add a channel and not support
> it until the following patches.
> 
> >  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,
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-03-20  1:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-13 16:05 [PATCH v2 0/3] staging:iio:ad2s1210: Rework read/write operation for fclkin and fexin Rodrigo Siqueira
2018-03-13 16:05 ` [PATCH v2 1/3] staging:iio:ad2s1210: Add channel for fclkin and fexcit Rodrigo Siqueira
2018-03-17 21:12   ` Jonathan Cameron
2018-03-20  1:33     ` Rodrigo Siqueira
2018-03-13 16:05 ` [PATCH v2 2/3] staging:iio:ad2s1210: Add frequency handler in read_raw Rodrigo Siqueira
2018-03-13 16:06 ` [PATCH v2 3/3] staging:iio:ad2s1210: Add write_raw to handle frequency Rodrigo Siqueira

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).